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

fix: Use correct rules for ephemeralcontainers #58

Merged
merged 3 commits into from
Jul 18, 2024
Merged

Conversation

viccuad
Copy link
Member

@viccuad viccuad commented Jul 17, 2024

Description

Relates to #57

Set correct rules for ephemeralcontainers.
Prepare for release.

Test

CI

Additional Information

Tradeoff

Potential improvement

@viccuad viccuad requested a review from a team as a code owner July 17, 2024 12:34
@viccuad viccuad self-assigned this Jul 17, 2024
@@ -76,6 +76,6 @@ annotations:
- v1
resources:
- pods
- pods/ephemeralcontainers
- ephemeralcontainers
Copy link
Member

Choose a reason for hiding this comment

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

This is still wrong, because EphemeralContainers are not a Kubernetes resource, but a sub-resource of Pod.

If you run the following command:

kubectl api-resources

you will see that ephemeral containers are not part of the output.

Looking into this section of the Dynamic Admission Controller webhooks, it seems that doing pods/ephemeralcontainers is fine.

However, I think that if you have the "parent" resource inside of the rules (like pods), it doesn't make a big difference to have also pods/ephemeralresources.

It would matter for a policy that is interested at evaluating only the contents of ephemeral containers, which is not the case here.

To summarize:

  • this policy: I would just remove ephemeralcontainers from there
  • audit-scanner: when looking into policies that have subresources as targets, things get complicated... because we have to generate a different request. See the 1st example here: the scale sub-resource of a Deployment has been changed, the policy is going not going to receive the whole Deployment inside of admissionreview.request.object, but the Scale object. This is something worth to be tracked with a dedicated issue on audit-controller

Copy link
Member Author

@viccuad viccuad Jul 17, 2024

Choose a reason for hiding this comment

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

Well spotted. I have amended the PR to drop ephemeralcontainers.

On the audit-scanner side, I'm not sure it's a bug or a missing feature; if a policy declares a rules for a subresource, it must be able to validate a request that only contains that subresource.

For this specific policy we must ensure we don't set the subresource then.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think for audit-scanner we have to handle the case of a policy that is targeting only a sub-resource. I don't think this is something common, we could just track it and work on that when someone complains about that

Copy link
Member Author

Choose a reason for hiding this comment

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

In my opinion the audit-scanner should we working seamlessly already; I don't think it sees a difference between a resource or a subresource. Hence why the policy was already being triggered by the audit-scanner.

Nevertheless, I opened kubewarden/audit-scanner#322 for bookeeping.

viccuad and others added 3 commits July 17, 2024 16:23
pods/ephemeralcontainers is a subresource of pods, including pods
is enough.

Signed-off-by: Víctor Cuadrado Juan <[email protected]>
Signed-off-by: Víctor Cuadrado Juan <[email protected]>
Signed-off-by: Víctor Cuadrado Juan <[email protected]>
@viccuad viccuad requested a review from flavio July 17, 2024 14:26
@viccuad viccuad merged commit 5506c6c into main Jul 18, 2024
12 checks passed
@viccuad
Copy link
Member Author

viccuad commented Jul 18, 2024

tagged in main as 0.1.10.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants