Merge pull request #870 from Davoska/OCPBUGS-702-the-cabundle-field-o…

…f-alertmanagerconfigs.monitoring.coreos.com-crd-is-getting-removed

OCPBUGS-702: Fix removing `caBundle` field of CRDs when `...inject-cabundle=true`

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{})

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
 	}
 }

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)
+}

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"