Merge pull request operator-framework#359 from ecordell/fix-uid

Fix InstallPlanReference in Subscription Status
ecordell · May 29, 2018 · 6a94fc3 · 6a94fc3
2 parents 525a7e9 + 1bad32f
commit 6a94fc3
Show file tree

Hide file tree

Showing 8 changed files with 134 additions and 55 deletions.
diff --git a/Makefile b/Makefile
@@ -155,10 +155,10 @@ generate-mock-client: $(counterfeiter)
 make gen-all: gen-ci codegen generate-mock-client
 
 # make ver=0.3.0 tectonic-release
-make tectonic-release:
+tectonic-release:
 	./scripts/package-release.sh $(ver) deploy/tectonic-alm-operator/manifests/$(ver) deploy/tectonic-alm-operator/values.yaml
 
 # make ver=0.3.0 release
-make release:
+release:
 	./scripts/package-release.sh $(ver) deploy/upstream/manifests/$(ver) deploy/upstream/values.yaml
 
diff --git a/pkg/controller/operators/catalog/subscriptions.go b/pkg/controller/operators/catalog/subscriptions.go
@@ -7,6 +7,7 @@ import (
 	ipv1alpha1 "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/installplan/v1alpha1"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/subscription/v1alpha1"
 
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
 	log "github.com/sirupsen/logrus"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
@@ -87,15 +88,7 @@ func (o *Operator) syncSubscription(sub *v1alpha1.Subscription) (*v1alpha1.Subsc
 				Approval:                   sub.GetInstallPlanApproval(),
 			},
 		}
-		owner := []metav1.OwnerReference{
-			{
-				APIVersion: v1alpha1.SchemeGroupVersion.String(),
-				Kind:       v1alpha1.SubscriptionKind,
-				Name:       sub.GetName(),
-				UID:        sub.GetUID(),
-			},
-		}
-		ip.SetOwnerReferences(owner)
+		ownerutil.AddNonBlockingOwner(ip, sub)
 		ip.SetGenerateName(fmt.Sprintf("install-%s-", sub.Status.CurrentCSV))
 		ip.SetNamespace(sub.GetNamespace())
 		res, err := o.client.InstallplanV1alpha1().InstallPlans(sub.GetNamespace()).Create(ip)
@@ -108,8 +101,8 @@ func (o *Operator) syncSubscription(sub *v1alpha1.Subscription) (*v1alpha1.Subsc
 		sub.Status.Install = &v1alpha1.InstallPlanReference{
 			UID:        res.GetUID(),
 			Name:       res.GetName(),
-			APIVersion: res.APIVersion,
-			Kind:       res.Kind,
+			APIVersion: ipv1alpha1.SchemeGroupVersion.String(),
+			Kind:       ipv1alpha1.InstallPlanKind,
 		}
 		return sub, nil
 	}

diff --git a/pkg/controller/operators/catalog/subscriptions_test.go b/pkg/controller/operators/catalog/subscriptions_test.go
@@ -20,12 +20,25 @@ import (
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned/fake"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/fakes"
+	"k8s.io/apimachinery/pkg/util/diff"
+)
+
+var (
+	blockOwnerDeletion = false
+	isController       = false
 )
 
 func RequireActions(t *testing.T, expected, actual []core.Action) {
 	require.EqualValues(t, len(expected), len(actual), "Expected\n\t%#v\ngot\n\t%#v", expected, actual)
 	for i, a := range actual {
 		e := expected[i]
+		switch c := e.(type) {
+		case core.CreateActionImpl:
+			ac := a.(core.CreateActionImpl)
+			cObj := c.Object
+			acObj := ac.Object
+			require.True(t, equality.Semantic.DeepEqual(cObj, acObj), "Expected\n\t%#v\ngot\n\t%#v\n\tdiff:%s", cObj, acObj, diff.ObjectDiff(cObj, acObj))
+		}
 		require.True(t, equality.Semantic.DeepEqual(e, a), "Expected\n\t%#v\ngot\n\t%#v", e, a)
 	}
 }
@@ -150,7 +163,11 @@ func TestSyncSubscription(t *testing.T) {
 					CurrentCSV:  "latest-and-greatest",
 					LastUpdated: earliestTime,
 					State:       v1alpha1.SubscriptionStateUpgradePending,
-					Install:     &v1alpha1.InstallPlanReference{Name: "existing-install"},
+					Install: &v1alpha1.InstallPlanReference{
+						Kind:       ipv1alpha1.InstallPlanKind,
+						APIVersion: ipv1alpha1.SchemeGroupVersion.String(),
+						Name:       "existing-install",
+					},
 				},
 			}},
 			expected: expected{
@@ -168,7 +185,11 @@ func TestSyncSubscription(t *testing.T) {
 						CurrentCSV:  "latest-and-greatest",
 						LastUpdated: earliestTime,
 						State:       v1alpha1.SubscriptionStateUpgradePending,
-						Install:     &v1alpha1.InstallPlanReference{Name: "existing-install"},
+						Install: &v1alpha1.InstallPlanReference{
+							Kind:       ipv1alpha1.InstallPlanKind,
+							APIVersion: ipv1alpha1.SchemeGroupVersion.String(),
+							Name:       "existing-install",
+						},
 					},
 				},
 				err: "",
@@ -333,7 +354,11 @@ func TestSyncSubscription(t *testing.T) {
 				},
 				Status: v1alpha1.SubscriptionStatus{
 					CurrentCSV: "pending",
-					Install:    &v1alpha1.InstallPlanReference{Name: "existing-install"},
+					Install: &v1alpha1.InstallPlanReference{
+						Kind:       ipv1alpha1.InstallPlanKind,
+						APIVersion: ipv1alpha1.SchemeGroupVersion.String(),
+						Name:       "existing-install",
+					},
 				},
 			}},
 			expected: expected{
@@ -372,7 +397,9 @@ func TestSyncSubscription(t *testing.T) {
 				Status: v1alpha1.SubscriptionStatus{
 					CurrentCSV: "latest-and-greatest",
 					Install: &v1alpha1.InstallPlanReference{
-						Name: "dead-install",
+						Kind:       ipv1alpha1.InstallPlanKind,
+						APIVersion: ipv1alpha1.SchemeGroupVersion.String(),
+						Name:       "dead-install",
 					},
 				},
 			}},
@@ -386,10 +413,12 @@ func TestSyncSubscription(t *testing.T) {
 						Namespace:    "fairy-land",
 						OwnerReferences: []metav1.OwnerReference{
 							{
-								APIVersion: "app.coreos.com/v1alpha1",
-								Kind:       "Subscription-v1",
-								Name:       "test-subscription",
-								UID:        types.UID("subscription-uid"),
+								APIVersion:         "app.coreos.com/v1alpha1",
+								Kind:               "Subscription-v1",
+								Name:               "test-subscription",
+								UID:                types.UID("subscription-uid"),
+								BlockOwnerDeletion: &blockOwnerDeletion,
+								Controller:         &isController,
 							},
 						},
 					},
