fix(olm): defer provided api update in operator groups

This is the case of a bug where the CSV status wasn't updated due to an
invalid CSV. The cause for this was twofold:

1) The CSV and OperatorGroup reconcile loops were undoing each others
changes continously, giving no chance for the CSV to get synced beyond
provided api conflict detection.

2) Due to the way the provided APIs are produced differently for
operator groups (uses GVKSTringToProvidedAPISet) and OLM (uses
NewOperatorFromV1Alpha1CSV) when an invalid CSV is in use, the resolver
attempts to continue to instruct that an operator group annotation
update is needed beyond the point of that being true.

The changes are to ensure that OLM does not get stuck attempting to
update the operator group annotations when they are no longer needed
combined with ensuring that the operator group sync loop does not try to
prematurely dismiss a provided api before the CSV has a chance to check
the requirements and produce status.

In order to accomplish "premature detection", providedAPIsFromCSVs was
modified to not only return the APIs but also the CSV that provides a
given API.

Author: Jeff Peeler

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)
+}