Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

[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

Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
replace k8s.io/kube-openapi => k8s.io/kube-openapi v0.0.0-20250318190949-c8a335a9a2ff
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
package descheduler

import (
"context"
"fmt"
"io"

"k8s.io/apimachinery/pkg/api/validation"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/validation/field"
"k8s.io/apiserver/pkg/admission"

configv1 "github.com/openshift/api/config/v1"
deschedulerv1 "github.com/openshift/cluster-kube-descheduler-operator/pkg/apis/descheduler/v1"
"k8s.io/kubernetes/openshift-kube-apiserver/admission/customresourcevalidation"
)

const PluginName = "config.openshift.io/ValidateDescheduler"

Choose a reason for hiding this comment

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

KubeDescheduler is in the operator.openshift.io API group. Might makes sense to have this reflect that as well.

Reference: https://github.com/openshift/cluster-kube-descheduler-operator/blob/ccddc666b5b9607d29aa4f8614267d38e4ba6633/pkg/apis/descheduler/v1/register.go#L16


// Register plugin.
func Register(plugins *admission.Plugins) {
plugins.Register(PluginName, func(config io.Reader) (admission.Interface, error) {
return customresourcevalidation.NewValidator(
map[schema.GroupResource]bool{
configv1.Resource("deschedulers"): true,

Choose a reason for hiding this comment

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

This won't resolve correctly for a couple reasons:

  1. The resource would be kubedeschedulers not deschedulers.
  2. The resource is part of the operator.openshift.io group, not config.openshift.io.

I think instead you probably want:

Suggested change
configv1.Resource("deschedulers"): true,
deschedulerv1.Resource("kubedeschedulers"): true,

},
map[schema.GroupVersionKind]customresourcevalidation.ObjectValidator{
configv1.GroupVersion.WithKind("Descheduler"): deschedulerV1{},

Choose a reason for hiding this comment

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

Similarly to my comment above, you probably want:

Suggested change
configv1.GroupVersion.WithKind("Descheduler"): deschedulerV1{},
deschedulerv1.SchemeGroupVersion.WithKind("KubeDescheduler"): deschedulerV1{},

})
})
}

func toDeschedulerV1(uncastObj runtime.Object) (*deschedulerv1.KubeDescheduler, field.ErrorList) {
if uncastObj == nil {
return nil, nil
}

allErrs := field.ErrorList{}

obj, ok := uncastObj.(*deschedulerv1.KubeDescheduler)

if !ok {
return nil, append(allErrs,
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"}))
Comment on lines +45 to +46

Choose a reason for hiding this comment

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

Suggested change
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"}))

}

return obj, nil
}

type deschedulerV1 struct{}

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))
}
}
Comment on lines +57 to +61

Choose a reason for hiding this comment

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

I don't see where the .spec.policy field exists for a kubedeschedulers resource


return allErrs
}
Comment on lines +54 to +64

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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. :)


func (deschedulerV1) ValidateCreate(_ context.Context, uncastObj runtime.Object) field.ErrorList {
obj, allErrs := toDeschedulerV1(uncastObj)
if len(allErrs) > 0 {
return allErrs
}

allErrs = append(allErrs, validation.ValidateObjectMeta(&obj.ObjectMeta, false, customresourcevalidation.RequireNameCluster, field.NewPath("metadata"))...)
allErrs = append(allErrs, validateDeschedulerSpec(obj.Spec)...)

return allErrs
}

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
}
Comment on lines +78 to +107

Choose a reason for hiding this comment

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

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.