Retrieve CA from conversion webhooks for CA Hash

awgreene · awgreene · commit 55c630cea496 · 2020-10-07T15:36:37.000-04:00
Problem: If a CSV defines a Conversion Webhook but does not include an
API Service or an Admission Webhook the CSV will cycle through the
Pending, ReadyToInstall, and Installing phases indefinitely.

Cause: When deploying an operator with an API Service, Conversion
Webhook, or an Admission Webhook, OLM should retrieve the CA from an
existing resource in order to calculate a CA Hash annotation. This
annotation influences a Deployment Hash that OLM relies on to confirm
that the deployment is installed correctly. OLM currently does not
retrieve the CA from Conversion Webhooks, resulting in a bad
Deployment Hash which causes OLM to attempt to reinstall the CSV.

Solution: Update OLM so it will use the existing Conversion Webhook to
retrieve that value of the CA and correctly calculate the Deployment
Hash.
diff --git a/pkg/controller/operators/olm/apiservices.go b/pkg/controller/operators/olm/apiservices.go
@@ -265,7 +265,9 @@ func (a *Operator) getCABundle(csv *v1alpha1.ClusterServiceVersion) ([]byte, err
 		webhookLabels := ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind)
 		webhookLabels[install.WebhookDescKey] = desc.GenerateName
 		webhookSelector := labels.SelectorFromSet(webhookLabels).String()
-		if desc.Type == v1alpha1.MutatingAdmissionWebhook {
+
+		switch desc.Type {
+		case v1alpha1.MutatingAdmissionWebhook:
 			existingWebhooks, err := a.opClient.KubernetesInterface().AdmissionregistrationV1().MutatingWebhookConfigurations().List(context.TODO(), metav1.ListOptions{LabelSelector: webhookSelector})
 			if err != nil {
 				return nil, fmt.Errorf("could not retrieve generated MutatingWebhookConfiguration: %v", err)
@@ -274,8 +276,7 @@ func (a *Operator) getCABundle(csv *v1alpha1.ClusterServiceVersion) ([]byte, err
 			if len(existingWebhooks.Items) > 0 {
 				return existingWebhooks.Items[0].Webhooks[0].ClientConfig.CABundle, nil
 			}
-
-		} else if desc.Type == v1alpha1.ValidatingAdmissionWebhook {
+		case v1alpha1.ValidatingAdmissionWebhook:
 			existingWebhooks, err := a.opClient.KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().List(context.TODO(), metav1.ListOptions{LabelSelector: webhookSelector})
 			if err != nil {
 				return nil, fmt.Errorf("could not retrieve generated ValidatingWebhookConfiguration: %v", err)
@@ -284,6 +285,18 @@ func (a *Operator) getCABundle(csv *v1alpha1.ClusterServiceVersion) ([]byte, err
 			if len(existingWebhooks.Items) > 0 {
 				return existingWebhooks.Items[0].Webhooks[0].ClientConfig.CABundle, nil
 			}
+		case v1alpha1.ConversionWebhook:
+			for _, conversionCRD := range desc.ConversionCRDs {
+				// check if CRD exists on cluster
+				crd, err := a.opClient.ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), conversionCRD, metav1.GetOptions{})
+				if err != nil {
+					continue
+				}
+				if crd.Spec.Conversion == nil || crd.Spec.Conversion.Webhook == nil || crd.Spec.Conversion.Webhook.ClientConfig == nil && crd.Spec.Conversion.Webhook.ClientConfig.CABundle == nil {
+					continue
+				}
+				return crd.Spec.Conversion.Webhook.ClientConfig.CABundle, nil
+			}
 		}
 	}
 	return nil, fmt.Errorf("Unable to find ca")
@@ -480,7 +493,6 @@ func (a *Operator) updateDeploymentSpecsWithApiServiceData(csv *v1alpha1.Cluster
 
 func (a *Operator) cleanUpRemovedWebhooks(csv *v1alpha1.ClusterServiceVersion) error {
 	webhookLabels := ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind)
-	webhookLabels = ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind)
 	webhookSelector := labels.SelectorFromSet(webhookLabels).String()
 
 	csvWebhookGenerateNames := make(map[string]struct{}, len(csv.Spec.WebhookDefinitions))
@@ -562,7 +574,7 @@ func (a *Operator) areWebhooksAvailable(csv *v1alpha1.ClusterServiceVersion) (bo
 					return false, err
 				}
 
-				if crd.Spec.Conversion.Strategy != "Webhook" || crd.Spec.Conversion.Webhook == nil || crd.Spec.Conversion.Webhook.ClientConfig == nil && crd.Spec.Conversion.Webhook.ClientConfig.CABundle == nil {
+				if crd.Spec.Conversion == nil || crd.Spec.Conversion.Strategy != "Webhook" || crd.Spec.Conversion.Webhook == nil || crd.Spec.Conversion.Webhook.ClientConfig == nil && crd.Spec.Conversion.Webhook.ClientConfig.CABundle == nil {
 					return false, fmt.Errorf("ConversionWebhook not ready")
 				}
 				webhookCount++
diff --git a/pkg/controller/operators/olm/operator_test.go b/pkg/controller/operators/olm/operator_test.go
@@ -21,6 +21,7 @@ import (
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
@@ -3091,6 +3092,217 @@ func TestTransitionCSV(t *testing.T) {
 	}
 }
 
+func TestCaBundleRetrevial(t *testing.T) {
+	logrus.SetLevel(logrus.DebugLevel)
+	namespace := "ns"
+	missingCAError := fmt.Errorf("Unable to find ca")
+	caBundle := []byte("Foo")
+
+	type initial struct {
+		csvs []runtime.Object
+		crds []runtime.Object
+		objs []runtime.Object
+	}
+	type expected struct {
+		caBundle []byte
+		err      error
+	}
+	tests := []struct {
+		name     string
+		initial  initial
+		expected expected
+	}{
+		{
+			name: "MissingCAResource",
+			initial: initial{
+				csvs: []runtime.Object{
+					csv("csv1",
+						namespace,
+						"0.0.0",
+						"",
+						installStrategy("csv1-dep1",
+							nil,
+							[]v1alpha1.StrategyDeploymentPermissions{},
+						),
+						[]*apiextensionsv1.CustomResourceDefinition{crd("c1", "v1", "g1")},
+						[]*apiextensionsv1.CustomResourceDefinition{},
+						v1alpha1.CSVPhaseInstalling,
+					),
+				},
+			},
+			expected: expected{
+				caBundle: nil,
+				err:      missingCAError,
+			},
+		},
+		{
+			name: "RetrieveCAFromConversionWebhook",
+			initial: initial{
+				csvs: []runtime.Object{
+					csvWithConversionWebhook(csv("csv1",
+						namespace,
+						"0.0.0",
+						"",
+						installStrategy("csv1-dep1",
+							nil,
+							[]v1alpha1.StrategyDeploymentPermissions{},
+						),
+						[]*apiextensionsv1.CustomResourceDefinition{crd("c1", "v1", "g1")},
+						[]*apiextensionsv1.CustomResourceDefinition{},
+						v1alpha1.CSVPhaseInstalling,
+					), "csv1-dep1", []string{"c1.g1"}),
+				},
+				crds: []runtime.Object{
+					crdWithConversionWebhook(crd("c1", "v1", "g1"), caBundle),
+				},
+			},
+			expected: expected{
+				caBundle: caBundle,
+				err:      nil,
+			},
+		},
+		{
+			name: "FailToRetrieveCAFromConversionWebhook",
+			initial: initial{
+				csvs: []runtime.Object{
+					csvWithConversionWebhook(csv("csv1",
+						namespace,
+						"0.0.0",
+						"",
+						installStrategy("csv1-dep1",
+							nil,
+							[]v1alpha1.StrategyDeploymentPermissions{},
+						),
+						[]*apiextensionsv1.CustomResourceDefinition{crd("c1", "v1", "g1")},
+						[]*apiextensionsv1.CustomResourceDefinition{},
+						v1alpha1.CSVPhaseInstalling,
+					), "csv1-dep1", []string{"c1.g1"}),
+				},
+				crds: []runtime.Object{
+					crd("c1", "v1", "g1"),
+				},
+			},
+			expected: expected{
+				caBundle: nil,
+				err:      missingCAError,
+			},
+		},
+		{
+			name: "RetrieveFromValidatingAdmissionWebhook",
+			initial: initial{
+				csvs: []runtime.Object{
+					csvWithValidatingAdmissionWebhook(csv("csv1",
+						namespace,
+						"0.0.0",
+						"",
+						installStrategy("csv1-dep1",
+							nil,
+							[]v1alpha1.StrategyDeploymentPermissions{},
+						),
+						[]*apiextensionsv1.CustomResourceDefinition{crd("c1", "v1", "g1")},
+						[]*apiextensionsv1.CustomResourceDefinition{},
+						v1alpha1.CSVPhaseInstalling,
+					), "csv1-dep1", []string{"c1.g1"}),
+				},
+				objs: []runtime.Object{
+					&admissionregistrationv1.ValidatingWebhookConfiguration{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "webhook",
+							Namespace: namespace,
+							Labels: map[string]string{
+								"olm.owner":                             "csv1",
+								"olm.owner.namespace":                   namespace,
+								"olm.owner.kind":                        v1alpha1.ClusterServiceVersionKind,
+								"olm.webhook-description-generate-name": "",
+							},
+						},
+						Webhooks: []admissionregistrationv1.ValidatingWebhook{
+							{
+								Name: "Webhook",
+								ClientConfig: admissionregistrationv1.WebhookClientConfig{
+									CABundle: caBundle,
+								},
+							},
+						},
+					},
+				},
+			},
+			expected: expected{
+				caBundle: caBundle,
+				err:      nil,
+			},
+		},
+		{
+			name: "RetrieveFromMutatingAdmissionWebhook",
+			initial: initial{
+				csvs: []runtime.Object{
+					csvWithMutatingAdmissionWebhook(csv("csv1",
+						namespace,
+						"0.0.0",
+						"",
+						installStrategy("csv1-dep1",
+							nil,
+							[]v1alpha1.StrategyDeploymentPermissions{},
+						),
+						[]*apiextensionsv1.CustomResourceDefinition{crd("c1", "v1", "g1")},
+						[]*apiextensionsv1.CustomResourceDefinition{},
+						v1alpha1.CSVPhaseInstalling,
+					), "csv1-dep1", []string{"c1.g1"}),
+				},
+				objs: []runtime.Object{
+					&admissionregistrationv1.MutatingWebhookConfiguration{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "webhook",
+							Namespace: namespace,
+							Labels: map[string]string{
+								"olm.owner":                             "csv1",
+								"olm.owner.namespace":                   namespace,
+								"olm.owner.kind":                        v1alpha1.ClusterServiceVersionKind,
+								"olm.webhook-description-generate-name": "",
+							},
+						},
+						Webhooks: []admissionregistrationv1.MutatingWebhook{
+							{
+								Name: "Webhook",
+								ClientConfig: admissionregistrationv1.WebhookClientConfig{
+									CABundle: caBundle,
+								},
+							},
+						},
+					},
+				},
+			},
+			expected: expected{
+				caBundle: caBundle,
+				err:      nil,
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Create test operator
+			ctx, cancel := context.WithCancel(context.TODO())
+			defer cancel()
+			op, err := NewFakeOperator(
+				ctx,
+				withNamespaces(namespace, "kube-system"),
+				withClientObjs(tt.initial.csvs...),
+				withK8sObjs(tt.initial.objs...),
+				withExtObjs(tt.initial.crds...),
+				withOperatorNamespace(namespace),
+			)
+			require.NoError(t, err)
+
+			// run csv sync for each CSV
+			for _, csv := range tt.initial.csvs {
+				caBundle, err := op.getCABundle(csv.(*v1alpha1.ClusterServiceVersion))
+				require.Equal(t, tt.expected.err, err)
+				require.Equal(t, tt.expected.caBundle, caBundle)
+			}
+		})
+	}
+}
+
 // TestUpdates verifies that a set of expected phase transitions occur when multiple CSVs are present
 // and that they do not depend on sync order or event order
 func TestUpdates(t *testing.T) {
@@ -4531,3 +4743,45 @@ func TestAPIServiceResourceErrorActionable(t *testing.T) {
 	}
 
 }
+
+func crdWithConversionWebhook(crd *apiextensionsv1.CustomResourceDefinition, caBundle []byte) *apiextensionsv1.CustomResourceDefinition {
+	crd.Spec.Conversion = &apiextensionsv1.CustomResourceConversion{
+		Strategy: "Webhook",
+		Webhook: &apiextensionsv1.WebhookConversion{
+			ConversionReviewVersions: []string{"v1beta1"},
+			ClientConfig: &apiextensionsv1.WebhookClientConfig{
+				CABundle: caBundle,
+			},
+		},
+	}
+	return crd
+}
+
+func csvWithConversionWebhook(csv *v1alpha1.ClusterServiceVersion, deploymentName string, conversionCRDs []string) *v1alpha1.ClusterServiceVersion {
+	return csvWithWebhook(csv, deploymentName, conversionCRDs, v1alpha1.ConversionWebhook)
+}
+
+func csvWithValidatingAdmissionWebhook(csv *v1alpha1.ClusterServiceVersion, deploymentName string, conversionCRDs []string) *v1alpha1.ClusterServiceVersion {
+	return csvWithWebhook(csv, deploymentName, conversionCRDs, v1alpha1.ValidatingAdmissionWebhook)
+}
+
+func csvWithMutatingAdmissionWebhook(csv *v1alpha1.ClusterServiceVersion, deploymentName string, conversionCRDs []string) *v1alpha1.ClusterServiceVersion {
+	return csvWithWebhook(csv, deploymentName, conversionCRDs, v1alpha1.MutatingAdmissionWebhook)
+}
+
+func csvWithWebhook(csv *v1alpha1.ClusterServiceVersion, deploymentName string, conversionCRDs []string, webhookType v1alpha1.WebhookAdmissionType) *v1alpha1.ClusterServiceVersion {
+	sideEffectNone := admissionregistrationv1.SideEffectClassNone
+	targetPort := intstr.FromInt(443)
+	csv.Spec.WebhookDefinitions = []v1alpha1.WebhookDescription{
+		{
+			Type:                    webhookType,
+			DeploymentName:          deploymentName,
+			ContainerPort:           443,
+			TargetPort:              &targetPort,
+			SideEffects:             &sideEffectNone,
+			ConversionCRDs:          conversionCRDs,
+			AdmissionReviewVersions: []string{"v1beta1"},
+		},
+	}
+	return csv
+}
