Refactor installer to surface certificate rotation

pkg/controller/install/certresources.go

@@ -160,7 +160,7 @@ func (i *StrategyDeploymentInstaller) getCertResources() []certResource {
 }
 
 func (i *StrategyDeploymentInstaller) certResourcesForDeployment(deploymentName string) []certResource {
-	result := []certResource{}
+	var result []certResource
 	for _, desc := range i.getCertResources() {
 		if desc.getDeploymentName() == deploymentName {
 			result = append(result, desc)
@@ -185,8 +185,8 @@ func (i *StrategyDeploymentInstaller) installCertRequirements(strategy Strategy)
 	}
 
 	// Create the CA
-	expiration, _ := CalculateCertExpirationAndRotateAt()
-	ca, err := certs.GenerateCA(expiration, Organization)
+	i.certificateExpirationTime = CalculateCertExpiration(time.Now())
+	ca, err := certs.GenerateCA(i.certificateExpirationTime, Organization)
 	if err != nil {
 		logger.Debug("failed to generate CA")
 		return nil, err
@@ -201,7 +201,7 @@ func (i *StrategyDeploymentInstaller) installCertRequirements(strategy Strategy)
 		}
 
 		// Update the deployment for each certResource
-		newDepSpec, caPEM, err := i.installCertRequirementsForDeployment(sddSpec.Name, ca, expiration, sddSpec.Spec, getServicePorts(certResources))
+		newDepSpec, caPEM, err := i.installCertRequirementsForDeployment(sddSpec.Name, ca, i.certificateExpirationTime, sddSpec.Spec, getServicePorts(certResources))
 		if err != nil {
 			return nil, err
 		}
@@ -213,6 +213,14 @@ func (i *StrategyDeploymentInstaller) installCertRequirements(strategy Strategy)
 	return strategyDetailsDeployment, nil
 }
 
+func (i *StrategyDeploymentInstaller) CertsRotateAt() time.Time {
+	return CalculateCertRotatesAt(i.certificateExpirationTime)
+}
+
+func (i *StrategyDeploymentInstaller) CertsRotated() bool {
+	return i.certificatesRotated
+}
+
 func ShouldRotateCerts(csv *v1alpha1.ClusterServiceVersion) bool {
 	now := metav1.Now()
 	if !csv.Status.CertsRotateAt.IsZero() && csv.Status.CertsRotateAt.Before(&now) {
@@ -222,10 +230,12 @@ func ShouldRotateCerts(csv *v1alpha1.ClusterServiceVersion) bool {
 	return false
 }
 
-func CalculateCertExpirationAndRotateAt() (expiration time.Time, rotateAt time.Time) {
-	expiration = time.Now().Add(DefaultCertValidFor)
-	rotateAt = expiration.Add(-1 * DefaultCertMinFresh)
-	return
+func CalculateCertExpiration(startingFrom time.Time) time.Time {
+	return startingFrom.Add(DefaultCertValidFor)
+}
+
+func CalculateCertRotatesAt(certExpirationTime time.Time) time.Time {
+	return certExpirationTime.Add(-1 * DefaultCertMinFresh)
 }
 
 func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deploymentName string, ca *certs.KeyPair, expiration time.Time, depSpec appsv1.DeploymentSpec, ports []corev1.ServicePort) (*appsv1.DeploymentSpec, []byte, error) {
@@ -316,9 +326,12 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
 			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, nil, err
+		} else {
+			if _, err := i.strategyClient.GetOpClient().UpdateSecret(secret); err != nil {
+				logger.Warnf("could not update secret %s", secret.GetName())
+				return nil, nil, err
+			}
+			i.certificatesRotated = true
 		}
 	} else if apierrors.IsNotFound(err) {
 		// Create the secret
@@ -335,6 +348,7 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
 				return nil, nil, err
 			}
 		}
+		i.certificatesRotated = true
 	} else {
 		return nil, nil, err
 	}

pkg/controller/install/deployment.go

