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

Add DenyPSALabel admission plugin #10950

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

galal-hussein
Copy link
Contributor

@galal-hussein galal-hussein commented Sep 25, 2024

Proposed Changes

The PR adds a new admission plugin to the apiserver, this has become available by the recent patch k3s-io/kubernetes@94d3e60 which allows the users to register a new plugin dynamically when starting the API server.

The plugin "DenyPSALabel" will deny the overriding of the default PSA security configuration passed to the API server, by default this plugin will not start nor any behavior will be different unless the user passes the following arguments to k3s:

k3s server --deny-psa-label --kube-apiserver-arg="enable-admission-plugin=DenyPSALabel"

The first flag will register the plugin to the apiserver and the second flag will enable it in the runtime

Types of Changes

Testing

  • After starting the feature user shouldnt be able to add the following label to the namespace, for example:
kubectl label --dry-run=server --overwrite ns --all \
    pod-security.kubernetes.io/enforce=baseline

should result in an error.

Linked Issues

User-Facing Change


Further Comments

@galal-hussein galal-hussein requested a review from a team as a code owner September 25, 2024 21:01
pkg/admission/denypsalabel/admission.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Sep 25, 2024

Codecov Report

Attention: Patch coverage is 3.22581% with 30 lines in your changes missing coverage. Please review.

Project coverage is 40.11%. Comparing base (ed14f7f) to head (16242c6).
Report is 19 commits behind head on master.

Files with missing lines Patch % Lines
pkg/admission/denypsalabel/admission.go 0.00% 26 Missing ⚠️
pkg/daemons/control/server.go 0.00% 3 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (ed14f7f) and HEAD (16242c6). Click for more details.

HEAD has 3 uploads less than BASE
Flag BASE (ed14f7f) HEAD (16242c6)
unittests 1 0
inttests 10 9
e2etests 7 6
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10950      +/-   ##
==========================================
- Coverage   49.64%   40.11%   -9.54%     
==========================================
  Files         178      162      -16     
  Lines       14801    14354     -447     
==========================================
- Hits         7348     5758    -1590     
- Misses       6105     7405    +1300     
+ Partials     1348     1191     -157     
Flag Coverage Δ
e2etests 36.42% <3.22%> (-9.55%) ⬇️
inttests 19.63% <3.22%> (-0.09%) ⬇️
unittests ?

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: galal-hussein <[email protected]>
pkg/admission/denypsalabel/admission.go Outdated Show resolved Hide resolved
pkg/admission/denypsalabel/admission.go Outdated Show resolved Hide resolved
pkg/admission/denypsalabel/admission.go Outdated Show resolved Hide resolved
pkg/cli/cmds/server.go Outdated Show resolved Hide resolved
Signed-off-by: galal-hussein <[email protected]>
brandond
brandond previously approved these changes Sep 26, 2024
vitorsavian
vitorsavian previously approved these changes Sep 30, 2024
Copy link
Member

@dereknola dereknola left a comment

Choose a reason for hiding this comment

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

Before this is merged, @galal-hussein can you fill out this PR more? There isn't any linked issues and there is no testing for a new feature. You should be able to add a new testlet to the startup integration test https://github.com/k3s-io/k3s/blob/master/tests/integration/startup/startup_int_test.go

@galal-hussein
Copy link
Contributor Author

@dereknola sure will fix that

dereknola
dereknola previously approved these changes Oct 7, 2024
It("change label of namespace", func() {
res, err := testutil.K3sCmd("kubectl label --dry-run=server --overwrite ns --all pod-security.kubernetes.io/enforce=baseline")
Expect(err).To(HaveOccurred())
Expect(res).To(ContainSubstring("denying use of PSA label on namespace"))
Copy link
Member

Choose a reason for hiding this comment

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

I think that this test will fail as-is, since this is not the string that is output any longer.

Suggested change
Expect(res).To(ContainSubstring("denying use of PSA label on namespace"))
Expect(res).To(ContainSubstring("Use of label with pod-security.kubernetes.io/ prefix on Namespace is denied by admission control"))

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.

4 participants