[WIP] OCPBUGS-62726: add descheduler validation plugin #2491
Conversation
Signed-off-by: Evan Hearne <[email protected]>
openshift-ci-robot
added
the
backports/unvalidated-commits
openshift-ci
bot
added
do-not-merge/work-in-progress
openshift-ci-robot
added
jira/valid-reference
|
@ehearne-redhat: This pull request references Jira Issue OCPBUGS-63132, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@ehearne-redhat: 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: ehearne-redhat 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 |
ehearne-redhat
changed the title
openshift-ci-robot
added
jira/invalid-bug
|
@ehearne-redhat: This pull request references Jira Issue OCPBUGS-62726, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
1 similar comment
|
/jira refresh |
|
@ehearne-redhat: This pull request references Jira Issue OCPBUGS-62726, which is invalid:
Comment In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@ehearne-redhat: This pull request references Jira Issue OCPBUGS-62726, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
Signed-off-by: Evan Hearne <[email protected]>
|
@ehearne-redhat: 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 |
There was a problem hiding this comment.
The reason why it is necessary is without it, Go is unable to obtain the correct version. It tries to get v0.30.0 or something like that if I remember correctly.
There was a problem hiding this comment.
[Error - 15:41:26] Request workspace/executeCommand failed.
Message: err: exit status 1: stderr: go: finding module for package github.com/openshift/cluster-kube-descheduler-operator/pkg/apis/descheduler/v1
go: k8s.io/[email protected]: reading k8s.io/kube-openapi/go.mod at revision v0.30.0: unknown revision v0.30.0| "k8s.io/kubernetes/openshift-kube-apiserver/admission/customresourcevalidation" | ||
| ) | ||
|
|
||
| const PluginName = "config.openshift.io/ValidateDescheduler" |
There was a problem hiding this comment.
KubeDescheduler is in the operator.openshift.io API group. Might makes sense to have this reflect that as well.
| plugins.Register(PluginName, func(config io.Reader) (admission.Interface, error) { | ||
| return customresourcevalidation.NewValidator( | ||
| map[schema.GroupResource]bool{ | ||
| configv1.Resource("deschedulers"): true, |
There was a problem hiding this comment.
This won't resolve correctly for a couple reasons:
- The resource would be
kubedeschedulersnotdeschedulers. - The resource is part of the
operator.openshift.iogroup, notconfig.openshift.io.
I think instead you probably want:
| configv1.Resource("deschedulers"): true, | |
| deschedulerv1.Resource("kubedeschedulers"): true, |
| configv1.Resource("deschedulers"): true, | ||
| }, | ||
| map[schema.GroupVersionKind]customresourcevalidation.ObjectValidator{ | ||
| configv1.GroupVersion.WithKind("Descheduler"): deschedulerV1{}, |
There was a problem hiding this comment.
Similarly to my comment above, you probably want:
| configv1.GroupVersion.WithKind("Descheduler"): deschedulerV1{}, | |
| deschedulerv1.SchemeGroupVersion.WithKind("KubeDescheduler"): deschedulerV1{}, |
| field.NotSupported(field.NewPath("kind"), fmt.Sprintf("%T", uncastObj), []string{"Descheduler"}), | ||
| field.NotSupported(field.NewPath("apiVersion"), fmt.Sprintf("%T", uncastObj), []string{"config.openshift.io/v1"})) |
There was a problem hiding this comment.
| field.NotSupported(field.NewPath("kind"), fmt.Sprintf("%T", uncastObj), []string{"Descheduler"}), | |
| field.NotSupported(field.NewPath("apiVersion"), fmt.Sprintf("%T", uncastObj), []string{"config.openshift.io/v1"})) | |
| field.NotSupported(field.NewPath("kind"), fmt.Sprintf("%T", uncastObj), []string{"KubeDescheduler"}), | |
| field.NotSupported(field.NewPath("apiVersion"), fmt.Sprintf("%T", uncastObj), []string{"operator.openshift.io/v1"})) |
| func validateDeschedulerSpec(spec deschedulerv1.KubeDeschedulerSpec) field.ErrorList { | ||
| allErrs := field.ErrorList{} | ||
|
|
||
| if name := spec.Policy.Name; len(name) > 0 { | ||
| for _, msg := range validation.NameIsDNSSubdomain(spec.Policy.Name, false) { | ||
| allErrs = append(allErrs, field.Invalid(field.NewPath("spec.Policy.name"), name, msg)) | ||
| } | ||
| } | ||
|
|
||
| return allErrs | ||
| } |
There was a problem hiding this comment.
Why do we need to validate the .spec field of the KubeDescheduler resource? If I understand correctly, we are only wanting to enforce the naming of the object to resolve this bug.
Enforcing additional validations on the .spec field of this resource introduces more complexity into the considerations we need to have here. Specifically how we handle potentially breaking changes to user expectations.
There was a problem hiding this comment.
There isn't a specific reason to validate, and there does not exist a .spec field from the v1 package --> https://pkg.go.dev/github.com/openshift/cluster-kube-descheduler-operator/pkg/apis/descheduler/v1 .
I will remove this. :)
| if name := spec.Policy.Name; len(name) > 0 { | ||
| for _, msg := range validation.NameIsDNSSubdomain(spec.Policy.Name, false) { | ||
| allErrs = append(allErrs, field.Invalid(field.NewPath("spec.Policy.name"), name, msg)) | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't see where the .spec.policy field exists for a kubedeschedulers resource
| func (deschedulerV1) ValidateUpdate(_ context.Context, uncastObj runtime.Object, uncastOldObj runtime.Object) field.ErrorList { | ||
| obj, allErrs := toDeschedulerV1(uncastObj) | ||
| if len(allErrs) > 0 { | ||
| return allErrs | ||
| } | ||
| oldObj, allErrs := toDeschedulerV1(uncastOldObj) | ||
| if len(allErrs) > 0 { | ||
| return allErrs | ||
| } | ||
|
|
||
| allErrs = append(allErrs, validation.ValidateObjectMetaUpdate(&obj.ObjectMeta, &oldObj.ObjectMeta, field.NewPath("metadata"))...) | ||
| allErrs = append(allErrs, validateDeschedulerSpec(obj.Spec)...) | ||
|
|
||
| return allErrs | ||
| } | ||
|
|
||
| func (deschedulerV1) ValidateStatusUpdate(_ context.Context, uncastObj runtime.Object, uncastOldObj runtime.Object) field.ErrorList { | ||
| obj, errs := toDeschedulerV1(uncastObj) | ||
| if len(errs) > 0 { | ||
| return errs | ||
| } | ||
| oldObj, errs := toDeschedulerV1(uncastOldObj) | ||
| if len(errs) > 0 { | ||
| return errs | ||
| } | ||
|
|
||
| errs = append(errs, validation.ValidateObjectMetaUpdate(&obj.ObjectMeta, &oldObj.ObjectMeta, field.NewPath("metadata"))...) | ||
| errs = append(errs, validateDeschedulerSpec(obj.Spec)...) | ||
| return errs | ||
| } |
There was a problem hiding this comment.
I'm not sure we need to validate these operations. I think we would only care to validate that the .metadata.name matches cluster on creates.
I believe by default, Kubernetes will enforce immutability on things like custom resource .metadata.name updates.
|
Closing PR as CEL validation ( suggested by @everettraven ) can be used for this problem as seen here --> https://github.com/openshift/api/blob/286504b695bcef7a455f512918b0ee5e5fe6d651/operator/v1/types_olm.go#L22 . |
|
@ehearne-redhat: 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. |
|
@ehearne-redhat: This pull request references Jira Issue OCPBUGS-62726. The bug has been updated to no longer refer to the pull request using the external bug tracker. All external bug links have been closed. The bug has been moved to the NEW state. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
ehearne-redhat
deleted the
OCPBUGS-62726-enforce-name-cluster-kube-desched
branch
3 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
Provides an admission plugin for kube-descheduler instances so that an error is logged when
metadata.nameis notcluster.Which issue(s) this PR is related to:
Fixes https://issues.redhat.com/browse/OCPBUGS-62726 .
Special notes for your reviewer:
This is currently a WIP PR.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: