Merge pull request #1761 from awgreene/reusue-active-certs

Bug 1868712: OLM should reuse existing CA if they have not expired

Author: openshift-merge-robot

pkg/controller/install/certresources.go

@@ -27,6 +27,8 @@ const (
 	DefaultCertMinFresh = time.Hour * 24
 	// DefaultCertValidFor is the default duration a cert can be valid for - 2 years
 	DefaultCertValidFor = time.Hour * 24 * 730
+	// OLMCAPEMKey is the CAPEM
+	OLMCAPEMKey = "olmCAKey"
 	// OLMCAHashAnnotationKey is the label key used to store the hash of the CA cert
 	OLMCAHashAnnotationKey = "olmcahash"
 	// Organization is the organization name used in the generation of x509 certs
@@ -185,25 +187,28 @@ func (i *StrategyDeploymentInstaller) installCertRequirements(strategy Strategy)
 		}
 
 		// Update the deployment for each certResource
-		newDepSpec, err := i.installCertRequirementsForDeployment(sddSpec.Name, ca, rotateAt, sddSpec.Spec, getServicePorts(certResources))
+		newDepSpec, caPEM, err := i.installCertRequirementsForDeployment(sddSpec.Name, ca, rotateAt, sddSpec.Spec, getServicePorts(certResources))
 		if err != nil {
 			return nil, err
 		}
 
-		caPEM, _, err := ca.ToPEM()
-		if err != nil {
-			logger.Warnf("unable to convert CA certificate to PEM format for Deployment %s", sddSpec.Name)
-			return nil, err
-		}
-
 		i.updateCertResourcesForDeployment(sddSpec.Name, caPEM)
 
 		strategyDetailsDeployment.DeploymentSpecs[n].Spec = *newDepSpec
 	}
 	return strategyDetailsDeployment, nil
 }
 
-func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deploymentName string, ca *certs.KeyPair, rotateAt time.Time, depSpec appsv1.DeploymentSpec, ports []corev1.ServicePort) (*appsv1.DeploymentSpec, error) {
+func ShouldRotateCerts(csv *v1alpha1.ClusterServiceVersion) bool {
+	now := metav1.Now()
+	if !csv.Status.CertsRotateAt.IsZero() && csv.Status.CertsRotateAt.Before(&now) {
+		return true
+	}
+
+	return false
+}
+
+func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deploymentName string, ca *certs.KeyPair, rotateAt time.Time, depSpec appsv1.DeploymentSpec, ports []corev1.ServicePort) (*appsv1.DeploymentSpec, []byte, error) {
 	logger := log.WithFields(log.Fields{})
 
 	// Create a service for the deployment
@@ -215,27 +220,27 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
 	}
 	service.SetName(ServiceName(deploymentName))
 	service.SetNamespace(i.owner.GetNamespace())
+	ownerutil.AddNonBlockingOwner(service, i.owner)
 
 	existingService, err := i.strategyClient.GetOpLister().CoreV1().ServiceLister().Services(i.owner.GetNamespace()).Get(service.GetName())
 	if err == nil {
 		if !ownerutil.Adoptable(i.owner, existingService.GetOwnerReferences()) {
-			return nil, fmt.Errorf("service %s not safe to replace: extraneous ownerreferences found", service.GetName())
+			return nil, nil, fmt.Errorf("service %s not safe to replace: extraneous ownerreferences found", service.GetName())
 		}
 		service.SetOwnerReferences(existingService.GetOwnerReferences())
 
 		// Delete the Service to replace
 		deleteErr := i.strategyClient.GetOpClient().DeleteService(service.GetNamespace(), service.GetName(), &metav1.DeleteOptions{})
 		if err != nil && !k8serrors.IsNotFound(deleteErr) {
-			return nil, fmt.Errorf("could not delete existing service %s", service.GetName())
+			return nil, nil, fmt.Errorf("could not delete existing service %s", service.GetName())
 		}
 	}
-	ownerutil.AddNonBlockingOwner(service, i.owner)
 
 	// Attempt to create the Service
 	_, err = i.strategyClient.GetOpClient().CreateService(service)
 	if err != nil {
 		logger.Warnf("could not create service %s", service.GetName())
-		return nil, fmt.Errorf("could not create service %s: %s", service.GetName(), err.Error())
+		return nil, nil, fmt.Errorf("could not create service %s: %s", service.GetName(), err.Error())
 	}
 
 	// Create signed serving cert