@@ -413,8 +442,10 @@ func TestSyncSubscription(t *testing.T) {
 					Status: v1alpha1.SubscriptionStatus{
 						CurrentCSV: "latest-and-greatest",
 						Install: &v1alpha1.InstallPlanReference{
-							UID:  types.UID("UID-OK"),
-							Name: "installplan-1",
+							Kind:       ipv1alpha1.InstallPlanKind,
+							APIVersion: ipv1alpha1.SchemeGroupVersion.String(),
+							UID:        types.UID("UID-OK"),
+							Name:       "installplan-1",
 						},
 						State: v1alpha1.SubscriptionStateUpgradePending,
 					},
@@ -459,10 +490,12 @@ func TestSyncSubscription(t *testing.T) {
 						Namespace:    "fairy-land",
 						OwnerReferences: []metav1.OwnerReference{
 							{
-								APIVersion: "app.coreos.com/v1alpha1",
-								Kind:       "Subscription-v1",
-								Name:       "test-subscription",
-								UID:        types.UID("subscription-uid"),
+								APIVersion:         "app.coreos.com/v1alpha1",
+								Kind:               "Subscription-v1",
+								Name:               "test-subscription",
+								UID:                types.UID("subscription-uid"),
+								BlockOwnerDeletion: &blockOwnerDeletion,
+								Controller:         &isController,
 							},
 						},
 					},
@@ -486,8 +519,10 @@ func TestSyncSubscription(t *testing.T) {
 					Status: v1alpha1.SubscriptionStatus{
 						CurrentCSV: "latest-and-greatest",
 						Install: &v1alpha1.InstallPlanReference{
-							UID:  types.UID("UID-OK"),
-							Name: "installplan-1",
+							Kind:       ipv1alpha1.InstallPlanKind,
+							APIVersion: ipv1alpha1.SchemeGroupVersion.String(),
+							UID:        types.UID("UID-OK"),
+							Name:       "installplan-1",
 						},
 						State: v1alpha1.SubscriptionStateUpgradePending,
 					},
@@ -535,10 +570,12 @@ func TestSyncSubscription(t *testing.T) {
 						Namespace:    "fairy-land",
 						OwnerReferences: []metav1.OwnerReference{
 							{
-								APIVersion: "app.coreos.com/v1alpha1",
-								Kind:       "Subscription-v1",
-								Name:       "test-subscription",
-								UID:        types.UID("subscription-uid"),
+								APIVersion:         "app.coreos.com/v1alpha1",
+								Kind:               "Subscription-v1",
+								Name:               "test-subscription",
+								UID:                types.UID("subscription-uid"),
+								BlockOwnerDeletion: &blockOwnerDeletion,
+								Controller:         &isController,
 							},
 						},
 					},
@@ -563,8 +600,10 @@ func TestSyncSubscription(t *testing.T) {
 					Status: v1alpha1.SubscriptionStatus{
 						CurrentCSV: "latest-and-greatest",
 						Install: &v1alpha1.InstallPlanReference{
-							UID:  types.UID("UID-OK"),
-							Name: "installplan-1",
+							Kind:       ipv1alpha1.InstallPlanKind,
+							APIVersion: ipv1alpha1.SchemeGroupVersion.String(),
+							UID:        types.UID("UID-OK"),
+							Name:       "installplan-1",
 						},
 						State: v1alpha1.SubscriptionStateUpgradePending,
 					},
@@ -608,10 +647,12 @@ func TestSyncSubscription(t *testing.T) {
 						Namespace:    "fairy-land",
 						OwnerReferences: []metav1.OwnerReference{
 							{
-								APIVersion: "app.coreos.com/v1alpha1",
-								Kind:       "Subscription-v1",
-								Name:       "test-subscription",
-								UID:        types.UID("subscription-uid"),
+								APIVersion:         "app.coreos.com/v1alpha1",
+								Kind:               "Subscription-v1",
+								Name:               "test-subscription",
+								UID:                types.UID("subscription-uid"),
+								BlockOwnerDeletion: &blockOwnerDeletion,
+								Controller:         &isController,
 							},
 						},
 					},

diff --git a/pkg/lib/ownerutil/util.go b/pkg/lib/ownerutil/util.go
@@ -2,7 +2,11 @@ package ownerutil
 
 import (
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis"
-	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/clusterserviceversion/v1alpha1"
+	csv1alpha1 "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/catalogsource/v1alpha1"
+	csvv1alpha1 "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/clusterserviceversion/v1alpha1"
+	ipv1alpha1 "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/installplan/v1alpha1"
+	subv1alpha1 "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/subscription/v1alpha1"
+	log "github.com/sirupsen/logrus"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 )
@@ -22,20 +26,10 @@ func IsOwnedBy(object metav1.Object, owner Owner) bool {
 	return false
 }
 
+// AddNonBlockingOwner adds a nonblocking owner to the ownerref list.
 func AddNonBlockingOwner(object metav1.Object, owner Owner) {
-	// TODO: Remove as soon as possible
-	// This is a hack, for some reason CSVs that we get out of the informer are missing
-	// TypeMeta, which means we can't get the APIVersion or Kind generically here.
-	// The underlying issue should be found and fixes as soon as possible
-	// This needs to be removed before a new APIVersion is cut
-	if _, ok := owner.(*v1alpha1.ClusterServiceVersion); ok {
-		owner.SetGroupVersionKind(schema.GroupVersionKind{
-			Group:   apis.GroupName,
-			Version: v1alpha1.GroupVersion,
-			Kind:    v1alpha1.ClusterServiceVersionKind,
-		})
-	}
-
+	// Most of the time we won't have TypeMeta on the object, so we infer it for types we know about
+	inferGroupVersionKind(owner)
 	blockOwnerDeletion := false
 	isController := false
 
@@ -55,3 +49,43 @@ func AddNonBlockingOwner(object metav1.Object, owner Owner) {
 	})
 	object.SetOwnerReferences(ownerRefs)
 }
+
+// inferGroupVersionKind adds TypeMeta to an owner so that it can be written to an ownerref.
+// TypeMeta is generally only known at serialization time, so we often won't know what GVK an owner has.
+// For the types we know about, we can add the GVK of the apis that we're using the interact with the object.
+func inferGroupVersionKind(owner Owner) {
+	if !owner.GroupVersionKind().Empty() {
+		// owner already has TypeMeta, no inference needed
+		return
+	}
+
+	switch v := owner.(type) {
+	case *csvv1alpha1.ClusterServiceVersion:
+		owner.SetGroupVersionKind(schema.GroupVersionKind{
+			Group:   apis.GroupName,
+			Version: csvv1alpha1.GroupVersion,
+			Kind:    csvv1alpha1.ClusterServiceVersionKind,
+		})
+	case *ipv1alpha1.InstallPlan:
+		owner.SetGroupVersionKind(schema.GroupVersionKind{
+			Group:   apis.GroupName,
+			Version: ipv1alpha1.GroupVersion,
+			Kind:    ipv1alpha1.InstallPlanKind,
+		})
+	case *subv1alpha1.Subscription:
+		owner.SetGroupVersionKind(schema.GroupVersionKind{
+			Group:   apis.GroupName,
+			Version: subv1alpha1.GroupVersion,
+			Kind:    subv1alpha1.SubscriptionKind,
+		})
+	case *csv1alpha1.CatalogSource:
+		owner.SetGroupVersionKind(schema.GroupVersionKind{
+			Group:   apis.GroupName,
+			Version: csv1alpha1.GroupVersion,
+			Kind:    csv1alpha1.CatalogSourceKind,
+		})
+	default:
+		log.Warnf("could not infer GVK for object: %#v, %#v", v, owner)
+	}
+	return
+}
diff --git a/scripts/run_e2e_docker.sh b/scripts/run_e2e_docker.sh
@@ -14,7 +14,7 @@ echo "namespace: ${namespace}" >> ${tmpdir}/e2e-values.yaml
 echo "watchedNamespaces: ${namespace}" >> ${tmpdir}/e2e-values.yaml
 echo "catalog_namespace: ${namespace}" >> ${tmpdir}/e2e-values.yaml
 
-./scripts/package-release.sh ver=1.0.0-e2e test/e2e/resources ${tmpdir}/e2e-values.yaml
+./scripts/package-release.sh 1.0.0-e2e test/e2e/resources ${tmpdir}/e2e-values.yaml
 
 function cleanup {
  	kubectl delete namespace ${namespace}

diff --git a/scripts/run_e2e_local.sh b/scripts/run_e2e_local.sh
@@ -15,7 +15,7 @@ echo "namespace: ${namespace}" >> ${tmpdir}/e2e-values.yaml
 echo "watchedNamespaces: ${namespace}" >> ${tmpdir}/e2e-values.yaml
 echo "catalog_namespace: ${namespace}" >> ${tmpdir}/e2e-values.yaml
 
-./scripts/package-release.sh ver=1.0.0-e2e test/e2e/resources ${tmpdir}/e2e-values.yaml
+./scripts/package-release.sh 1.0.0-e2e test/e2e/resources ${tmpdir}/e2e-values.yaml
 
 function cleanup {
  	kubectl delete namespace ${namespace}

diff --git a/test/e2e/e2e-values-shift.yaml b/test/e2e/e2e-values-shift.yaml
@@ -31,3 +31,9 @@ e2e:
     ref: quay.io/coreos/olm-e2e:local
 
 job_name: e2e
+
+
+catalog_sources:
+ - tectonic-ocs
+ - tectonic-components
+ - upstream-components
diff --git a/test/e2e/e2e-values.yaml b/test/e2e/e2e-values.yaml
@@ -21,3 +21,8 @@ e2e:
     ref: quay.io/coreos/olm-e2e:local
 
 job_name: e2e
+
+catalog_sources:
+ - tectonic-ocs
+ - tectonic-components
+ - upstream-components