@@ -3,6 +3,7 @@ package install
 import (
 	"fmt"
 	"hash/fnv"
+	"time"
 
 	log "github.com/sirupsen/logrus"
 	appsv1 "k8s.io/api/apps/v1"
@@ -22,13 +23,15 @@ import (
 const DeploymentSpecHashLabelKey = "olm.deployment-spec-hash"
 
 type StrategyDeploymentInstaller struct {
-	strategyClient         wrappers.InstallStrategyDeploymentInterface
-	owner                  ownerutil.Owner
-	previousStrategy       Strategy
-	templateAnnotations    map[string]string
-	initializers           DeploymentInitializerFuncChain
-	apiServiceDescriptions []certResource
-	webhookDescriptions    []certResource
+	strategyClient            wrappers.InstallStrategyDeploymentInterface
+	owner                     ownerutil.Owner
+	previousStrategy          Strategy
+	templateAnnotations       map[string]string
+	initializers              DeploymentInitializerFuncChain
+	apiServiceDescriptions    []certResource
+	webhookDescriptions       []certResource
+	certificateExpirationTime time.Time
+	certificatesRotated       bool
 }
 
 var _ Strategy = &v1alpha1.StrategyDetailsDeployment{}
@@ -77,13 +80,15 @@ func NewStrategyDeploymentInstaller(strategyClient wrappers.InstallStrategyDeplo
 	}
 
 	return &StrategyDeploymentInstaller{
-		strategyClient:         strategyClient,
-		owner:                  owner,
-		previousStrategy:       previousStrategy,
-		templateAnnotations:    templateAnnotations,
-		initializers:           initializers,
-		apiServiceDescriptions: apiDescs,
-		webhookDescriptions:    webhookDescs,
+		strategyClient:            strategyClient,
+		owner:                     owner,
+		previousStrategy:          previousStrategy,
+		templateAnnotations:       templateAnnotations,
+		initializers:              initializers,
+		apiServiceDescriptions:    apiDescs,
+		webhookDescriptions:       webhookDescs,
+		certificatesRotated:       false,
+		certificateExpirationTime: time.Time{},
 	}
 }
 

pkg/controller/install/resolver.go

@@ -5,6 +5,7 @@ package install
 
 import (
 	"fmt"
+	"time"
 
 	"github.com/operator-framework/api/pkg/operators/v1alpha1"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/wrappers"
@@ -20,6 +21,8 @@ type Strategy interface {
 type StrategyInstaller interface {
 	Install(strategy Strategy) error
 	CheckInstalled(strategy Strategy) (bool, error)
+	CertsRotateAt() time.Time
+	CertsRotated() bool
 }
 
 type StrategyResolverInterface interface {
@@ -68,3 +71,11 @@ func (i *NullStrategyInstaller) Install(s Strategy) error {
 func (i *NullStrategyInstaller) CheckInstalled(s Strategy) (bool, error) {
 	return true, nil
 }
+
+func (i *NullStrategyInstaller) CertsRotateAt() time.Time {
+	return time.Time{}
+}
+
+func (i *NullStrategyInstaller) CertsRotated() bool {
+	return false
+}

pkg/controller/operators/olm/operator.go

@@ -1888,20 +1888,9 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
 			return
 		}
 
-		// Only update certificate status if:
-		//  - the CSV has CAResources; and
-		//  - the certificate lastUpdated and rotateAt timestamps have not already been set, or the certificate should be rotated
-		// Note: the code here is a bit wonky and it wasn't clear how to clean it up without some major refactoring:
-		// the installer is in charge of generating and rotating the certificates. It detects whether a certificate should be rotated by
-		// looking at the csv.status.RotateAt value. But, it does not update this value or surface
-		// the certificate expiry information. So, the rotatedAt value is to be re-calculated here. This is bad because you have
-		// two different components doing the same thing (installer and operator are both calculating RotateAt). If we're not careful
-		// there could be skew
-		// See pkg/controller/install/certresources.go
-		if shouldUpdateCertificateDates(out) {
+		if installer.CertsRotated() {
 			now := metav1.Now()
-			_, rotateAt := install.CalculateCertExpirationAndRotateAt()
-			rotateTime := metav1.NewTime(rotateAt)
+			rotateTime := metav1.NewTime(installer.CertsRotateAt())
 			out.Status.CertsLastUpdated = &now
 			out.Status.CertsRotateAt = &rotateTime
 		}
@@ -2531,11 +2520,3 @@ func (a *Operator) ensureLabels(in *v1alpha1.ClusterServiceVersion, labelSets ..
 	out, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(out.GetNamespace()).Update(context.TODO(), out, metav1.UpdateOptions{})
 	return out, err
 }
-
-// shouldUpdateCertificateDates checks the csv status to decide whether
-// status.CertsLastUpdated and status.CertsRotateAt should be updated
-// returns true if the CSV has CAResources and status.RotatedAt is not set OR its time to rotate the certificates
-func shouldUpdateCertificateDates(csv *v1alpha1.ClusterServiceVersion) bool {
-	isNotSet := csv.Status.CertsRotateAt == nil || csv.Status.CertsRotateAt.IsZero()
-	return csv.HasCAResources() && (isNotSet || install.ShouldRotateCerts(csv))
-}

pkg/controller/operators/olm/operator_test.go

@@ -95,6 +95,14 @@ func (i *TestInstaller) CheckInstalled(s install.Strategy) (bool, error) {
 	return true, nil
 }
 
+func (i *TestInstaller) CertsRotateAt() time.Time {
+	return time.Time{}
+}
+
+func (i *TestInstaller) CertsRotated() bool {
+	return false
+}
+
 func ownerLabelFromCSV(name, namespace string) map[string]string {
 	return map[string]string{
 		ownerutil.OwnerKey:          name,
@@ -5112,9 +5120,6 @@ func TestCARotation(t *testing.T) {
 	logrus.SetLevel(logrus.DebugLevel)
 	namespace := "ns"
 
-	//apiHash, err := resolvercache.APIKeyToGVKHash(opregistry.APIKey{Group: "g1", Version: "v1", Kind: "c1"})
-	//require.NoError(t, err)
-
 	defaultOperatorGroup := &operatorsv1.OperatorGroup{
 		TypeMeta: metav1.TypeMeta{
 			Kind:       "OperatorGroup",
@@ -5137,9 +5142,8 @@ func TestCARotation(t *testing.T) {
 	}
 
 	// Generate valid and expired CA fixtures
-	expirationTime, rotationTime := install.CalculateCertExpirationAndRotateAt()
-	expiresAt := metav1.Time{Time: expirationTime}
-	rotateAt := metav1.Time{Time: rotationTime}
+	expiresAt := metav1.NewTime(install.CalculateCertExpiration(time.Now()))
+	rotateAt := metav1.NewTime(install.CalculateCertRotatesAt(expiresAt.Time))
 
 	lastUpdate := metav1.Time{Time: time.Now().UTC()}
 
@@ -5358,18 +5362,17 @@ func TestCARotation(t *testing.T) {
 					require.NoError(t, err)
 					require.NotNil(t, serviceSecret)
 
-					// Extract certificate
+					// Extract certificate validity period
 					start, end, err := GetServiceCertificaValidityPeriod(serviceSecret)
 					require.NoError(t, err)
 					require.NotNil(t, start)
 					require.NotNil(t, end)
 
-					// Compare csv status timestamps with certificate timestamps
-					// NOTE: These values (csv.Status.Certs* and the certificate expiry and rotation are calculated
-					// with the same method but independently, therefore a second granularity will need to suffice.
-					// See https://github.com/operator-framework/operator-lifecycle-manager/issues/2764 for more info.
 					rotationTime := end.Add(-1 * install.DefaultCertMinFresh)
-					require.Equal(t, start.Unix(), outCSV.Status.CertsLastUpdated.Unix())
+					// The csv status is updated after the certificate is created/rotated
+					require.LessOrEqual(t, start.Unix(), outCSV.Status.CertsLastUpdated.Unix())
+
+					// Rotation time should always be the same between the certificate and the status
 					require.Equal(t, rotationTime.Unix(), outCSV.Status.CertsRotateAt.Unix())
 				}
 			}