Merge pull request #796 from perdasilva/perdasilva/manual-sync-2406

NO-ISSUE: Manual Sync

Author: openshift-merge-bot[bot]

Makefile

@@ -130,7 +130,7 @@ e2e/operator-registry: ## Run e2e registry tests
 	$(MAKE) e2e WHAT=operator-registry
 
 e2e/olm: ## Run e2e olm tests
-	$(MAKE) e2e WHAT=operator-lifecycle-manager E2E_CATALOG_NS=openshift-marketplace E2E_INSTALL_NS=openshift-operator-lifecycle-manager E2E_TEST_NS=openshift-operators E2E_TIMEOUT=120m KUBECTL=oc
+	$(MAKE) e2e WHAT=operator-lifecycle-manager E2E_CATALOG_NS=openshift-marketplace E2E_INSTALL_NS=openshift-operator-lifecycle-manager E2E_TEST_NS=openshift-operators E2E_TIMEOUT=135m KUBECTL=oc
 
 .PHONY: vendor
 vendor:

staging/operator-lifecycle-manager/pkg/controller/bundle/bundle_unpacker.go

@@ -863,14 +863,26 @@ func sortUnpackJobs(jobs []*batchv1.Job, maxRetainedJobs int) (latest *batchv1.J
 	// sort jobs so that latest job is first
 	// with preference for non-failed jobs
 	sort.Slice(jobs, func(i, j int) bool {
+		if jobs[i] == nil || jobs[j] == nil {
+			return jobs[i] != nil
+		}
 		condI, failedI := getCondition(jobs[i], batchv1.JobFailed)
 		condJ, failedJ := getCondition(jobs[j], batchv1.JobFailed)
 		if failedI != failedJ {
 			return !failedI // non-failed job goes first
 		}
 		return condI.LastTransitionTime.After(condJ.LastTransitionTime.Time)
 	})
+	if jobs[0] == nil {
+		// all nil jobs
+		return
+	}
 	latest = jobs[0]
+	nilJobsIndex := len(jobs) - 1
+	for ; nilJobsIndex >= 0 && jobs[nilJobsIndex] == nil; nilJobsIndex-- {
+	}
+
+	jobs = jobs[:nilJobsIndex+1] // exclude nil jobs from list of jobs to delete
 	if len(jobs) <= maxRetainedJobs {
 		return
 	}
@@ -880,7 +892,7 @@ func sortUnpackJobs(jobs []*batchv1.Job, maxRetainedJobs int) (latest *batchv1.J
 	}
 
 	// cleanup old failed jobs, n-1 recent jobs and the oldest job
-	for i := 0; i < maxRetainedJobs && i+maxRetainedJobs < len(jobs); i++ {
+	for i := 0; i < maxRetainedJobs && i+maxRetainedJobs < len(jobs)-1; i++ {
 		toDelete = append(toDelete, jobs[maxRetainedJobs+i])
 	}
 

staging/operator-lifecycle-manager/pkg/controller/bundle/bundle_unpacker_test.go

@@ -1997,6 +1997,15 @@ func TestSortUnpackJobs(t *testing.T) {
 			},
 		}
 	}
