Fix bug preventing Helm CRDs from being installed (#2500)

controller/state.go

@@ -73,6 +73,16 @@ type comparisonResult struct {
 	appSourceType    v1alpha1.ApplicationSourceType
 }
 
+func (cr *comparisonResult) targetObjs() []*unstructured.Unstructured {
+	objs := cr.hooks
+	for _, r := range cr.managedResources {
+		if r.Target != nil {
+			objs = append(objs, r.Target)
+		}
+	}
+	return objs
+}
+
 // appStateManager allows to compare applications to git
 type appStateManager struct {
 	metricsServer  *metrics.MetricsServer

controller/state_test.go

@@ -385,3 +385,10 @@ func TestSetManagedResourcesKnownOrphanedResourceExceptions(t *testing.T) {
 	assert.Len(t, tree.OrphanedNodes, 1)
 	assert.Equal(t, "guestbook", tree.OrphanedNodes[0].Name)
 }
+
+func Test_comparisonResult_obs(t *testing.T) {
+	assert.Len(t, (&comparisonResult{}).targetObjs(), 0)
+	assert.Len(t, (&comparisonResult{managedResources: []managedResource{{}}}).targetObjs(), 0)
+	assert.Len(t, (&comparisonResult{managedResources: []managedResource{{Target: test.NewPod()}}}).targetObjs(), 1)
+	assert.Len(t, (&comparisonResult{hooks: []*unstructured.Unstructured{{}}}).targetObjs(), 1)
+}

controller/sync.go

@@ -578,13 +578,13 @@ func (sc *syncContext) pruneObject(liveObj *unstructured.Unstructured, prune, dr
 }
 
 func (sc *syncContext) hasCRDOfGroupKind(group string, kind string) bool {
-	for _, res := range sc.compareResult.managedResources {
-		if res.Target != nil && kube.IsCRD(res.Target) {
-			crdGroup, ok, err := unstructured.NestedString(res.Target.Object, "spec", "group")
+	for _, obj := range sc.compareResult.targetObjs() {
+		if kube.IsCRD(obj) {
+			crdGroup, ok, err := unstructured.NestedString(obj.Object, "spec", "group")
 			if err != nil || !ok {
 				continue
 			}
-			crdKind, ok, err := unstructured.NestedString(res.Target.Object, "spec", "names", "kind")
+			crdKind, ok, err := unstructured.NestedString(obj.Object, "spec", "names", "kind")
 			if err != nil || !ok {
 				continue
 			}

controller/sync_test.go

@@ -688,3 +688,12 @@ func Test_syncContext_liveObj(t *testing.T) {
 		})
 	}
 }
+
+func Test_syncContext_hasCRDOfGroupKind(t *testing.T) {
+	// target
+	assert.False(t, (&syncContext{compareResult: &comparisonResult{managedResources: []managedResource{{Target: test.NewCRD()}}}}).hasCRDOfGroupKind("", ""))
+	assert.True(t, (&syncContext{compareResult: &comparisonResult{managedResources: []managedResource{{Target: test.NewCRD()}}}}).hasCRDOfGroupKind("argoproj.io", "TestCrd"))
+	// hook
+	assert.False(t, (&syncContext{compareResult: &comparisonResult{hooks: []*unstructured.Unstructured{test.NewCRD()}}}).hasCRDOfGroupKind("", ""))
+	assert.True(t, (&syncContext{compareResult: &comparisonResult{hooks: []*unstructured.Unstructured{test.NewCRD()}}}).hasCRDOfGroupKind("argoproj.io", "TestCrd"))
+}

test/e2e/fixture/fixture.go

@@ -319,6 +319,7 @@ func EnsureCleanState(t *testing.T) {
 		&v1.DeleteOptions{PropagationPolicy: &policy}, v1.ListOptions{LabelSelector: testingLabel + "=true"}))
 
 	FailOnErr(Run("", "kubectl", "delete", "ns", "-l", testingLabel+"=true", "--field-selector", "status.phase=Active", "--wait=false"))
+	FailOnErr(Run("", "kubectl", "delete", "crd", "-l", testingLabel+"=true", "--wait=false"))
 
 	// reset settings
 	s, err := settingsManager.GetSettings()

test/e2e/helm_test.go

@@ -64,20 +64,6 @@ func TestHelmHookDeletePolicy(t *testing.T) {
 		Expect(NotPod(func(p v1.Pod) bool { return p.Name == "hook" }))
 }
 
-func TestHelmCrdInstallIsCreated(t *testing.T) {
-	Given(t).
-		Path("hook").
-		When().
-		PatchFile("hook.yaml", `[{"op": "replace", "path": "/metadata/annotations", "value": {"helm.sh/hook": "crd-install"}}]`).
-		Create().
-		Sync().
-		Then().
-		Expect(OperationPhaseIs(OperationSucceeded)).
-		Expect(HealthIs(HealthStatusHealthy)).
-		Expect(SyncStatusIs(SyncStatusCodeSynced)).
-		Expect(ResourceResultNumbering(2))
-}
-
 func TestDeclarativeHelm(t *testing.T) {
 	Given(t).
 		Path("helm").
@@ -132,6 +118,19 @@ func TestHelmValues(t *testing.T) {
 		})
 }
 
+func TestHelmCrdHook(t *testing.T) {
+	Given(t).
+		Path("helm-crd").
+		When().
+		Create().
+		Sync().
+		Then().
+		Expect(OperationPhaseIs(OperationSucceeded)).
+		Expect(HealthIs(HealthStatusHealthy)).
+		Expect(SyncStatusIs(SyncStatusCodeSynced)).
+		Expect(ResourceResultNumbering(2))
+}
+
 func TestHelmReleaseName(t *testing.T) {
 	Given(t).
 		Path("helm").

test/e2e/testdata/helm-crd/Chart.yaml

@@ -0,0 +1,2 @@
+version: 1.0.0
+name: helm-crd

test/e2e/testdata/helm-crd/templates/crd.yaml

@@ -0,0 +1,16 @@
+apiVersion: apiextensions.k8s.io/v1beta1
+kind: CustomResourceDefinition
+metadata:
+  name: crontabs.stable.example.com
+  annotations:
+    "helm.sh/hook": crd-install
+spec:
+  group: stable.example.com
+  version: v1
+  scope: Namespaced
+  names:
+    plural: crontabs
+    singular: crontab
+    kind: CronTab
+    shortNames:
+      - ct

test/e2e/testdata/helm-crd/templates/instance.yaml

@@ -0,0 +1,4 @@
+apiVersion: stable.example.com/v1
+kind: CronTab
+metadata:
+  name: {{ .Release.Name }}-inst

test/testdata.go

@@ -3,6 +3,7 @@ package test
 import (
 	"encoding/json"
 
+	"github.com/ghodss/yaml"
 	appsv1 "k8s.io/api/apps/v1"
 	apiv1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -53,6 +54,24 @@ func NewPod() *unstructured.Unstructured {
 	}
 	return &un
 }
+func NewCRD() *unstructured.Unstructured {
+	var un unstructured.Unstructured
+	err := yaml.Unmarshal([]byte(`apiVersion: apiextensions.k8s.io/v1beta1
+kind: CustomResourceDefinition
+metadata:
+  name: testcrds.argoproj.io
+spec:
+  group: argoproj.io
+  version: v1
+  scope: Namespaced
+  names:
+    plural: testcrds
+    kind: TestCrd`), &un)
+	if err != nil {
+		panic(err)
+	}
+	return &un
+}
 
 // DEPRECATED
 // use `Hook(NewPod())` or similar instead

test/testdata_test.go

@@ -14,3 +14,7 @@ func TestAnnotate(t *testing.T) {
 	assert.Equal(t, "bar", obj.GetAnnotations()["foo"])
 	assert.Equal(t, "qux", obj.GetAnnotations()["baz"])
 }
+
+func TestNewCRD(t *testing.T) {
+	assert.Equal(t, "CustomResourceDefinition", NewCRD().GetKind())
+}

util/helm/cmd.go

@@ -9,9 +9,10 @@ import (
 	"path/filepath"
 	"regexp"
 
+	argoexec "github.com/argoproj/pkg/exec"
+
 	"github.com/argoproj/argo-cd/util"
 	"github.com/argoproj/argo-cd/util/security"
-	argoexec "github.com/argoproj/pkg/exec"
 )
 
 // A thin wrapper around the "helm" command, adding logging and error translation.

util/hook/helm/hook.go

@@ -3,6 +3,7 @@ package helm
 import "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 
 func IsHook(obj *unstructured.Unstructured) bool {
-	_, ok := obj.GetAnnotations()["helm.sh/hook"]
-	return ok
+	value, ok := obj.GetAnnotations()["helm.sh/hook"]
+	// Helm use the same annotation to identify CRD as hooks, but they are not.
+	return ok && value != "crd-install"
 }

util/hook/helm/hook_test.go

@@ -11,4 +11,6 @@ import (
 func TestIsHook(t *testing.T) {
 	assert.False(t, IsHook(NewPod()))
 	assert.True(t, IsHook(Annotate(NewPod(), "helm.sh/hook", "anything")))
+	// helm calls "crd-install" a hook, but it really can't be treated as such
+	assert.False(t, IsHook(Annotate(NewCRD(), "helm.sh/hook", "crd-install")))
 }

util/hook/helm/type.go

@@ -10,7 +10,6 @@ import (
 type Type string
 
 const (
-	CRDInstall  Type = "crd-install"
 	PreInstall  Type = "pre-install"
 	PreUpgrade  Type = "pre-upgrade"
 	PostUpgrade Type = "post-upgrade"
@@ -19,15 +18,13 @@ const (
 
 func NewType(t string) (Type, bool) {
 	return Type(t),
-		t == string(CRDInstall) ||
-			t == string(PreInstall) ||
+		t == string(PreInstall) ||
 			t == string(PreUpgrade) ||
 			t == string(PostUpgrade) ||
 			t == string(PostInstall)
 }
 
 var hookTypes = map[Type]v1alpha1.HookType{
-	CRDInstall:  v1alpha1.HookTypePreSync,
 	PreInstall:  v1alpha1.HookTypePreSync,
 	PreUpgrade:  v1alpha1.HookTypePreSync,
 	PostUpgrade: v1alpha1.HookTypePostSync,

util/hook/helm/type_test.go

@@ -11,11 +11,12 @@ import (
 
 func TestTypes(t *testing.T) {
 	assert.Nil(t, Types(NewPod()))
-	assert.Equal(t, []Type{CRDInstall}, Types(Annotate(NewPod(), "helm.sh/hook", "crd-install")))
 	assert.Equal(t, []Type{PreInstall}, Types(Annotate(NewPod(), "helm.sh/hook", "pre-install")))
 	assert.Equal(t, []Type{PreUpgrade}, Types(Annotate(NewPod(), "helm.sh/hook", "pre-upgrade")))
 	assert.Equal(t, []Type{PostUpgrade}, Types(Annotate(NewPod(), "helm.sh/hook", "post-upgrade")))
 	assert.Equal(t, []Type{PostInstall}, Types(Annotate(NewPod(), "helm.sh/hook", "post-install")))
+	// helm calls "crd-install" a hook, but it really can't be treated as such
+	assert.Empty(t, Types(Annotate(NewPod(), "helm.sh/hook", "crd-install")))
 	// we do not consider these supported hooks
 	assert.Nil(t, Types(Annotate(NewPod(), "helm.sh/hook", "pre-rollback")))
 	assert.Nil(t, Types(Annotate(NewPod(), "helm.sh/hook", "post-rollback")))
@@ -24,7 +25,6 @@ func TestTypes(t *testing.T) {
 }
 
 func TestType_HookType(t *testing.T) {
-	assert.Equal(t, v1alpha1.HookTypePreSync, CRDInstall.HookType())
 	assert.Equal(t, v1alpha1.HookTypePreSync, PreInstall.HookType())
 	assert.Equal(t, v1alpha1.HookTypePreSync, PreUpgrade.HookType())
 	assert.Equal(t, v1alpha1.HookTypePostSync, PostUpgrade.HookType())