@@ -246,33 +251,34 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
 	servingPair, err := certs.CreateSignedServingPair(rotateAt, Organization, ca, hosts)
 	if err != nil {
 		logger.Warnf("could not generate signed certs for hosts %v", hosts)
-		return nil, err
+		return nil, nil, err
 	}
 
 	// Create Secret for serving cert
 	certPEM, privPEM, err := servingPair.ToPEM()
 	if err != nil {
 		logger.Warnf("unable to convert serving certificate and private key to PEM format for Service %s", service.GetName())
-		return nil, err
+		return nil, nil, err
 	}
 
+	// Add olmcahash as a label to the caPEM
+	caPEM, _, err := ca.ToPEM()
+	if err != nil {
+		logger.Warnf("unable to convert CA certificate to PEM format for Service %s", service)
+		return nil, nil, err
+	}
+	caHash := certs.PEMSHA256(caPEM)
+
 	secret := &corev1.Secret{
 		Data: map[string][]byte{
-			"tls.crt": certPEM,
-			"tls.key": privPEM,
+			"tls.crt":   certPEM,
+			"tls.key":   privPEM,
+			OLMCAPEMKey: caPEM,
 		},
 		Type: corev1.SecretTypeTLS,
 	}
 	secret.SetName(SecretName(service.GetName()))
 	secret.SetNamespace(i.owner.GetNamespace())
-
-	// Add olmcahash as a label to the caPEM
-	caPEM, _, err := ca.ToPEM()
-	if err != nil {
-		logger.Warnf("unable to convert CA certificate to PEM format for Service %s", service)
-		return nil, err
-	}
-	caHash := certs.PEMSHA256(caPEM)
 	secret.SetAnnotations(map[string]string{OLMCAHashAnnotationKey: caHash})
 
 	existingSecret, err := i.strategyClient.GetOpLister().CoreV1().SecretLister().Secrets(i.owner.GetNamespace()).Get(secret.GetName())
@@ -283,20 +289,27 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
 		}
 
 		// Attempt an update
-		if _, err := i.strategyClient.GetOpClient().UpdateSecret(secret); err != nil {
+		// TODO: Check that the secret was not modified
+		if existingCAPEM, ok := existingSecret.Data[OLMCAPEMKey]; ok && !ShouldRotateCerts(i.owner.(*v1alpha1.ClusterServiceVersion)) {
+			logger.Warnf("reusing existing cert %s", secret.GetName())
+			secret = existingSecret
+			caPEM = existingCAPEM
+			caHash = certs.PEMSHA256(caPEM)
+		} else if _, err := i.strategyClient.GetOpClient().UpdateSecret(secret); err != nil {
 			logger.Warnf("could not update secret %s", secret.GetName())
-			return nil, err
+			return nil, nil, err
 		}
+
 	} else if k8serrors.IsNotFound(err) {
 		// Create the secret
 		ownerutil.AddNonBlockingOwner(secret, i.owner)
 		_, err = i.strategyClient.GetOpClient().CreateSecret(secret)
 		if err != nil {
 			log.Warnf("could not create secret %s", secret.GetName())
-			return nil, err
+			return nil, nil, err
 		}
 	} else {
-		return nil, err
+		return nil, nil, err
 	}
 
 	// create Role and RoleBinding to allow the deployment to mount the Secret
@@ -323,18 +336,18 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
 		// Attempt an update
 		if _, err := i.strategyClient.GetOpClient().UpdateRole(secretRole); err != nil {
 			logger.Warnf("could not update secret role %s", secretRole.GetName())
-			return nil, err
+			return nil, nil, err
 		}
 	} else if k8serrors.IsNotFound(err) {
 		// Create the role
 		ownerutil.AddNonBlockingOwner(secretRole, i.owner)
 		_, err = i.strategyClient.GetOpClient().CreateRole(secretRole)
 		if err != nil {
 			log.Warnf("could not create secret role %s", secretRole.GetName())
-			return nil, err
+			return nil, nil, err
 		}
 	} else {
-		return nil, err
+		return nil, nil, err
 	}
 
 	if depSpec.Template.Spec.ServiceAccountName == "" {
@@ -369,18 +382,18 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
 		// Attempt an update
 		if _, err := i.strategyClient.GetOpClient().UpdateRoleBinding(secretRoleBinding); err != nil {
 			logger.Warnf("could not update secret rolebinding %s", secretRoleBinding.GetName())
-			return nil, err
+			return nil, nil, err
 		}
 	} else if k8serrors.IsNotFound(err) {
 		// Create the role
 		ownerutil.AddNonBlockingOwner(secretRoleBinding, i.owner)
 		_, err = i.strategyClient.GetOpClient().CreateRoleBinding(secretRoleBinding)
 		if err != nil {
 			log.Warnf("could not create secret rolebinding with dep spec: %#v", depSpec)
-			return nil, err
+			return nil, nil, err
 		}
 	} else {
-		return nil, err
+		return nil, nil, err
 	}
 
 	// create ClusterRoleBinding to system:auth-delegator Role
@@ -407,27 +420,27 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
 		if ownerutil.AdoptableLabels(existingAuthDelegatorClusterRoleBinding.GetLabels(), true, i.owner) {
 			logger.WithFields(log.Fields{"obj": "authDelegatorCRB", "labels": existingAuthDelegatorClusterRoleBinding.GetLabels()}).Debug("adopting")
 			if err := ownerutil.AddOwnerLabels(authDelegatorClusterRoleBinding, i.owner); err != nil {
-				return nil, err
+				return nil, nil, err
 			}
 		}
 
 		// Attempt an update.
 		if _, err := i.strategyClient.GetOpClient().UpdateClusterRoleBinding(authDelegatorClusterRoleBinding); err != nil {
 			logger.Warnf("could not update auth delegator clusterrolebinding %s", authDelegatorClusterRoleBinding.GetName())
-			return nil, err
+			return nil, nil, err
 		}
 	} else if k8serrors.IsNotFound(err) {
 		// Create the role.
 		if err := ownerutil.AddOwnerLabels(authDelegatorClusterRoleBinding, i.owner); err != nil {
-			return nil, err
+			return nil, nil, err
 		}
 		_, err = i.strategyClient.GetOpClient().CreateClusterRoleBinding(authDelegatorClusterRoleBinding)
 		if err != nil {
 			log.Warnf("could not create auth delegator clusterrolebinding %s", authDelegatorClusterRoleBinding.GetName())
-			return nil, err
+			return nil, nil, err
 		}
 	} else {
-		return nil, err
+		return nil, nil, err
 	}
 
 	// Create RoleBinding to extension-apiserver-authentication-reader Role in the kube-system namespace.
@@ -455,26 +468,26 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
 		if ownerutil.AdoptableLabels(existingAuthReaderRoleBinding.GetLabels(), true, i.owner) {
 			logger.WithFields(log.Fields{"obj": "existingAuthReaderRB", "labels": existingAuthReaderRoleBinding.GetLabels()}).Debug("adopting")
 			if err := ownerutil.AddOwnerLabels(authReaderRoleBinding, i.owner); err != nil {
-				return nil, err
+				return nil, nil, err
 			}
 		}
 		// Attempt an update.
 		if _, err := i.strategyClient.GetOpClient().UpdateRoleBinding(authReaderRoleBinding); err != nil {
 			logger.Warnf("could not update auth reader role binding %s", authReaderRoleBinding.GetName())
-			return nil, err
+			return nil, nil, err
 		}
 	} else if k8serrors.IsNotFound(err) {
 		// Create the role.
 		if err := ownerutil.AddOwnerLabels(authReaderRoleBinding, i.owner); err != nil {
-			return nil, err
+			return nil, nil, err
 		}
 		_, err = i.strategyClient.GetOpClient().CreateRoleBinding(authReaderRoleBinding)
 		if err != nil {
 			log.Warnf("could not create auth reader role binding %s", authReaderRoleBinding.GetName())
-			return nil, err
+			return nil, nil, err
 		}
 	} else {
-		return nil, err
+		return nil, nil, err
 	}
 
 	// Update deployment with secret volume mount.
@@ -539,5 +552,5 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
 	// is used by the apiserver if not hot reloading.
 	depSpec.Template.ObjectMeta.SetAnnotations(map[string]string{OLMCAHashAnnotationKey: caHash})
 
-	return &depSpec, nil
+	return &depSpec, caPEM, nil
 }

pkg/controller/operators/olm/apiservices.go

@@ -25,15 +25,6 @@ const (
 	PackageserverName = "v1.packages.operators.coreos.com"
 )
 
