UPSTREAM: <carry>: add minimumkubeletversion admission package #2201
Conversation
openshift-ci-robot
added
the
backports/unvalidated-commits
|
@haircommander: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: haircommander The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-kube-apiserver/admission/customresourcevalidation/node/validate_node_config.go
Outdated
Show resolved
Hide resolved
| admission.ValidationInterface | ||
|
|
||
| nodeLister func() corev1listers.NodeLister | ||
| waitForReady func() func() bool |
There was a problem hiding this comment.
I would prefer this to be waitForNodeInformerSyncedFn func() bool
There was a problem hiding this comment.
I think it needs to be func() func() bool because at the time of constrution the validator may not have c.handler initialized, as it's only initialized when SetExternalKubeInformerFactory is called which i think is after the object initialization here
There was a problem hiding this comment.
Mhm, I think the content of the function changes, but the "pointer" to the function remains the same, so we should be fine, no ? .
In general, the plugins are initialized, validated, and only then wired to handlers.
Maybe we could write a small program that would demonstrate the issue ?
There was a problem hiding this comment.
okay you seem to be right, I was worried it wasn't saved as a pointer https://go.dev/play/p/h_vI248v25o
There was a problem hiding this comment.
hmm though https://go.dev/play/p/2P8e4jurISL gives pause. I'm leaving it your way for now
There was a problem hiding this comment.
hmm, I think that our scenario is more like https://go.dev/play/p/fHVBh-U_Gtk, no ?
openshift-kube-apiserver/admission/customresourcevalidation/node/validate_node_config.go
Outdated
Show resolved
Hide resolved
openshift-kube-apiserver/admission/customresourcevalidation/node/validate_node_config.go
Outdated
Show resolved
Hide resolved
openshift-kube-apiserver/admission/customresourcevalidation/node/validate_node_config.go
Outdated
Show resolved
Hide resolved
openshift-kube-apiserver/admission/customresourcevalidation/node/validate_node_config.go
Outdated
Show resolved
Hide resolved
openshift-kube-apiserver/admission/customresourcevalidation/node/validate_node_config.go
Show resolved
Hide resolved
openshift-kube-apiserver/admission/customresourcevalidation/node/validate_node_config.go
Outdated
Show resolved
Hide resolved
openshift-kube-apiserver/admission/customresourcevalidation/node/validate_node_config_test.go
Show resolved
Hide resolved
haircommander
changed the title
haircommander
force-pushed
the
minKubeletVersion2
branch
from
1a4808c to
4fb1988
Compare
|
@haircommander: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
I took your suggestions except for the |
|
|
||
| nodes, err := c.nodeListerFn().List(labels.Everything()) | ||
| if err != nil { | ||
| return field.Forbidden(fieldPath, fmt.Sprintf("Getting nodes to compare minimum version %v", err.Error())) |
There was a problem hiding this comment.
I don't think the type of the error matters much because these errors are reported as apierrors.NewInvalid, but maybe we could change this one to field.NotFound(...)
| @@ -0,0 +1,218 @@ | |||
| package node | |||
There was a problem hiding this comment.
Please update the PR so that I can see the diff applied to this file and the others. That will make it easier to review further.
There was a problem hiding this comment.
OK, so the only outstanding comment is #2201 (comment).
Other than that this LGTM.
openshift-kube-apiserver/authorization/minimumkubeletversion/minimum_kubelet_version.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Peter Hunt <[email protected]>
…ostic of field Signed-off-by: Peter Hunt <[email protected]>
Signed-off-by: Peter Hunt <[email protected]>
haircommander
force-pushed
the
minKubeletVersion2
branch
from
4fb1988 to
99174f9
Compare
|
@haircommander: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
Signed-off-by: Peter Hunt <[email protected]>
haircommander
force-pushed
the
minKubeletVersion2
branch
from
99174f9 to
99f96bb
Compare
|
@haircommander: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
/retest |
|
@haircommander: The following tests failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
What type of PR is this?
What this PR does / why we need it:
#2104 + admission pieces, as requested by #2104 (comment)
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: