[WIP] admission/podsecuritypolicy: externalize podsecuritypolicy admission controller #67846
Conversation
k8s-ci-robot
added
release-note-none
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: xmudrii If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| @@ -20,24 +20,24 @@ import ( | |||
| "fmt" | |||
| "strings" | |||
|
|
|||
| policyv1beta1 "k8s.io/api/policy/v1beta1" | |||
There was a problem hiding this comment.
Is it really required to move also PSP-related things to external API? Is it possible to move admission plugin alone?
There was a problem hiding this comment.
I may be wrong, but I think this is required. If I'm mistaken, please correct me.
For example, one of problems I had is this function. As lister is changed to external one, it now must take policyv1beta1 external type instead of internal type.
However, that function invokes psp.NewSimpleProvider, which takes internal policy type. Invoking this function with external policy causes type mismatch.
If I update psp.NewSimpleProvider to use external type, I must update all other related functions to use external type, or otherwise I'll run into a big number of issues and errors. And therefore, most of PSP-related things are moved to external type in this PR.
My guess is that this is okay. I'm unsure is there a better way.
There was a problem hiding this comment.
Why should it be bad? As a rule of thumb: internal types should only be used in the registries, i.e. the serving part of the API server. The admission plugin though are normal consumers and should use the external types.
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
|
@xmudrii: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
@stlaz JFYI |
| string(policyv1beta1.DownwardAPI), | ||
| string(policyv1beta1.FC), | ||
| string(policyv1beta1.ConfigMap), | ||
| //string(policyv1beta1.VsphereVolume), |
There was a problem hiding this comment.
obviously internal psp has some definitions which the external doesn't have. can we straightly copy these missing ones instead of commenting like this?
@php-coder is it intended? i think it should have been done in #61608 #54933.
There was a problem hiding this comment.
The commenting here is just a workaround for test failures. 🙂
I'm not sure what to do with those, so I would appreciate any ideas. I think copying those to external could work, but I would like to get some approval first.
There was a problem hiding this comment.
As far as I see the list of volumes types is just incomplete. Please create a commit filling it up with everything that is missing.
| case v.ScaleIO != nil: | ||
| return policy.ScaleIO, nil | ||
| return policyv1beta1.AzureDisk, nil | ||
| //case v.PhotonPersistentDisk != nil: |
|
I will leave this PR on @yue9944882. |
What this PR does / why we need it:
This PR is Work-in-Progress. Do not merge.
This PR addresses portion of #66680, by externalizing PodSecurityPolicy Admission controller.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Addresses portion of #66680.
Release note: