Add denyserviceexternalips admission (KEP 2200) #97395
Conversation
thockin
added
the
do-not-merge/hold
k8s-ci-robot
added
release-note
k8s-ci-robot
added
area/apiserver
sig/api-machinery
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: thockin 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 |
k8s-ci-robot
added
the
approved
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
thockin
force-pushed
the
externalips-admission
branch
from
9b867fd
to
62096d2
Compare
| // ValidateInitialization ensures lister is set. | ||
| func (plug *externalIPsDenierPlugin) ValidateInitialization() error { | ||
| if plug.lister == nil { | ||
| return fmt.Errorf("missing lister") |
There was a problem hiding this comment.
nit, other plugins seems to provide more information on the error
return fmt.Errorf("%s requires a service lister", PluginName)
There was a problem hiding this comment.
I totally copied this from some other plugin. I do not know what the best-practice here.
| // externalIPsDenierPlugin holds state for and implements the admission plugin. | ||
| type externalIPsDenierPlugin struct { | ||
| *admission.Handler | ||
| lister corev1listers.ServiceLister |
There was a problem hiding this comment.
just for learning, for what is being used the lister?
I can´t find any reference
There was a problem hiding this comment.
Copied from somewhere - it looks like I picked a bad example and can do it more simply. Will push a new version soon.
There was a problem hiding this comment.
The verify job fails because you still have the lister
thockin
force-pushed
the
externalips-admission
branch
from
62096d2
to
f81f1b2
Compare
|
New version pushed - passes unit tests, still need to actually deploy it |
|
Manually tested in my own cluster. Seems to work as intended. |
thockin
force-pushed
the
externalips-admission
branch
from
f81f1b2
to
c0cfaf9
Compare
thockin
force-pushed
the
externalips-admission
branch
from
c0cfaf9
to
a829907
Compare
|
/retest
|
| return nil | ||
| } | ||
|
|
||
| klog.V(4).Infof("Denying new use of ExternalIPs on Service %s/%s", newSvc.Namespace, newSvc.Name) |
There was a problem hiding this comment.
maybe give a path that matches the style of our paths in validation as a nicer hint.
There was a problem hiding this comment.
which paths do you mean? namespaces/n/services/s or something else?
|
impl looks fine if we approve the kep. |
|
lgtm |
|
This is held until the KEP itself merges (kubernetes/enhancements#2176) so you can say the magic word if you want to. Or we can wait :) |
|
KEP has merged. |
|
#97395 (comment) |
k8s-ci-robot
added
the
lgtm
thockin
removed
the
do-not-merge/hold
|
will these changes be backported to v1.17, is it affected by CVE-2020-8554? |
|
I think that previous versions should be using the published external webhook |
|
This is awesome and a much appreciated enhancement. Given the clean design and implementation, I second @prashant1221 request to consider cherry-picking this to 1.18, 1.19 and 1.20. While the webhook works, it certainly isn't simple to setup and brings its own set of problems that many folks would prefer to avoid. |
/kind feature
What this PR does / why we need it:
Implements KEP 2200 (kubernetes/enhancements#2176). Adds an optional admission controller to block use of the externalIPs feature (#97110)
Which issue(s) this PR fixes:
xref #97110 - we'll need docs and stuff too