Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use ubi8 container base image #195

Merged
merged 10 commits into from
May 19, 2022
Merged

Conversation

KalmanMeth
Copy link
Collaborator

No description provided.

@KalmanMeth KalmanMeth linked an issue May 1, 2022 that may be closed by this pull request
@codecov-commenter
Copy link

codecov-commenter commented May 1, 2022

Codecov Report

Merging #195 (3a1f65e) into main (f13dc86) will decrease coverage by 0.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #195      +/-   ##
==========================================
- Coverage   58.25%   58.16%   -0.09%     
==========================================
  Files          58       58              
  Lines        3349     3361      +12     
==========================================
+ Hits         1951     1955       +4     
- Misses       1269     1276       +7     
- Partials      129      130       +1     
Flag Coverage Δ
unittests 58.16% <ø> (-0.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/pipeline/encode/encode_kafka.go 66.66% <0.00%> (-1.13%) ⬇️
pkg/pipeline/transform/transform_network.go 60.00% <0.00%> (-0.84%) ⬇️
pkg/pipeline/pipeline_builder.go 59.50% <0.00%> (-0.43%) ⬇️
pkg/pipeline/encode/encode.go 100.00% <0.00%> (ø)
pkg/confgen/grafana_jsonnet.go 0.00% <0.00%> (ø)
pkg/confgen/flowlogs2metrics_config.go 0.00% <0.00%> (ø)
pkg/pipeline/encode/encode_prom.go 81.77% <0.00%> (+0.08%) ⬆️
pkg/pipeline/transform/kubernetes/kubernetes.go 15.42% <0.00%> (+1.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f13dc86...3a1f65e. Read the comment docs.

@KalmanMeth
Copy link
Collaborator Author

During testing:
#19 [builder 11/13] RUN rm -rf bin
#19 sha256:e43a290424418aed9f6c6df124a2219c17fe689013c400100d48d81d458e665e
#19 0.983 rm: cannot remove 'bin/golangci-lint-v1.43.0': Permission denied
#19 0.983 rm: cannot remove 'bin/kind-v0.11.1': Permission denied
#19 ERROR: executor failed running [/bin/sh -c rm -rf bin]: exit code: 1

Not sure why permission is denied.

Commented out the command RUN rm -rf bin.

@eranra eranra requested a review from jotak May 1, 2022 18:33
@eranra
Copy link
Collaborator

eranra commented May 1, 2022

@jotak can you please check this PR ... is there a better way to get golang 1.17 ???

@@ -12,7 +21,7 @@ RUN go mod download
RUN go mod download -modfile=.bingo/golangci-lint.mod

COPY . ./
RUN rm -rf bin
#RUN rm -rf bin
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The need to delete the bin folder is to make sure that we are re-building/downloading the binaries using .bingo. when we build the container image and not use the downloaded ones from git

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a better way to get rid of the bin directory in the docker image is to add a .dockerignore file to the repository with a line to exclude bin/.
https://docs.docker.com/engine/reference/builder/#dockerignore-file

Copy link
Collaborator

@ronensc ronensc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the timeouts in the e2e tests, I recommend adding logging of timestamps in strategic places so we could measure which parts takes the most time to run.

@@ -0,0 +1,2 @@
.git
/bin/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .dockerignore is not processed because of:

  1. Typo in filename: .dockeringnore: .dockerignore
  2. It should be in the root directory of the repository.

@@ -0,0 +1,2 @@
.git
/bin/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to strip the leading slash / to avoid confusion with root.

@@ -0,0 +1,2 @@
.git
/bin/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found that there is an inconsistency between docker and podman regarding handling .dockerignore.
While both process it when it is in the repo's root directory, docker also allows <dockerfile-name>.dockerignore
https://stackoverflow.com/a/57774684/2749989

@jotak
Copy link
Member

jotak commented May 9, 2022

about the go-toolset image and go1.17: we have the same workaround on other images. Maybe we can have our own "netobserv/go-toolset" image to avoid fixing all components one by one ...

@jotak
Copy link
Member

jotak commented May 9, 2022

@KalmanMeth @eranra the final stage here is still using ubuntu, I think we need either to change all stages to ubi8, or none (the whole purpose being to have a common base across components to make it easier to maintain / check vulnerabilities & upgrades).

But to be honest, I see also that there's included some net tools with the image, I don't want to force moving to ubi8 if you have the tooling you want with this current Dockerfile, the goal is not to make troubleshooting harder.

@eranra
Copy link
Collaborator

eranra commented May 11, 2022

@KalmanMeth @eranra the final stage here is still using ubuntu, I think we need either to change all stages to ubi8, or none (the whole purpose being to have a common base across components to make it easier to maintain / check vulnerabilities & upgrades).

But to be honest, I see also that there's included some net tools with the image, I don't want to force moving to ubi8 if you have the tooling you want with this current Dockerfile, the goal is not to make troubleshooting harder.

@KalmanMeth what we want to make sure is that even with ubi8 we still have the simple tools to understand processes and networks. I also assume that the ubi8 image do not include or allow any package manager such as yum or apt so what I propose is to copy get those tools in the builder stage and make sure that we copy them into the ubi container. Exacely like what we are doing to the executables we compile https://github.com/netobserv/flowlogs-pipeline/pull/195/files#diff-e818f6b11598d2656922413d3912abb820c175e2f739549dbda55cc9559bd6fdL28

This will allow us on one hand to work with ubi8 as needed for security but have those tools handy if needed.

@jotak does this make sense and acceptable?

@KalmanMeth
Copy link
Collaborator Author

@eranra @jotak I'm trying to use ubi8/ubi-minimal, and used microdnf (instead of yum) to install the packages needed for ping, ifconfig, curl. I still get build errors from github, although it runs to completion on my own machine.

@jotak
Copy link
Member

jotak commented May 19, 2022

thanks @KalmanMeth @eranra and sorry for the delay 😬

/lgtm

@jotak jotak merged commit 8c5e6ef into netobserv:main May 19, 2022
@KalmanMeth KalmanMeth deleted the change-base-image branch May 24, 2022 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change base image of flowlogs-pipeline deployment from ubuntu to ubi8
5 participants