+	nilConditionJob := &batchv1.Job{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:   "nc",
+			Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue, bundleUnpackRefLabel: "test"},
+		},
+		Status: batchv1.JobStatus{
+			Conditions: nil,
+		},
+	}
 	failedJobs := []*batchv1.Job{
 		testJob("f-1", true, 1),
 		testJob("f-2", true, 2),
@@ -2028,6 +2037,24 @@ func TestSortUnpackJobs(t *testing.T) {
 		}, {
 			name:        "empty job list",
 			maxRetained: 1,
+		}, {
+			name:        "nil job in list",
+			maxRetained: 1,
+			jobs: []*batchv1.Job{
+				failedJobs[2],
+				nil,
+				failedJobs[1],
+			},
+			expectedLatest: failedJobs[2],
+		}, {
+			name:        "nil condition",
+			maxRetained: 3,
+			jobs: []*batchv1.Job{
+				failedJobs[2],
+				nilConditionJob,
+				failedJobs[1],
+			},
+			expectedLatest: nilConditionJob,
 		}, {
 			name:        "retain oldest",
 			maxRetained: 1,

staging/operator-lifecycle-manager/pkg/controller/install/certresources.go

@@ -12,6 +12,7 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
+	"k8s.io/apimachinery/pkg/util/sets"
 	corev1ac "k8s.io/client-go/applyconfigurations/core/v1"
 	rbacv1ac "k8s.io/client-go/applyconfigurations/rbac/v1"
 
@@ -161,6 +162,14 @@ func ServiceName(deploymentName string) string {
 	return deploymentName + "-service"
 }
 
+func HostnamesForService(serviceName, namespace string) []string {
+	return []string{
+		fmt.Sprintf("%s.%s", serviceName, namespace),
+		fmt.Sprintf("%s.%s.svc", serviceName, namespace),
+		fmt.Sprintf("%s.%s.svc.cluster.local", serviceName, namespace),
+	}
+}
+
 func (i *StrategyDeploymentInstaller) getCertResources() []certResource {
 	return append(i.apiServiceDescriptions, i.webhookDescriptions...)
 }
@@ -227,15 +236,74 @@ func (i *StrategyDeploymentInstaller) CertsRotated() bool {
 	return i.certificatesRotated
 }
 
-func ShouldRotateCerts(csv *v1alpha1.ClusterServiceVersion) bool {
+// shouldRotateCerts indicates whether an apiService cert should be rotated due to being
+// malformed, invalid, expired, inactive or within a specific freshness interval (DefaultCertMinFresh) before expiry.
+func shouldRotateCerts(certSecret *corev1.Secret, hosts []string) bool {
 	now := metav1.Now()
-	if !csv.Status.CertsRotateAt.IsZero() && csv.Status.CertsRotateAt.Before(&now) {
+	caPEM, ok := certSecret.Data[OLMCAPEMKey]
+	if !ok {
+		// missing CA cert in secret
+		return true
+	}
+	certPEM, ok := certSecret.Data["tls.crt"]
+	if !ok {
+		// missing cert in secret
+		return true
+	}
+
+	ca, err := certs.PEMToCert(caPEM)
+	if err != nil {
+		// malformed CA cert
+		return true
+	}
+	cert, err := certs.PEMToCert(certPEM)
+	if err != nil {
+		// malformed cert
 		return true
 	}
 
+	// check for cert freshness
+	if !certs.Active(ca) || now.Time.After(CalculateCertRotatesAt(ca.NotAfter)) ||
+		!certs.Active(cert) || now.Time.After(CalculateCertRotatesAt(cert.NotAfter)) {
+		return true
+	}
+
+	// Check validity of serving cert and if serving cert is trusted by the CA
+	for _, host := range hosts {
+		if err := certs.VerifyCert(ca, cert, host); err != nil {
+			return true
+		}
+	}
 	return false
 }
 
+func (i *StrategyDeploymentInstaller) ShouldRotateCerts(s Strategy) (bool, error) {
+	strategy, ok := s.(*v1alpha1.StrategyDetailsDeployment)
+	if !ok {
+		return false, fmt.Errorf("failed to install %s strategy with deployment installer: unsupported deployment install strategy", strategy.GetStrategyName())
+	}
+
+	hasCerts := sets.New[string]()
+	for _, c := range i.getCertResources() {
+		hasCerts.Insert(c.getDeploymentName())
+	}
+	for _, sddSpec := range strategy.DeploymentSpecs {
+		if hasCerts.Has(sddSpec.Name) {
+			certSecret, err := i.strategyClient.GetOpLister().CoreV1().SecretLister().Secrets(i.owner.GetNamespace()).Get(SecretName(ServiceName(sddSpec.Name)))
+			if err == nil {
+				if shouldRotateCerts(certSecret, HostnamesForService(ServiceName(sddSpec.Name), i.owner.GetNamespace())) {
+					return true, nil
+				}
+			} else if apierrors.IsNotFound(err) {
+				return true, nil
+			} else {
+				return false, err
+			}
+		}
+	}
+	return false, nil
+}
+
 func CalculateCertExpiration(startingFrom time.Time) time.Time {
 	return startingFrom.Add(DefaultCertValidFor)
 }
@@ -271,10 +339,7 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
 	}
 
 	// Create signed serving cert
-	hosts := []string{
-		fmt.Sprintf("%s.%s", serviceName, i.owner.GetNamespace()),
-		fmt.Sprintf("%s.%s.svc", serviceName, i.owner.GetNamespace()),
-	}
+	hosts := HostnamesForService(serviceName, i.owner.GetNamespace())
 	servingPair, err := certGenerator.Generate(expiration, Organization, ca, hosts)
 	if err != nil {
 		logger.Warnf("could not generate signed certs for hosts %v", hosts)
@@ -323,10 +388,10 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
 
 		// Attempt an update
 		// TODO: Check that the secret was not modified
-		if existingCAPEM, ok := existingSecret.Data[OLMCAPEMKey]; ok && !ShouldRotateCerts(i.owner.(*v1alpha1.ClusterServiceVersion)) && existingSecret.Annotations[OpenShiftComponent] != "" {
+		if !shouldRotateCerts(existingSecret, HostnamesForService(serviceName, i.owner.GetNamespace())) {
 			logger.Warnf("reusing existing cert %s", secret.GetName())
 			secret = existingSecret
-			caPEM = existingCAPEM
+			caPEM = existingSecret.Data[OLMCAPEMKey]
 			caHash = certs.PEMSHA256(caPEM)
 		} else {
 			if _, err := i.strategyClient.GetOpClient().UpdateSecret(secret); err != nil {

staging/operator-lifecycle-manager/pkg/controller/install/resolver.go

@@ -21,6 +21,7 @@ type Strategy interface {
 type StrategyInstaller interface {
 	Install(strategy Strategy) error
 	CheckInstalled(strategy Strategy) (bool, error)
+	ShouldRotateCerts(strategy Strategy) (bool, error)
 	CertsRotateAt() time.Time
 	CertsRotated() bool
 }
@@ -79,3 +80,7 @@ func (i *NullStrategyInstaller) CertsRotateAt() time.Time {
 func (i *NullStrategyInstaller) CertsRotated() bool {
 	return false
 }
+
+func (i *NullStrategyInstaller) ShouldRotateCerts(s Strategy) (bool, error) {
+	return false, nil
+}

staging/operator-lifecycle-manager/pkg/controller/operators/olm/apiservices.go

@@ -93,17 +93,12 @@ func (a *Operator) checkAPIServiceResources(csv *v1alpha1.ClusterServiceVersion,
 
 		// Check if CA is Active
 		caBundle := apiService.Spec.CABundle
-		ca, err := certs.PEMToCert(caBundle)
+		_, err = certs.PEMToCert(caBundle)
 		if err != nil {
 			logger.Warnf("could not convert APIService CA bundle to x509 cert")
 			errs = append(errs, err)
 			continue
 		}
-		if !certs.Active(ca) {
-			logger.Warnf("CA cert not active")
-			errs = append(errs, fmt.Errorf("found the CA cert is not active"))
-			continue
-		}
 
 		// Check if serving cert is active
 		secretName := install.SecretName(serviceName)
@@ -113,17 +108,12 @@ func (a *Operator) checkAPIServiceResources(csv *v1alpha1.ClusterServiceVersion,
 			errs = append(errs, err)
 			continue
 		}
-		cert, err := certs.PEMToCert(secret.Data["tls.crt"])
+		_, err = certs.PEMToCert(secret.Data["tls.crt"])
 		if err != nil {
 			logger.Warnf("could not convert serving cert to x509 cert")
 			errs = append(errs, err)
 			continue
 		}
-		if !certs.Active(cert) {
-			logger.Warnf("serving cert not active")
-			errs = append(errs, fmt.Errorf("found the serving cert not active"))
-			continue
-		}
 
 		// Check if CA hash matches expected
 		caHash := hashFunc(caBundle)
@@ -133,18 +123,6 @@ func (a *Operator) checkAPIServiceResources(csv *v1alpha1.ClusterServiceVersion,
 			continue
 		}
 
-		// Check if serving cert is trusted by the CA
-		hosts := []string{
-			fmt.Sprintf("%s.%s", service.GetName(), csv.GetNamespace()),
-			fmt.Sprintf("%s.%s.svc", service.GetName(), csv.GetNamespace()),
-		}
-		for _, host := range hosts {
-			if err := certs.VerifyCert(ca, cert, host); err != nil {
-				errs = append(errs, fmt.Errorf("could not verify cert: %s", err.Error()))
-				continue
-			}
-		}
-
 		// Ensure the existing Deployment has a matching CA hash annotation
 		deployment, err := a.lister.AppsV1().DeploymentLister().Deployments(csv.GetNamespace()).Get(desc.DeploymentName)
 		if apierrors.IsNotFound(err) || err != nil {

staging/operator-lifecycle-manager/pkg/controller/operators/olm/operator.go

@@ -2242,7 +2242,10 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
 		}
 
 		// Check if it's time to refresh owned APIService certs
-		if install.ShouldRotateCerts(out) {
+		if shouldRotate, err := installer.ShouldRotateCerts(strategy); err != nil {
+			logger.WithError(err).Info("cert validity check")
+			return
+		} else if shouldRotate {
 			logger.Debug("CSV owns resources that require a cert refresh")
 			out.SetPhaseWithEvent(v1alpha1.CSVPhasePending, v1alpha1.CSVReasonNeedsCertRotation, "CSV owns resources that require a cert refresh", now, a.recorder)
 			return
@@ -2352,7 +2355,10 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
 		}
 
 		// Check if it's time to refresh owned APIService certs
-		if install.ShouldRotateCerts(out) {
+		if shouldRotate, err := installer.ShouldRotateCerts(strategy); err != nil {
+			logger.WithError(err).Info("cert validity check")
+			return
+		} else if shouldRotate {
 			logger.Debug("CSV owns resources that require a cert refresh")
 			out.SetPhaseWithEvent(v1alpha1.CSVPhasePending, v1alpha1.CSVReasonNeedsCertRotation, "owned APIServices need cert refresh", now, a.recorder)
 			return