Merge pull request #25 from jetstack/fix_secrets_order

Fix secrets order

Author: irbekrm

internal/command/operator.go

@@ -290,7 +290,7 @@ Note: If --auto-registry-credentials and --registry-credentials-path are unset,
 
 			cdv, err := venafi.ParseCertDiscoveryVenafiConfig(certDiscoveryVenafiConnection, vcs, certDiscoveryVenafi)
 			if err != nil {
-				return fmt.Errorf("error parsing cert-discovery-venafi config")
+				return fmt.Errorf("error parsing cert-discovery-venafi config: %w", err)
 			}
 			options.CertDiscoveryVenafi = cdv
 

internal/operator/operator.go

@@ -60,27 +60,14 @@ type ApplyOperatorYAMLOptions struct {
 // Secure operator which is then applied via the Applier implementation. It can be customised via the provided
 // ApplyOperatorYAMLOptions type.
 func ApplyOperatorYAML(ctx context.Context, applier Applier, options ApplyOperatorYAMLOptions) error {
-	var file io.Reader
-	var err error
-
-	if options.Version == "" {
-		file, err = latestManifest()
-	} else {
-		file, err = manifestVersion(options.Version)
-	}
-
-	if err != nil {
-		return err
-	}
 
 	buf := bytes.NewBuffer([]byte{})
-	if _, err = io.Copy(buf, file); err != nil {
-		return err
-	}
 
-	// if there is no registry credentials, we assume that the images can be
+	// Write any secrets to the buffer first, so they get applied to cluster
+	// before any Deployments that use them.
+	// If there is no registry credentials, we assume that the images can be
 	// pulled from a public registry or that the image pull secrets are already
-	// in place
+	// in place.
 	if options.RegistryCredentials != "" {
 		secret, err := ImagePullSecret(options.RegistryCredentials)
 		if err != nil {
@@ -93,11 +80,27 @@ func ApplyOperatorYAML(ctx context.Context, applier Applier, options ApplyOperat
 		}
 		secretReader := bytes.NewBuffer(secretData)
 
-		buf.WriteString("---\n")
-
 		if _, err = io.Copy(buf, secretReader); err != nil {
 			return err
 		}
+		buf.WriteString("---\n")
+	}
+
+	var file io.Reader
+	var err error
+
+	if options.Version == "" {
+		file, err = latestManifest()
+	} else {
+		file, err = manifestVersion(options.Version)
+	}
+
+	if err != nil {
+		return err
+	}
+
+	if _, err = io.Copy(buf, file); err != nil {
+		return err
 	}
 
 	tpl, err := template.New("install").Parse(buf.String())
@@ -321,11 +324,13 @@ func ApplyInstallationYAML(ctx context.Context, applier Applier, options ApplyIn
 		registryCredentials = string(registryCredentialsBytes)
 	}
 
-	secret, err := ImagePullSecret(registryCredentials)
-	if err != nil {
-		return fmt.Errorf("failed to parse image pull secret: %w", err)
+	if registryCredentials != "" {
+		secret, err := ImagePullSecret(registryCredentials)
+		if err != nil {
+			return fmt.Errorf("failed to parse image pull secret: %w", err)
+		}
+		manifestTemplates.secrets = append(manifestTemplates.secrets, secret)
 	}
-	manifestTemplates.secrets = append(manifestTemplates.secrets, secret)
 
 	if err := generateVenafiIssuerManifests(manifestTemplates, options); err != nil {
 		return fmt.Errorf("error building manifests for Venafi issuers: %w", err)
@@ -377,16 +382,11 @@ type manifests struct {
 }
 
 func marshalManifests(mf *manifests) (io.Reader, error) {
-	// Add Installation to the buffer
-	data, err := yaml.Marshal(mf.installation)
-	if err != nil {
-		return nil, fmt.Errorf("error marshalling Installation resource: %w", err)
-	}
-	buf := bytes.NewBuffer(data)
+	buf := bytes.NewBuffer([]byte{})
 
-	// Add all Secrets to the buffer
+	// Add all Secrets to the buffer first to ensure that they get applied
+	// to the cluster before any Deployments that might want to use them.
 	for _, secret := range mf.secrets {
-		buf.WriteString("---\n")
 		secretJson, err := yaml.Marshal(secret)
 		if err != nil {
 			return nil, fmt.Errorf("failed to marshal Secret data: %w", err)
@@ -395,6 +395,20 @@ func marshalManifests(mf *manifests) (io.Reader, error) {
 		if _, err = io.Copy(buf, secretReader); err != nil {
 			return nil, fmt.Errorf("error writing secret data to buffer: %w", err)
 		}
+		buf.WriteString("---\n")
+	}
+	if mf.installation.Spec.CertManager == nil {
+		panic("cert manager is nil")
+	}
+	installationData, err := yaml.Marshal(mf.installation)
+	if err != nil {
+		return nil, fmt.Errorf("error marshalling Installation resource: %w", err)
+	}
+
+	installationBuffer := bytes.NewReader(installationData)
+
+	if _, err = io.Copy(buf, installationBuffer); err != nil {
+		return nil, fmt.Errorf("Error writing installation data to buffer: %w", err)
 	}
 
 	return buf, nil

internal/operator/operator_test.go

@@ -5,10 +5,11 @@ import (
 	"context"
 	"encoding/json"
 	"io"
+	"strings"
 	"testing"
 
 	"github.com/cert-manager/cert-manager/pkg/apis/certmanager"
-	certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
+	cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
 	operatorv1alpha1 "github.com/jetstack/js-operator/pkg/apis/operator/v1alpha1"
 	"github.com/stretchr/testify/assert"
 	corev1 "k8s.io/api/core/v1"
@@ -145,8 +146,12 @@ func TestApplyInstallationYAML(t *testing.T) {
 		err := operator.ApplyInstallationYAML(ctx, applier, options)
 		assert.NoError(t, err)
 
+		var secret corev1.Secret
 		var actual operatorv1alpha1.Installation
-		assert.NoError(t, yaml.Unmarshal(applier.data.Bytes(), &actual))
+		s := strings.Split(string(applier.data.Bytes()), "---")
+		assert.Len(t, s, 2)
+		assert.NoError(t, yaml.Unmarshal([]byte(s[0]), &secret))
+		assert.NoError(t, yaml.Unmarshal([]byte(s[1]), &actual))
 
 		assert.NotNil(t, actual.Spec.VenafiOauthHelper)
 		assert.Contains(t, actual.Spec.VenafiOauthHelper.ImagePullSecrets, "jse-gcr-creds")
@@ -180,8 +185,12 @@ func TestApplyInstallationYAML(t *testing.T) {
 		err := operator.ApplyInstallationYAML(ctx, applier, options)
 		assert.NoError(t, err)
 
+		var secret corev1.Secret
 		var installation operatorv1alpha1.Installation
-		assert.NoError(t, yaml.Unmarshal(applier.data.Bytes(), &installation))
+		s := strings.Split(string(applier.data.Bytes()), "---")
+		assert.Len(t, s, 2)
+		assert.NoError(t, yaml.Unmarshal([]byte(s[0]), &secret))
+		assert.NoError(t, yaml.Unmarshal([]byte(s[1]), &installation))
 
 		assert.NotNil(t, installation.Spec.CertDiscoveryVenafi)
 		assert.Equal(t, installation.Spec.CertDiscoveryVenafi.TPP.URL, "foo")
@@ -203,12 +212,13 @@ func TestApplyInstallationYAML(t *testing.T) {
 
 		err := operator.ApplyInstallationYAML(ctx, applier, options)
 		assert.NoError(t, err)
+		var installation operatorv1alpha1.Installation
+		s := strings.Split(string(applier.data.Bytes()), "---")
+		assert.Len(t, s, 3)
+		assert.NoError(t, yaml.Unmarshal([]byte(s[2]), &installation))
 
-		var actual operatorv1alpha1.Installation
-		assert.NoError(t, yaml.Unmarshal(applier.data.Bytes(), &actual))
-
-		assert.NotNil(t, actual.Spec.CertDiscoveryVenafi)
-		assert.Contains(t, actual.Spec.CertDiscoveryVenafi.ImagePullSecrets, "jse-gcr-creds")
+		assert.NotNil(t, installation.Spec.CertDiscoveryVenafi)
+		assert.Contains(t, installation.Spec.CertDiscoveryVenafi.ImagePullSecrets, "jse-gcr-creds")
 	})
 
 	t.Run("It should have a blank Istio CSR block when no issuer is provided", func(t *testing.T) {
@@ -244,7 +254,7 @@ func TestApplyInstallationYAML(t *testing.T) {
 			issuer := actual.Spec.IstioCSR.IssuerRef
 
 			assert.EqualValues(t, certmanager.GroupName, issuer.Group)
-			assert.EqualValues(t, certmanagerv1.IssuerKind, issuer.Kind)
+			assert.EqualValues(t, cmapi.IssuerKind, issuer.Kind)
 			assert.EqualValues(t, options.IstioCSRIssuer, issuer.Name)
 		}
 	})