Policy merging discrepancy between Kubernetes and Universal with defaults

[michaelbeaumont]

What happened?

We use the kubebuilder:default annotation to generate an OpenAPI spec describing our resources. When we create our CRDs with these OpenAPI specs, Kubernetes then sets the value of omitted fields to the default. On Universal, we don't do the same, this part of the spec serves only as documentation

This means:

• Merging won't work as intended. Given a kubebuilder:default=30s for interval:

---
targetRef:
kind: Mesh
to:
  - default:
      interval: 100s
      timeout: 200s
---
targetRef:
  kind: MeshService
  name: backend
to:
  - default:
      timeout: 300s

on Universal we get {interval: 100s, timeout: 300s} but on Kubernetes we get {interval: 30s, timeout: 300s} because the API server sets interval: 30s on the second resource.

• when writing policy implementations, we have to duplicate the default everywhere we use the fields for the universal case, even though on Kubernetes they'll always be set

[github-actions]

This issue was inactive for 90 days. It will be reviewed in the next triage meeting and might be closed.
If you think this issue is still relevant, please comment on it or attend the next triage meeting.

[github-actions]

This issue was inactive for 90 days. It will be reviewed in the next triage meeting and might be closed.
If you think this issue is still relevant, please comment on it or attend the next triage meeting.

[github-actions]

This issue was inactive for 90 days. It will be reviewed in the next triage meeting and might be closed.
If you think this issue is still relevant, please comment on it or attend the next triage meeting.

[github-actions]

This issue was inactive for 90 days. It will be reviewed in the next triage meeting and might be closed.
If you think this issue is still relevant, please comment on it or attend the next triage meeting.

[github-actions]

This issue was inactive for 90 days. It will be reviewed in the next triage meeting and might be closed.
If you think this issue is still relevant, please comment on it or attend the next triage meeting.

[lahabana]

Looking at the code we have a Defaulter interface that does this fairly well:

kuma/pkg/core/resources/manager/manager.go

Lines 59 to 66 in f5f63bd

	if defaulter, ok := resource.(model.Defaulter); ok {
	if err := defaulter.Default(); err != nil {
	return err
	}
	}
	if err := model.Validate(resource); err != nil {
	return err
	}

We should likely make it work in a similar way as Validate in the generator (if there's a validate method do something) we could then auto generate the defaulter that use kubebuilder annotations.

If we are to do this and always rely on kubebuilder annotations for defaults we should remove this:

kuma/pkg/plugins/runtime/k8s/webhooks/defaulter.go

Lines 53 to 58 in e590c3d

	if defaulter, ok := resource.(core_model.Defaulter); ok {
	if err := defaulter.Default(); err != nil {
	return admission.Errored(http.StatusInternalServerError, err)
	}
	}

and fix MeshService and Mesh which are the 2 existing implems of Defaulter

The other alternative is to disallow the use of kubebuilder:default but this is unfortunate as this generates nice API specs (.e.g: this annotation generates this openAPI spec).

This also seems to be in the openAPI spec, can't we just use openAPI magic to have the defaults ensured? A bit like we do in:

kuma/pkg/core/resources/model/rest/unmarshaller.go

Line 75 in ce2ce56

validator := validate.NewSchemaValidator(desc.Schema, nil, "", strfmt.Default)

[michaelbeaumont]

This also seems to be in the openAPI spec, can't we just use openAPI magic to have the defaults ensured? A bit like we do in:

yeah this was the original plan I had in mind at least