Upgrade k8s.io/* to v0.23, sigs.k8s.io/controller-runtime to v0.11

[timebertt]

How to categorize this issue?

What would you like to be added:

We should upgrade to the latest versions of our go upstream dependencies:

• sigs.k8s.io/controller-runtime@v0.11.x: https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.11.0
• k8s.io/*@v0.23.x
• sigs.k8s.io/controller-tools@v0.8.x: https://github.com/kubernetes-sigs/controller-tools/releases/tag/v0.8.0

Important changes / Action items

Here is a list of a few upstream changes to look out for when vendoring g/g, k/* and c-r in any of our repos (e.g. extensions). Please consider the release notes of controller-runtime@v0.11.0 and upwards as well for a more complete list and more details.

⚠️ breaking / most notable changes

• Update k8s.io/* dependencies to v0.23.0
• Major version bump of logr (v0.4.0 -> v1.2.0)
• Avoid shallow copies of webhooks and CRDs in testenv (⚠ Avoid shallow copies of webhooks and CRDs in testenv kubernetes-sigs/controller-runtime#1667)

ℹ️ other changes / good to know / for information only:

• drop managed fields from audit entries kubernetes/kubernetes#94986 A new field omitManagedFields has been added to both audit.Policy and audit.PolicyRule so cluster operators can opt in to omit managed fields of the request and response bodies from being written to the API audit log.

• Try yet again to add metrics about LIST handling kubernetes/kubernetes#104983 kube-apiserver's Prometheus metrics have been extended

  • consider adding metrics for shoot kube-apiserver ➡️Add metrics about the costs of handling LIST requests #5445

• Add pod os field kubernetes/kubernetes#104693 Introduce OS field in the Pod Spec

  • This is an alpha field and requires the IdentifyPodOS feature

• ✨ Allow webhooks to register custom validators/defaulter types kubernetes-sigs/controller-runtime#1676

  • replace custom coding in seed-admission-controller extensionresources webhook (webhook calling validation code for extension resources) with builder.WebhookBuilder.WithValidator ➡️ Use Custom Validator of c-r for webhook #5782

• Fix workqueue memory leak kubernetes/kubernetes#104991 included in 1.23 might solve gardenlet leaks memory #4609

[rfranzke]

/assign @acumino

[timebertt]

@acumino The API server metrics added in kubernetes/kubernetes#104983 look amazingly useful to me.
Can you open a PR to whitelist these metrics in the Shoot monitoring stack?

gardener/pkg/operation/botanist/component/kubeapiserver/monitoring.go

Line 184 in 29a0c44

monitoringAllowedMetrics = []string{

[acumino]

@timebertt I was checking on this
consider replacing custom coding in seed-admission-controller with builder.WebhookBuilder.{WithDefaulter,WithValidator}.
Seems It's not right and we need to follow our old way, which is using server.Register().

To use builder.WebhookBuilder.{WithDefaulter, WithValidator} it needs to implement admission.{CustomValidator, CustomDefaulter} interface.
If you check, the arguments of the function of that interface it requires a runtime object, and validation/mutation is done on that object.
Also before processing the request, it decodes the object here.

But in g/g seed-admission-controller case here the Handle dependes on multiple resources and not one. So we can't use builder.WebhookBuilder.{WithDefaulter,WithValidator}.

[timebertt]

Thanks for checking the details.
Regarding extensioncrds: you're right the current structure cannot simply be refactored into admission.CustomValidator. And probably it doesn't provide much value there. So we can skip this.
Regarding podschedulername: this is already simple enough and uses as much upstream logic as possible with admission.HandlerFunc.

Regarding extensionresources: this code here could be refactored to use admission.CustomValidator:

gardener/pkg/seedadmissioncontroller/webhooks/admission/extensionresources/admission.go

Lines 51 to 60 in 2beb979

	artifacts := map[metav1.GroupVersionResource]*artifact{
	gvr("backupbuckets"): {
	newObject: func() client.Object { return new(extensionsv1alpha1.BackupBucket) },
	validateCreateResource: func(n, _ client.Object) field.ErrorList {
	return validation.ValidateBackupBucket(n.(*extensionsv1alpha1.BackupBucket))
	},
	validateUpdateResource: func(n, o client.Object) field.ErrorList {
	return validation.ValidateBackupBucketUpdate(n.(*extensionsv1alpha1.BackupBucket), o.(*extensionsv1alpha1.BackupBucket))
	},
	},

I think, this would greatly benefit from it in terms of readability and minimize custom coding.
Basically we can drop this code here in favor of reusing the upstream implementation:

gardener/pkg/seedadmissioncontroller/webhooks/admission/extensionresources/admission.go

Lines 182 to 281 in 2beb979

	type handler struct {
	decoder *admission.Decoder
	logger logr.Logger
	artifacts map[metav1.GroupVersionResource]*artifact
	allowInvalidExtensionResources bool
	}
	var _ admission.Handler = &handler{}
	func (h *handler) InjectDecoder(d *admission.Decoder) error {
	h.decoder = d
	return nil
	}
	func (h *handler) Handle(ctx context.Context, request admission.Request) admission.Response {
	_, cancel := context.WithTimeout(ctx, 10*time.Second)
	defer cancel()
	artifact, ok := h.artifacts[request.Resource]
	if !ok {
	return admission.Allowed("validation not found for the given resource")
	}
	switch request.Operation {
	case admissionv1.Create:
	return h.handleValidation(request, artifact.newObject, artifact.validateCreateResource)
	case admissionv1.Update:
	return h.handleValidation(request, artifact.newObject, artifact.validateUpdateResource)
	default:
	return admission.Allowed("operation is not CREATE or UPDATE")
	}
	}
	type (
	newObjectFunc func() client.Object
	validateFunc func(new, old client.Object) field.ErrorList
	)
	// artifact servers as a helper to setup the corresponding function.
	type artifact struct {
	// newObject is a simple function that creates and returns a new resource.
	newObject newObjectFunc
	// validateCreateResource is a wrapper function for the different create validation functions.
	validateCreateResource validateFunc
	// validateUpdateResource is a wrapper function for the different update validation functions.
	validateUpdateResource validateFunc
	}
	func (h handler) handleValidation(request admission.Request, newObject newObjectFunc, validate validateFunc) admission.Response {
	obj := newObject()
	if err := h.decoder.DecodeRaw(request.Object, obj); err != nil {
	h.logger.Error(err, "Could not decode object", "object", request.Object)
	return admission.Errored(http.StatusUnprocessableEntity, fmt.Errorf("could not decode object %v: %w", request.Object, err))
	}
	h.logger.Info("Validating extension resource", "resource", request.Resource, "name", kutil.ObjectName(obj), "operation", request.Operation)
	var oldObj client.Object
	if len(request.OldObject.Raw) != 0 {
	oldObj = newObject()
	if err := h.decoder.DecodeRaw(request.OldObject, oldObj); err != nil {
	h.logger.Error(err, "Could not decode old object", "oldObject", oldObj)
	return admission.Errored(http.StatusBadRequest, fmt.Errorf("could not decode old object %v: %v", oldObj, err))
	}
	}
	errors := validate(obj, oldObj)
	if len(errors) != 0 {
	err := apierrors.NewInvalid(schema.GroupKind{
	Group: request.Kind.Group,
	Kind: request.Kind.Kind,
	}, kutil.ObjectName(obj), errors)
	h.logger.Info("Invalid extension resource detected", "operation", request.Operation, "error", err.Error())
	if h.allowInvalidExtensionResources {
	return admission.Allowed(err.Error())
	}
	return admission.Denied(err.Error())
	}
	return admission.Allowed("validation successful")
	}
	func gvr(resource string) metav1.GroupVersionResource {
	return metav1.GroupVersionResource{
	Group: extensionsv1alpha1.SchemeGroupVersion.Group,
	Version: extensionsv1alpha1.SchemeGroupVersion.Version,
	Resource: resource,
	}
	}
	func gvrDruid(resource string) metav1.GroupVersionResource {
	return metav1.GroupVersionResource{
	Group: druidv1alpha1.GroupVersion.Group,
	Version: druidv1alpha1.GroupVersion.Version,
	Resource: resource,
	}
	}

[acumino]

In case of extensionresources also I think the current implementation is simple, since in extensionresource also we are dealing with multiple gvr if we want to follow admission.CustomValidator we have to implement it for every gvr and all of them need to be added to manager separately this will make our code bulkier only. The current approach have a little custom coding but I think this is a better implementation. WDYT?

[timebertt]

Hmm, the current approach does have quite some custom coding, see the second code block I referenced. Basically half the file can be dropped by reusing upstream implementations.
Setting the webhook for every GVK on the other hand is very simple, doesn't increase maintenance burden and is easy to understand. So I see a clear advantage of refactoring this webhook, do you agree?

[acumino]

The implementation will be mostly easy only. But I feel, we are not going to have many benefits from it, I can open a PR in the coming days, and then we can look if we want the newer way.

[acumino]

One more hurdle which I missed previously is that the CustomValidator returns error check here whereas our functions returns field.ErrorList check here.
So to adapt CustomValidator we need to rewrite our validation function i.e. whenever one field is invalid return error at that place only instead of listing down all errors and returning that as field.ErrorList which is our current approach.

[acumino]

One more hurdle which I missed previously is that the CustomValidator returns error check here whereas our functions returns field.ErrorList check here. So to adapt CustomValidator we need to rewrite our validation function i.e. whenever one field is invalid return error at that place only instead of listing down all errors and returning that as field.ErrorList which is our current approach.

@timebertt Can you take a look here?

[timebertt]

You can call ToAggregate() on the ErrorList:

gardener/vendor/k8s.io/apimachinery/pkg/util/validation/field/errors.go

Line 241 in 0296476

func (list ErrorList) ToAggregate() utilerrors.Aggregate {

Then you don't need to change the validation code and can simply call it in the CustomValidator implementation.

[acumino]

/close
All required tasks have been completed.

[gardener-prow]

@acumino: Closing this issue.

In response to this:

/close
All required tasks have been completed.

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.