Merge pull request #1114 from jpeeler/fix-status-invalid-csv

Bug 1767004: defer provided api update in operator groups

Author: openshift-merge-robot

pkg/controller/operators/olm/operator.go

@@ -1252,6 +1252,12 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
 		if unionedAnnotations == nil {
 			unionedAnnotations = make(map[string]string)
 		}
+		if unionedAnnotations[v1.OperatorGroupProvidedAPIsAnnotationKey] == union.String() {
+			// resolver may think apis need adding with invalid input, so continue when there's no work
+			// to be done so that the CSV can progress far enough to get requirements checked
+			a.logger.Debug("operator group annotations up to date, continuing")
+			break
+		}
 		unionedAnnotations[v1.OperatorGroupProvidedAPIsAnnotationKey] = union.String()
 		operatorGroup.SetAnnotations(unionedAnnotations)
 		if _, err := a.client.OperatorsV1().OperatorGroups(operatorGroup.GetNamespace()).Update(operatorGroup); err != nil && !k8serrors.IsNotFound(err) {

pkg/controller/operators/olm/operator_test.go

@@ -4264,7 +4264,7 @@ func TestSyncOperatorGroups(t *testing.T) {
 			require.NoError(t, err)
 
 			// Sync csvs enough to get them back to succeeded state
-			for i := 0; i < 8; i++ {
+			for i := 0; i < 16; i++ {
 				opGroupCSVs, err := op.client.OperatorsV1alpha1().ClusterServiceVersions(operatorNamespace).List(metav1.ListOptions{})
 				require.NoError(t, err)
 
@@ -4292,7 +4292,7 @@ func TestSyncOperatorGroups(t *testing.T) {
 						require.NoError(t, err)
 					}
 
-					if i == 8 {
+					if i == 16 {
 						err = wait.PollImmediate(1*time.Millisecond, 10*time.Second, func() (bool, error) {
 							for namespace, objects := range tt.final.objects {
 								if err := RequireObjectsInCache(t, op.lister, namespace, objects, true); err != nil {

pkg/controller/operators/olm/operatorgroup.go

@@ -19,6 +19,7 @@ import (
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
+	opregistry "github.com/operator-framework/operator-registry/pkg/registry"
 )
 
 const (
@@ -126,7 +127,13 @@ func (a *Operator) syncOperatorGroups(obj interface{}) error {
 	groupSurface := resolver.NewOperatorGroup(op)
 	groupProvidedAPIs := groupSurface.ProvidedAPIs()
 	providedAPIsForCSVs := a.providedAPIsFromCSVs(op, logger)
-	providedAPIsForGroup := providedAPIsForCSVs.Union(groupProvidedAPIs)
+	providedAPIsForGroup := make(resolver.APISet)
+	for api := range providedAPIsForCSVs {
+		providedAPIsForGroup[api] = struct{}{}
+	}
+	for api := range groupProvidedAPIs {
+		providedAPIsForGroup[api] = struct{}{}
+	}
 
 	csvs, err := a.findCSVsThatProvideAnyOf(providedAPIsForGroup)
 	if err != nil {
@@ -212,9 +219,9 @@ func (a *Operator) annotateCSVs(group *v1.OperatorGroup, targetNamespaces []stri
 	return errors.NewAggregate(updateErrs)
 }
 
-func (a *Operator) providedAPIsFromCSVs(group *v1.OperatorGroup, logger *logrus.Entry) resolver.APISet {
+func (a *Operator) providedAPIsFromCSVs(group *v1.OperatorGroup, logger *logrus.Entry) map[opregistry.APIKey]*v1alpha1.ClusterServiceVersion {
 	set := a.csvSet(group.Namespace, v1alpha1.CSVPhaseAny)
-	providedAPIsFromCSVs := make(resolver.APISet)
+	providedAPIsFromCSVs := make(map[opregistry.APIKey]*v1alpha1.ClusterServiceVersion)
 	for _, csv := range set {
 		// Don't union providedAPIsFromCSVs if the CSV is copied (member of another OperatorGroup)
 		if csv.IsCopied() {
@@ -230,20 +237,40 @@ func (a *Operator) providedAPIsFromCSVs(group *v1.OperatorGroup, logger *logrus.
 			logger.WithError(err).Warn("could not create OperatorSurface from csv")
 			continue
 		}
-		providedAPIsFromCSVs = providedAPIsFromCSVs.Union(operatorSurface.ProvidedAPIs().StripPlural())
+		for providedAPI := range operatorSurface.ProvidedAPIs().StripPlural() {
+			providedAPIsFromCSVs[providedAPI] = csv
+		}
 	}
 	return providedAPIsFromCSVs
 }
 
-func (a *Operator) pruneProvidedAPIs(group *v1.OperatorGroup, groupProvidedAPIs, providedAPIsFromCSVs resolver.APISet, logger *logrus.Entry) {
+func (a *Operator) pruneProvidedAPIs(group *v1.OperatorGroup, groupProvidedAPIs resolver.APISet, providedAPIsFromCSVs map[opregistry.APIKey]*v1alpha1.ClusterServiceVersion, logger *logrus.Entry) {
 	// Don't prune providedAPIsFromCSVs if static
 	if group.Spec.StaticProvidedAPIs {
 		a.logger.Debug("group has static provided apis. skipping provided api pruning")
 		return
 	}
 
+	intersection := make(resolver.APISet)
+	for api := range providedAPIsFromCSVs {
+		if _, ok := groupProvidedAPIs[api]; ok {
+			intersection[api] = struct{}{}
+		} else {
+			csv := providedAPIsFromCSVs[api]
+			_, err := a.lister.OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(csv.GetNamespace()).Get(csv.GetName())
+			if k8serrors.IsNotFound(err) {
+				continue
+			}
+			if csv.DeletionTimestamp == nil && (csv.Status.Phase == v1alpha1.CSVPhaseNone || csv.Status.Phase == v1alpha1.CSVPhasePending) {
+				logger.Debugf("aborting operator group provided API update due to CSV %v in phase %v", csv.GetName(), csv.Status.Phase)
+				return
+			}
+		}
+	}
+
 	// Prune providedAPIs annotation if the cluster has fewer providedAPIs (handles CSV deletion)
-	if intersection := groupProvidedAPIs.Intersection(providedAPIsFromCSVs); len(intersection) < len(groupProvidedAPIs) {
+	//if intersection := groupProvidedAPIs.Intersection(providedAPIsFromCSVs); len(intersection) < len(groupProvidedAPIs) {
+	if len(intersection) < len(groupProvidedAPIs) {
 		difference := groupProvidedAPIs.Difference(intersection)
 		logger := logger.WithFields(logrus.Fields{
 			"providedAPIsOnCluster":  providedAPIsFromCSVs,

test/e2e/csv_e2e_test.go

@@ -3212,3 +3212,117 @@ func TestCreateCSVRequirementsEvents(t *testing.T) {
 }
 
 // TODO: test behavior when replaces field doesn't point to existing CSV
+
+func TestCSVStatusInvalidCSV(t *testing.T) {
+	defer cleaner.NotifyTestComplete(t, true)
+
+	c := newKubeClient(t)
+	crc := newCRClient(t)
+
+	// Create CRD
+	crdPlural := genName("ins")
+	crdName := crdPlural + ".cluster.com"
+	cleanupCRD, err := createCRD(c, apiextensions.CustomResourceDefinition{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: crdName,
+		},
+		Spec: apiextensions.CustomResourceDefinitionSpec{
+			Group:   "cluster.com",
+			Version: "v1alpha1",
+			Names: apiextensions.CustomResourceDefinitionNames{
+				Plural:   crdPlural,
+				Singular: crdPlural,
+				Kind:     crdPlural,
+				ListKind: "list" + crdPlural,
+			},
+			Scope: "Namespaced",
+		},
+	})
+	require.NoError(t, err)
+	defer cleanupCRD()
+
+	// create CSV
+	strategy := install.StrategyDetailsDeployment{
+		DeploymentSpecs: []install.StrategyDeploymentSpec{
+			{
+				Name: genName("dep-"),
+				Spec: newNginxDeployment(genName("nginx-")),
+			},
+		},
+	}
+	strategyRaw, err := json.Marshal(strategy)
+	require.NoError(t, err)
+
+	csv := v1alpha1.ClusterServiceVersion{
+		TypeMeta: metav1.TypeMeta{
+			Kind:       v1alpha1.ClusterServiceVersionKind,
+			APIVersion: v1alpha1.ClusterServiceVersionAPIVersion,
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name: genName("csv"),
+		},
+		Spec: v1alpha1.ClusterServiceVersionSpec{
+			InstallModes: []v1alpha1.InstallMode{
+				{
+					Type:      v1alpha1.InstallModeTypeOwnNamespace,
+					Supported: true,
+				},
+				{
+					Type:      v1alpha1.InstallModeTypeSingleNamespace,
+					Supported: true,
+				},
+				{
+					Type:      v1alpha1.InstallModeTypeMultiNamespace,
+					Supported: true,
+				},
+				{
+					Type:      v1alpha1.InstallModeTypeAllNamespaces,
+					Supported: true,
+				},
+			},
+			InstallStrategy: v1alpha1.NamedInstallStrategy{
+				StrategyName:    install.InstallStrategyNameDeployment,
+				StrategySpecRaw: strategyRaw,
+			},
+			CustomResourceDefinitions: v1alpha1.CustomResourceDefinitions{
+				Owned: []v1alpha1.CRDDescription{
+					{
+						Name:        crdName,
+						Version:     "apiextensions.k8s.io/v1alpha1", // purposely invalid, should be just v1alpha1 to match CRD
+						Kind:        crdPlural,
+						DisplayName: crdName,
+						Description: "In the cluster2",
+					},
+				},
+			},
+		},
+	}
+
+	cleanupCSV, err := createCSV(t, c, crc, csv, testNamespace, true, false)
+	require.NoError(t, err)
+	defer cleanupCSV()
+
+	notServedStatus := v1alpha1.RequirementStatus{
+		Group:   "apiextensions.k8s.io",
+		Version: "v1beta1",
+		Kind:    "CustomResourceDefinition",
+		Name:    crdName,
+		Status:  v1alpha1.RequirementStatusReasonNotPresent,
+		Message: "CRD version not served",
+	}
+	csvCheckPhaseAndRequirementStatus := func(csv *v1alpha1.ClusterServiceVersion) bool {
+		if csv.Status.Phase == v1alpha1.CSVPhasePending {
+			for _, status := range csv.Status.RequirementStatus {
+				if status.Message == notServedStatus.Message {
+					return true
+				}
+			}
+		}
+		return false
+	}
+
+	fetchedCSV, err := fetchCSV(t, crc, csv.Name, testNamespace, csvCheckPhaseAndRequirementStatus)
+	require.NoError(t, err)
+
+	require.Contains(t, fetchedCSV.Status.RequirementStatus, notServedStatus)
+}