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

feat: use custom socket configurations from v1alpha3 #930

Merged
merged 22 commits into from
Jan 11, 2024

Conversation

ashnamehrotra
Copy link
Contributor

What this PR does / why we need it:
Second PR to implement changes for configurable runtime socket.

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #647

Special notes for your reviewer:

Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
… and v1alpha3

Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
… runtime socket to containers based on name

Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
…tor and remover containers

Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
…meSpec to set needed env vars to scan

Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
…ot path

Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
@pmengelbert
Copy link
Contributor

pmengelbert commented Jan 4, 2024

I haven't looked at the changes yet, but by looking at the files that were modified I can see that no tests were added :)

  • Add e2e test that touches this code path, proves that the feature works
  • Add unit tests for any utility functions
  • Add unit tests for any public methods and functions

@ashnamehrotra
Copy link
Contributor Author

@pmengelbert the unit test file for the conversion was added in the other PR and the individual containers that we are changing already have unit tests to make sure they function, is there any other you think we need? I can maybe add one for setRuntimeSocketEnvVars in trivy?

Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
if runtime == "" {
runtime = utils.RuntimeContainerd
userConfig.Runtime = unversioned.RuntimeSpec{
Name: unversioned.Runtime(utils.EnvEraserRuntimeName),
Copy link
Contributor

Choose a reason for hiding this comment

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

This sets userConfig.Runtime.Name name to ERASER_RUNTIME_NAME ! It leads to the question of how this is working at all.

Comment on lines 41 to 44
{
desc: "alternative runtime",
config: Config{Runtime: "crio"},
expected: []string{"--format=json", "image", "--image-src", "crio", ref},
config: Config{Runtime: unversioned.RuntimeSpec{Name: unversioned.RuntimeCrio, Address: unversioned.CrioPath}},
expected: []string{"--format=json", "image", "--image-src", ImgSrcPodman, ref},
Copy link
Contributor

Choose a reason for hiding this comment

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

For funsies let's add an alternative test case for dockershim

Comment on lines 1 to 2
ARG NODE_VERSION
FROM kindest/node:v${NODE_VERSION}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd name it KUBERNETES_VERSION since that's exactly what it is.

Makefile Outdated
custom-node:
docker build -t custom-node:latest \
-f test/e2e/test-data/Dockerfile.customNode \
--build-arg NODE_VERSION=${KUBERNETES_VERSION} test/e2e/test-data
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned below, I would change the name of the build arg to KUBERNETES_VERSION. It makes it clearer where the value is ultimately coming from.

Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
Copy link
Contributor

@pmengelbert pmengelbert left a comment

Choose a reason for hiding this comment

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

LGTM, nice work!

@sozercan @salaxander can you give this a quick look just in case I missed something?

@pmengelbert pmengelbert merged commit 9c4e3a8 into eraser-dev:main Jan 11, 2024
92 of 93 checks passed
ashnamehrotra added a commit to ashnamehrotra/eraser that referenced this pull request Jan 25, 2024
Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
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.

Make containerd.sock configurable in your helm charts
2 participants