diff --git a/lib/resourceapply/apiext.go b/lib/resourceapply/apiext.go index 13d6f6b9d..322cc5984 100644 --- a/lib/resourceapply/apiext.go +++ b/lib/resourceapply/apiext.go @@ -28,14 +28,20 @@ func ApplyCustomResourceDefinitionv1(ctx context.Context, client apiextclientv1. return nil, false, nil } - modified := pointer.BoolPtr(false) + var original apiextv1.CustomResourceDefinition + existing.DeepCopyInto(&original) + + modified := pointer.Bool(false) resourcemerge.EnsureCustomResourceDefinitionV1(modified, existing, *required) if !*modified { return existing, false, nil } - if reconciling { - klog.V(2).Infof("Updating CRD %s due to diff: %v", required.Name, cmp.Diff(existing, required)) + if diff := cmp.Diff(&original, existing); diff != "" { + klog.V(2).Infof("Updating CRD %s due to diff: %v", required.Name, diff) + } else { + klog.V(2).Infof("Updating CRD %s with empty diff: possible hotloop after wrong comparison", required.Name) + } } actual, err := client.CustomResourceDefinitions().Update(ctx, existing, metav1.UpdateOptions{}) diff --git a/lib/resourcemerge/apiext.go b/lib/resourcemerge/apiext.go index f2d745958..c384e2eab 100644 --- a/lib/resourcemerge/apiext.go +++ b/lib/resourcemerge/apiext.go @@ -12,18 +12,45 @@ import ( // modified is set to true when existing had to be updated with required. func EnsureCustomResourceDefinitionV1(modified *bool, existing *apiextv1.CustomResourceDefinition, required apiextv1.CustomResourceDefinition) { EnsureObjectMeta(modified, &existing.ObjectMeta, required.ObjectMeta) + ensureCustomResourceDefinitionV1Defaults(&required) + ensureCustomResourceDefinitionV1CaBundle(&required, *existing) + if !equality.Semantic.DeepEqual(existing.Spec, required.Spec) { + *modified = true + existing.Spec = required.Spec + } +} - // apply defaults to blue print +func ensureCustomResourceDefinitionV1Defaults(required *apiextv1.CustomResourceDefinition) { if len(required.Spec.Names.Singular) == 0 { required.Spec.Names.Singular = strings.ToLower(required.Spec.Names.Kind) } if len(required.Spec.Names.ListKind) == 0 { required.Spec.Names.ListKind = required.Spec.Names.Kind + "List" } +} - // we stomp everything - if !equality.Semantic.DeepEqual(existing.Spec, required.Spec) { - *modified = true - existing.Spec = required.Spec +// ensureCustomResourceDefinitionV1CaBundle ensures that the field +// spec.Conversion.Webhook.ClientConfig.CABundle of a CRD is not managed by the CVO when +// the service-ca controller is responsible for the field. +func ensureCustomResourceDefinitionV1CaBundle(required *apiextv1.CustomResourceDefinition, existing apiextv1.CustomResourceDefinition) { + if val, ok := existing.ObjectMeta.Annotations[injectCABundleAnnotation]; !ok || val != "true" { + return + } + req := required.Spec.Conversion + if req == nil || + req.Webhook == nil || + req.Webhook.ClientConfig == nil { + return + } + if req.Strategy != apiextv1.WebhookConverter { + // The service CA bundle is only injected by the service-ca controller into + // the CRD if the CRD is configured to use a webhook for conversion + return + } + exc := existing.Spec.Conversion + if exc != nil && + exc.Webhook != nil && + exc.Webhook.ClientConfig != nil { + req.Webhook.ClientConfig.CABundle = exc.Webhook.ClientConfig.CABundle } } diff --git a/lib/resourcemerge/apiext_test.go b/lib/resourcemerge/apiext_test.go new file mode 100644 index 000000000..71e0e445b --- /dev/null +++ b/lib/resourcemerge/apiext_test.go @@ -0,0 +1,191 @@ +package resourcemerge + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + + "k8s.io/apimachinery/pkg/api/equality" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/pointer" +) + +func TestEnsureCustomResourceDefinitionV1(t *testing.T) { + tests := []struct { + name string + existing apiextv1.CustomResourceDefinition + required apiextv1.CustomResourceDefinition + + expectedModified bool + expected apiextv1.CustomResourceDefinition + }{ + { + name: "respect injected caBundle when the annotation `...inject-cabundle=true` is set", + existing: apiextv1.CustomResourceDefinition{ + ObjectMeta: v1.ObjectMeta{ + Annotations: map[string]string{ + injectCABundleAnnotation: "true", + }, + }, + Spec: apiextv1.CustomResourceDefinitionSpec{ + Conversion: &apiextv1.CustomResourceConversion{ + Strategy: apiextv1.WebhookConverter, + Webhook: &apiextv1.WebhookConversion{ + ClientConfig: &apiextv1.WebhookClientConfig{ + CABundle: []byte("CA bundle added by the ca operator"), + }, + }, + }, + }, + }, + required: apiextv1.CustomResourceDefinition{ + ObjectMeta: v1.ObjectMeta{ + Annotations: map[string]string{ + injectCABundleAnnotation: "true", + }, + }, + Spec: apiextv1.CustomResourceDefinitionSpec{ + Conversion: &apiextv1.CustomResourceConversion{ + Strategy: apiextv1.WebhookConverter, + Webhook: &apiextv1.WebhookConversion{ + ClientConfig: &apiextv1.WebhookClientConfig{ + CABundle: nil, + }, + }, + }, + }, + }, + + expectedModified: false, + expected: apiextv1.CustomResourceDefinition{ + ObjectMeta: v1.ObjectMeta{ + Annotations: map[string]string{ + injectCABundleAnnotation: "true", + }, + }, + Spec: apiextv1.CustomResourceDefinitionSpec{ + Conversion: &apiextv1.CustomResourceConversion{ + Strategy: apiextv1.WebhookConverter, + Webhook: &apiextv1.WebhookConversion{ + ClientConfig: &apiextv1.WebhookClientConfig{ + CABundle: []byte("CA bundle added by the ca operator"), + }, + }, + }, + }}, + }, + { + name: "respect injected caBundle when the annotation `...inject-cabundle=true` is set by the user", + existing: apiextv1.CustomResourceDefinition{ + ObjectMeta: v1.ObjectMeta{ + Annotations: map[string]string{ + injectCABundleAnnotation: "true", + }, + }, + Spec: apiextv1.CustomResourceDefinitionSpec{ + Conversion: &apiextv1.CustomResourceConversion{ + Strategy: apiextv1.WebhookConverter, + Webhook: &apiextv1.WebhookConversion{ + ClientConfig: &apiextv1.WebhookClientConfig{ + CABundle: []byte("CA bundle added by the ca operator"), + }, + }, + }, + }, + }, + required: apiextv1.CustomResourceDefinition{ + Spec: apiextv1.CustomResourceDefinitionSpec{ + Conversion: &apiextv1.CustomResourceConversion{ + Strategy: apiextv1.WebhookConverter, + Webhook: &apiextv1.WebhookConversion{ + ClientConfig: &apiextv1.WebhookClientConfig{ + CABundle: nil, + }, + }, + }, + }, + }, + + expectedModified: false, + expected: apiextv1.CustomResourceDefinition{ + ObjectMeta: v1.ObjectMeta{ + Annotations: map[string]string{ + injectCABundleAnnotation: "true", + }, + }, + Spec: apiextv1.CustomResourceDefinitionSpec{ + Conversion: &apiextv1.CustomResourceConversion{ + Strategy: apiextv1.WebhookConverter, + Webhook: &apiextv1.WebhookConversion{ + ClientConfig: &apiextv1.WebhookClientConfig{ + CABundle: []byte("CA bundle added by the ca operator"), + }, + }, + }, + }}, + }, + { + name: "remove injected caBundle when the annotation `...inject-cabundle=true` is not set", + existing: apiextv1.CustomResourceDefinition{ + Spec: apiextv1.CustomResourceDefinitionSpec{ + Conversion: &apiextv1.CustomResourceConversion{ + Strategy: apiextv1.WebhookConverter, + Webhook: &apiextv1.WebhookConversion{ + ClientConfig: &apiextv1.WebhookClientConfig{ + CABundle: []byte("CA bundle added by the user"), + }, + }, + }, + }, + }, + required: apiextv1.CustomResourceDefinition{ + Spec: apiextv1.CustomResourceDefinitionSpec{ + Conversion: &apiextv1.CustomResourceConversion{ + Strategy: apiextv1.WebhookConverter, + Webhook: &apiextv1.WebhookConversion{ + ClientConfig: &apiextv1.WebhookClientConfig{ + CABundle: nil, + }, + }, + }, + }, + }, + + expectedModified: true, + expected: apiextv1.CustomResourceDefinition{ + Spec: apiextv1.CustomResourceDefinitionSpec{ + Conversion: &apiextv1.CustomResourceConversion{ + Strategy: apiextv1.WebhookConverter, + Webhook: &apiextv1.WebhookConversion{ + ClientConfig: &apiextv1.WebhookClientConfig{ + CABundle: nil, + }, + }, + }, + }}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + defaultCustomResourceDefinitionV1(&test.existing, test.existing) + defaultCustomResourceDefinitionV1(&test.expected, test.expected) + modified := pointer.Bool(false) + EnsureCustomResourceDefinitionV1(modified, &test.existing, test.required) + if *modified != test.expectedModified { + t.Errorf("mismatch modified got: %v want: %v", *modified, test.expectedModified) + } + + if !equality.Semantic.DeepEqual(test.existing, test.expected) { + t.Errorf("unexpected: %s", cmp.Diff(test.expected, test.existing)) + } + }) + } +} + +// Ensures the structure contains any defaults not explicitly set by the test +func defaultCustomResourceDefinitionV1(in *apiextv1.CustomResourceDefinition, from apiextv1.CustomResourceDefinition) { + modified := pointer.Bool(false) + EnsureCustomResourceDefinitionV1(modified, in, from) +} diff --git a/lib/resourcemerge/helper.go b/lib/resourcemerge/helper.go new file mode 100644 index 000000000..c6b4fe2b2 --- /dev/null +++ b/lib/resourcemerge/helper.go @@ -0,0 +1,5 @@ +package resourcemerge + +// injectCABundleAnnotation is the annotation used to indicate into which resources +// the service-ca controller should inject the CA bundle. +const injectCABundleAnnotation = "service.beta.openshift.io/inject-cabundle"