Do settings as a pre-install hook

changelog/v0.11.2/move-settings-to-preinstall.yaml

@@ -0,0 +1,4 @@
+changelog:
+  - type: FIX
+    description: Gloo settings is now created as a pre-install step, to avoid a race condition with
+    issueLink: https://github.com/solo-io/gloo/issues/562

install/helm/gloo/templates/18-settings.yaml

@@ -5,6 +5,8 @@ kind: Settings
 metadata:
   name: default
   namespace: {{ .Release.Namespace }}
+  annotations:
+    "helm.sh/hook": pre-install
 spec:
   bindAddr: 0.0.0.0:{{ .Values.gloo.deployment.xdsPort }}
   discoveryNamespace: {{ .Values.settings.writeNamespace }}

pkg/cliutil/install/chart.go

@@ -1,9 +1,10 @@
 package install
 
 import (
+	"strings"
+
 	"github.com/ghodss/yaml"
 	"github.com/solo-io/gloo/install/helm/gloo/generate"
-	"strings"
 
 	"github.com/solo-io/gloo/pkg/cliutil"
 	"github.com/solo-io/go-utils/errors"

pkg/cliutil/install/filters.go

@@ -1,12 +1,14 @@
 package install
 
 import (
-	"github.com/ghodss/yaml"
-	"github.com/solo-io/go-utils/errors"
-	"k8s.io/apimachinery/pkg/apis/meta/v1"
 	"regexp"
 	"strings"
 
+	"github.com/ghodss/yaml"
+	"github.com/helm/helm/pkg/hooks"
+	"github.com/solo-io/go-utils/errors"
+	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
 	"k8s.io/helm/pkg/manifest"
 )
 
@@ -17,6 +19,14 @@ import (
 //   - err: if != nil, the whole manifest retrieval operation will fail
 type ManifestFilterFunc func(input []manifest.Manifest) (output []manifest.Manifest, err error)
 
+// We need to define this ourselves, because if we unmarshal into `apiextensions.CustomResourceDefinition`
+// we don't get the ObjectMeta (in the yaml they are nested under `metadata`, but the k8s struct has
+// them as top level fields...)
+type resourceType struct {
+	Metadata v1.ObjectMeta
+	v1.TypeMeta
+}
+
 // Returns only non-empty manifests
 var ExcludeEmptyManifests ManifestFilterFunc = func(input []manifest.Manifest) ([]manifest.Manifest, error) {
 	var output []manifest.Manifest
@@ -29,40 +39,93 @@ var ExcludeEmptyManifests ManifestFilterFunc = func(input []manifest.Manifest) (
 	return output, nil
 }
 
-// Filters out any CRD from each manifest
-var ExcludeCrds ManifestFilterFunc = func(input []manifest.Manifest) (output []manifest.Manifest, err error) {
-	for _, man := range input {
+type resourceMatcherFunc func(resource resourceType) (bool, error)
+
+var preInstallMatcher resourceMatcherFunc = func(resource resourceType) (bool, error) {
+	helmPreInstallHook, ok := resource.Metadata.Annotations[hooks.HookAnno]
+	if !ok || helmPreInstallHook != hooks.PreInstall {
+		return false, nil
+	}
+	return true, nil
+}
+
+var crdInstallMatcher resourceMatcherFunc = func(resource resourceType) (bool, error) {
+	crdKind := resource.TypeMeta.Kind == CrdKindName
+	if crdKind {
+		// Check whether the CRD is a Helm "crd-install" hook.
+		// If not, throw an error, because this will cause race conditions when installing with Helm (which is
+		// not the case here, but we want to validate the manifests whenever we have the chance)
+		helmCrdInstallHookAnnotation, ok := resource.Metadata.Annotations[hooks.HookAnno]
+		if !ok || helmCrdInstallHookAnnotation != hooks.CRDInstall {
+			return crdKind, errors.Errorf("CRD [%s] must be annotated as a Helm '%s' hook", resource.Metadata.Name, hooks.CRDInstall)
+		}
+	}
+	return crdKind, nil
+}
+
+var nonCrdInstallMatcher resourceMatcherFunc = func(resource resourceType) (bool, error) {
+	isCrdInstall, err := crdInstallMatcher(resource)
+	return !isCrdInstall, err
+}
 
+var nonPreInstallMatcher resourceMatcherFunc = func(resource resourceType) (bool, error) {
+	isPreInstall, err := preInstallMatcher(resource)
+	return !isPreInstall, err
+}
+
+var excludeByMatcher = func(input []manifest.Manifest, matches resourceMatcherFunc) (output []manifest.Manifest, resourceNames []string, err error) {
+	resourceNames = make([]string, 0)
+	for _, man := range input {
 		// Split manifest into individual YAML docs
-		nonCrdDocs := make([]string, 0)
+		nonMatching := make([]string, 0)
 		for _, doc := range strings.Split(man.Content, "---") {
 
-			// We need to define this ourselves, because if we unmarshal into `apiextensions.CustomResourceDefinition`
-			// we don't get the ObjectMeta (in the yaml they are nested under `metadata`, but the k8s struct has
-			// them as top level fields...)
-			var resource struct {
-				Metadata v1.ObjectMeta
-				v1.TypeMeta
-			}
+			var resource resourceType
 			if err := yaml.Unmarshal([]byte(doc), &resource); err != nil {
-				return nil, errors.Wrapf(err, "parsing resource: %s", doc)
+				return nil, nil, errors.Wrapf(err, "parsing resource: %s", doc)
 			}
 
-			// Keep only non-CRD resources
-			if resource.TypeMeta.Kind != CrdKindName {
-				nonCrdDocs = append(nonCrdDocs, doc)
+			isMatch, err := matches(resource)
+			if err != nil {
+				return nil, nil, err
+			}
+			if !isMatch {
+				resourceNames = append(resourceNames, resource.Metadata.Name)
+				nonMatching = append(nonMatching, doc)
 			}
 		}
 
 		output = append(output, manifest.Manifest{
 			Name:    man.Name,
 			Head:    man.Head,
-			Content: strings.Join(nonCrdDocs, YamlDocumentSeparator),
+			Content: strings.Join(nonMatching, YamlDocumentSeparator),
 		})
 	}
 	return
 }
 
+// Filters out any pre-install from each manifest
+var ExcludePreInstall ManifestFilterFunc = func(input []manifest.Manifest) (output []manifest.Manifest, err error) {
+	manifest, _, err := excludeByMatcher(input, preInstallMatcher)
+	return manifest, err
+}
+
+// Filters out anything but pre-install
+var IncludeOnlyPreInstall ManifestFilterFunc = func(input []manifest.Manifest) (output []manifest.Manifest, err error) {
+	manifest, _, err := excludeByMatcher(input, nonPreInstallMatcher)
+	return manifest, err
+}
+
+// Filters out any CRD from each manifest
+var ExcludeCrds ManifestFilterFunc = func(input []manifest.Manifest) (output []manifest.Manifest, err error) {
+	manifest, _, err := excludeByMatcher(input, crdInstallMatcher)
+	return manifest, err
+}
+
+var ExcludeNonCrds = func(input []manifest.Manifest) (output []manifest.Manifest, names []string, err error) {
+	return excludeByMatcher(input, nonCrdInstallMatcher)
+}
+
 // Filters out NOTES.txt files
 var ExcludeNotes ManifestFilterFunc = func(input []manifest.Manifest) (output []manifest.Manifest, err error) {
 	for _, man := range input {
@@ -85,6 +148,10 @@ func GetKnativeResourceFilterFunction() (ManifestFilterFunc, error) {
 		return nil, errors.Wrapf(err, "checking for knative installation")
 	}
 	skipKnativeInstall := installed && !ours
+	return KnativeResourceFilterFunction(skipKnativeInstall), nil
+}
+
+func KnativeResourceFilterFunction(skipKnativeInstall bool) ManifestFilterFunc {
 	return func(input []manifest.Manifest) ([]manifest.Manifest, error) {
 		var output []manifest.Manifest
 		for _, man := range input {
@@ -94,7 +161,7 @@ func GetKnativeResourceFilterFunction() (ManifestFilterFunc, error) {
 			output = append(output, man)
 		}
 		return output, nil
-	}, nil
+	}
 }
 
 var commentRegex = regexp.MustCompile("#.*")

pkg/cliutil/install/kubernetes.go

@@ -3,15 +3,16 @@ package install
 import (
 	"bytes"
 	"fmt"
+	"io"
+	"os"
+	"os/exec"
+	"time"
+
 	"github.com/solo-io/gloo/projects/gloo/cli/pkg/constants"
 	"github.com/solo-io/go-utils/errors"
 	"github.com/solo-io/go-utils/kubeutils"
-	"io"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes"
-	"os"
-	"os/exec"
-	"time"
 )
 
 func CheckKnativeInstallation() (isInstalled bool, isOurs bool, err error) {

projects/gloo/cli/pkg/cmd/install/install_suite_test.go

@@ -1,29 +1,13 @@
 package install_test
 
 import (
-	"os"
 	"testing"
 
-	"github.com/solo-io/solo-kit/pkg/utils/log"
-
-	"github.com/solo-io/gloo/projects/gloo/cli/pkg/testutils"
-	"github.com/solo-io/gloo/test/helpers"
-
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 )
 
 func TestInstall(t *testing.T) {
-	if os.Getenv("RUN_KUBE2E_TESTS") != "1" {
-		log.Warnf("This test builds and deploys images to dockerhub and kubernetes, " +
-			"and is disabled by default. To enable, set RUN_KUBE2E_TESTS=1 in your env.")
-		return
-	}
 	RegisterFailHandler(Fail)
 	RunSpecs(t, "Install Suite")
 }
-
-var _ = BeforeSuite(func() {
-	err := testutils.Make(helpers.GlooDir(), "prepare-helm")
-	Expect(err).NotTo(HaveOccurred())
-})