-func (a *Operator) shouldRotateCerts(csv *v1alpha1.ClusterServiceVersion) bool {
-	now := metav1.Now()
-	if !csv.Status.CertsRotateAt.IsZero() && csv.Status.CertsRotateAt.Before(&now) {
-		return true
-	}
-
-	return false
-}
-
 // apiServiceResourceErrorActionable returns true if OLM can do something about any one
 // of the apiService errors in errs; otherwise returns false
 //

pkg/controller/operators/olm/operator.go

@@ -1564,7 +1564,7 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
 		}
 
 		// Check if it's time to refresh owned APIService certs
-		if a.shouldRotateCerts(out) {
+		if install.ShouldRotateCerts(out) {
 			out.SetPhaseWithEvent(v1alpha1.CSVPhasePending, v1alpha1.CSVReasonNeedsCertRotation, "owned APIServices need cert refresh", now, a.recorder)
 			return
 		}
@@ -1654,7 +1654,7 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
 		}
 
 		// Check if it's time to refresh owned APIService certs
-		if a.shouldRotateCerts(out) {
+		if install.ShouldRotateCerts(out) {
 			out.SetPhaseWithEvent(v1alpha1.CSVPhasePending, v1alpha1.CSVReasonNeedsCertRotation, "owned APIServices need cert refresh", now, a.recorder)
 			return
 		}

test/e2e/webhook_e2e_test.go

@@ -177,6 +177,57 @@ var _ = Describe("CSVs with a Webhook", func() {
 				return true
 			}, time.Minute, 5*time.Second).Should(BeTrue())
 		})
+		It("Reuses existing valid certs", func() {
+			sideEffect := admissionregistrationv1.SideEffectClassNone
+			webhook := v1alpha1.WebhookDescription{
+				GenerateName:            webhookName,
+				Type:                    v1alpha1.ValidatingAdmissionWebhook,
+				DeploymentName:          genName("webhook-dep-"),
+				ContainerPort:           443,
+				AdmissionReviewVersions: []string{"v1beta1", "v1"},
+				SideEffects:             &sideEffect,
+			}
+			csv := createCSVWithWebhook(namespace.GetName(), webhook)
+
+			var err error
+			cleanupCSV, err = createCSV(c, crc, csv, namespace.GetName(), false, false)
+			Expect(err).Should(BeNil())
+
+			_, err = fetchCSV(crc, csv.Name, namespace.GetName(), csvSucceededChecker)
+			Expect(err).Should(BeNil())
+
+			// Get the existing secret
+			webhookSecretName := webhook.DeploymentName + "-service-cert"
+			existingSecret, err := c.KubernetesInterface().CoreV1().Secrets(namespace.GetName()).Get(context.TODO(), webhookSecretName, metav1.GetOptions{})
+			require.NoError(GinkgoT(), err)
+
+			// Modify the phase
+			Eventually(func() bool {
+				fetchedCSV, err := crc.OperatorsV1alpha1().ClusterServiceVersions(namespace.GetName()).Get(context.TODO(), csv.GetName(), metav1.GetOptions{})
+				if err != nil {
+					return false
+				}
+
+				fetchedCSV.Status.Phase = v1alpha1.CSVPhasePending
+
+				_, err = crc.OperatorsV1alpha1().ClusterServiceVersions(namespace.GetName()).UpdateStatus(context.TODO(), fetchedCSV, metav1.UpdateOptions{})
+				if err != nil {
+					return false
+				}
+				return true
+			}).Should(BeTrue(), "Unable to set CSV phase to Pending")
+
+			// Wait for webhook-operator to succeed
+			_, err = awaitCSV(GinkgoT(), crc, namespace.GetName(), csv.GetName(), csvSucceededChecker)
+			require.NoError(GinkgoT(), err)
+
+			// Get the updated secret
+			updatedSecret, err := c.KubernetesInterface().CoreV1().Secrets(namespace.GetName()).Get(context.TODO(), webhookSecretName, metav1.GetOptions{})
+			require.NoError(GinkgoT(), err)
+
+			require.Equal(GinkgoT(), existingSecret.GetAnnotations()[install.OLMCAHashAnnotationKey], updatedSecret.GetAnnotations()[install.OLMCAHashAnnotationKey])
+			require.Equal(GinkgoT(), existingSecret.Data[install.OLMCAPEMKey], updatedSecret.Data[install.OLMCAPEMKey])
+		})
 		It("Fails to install a CSV if multiple Webhooks share the same name", func() {
 			sideEffect := admissionregistrationv1.SideEffectClassNone
 			webhook := v1alpha1.WebhookDescription{