⚠️ updates from api audit (#1404)

* updates from api audit Signed-off-by: Jordan Keister <jordan@nimblewidget.com> * review updates Signed-off-by: Jordan Keister <jordan@nimblewidget.com> * e2e fixes Signed-off-by: Jordan Keister <jordan@nimblewidget.com> --------- Signed-off-by: Jordan Keister <jordan@nimblewidget.com>
operator-framework · Nov 8, 2024 · 6c2be08 · 6c2be08
1 parent d0865da
commit 6c2be08
Show file tree

Hide file tree

Showing 13 changed files with 386 additions and 350 deletions.
diff --git a/api/v1alpha1/clusterextension_types.go b/api/v1alpha1/clusterextension_types.go
diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go
diff --git a/config/base/crd/bases/olm.operatorframework.io_clusterextensions.yaml b/config/base/crd/bases/olm.operatorframework.io_clusterextensions.yaml
diff --git a/docs/api-reference/operator-controller-api-reference.md b/docs/api-reference/operator-controller-api-reference.md
diff --git a/internal/applier/helm.go b/internal/applier/helm.go
@@ -19,6 +19,7 @@ import (
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	apimachyaml "k8s.io/apimachinery/pkg/util/yaml"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/log"
 
 	helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client"
 
@@ -56,6 +57,26 @@ type Helm struct {
 	Preflights         []Preflight
 }
 
+// shouldSkipPreflight is a helper to determine if the preflight check is CRDUpgradeSafety AND
+// if it is set to enforcement None.
+func shouldSkipPreflight(ctx context.Context, preflight Preflight, ext *ocv1alpha1.ClusterExtension, state string) bool {
+	l := log.FromContext(ctx)
+	hasCRDUpgradeSafety := ext.Spec.Install.Preflight != nil && ext.Spec.Install.Preflight.CRDUpgradeSafety != nil
+	_, isCRDUpgradeSafetyInstance := preflight.(*crdupgradesafety.Preflight)
+
+	if hasCRDUpgradeSafety && isCRDUpgradeSafetyInstance {
+		if state == StateNeedsInstall || state == StateNeedsUpgrade {
+			l.Info("crdUpgradeSafety ", "policy", ext.Spec.Install.Preflight.CRDUpgradeSafety.Enforcement)
+		}
+		if ext.Spec.Install.Preflight.CRDUpgradeSafety.Enforcement == ocv1alpha1.CRDUpgradeSafetyEnforcementNone {
+			// Skip this preflight check because it is of type *crdupgradesafety.Preflight and the CRD Upgrade Safety
+			// policy is set to None
+			return true
+		}
+	}
+	return false
+}
+
 func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1alpha1.ClusterExtension, objectLabels map[string]string, storageLabels map[string]string) ([]client.Object, string, error) {
 	chrt, err := convert.RegistryV1ToHelmChart(ctx, contentFS, ext.Spec.Install.Namespace, []string{corev1.NamespaceAll})
 	if err != nil {
@@ -78,12 +99,8 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1alpha1.Clust
 	}
 
 	for _, preflight := range h.Preflights {
-		if ext.Spec.Install.Preflight != nil && ext.Spec.Install.Preflight.CRDUpgradeSafety != nil {
-			if _, ok := preflight.(*crdupgradesafety.Preflight); ok && ext.Spec.Install.Preflight.CRDUpgradeSafety.Policy == ocv1alpha1.CRDUpgradeSafetyPolicyDisabled {
-				// Skip this preflight check because it is of type *crdupgradesafety.Preflight and the CRD Upgrade Safety
-				// preflight check has been disabled
-				continue
-			}
+		if shouldSkipPreflight(ctx, preflight, ext, state) {
+			continue
 		}
 		switch state {
 		case StateNeedsInstall:

diff --git a/internal/conditionsets/conditionsets.go b/internal/conditionsets/conditionsets.go
@@ -16,9 +16,27 @@ limitations under the License.
 
 package conditionsets
 
+import (
+	"github.com/operator-framework/operator-controller/api/v1alpha1"
+)
+
 // ConditionTypes is the full set of ClusterExtension condition Types.
 // ConditionReasons is the full set of ClusterExtension condition Reasons.
 //
-// NOTE: These are populated by init() in api/v1alpha1/clusterextension_types.go
-var ConditionTypes []string
-var ConditionReasons []string
+// NOTE: unit tests in clusterextension_types_test will enforce completeness.
+var ConditionTypes = []string{
+	v1alpha1.TypeInstalled,
+	v1alpha1.TypeDeprecated,
+	v1alpha1.TypePackageDeprecated,
+	v1alpha1.TypeChannelDeprecated,
+	v1alpha1.TypeBundleDeprecated,
+	v1alpha1.TypeProgressing,
+}
+
+var ConditionReasons = []string{
+	v1alpha1.ReasonSucceeded,
+	v1alpha1.ReasonDeprecated,
+	v1alpha1.ReasonFailed,
+	v1alpha1.ReasonBlocked,
+	v1alpha1.ReasonRetrying,
+}
diff --git a/internal/controllers/clusterextension_admission_test.go b/internal/controllers/clusterextension_admission_test.go
@@ -77,7 +77,7 @@ func TestClusterExtensionSourceConfig(t *testing.T) {
 
 func TestClusterExtensionAdmissionPackageName(t *testing.T) {
 	tooLongError := "spec.source.catalog.packageName: Too long: may not be longer than 253"
-	regexMismatchError := "spec.source.catalog.packageName in body should match"
+	regexMismatchError := "packageName must be a valid DNS1123 subdomain"
 
 	testCases := []struct {
 		name    string
@@ -136,7 +136,7 @@ func TestClusterExtensionAdmissionPackageName(t *testing.T) {
 
 func TestClusterExtensionAdmissionVersion(t *testing.T) {
 	tooLongError := "spec.source.catalog.version: Too long: may not be longer than 64"
-	regexMismatchError := "spec.source.catalog.version in body should match"
+	regexMismatchError := "invalid version expression"
 
 	testCases := []struct {
 		name    string
@@ -236,7 +236,7 @@ func TestClusterExtensionAdmissionVersion(t *testing.T) {
 
 func TestClusterExtensionAdmissionChannel(t *testing.T) {
 	tooLongError := "spec.source.catalog.channels[0]: Too long: may not be longer than 253"
-	regexMismatchError := "spec.source.catalog.channels[0] in body should match"
+	regexMismatchError := "channels entries must be valid DNS1123 subdomains"
 
 	testCases := []struct {
 		name     string
@@ -293,7 +293,7 @@ func TestClusterExtensionAdmissionChannel(t *testing.T) {
 
 func TestClusterExtensionAdmissionInstallNamespace(t *testing.T) {
 	tooLongError := "spec.install.namespace: Too long: may not be longer than 63"
-	regexMismatchError := "spec.install.namespace in body should match"
+	regexMismatchError := "namespace must be a valid DNS1123 label"
 
 	testCases := []struct {
 		name      string
@@ -348,7 +348,7 @@ func TestClusterExtensionAdmissionInstallNamespace(t *testing.T) {
 
 func TestClusterExtensionAdmissionServiceAccount(t *testing.T) {
 	tooLongError := "spec.install.serviceAccount.name: Too long: may not be longer than 253"
-	regexMismatchError := "spec.install.serviceAccount.name in body should match"
+	regexMismatchError := "name must be a valid DNS1123 subdomain"
 
 	testCases := []struct {
 		name           string

diff --git a/internal/controllers/clusterextension_controller_test.go b/internal/controllers/clusterextension_controller_test.go
@@ -687,7 +687,7 @@ func TestClusterExtensionInstallationSucceeds(t *testing.T) {
 	t.Log("By checking the expected progressing conditions")
 	progressingCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeProgressing)
 	require.NotNil(t, progressingCond)
-	require.Equal(t, metav1.ConditionFalse, progressingCond.Status)
+	require.Equal(t, metav1.ConditionTrue, progressingCond.Status)
 	require.Equal(t, ocv1alpha1.ReasonSucceeded, progressingCond.Reason)
 
 	require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{}))

diff --git a/internal/controllers/common_controller.go b/internal/controllers/common_controller.go
@@ -83,14 +83,13 @@ func setInstallStatus(ext *ocv1alpha1.ClusterExtension, installStatus *ocv1alpha
 func setStatusProgressing(ext *ocv1alpha1.ClusterExtension, err error) {
 	progressingCond := metav1.Condition{
 		Type:               ocv1alpha1.TypeProgressing,
-		Status:             metav1.ConditionFalse,
+		Status:             metav1.ConditionTrue,
 		Reason:             ocv1alpha1.ReasonSucceeded,
 		Message:            "desired state reached",
 		ObservedGeneration: ext.GetGeneration(),
 	}
 
 	if err != nil {
-		progressingCond.Status = metav1.ConditionTrue
 		progressingCond.Reason = ocv1alpha1.ReasonRetrying
 		progressingCond.Message = err.Error()
 	}

diff --git a/internal/controllers/common_controller_test.go b/internal/controllers/common_controller_test.go
@@ -22,12 +22,12 @@ func TestSetStatusProgressing(t *testing.T) {
 		expected         metav1.Condition
 	}{
 		{
-			name:             "non-nil ClusterExtension, nil error, Progressing condition has status False with reason Success",
+			name:             "non-nil ClusterExtension, nil error, Progressing condition has status True with reason Success",
 			err:              nil,
 			clusterExtension: &ocv1alpha1.ClusterExtension{},
 			expected: metav1.Condition{
 				Type:    ocv1alpha1.TypeProgressing,
-				Status:  metav1.ConditionFalse,
+				Status:  metav1.ConditionTrue,
 				Reason:  ocv1alpha1.ReasonSucceeded,
 				Message: "desired state reached",
 			},

diff --git a/internal/resolve/catalog.go b/internal/resolve/catalog.go
@@ -43,13 +43,18 @@ func (r *CatalogResolver) Resolve(ctx context.Context, ext *ocv1alpha1.ClusterEx
 	versionRange := ext.Spec.Source.Catalog.Version
 	channels := ext.Spec.Source.Catalog.Channels
 
-	selector, err := metav1.LabelSelectorAsSelector(&ext.Spec.Source.Catalog.Selector)
-	if err != nil {
-		return nil, nil, nil, fmt.Errorf("desired catalog selector is invalid: %w", err)
-	}
-	// A nothing (empty) seletor selects everything
-	if selector == labels.Nothing() {
-		selector = labels.Everything()
+	// unless overridden, default to selecting all bundles
+	var selector = labels.Everything()
+	var err error
+	if ext.Spec.Source.Catalog != nil {
+		selector, err = metav1.LabelSelectorAsSelector(ext.Spec.Source.Catalog.Selector)
+		if err != nil {
+			return nil, nil, nil, fmt.Errorf("desired catalog selector is invalid: %w", err)
+		}
+		// A nothing (empty) selector selects everything
+		if selector == labels.Nothing() {
+			selector = labels.Everything()
+		}
 	}
 
 	var versionRangeConstraints *mmsemver.Constraints

diff --git a/internal/resolve/catalog_test.go b/internal/resolve/catalog_test.go
@@ -770,7 +770,7 @@ func TestInvalidClusterExtensionCatalogMatchExpressions(t *testing.T) {
 			Source: ocv1alpha1.SourceConfig{
 				Catalog: &ocv1alpha1.CatalogSource{
 					PackageName: "foo",
-					Selector: metav1.LabelSelector{
+					Selector: &metav1.LabelSelector{
 						MatchExpressions: []metav1.LabelSelectorRequirement{
 							{
 								Key:      "name",
@@ -802,7 +802,7 @@ func TestInvalidClusterExtensionCatalogMatchLabelsName(t *testing.T) {
 			Source: ocv1alpha1.SourceConfig{
 				Catalog: &ocv1alpha1.CatalogSource{
 					PackageName: "foo",
-					Selector: metav1.LabelSelector{
+					Selector: &metav1.LabelSelector{
 						MatchLabels: map[string]string{"": "value"},
 					},
 				},
@@ -828,7 +828,7 @@ func TestInvalidClusterExtensionCatalogMatchLabelsValue(t *testing.T) {
 			Source: ocv1alpha1.SourceConfig{
 				Catalog: &ocv1alpha1.CatalogSource{
 					PackageName: "foo",
-					Selector: metav1.LabelSelector{
+					Selector: &metav1.LabelSelector{
 						MatchLabels: map[string]string{"name": "&value"},
 					},
 				},
@@ -852,7 +852,9 @@ func TestClusterExtensionMatchLabel(t *testing.T) {
 	}
 	r := CatalogResolver{WalkCatalogsFunc: w.WalkCatalogs}
 	ce := buildFooClusterExtension(pkgName, []string{}, "", ocv1alpha1.UpgradeConstraintPolicyCatalogProvided)
-	ce.Spec.Source.Catalog.Selector.MatchLabels = map[string]string{"olm.operatorframework.io/metadata.name": "b"}
+	ce.Spec.Source.Catalog.Selector = &metav1.LabelSelector{
+		MatchLabels: map[string]string{"olm.operatorframework.io/metadata.name": "b"},
+	}
 
 	_, _, _, err := r.Resolve(context.Background(), ce, nil)
 	require.NoError(t, err)
@@ -871,7 +873,9 @@ func TestClusterExtensionNoMatchLabel(t *testing.T) {
 	}
 	r := CatalogResolver{WalkCatalogsFunc: w.WalkCatalogs}
 	ce := buildFooClusterExtension(pkgName, []string{}, "", ocv1alpha1.UpgradeConstraintPolicyCatalogProvided)
-	ce.Spec.Source.Catalog.Selector.MatchLabels = map[string]string{"olm.operatorframework.io/metadata.name": "a"}
+	ce.Spec.Source.Catalog.Selector = &metav1.LabelSelector{
+		MatchLabels: map[string]string{"olm.operatorframework.io/metadata.name": "a"},
+	}
 
 	_, _, _, err := r.Resolve(context.Background(), ce, nil)
 	require.Error(t, err)