Fix csv.status.RotatedAt update (#2752)

Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>
Signed-off-by: perdasilva <perdasilva@redhat.com>

Co-authored-by: Per Goncalves da Silva <pegoncal@redhat.com>

Author: perdasilva

pkg/controller/install/certresources.go

@@ -185,13 +185,12 @@ func (i *StrategyDeploymentInstaller) installCertRequirements(strategy Strategy)
 	}
 
 	// Create the CA
-	expiration := time.Now().Add(DefaultCertValidFor)
+	expiration, _ := CalculateCertExpirationAndRotateAt()
 	ca, err := certs.GenerateCA(expiration, Organization)
 	if err != nil {
 		logger.Debug("failed to generate CA")
 		return nil, err
 	}
-	rotateAt := expiration.Add(-1 * DefaultCertMinFresh)
 
 	for n, sddSpec := range strategyDetailsDeployment.DeploymentSpecs {
 		certResources := i.certResourcesForDeployment(sddSpec.Name)
@@ -202,7 +201,7 @@ func (i *StrategyDeploymentInstaller) installCertRequirements(strategy Strategy)
 		}
 
 		// Update the deployment for each certResource
-		newDepSpec, caPEM, err := i.installCertRequirementsForDeployment(sddSpec.Name, ca, rotateAt, sddSpec.Spec, getServicePorts(certResources))
+		newDepSpec, caPEM, err := i.installCertRequirementsForDeployment(sddSpec.Name, ca, expiration, sddSpec.Spec, getServicePorts(certResources))
 		if err != nil {
 			return nil, err
 		}
@@ -223,7 +222,13 @@ func ShouldRotateCerts(csv *v1alpha1.ClusterServiceVersion) bool {
 	return false
 }
 
-func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deploymentName string, ca *certs.KeyPair, rotateAt time.Time, depSpec appsv1.DeploymentSpec, ports []corev1.ServicePort) (*appsv1.DeploymentSpec, []byte, error) {
+func CalculateCertExpirationAndRotateAt() (expiration time.Time, rotateAt time.Time) {
+	expiration = time.Now().Add(DefaultCertValidFor)
+	rotateAt = expiration.Add(-1 * DefaultCertMinFresh)
+	return
+}
+
+func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deploymentName string, ca *certs.KeyPair, expiration time.Time, depSpec appsv1.DeploymentSpec, ports []corev1.ServicePort) (*appsv1.DeploymentSpec, []byte, error) {
 	logger := log.WithFields(log.Fields{})
 
 	// Create a service for the deployment
@@ -263,7 +268,7 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
 		fmt.Sprintf("%s.%s", service.GetName(), i.owner.GetNamespace()),
 		fmt.Sprintf("%s.%s.svc", service.GetName(), i.owner.GetNamespace()),
 	}
-	servingPair, err := certGenerator.Generate(rotateAt, Organization, ca, hosts)
+	servingPair, err := certGenerator.Generate(expiration, Organization, ca, hosts)
 	if err != nil {
 		logger.Warnf("could not generate signed certs for hosts %v", hosts)
 		return nil, nil, err

pkg/controller/operators/olm/operator.go

@@ -1888,10 +1888,19 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
 			return
 		}
 
-		if out.HasCAResources() {
+		// 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) {
 			now := metav1.Now()
-			expiration := now.Add(install.DefaultCertValidFor)
-			rotateAt := expiration.Add(-1 * install.DefaultCertMinFresh)
+			_, rotateAt := install.CalculateCertExpirationAndRotateAt()
 			rotateTime := metav1.NewTime(rotateAt)
 			out.Status.CertsLastUpdated = &now
 			out.Status.CertsRotateAt = &rotateTime
@@ -2522,3 +2531,11 @@ 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))
+}