remove need for internal k8s service hosts in provided cert/key pair

kustomize/base/quay.service.yaml

@@ -5,7 +5,7 @@ metadata:
   labels:
     quay-component: quay-app
 spec:
-  type: LoadBalancer
+  type: ClusterIP
   ports:
     - protocol: TCP
       name: https

pkg/kustomize/kustomize.go

@@ -462,6 +462,7 @@ func Inflate(ctx *quaycontext.QuayRegistryContext, quay *v1.QuayRegistry, baseCo
 		}
 	}
 
+	// FIXME(alecmerdler): Should we create certs if `route` component is unmanaged...?
 	log.Info("Ensuring `ssl.cert` and `ssl.key` pair for Quay app TLS")
 	tlsCert, tlsKey, err := EnsureTLSFor(ctx, quay, baseConfigBundle.Data["ssl.cert"], baseConfigBundle.Data["ssl.key"])
 	if err != nil {

pkg/kustomize/secrets.go

@@ -158,9 +158,7 @@ func BaseConfig() map[string]interface{} {
 }
 
 // EnsureTLSFor checks if given TLS cert/key pair are valid for the Quay registry to use for secure communication with clients,
-// and generates a TLS certificate/key pair if they are invalid.
-// In addition to `SERVER_HOSTNAME`, it sets certificate subject alternative names
-// for the internal k8s service hostnames (i.e. `registry-quay-app.quay-enterprise.svc`).
+// and generates a TLS certificate/key pair if they are not provided.
 func EnsureTLSFor(ctx *quaycontext.QuayRegistryContext, quay *v1.QuayRegistry, tlsCert, tlsKey []byte) ([]byte, []byte, error) {
 	fieldGroup, err := FieldGroupFor(ctx, "route", quay)
 	if err != nil {
@@ -169,23 +167,22 @@ func EnsureTLSFor(ctx *quaycontext.QuayRegistryContext, quay *v1.QuayRegistry, t
 
 	routeFieldGroup := fieldGroup.(*hostsettings.HostSettingsFieldGroup)
 
-	svc := quay.GetName() + "-quay-app"
 	hosts := []string{
 		routeFieldGroup.ServerHostname,
-		svc,
-		strings.Join([]string{svc, quay.GetNamespace(), "svc"}, "."),
-		strings.Join([]string{svc, quay.GetNamespace(), "svc", "cluster", "local"}, "."),
 	}
 
 	// Only add BUILDMAN_HOSTNAME as host if provided.
 	if ctx.BuildManagerHostname != "" {
 		hosts = append(hosts, strings.Split(ctx.BuildManagerHostname, ":")[0])
 	}
 
+	if tlsCert == nil && tlsKey == nil {
+		return cert.GenerateSelfSignedCertKey(routeFieldGroup.ServerHostname, []net.IP{}, hosts)
+	}
+
 	for _, host := range hosts {
-		if valid, _ := shared.ValidateCertPairWithHostname(tlsCert, tlsKey, host, fieldGroupNameFor("route")); !valid {
-			fmt.Printf("Host %s not valid for certificates provided. Generating self-signed certs", host) // change to logger?
-			return cert.GenerateSelfSignedCertKey(routeFieldGroup.ServerHostname, []net.IP{}, hosts)
+		if valid, validationErr := shared.ValidateCertPairWithHostname(tlsCert, tlsKey, host, fieldGroupNameFor("route")); !valid {
+			return nil, nil, fmt.Errorf("provided certificate/key pair not valid for host '%s': %s", host, validationErr.String())
 		}
 	}
 

pkg/kustomize/secrets_test.go

@@ -1,9 +1,8 @@
 package kustomize
 
 import (
-	"net"
+	"fmt"
 	"net/url"
-	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -319,94 +318,107 @@ func TestContainsComponentConfig(t *testing.T) {
 	}
 }
 
-func TestEnsureTLSFor(t *testing.T) {
+func certKeyPairFor(hostname string, alternateHostnames []string) [][]byte {
+	cert, key, err := cert.GenerateSelfSignedCertKey(hostname, nil, alternateHostnames)
+	if err != nil {
+		panic(err)
+	}
 
-	emptyCert := []byte{}
-	serverHostname := "serverhostname.com"
-	builderHostname := "builderhostname.com"
-	quayRegistry := quayRegistry("test")
+	return [][]byte{cert, key}
+}
 
-	quayContextWithoutBuilder := &quaycontext.QuayRegistryContext{
-		ServerHostname: serverHostname,
-	}
-	quayContextWithBuilder := &quaycontext.QuayRegistryContext{
-		BuildManagerHostname: builderHostname,
-		ServerHostname:       serverHostname,
-	}
+var ensureTLSForTests = []struct {
+	name                 string
+	routeManaged         bool
+	serverHostname       string
+	buildManagerHostname string
+	providedCertKeyPair  [][]byte
+	expectedErr          error
+}{
+	{
+		"ManagedRouteNoHostnameNoCerts",
+		true,
+		"",
+		"",
+		[][]byte{nil, nil},
+		nil,
+	},
+	{
+		"ManagedRouteProvidedHostnameProvidedIncorrectCerts",
+		true,
+		"registry.company.com",
+		"",
+		certKeyPairFor("nonexistent.company.com", nil),
+		fmt.Errorf("provided certificate/key pair not valid for host 'registry.company.com': x509: certificate is valid for nonexistent.company.com, not registry.company.com"),
+	},
+	{
+		"ManagedRouteProvidedHostnameNoCerts",
+		true,
+		"registry.company.com",
+		"",
+		[][]byte{nil, nil},
+		nil,
+	},
+	{
+		"ManagedRouteProvidedHostnameProvidedCerts",
+		true,
+		"registry.company.com",
+		"",
+		certKeyPairFor("registry.company.com", nil),
+		nil,
+	},
+	{
+		"ManagedRouteProvidedBuildmanagerHostnameProvidedIncorrectCerts",
+		true,
+		"registry.company.com",
+		"builds.company.com",
+		certKeyPairFor("registry.company.com", nil),
+		fmt.Errorf("provided certificate/key pair not valid for host 'builds.company.com': x509: certificate is valid for registry.company.com, not builds.company.com"),
+	},
+	{
+		"ManagedRouteProvidedBuildmanagerHostnameProvidedCerts",
+		true,
+		"registry.company.com",
+		"builds.company.com",
+		certKeyPairFor("registry.company.com", []string{"builds.company.com"}),
+		nil,
+	},
+	{
+		"ManagedRouteProvidedBuildmanagerHostnameNoCerts",
+		true,
+		"registry.company.com",
+		"builds.company.com",
+		[][]byte{nil, nil},
+		nil,
+	},
+}
 
-	svc := quayRegistry.GetName() + "-quay-app"
-	hostsWithoutBuilder := []string{
-		serverHostname,
-		svc,
-		strings.Join([]string{svc, quayRegistry.GetNamespace(), "svc"}, "."),
-		strings.Join([]string{svc, quayRegistry.GetNamespace(), "svc", "cluster", "local"}, "."),
-	}
-	// Generate certs without builder hostname in SAN
-	pubWithoutBuilder, privWithoutBuilder, err := cert.GenerateSelfSignedCertKey(serverHostname, []net.IP{}, hostsWithoutBuilder)
-	if err != nil {
-		t.Errorf(err.Error())
-	}
+func TestEnsureTLSFor(t *testing.T) {
+	assert := assert.New(t)
 
-	// Generate certs with builder hostname in SAN
-	hostsWithBuilder := append(hostsWithoutBuilder, builderHostname)
-	pubWithBuilder, privWithBuilder, err := cert.GenerateSelfSignedCertKey(serverHostname, []net.IP{}, hostsWithBuilder)
-	if err != nil {
-		t.Errorf(err.Error())
-	}
+	for _, test := range ensureTLSForTests {
+		quayRegistry := quayRegistry("test")
 
-	t.Run("Quay context has buildman hostname. Certs are empty, Operator should generate own.", func(t *testing.T) {
-		assert := assert.New(t)
-		// Empty certs, should generate own
-		recPublicKey, recPrivateKey, err := EnsureTLSFor(quayContextWithBuilder, quayRegistry, emptyCert, emptyCert)
-		if err != nil {
-			t.Errorf(err.Error())
+		quayContext := quaycontext.QuayRegistryContext{
+			ServerHostname:       test.serverHostname,
+			BuildManagerHostname: test.buildManagerHostname,
 		}
-		assert.NotEqual(recPublicKey, emptyCert)
-		assert.NotEqual(recPrivateKey, emptyCert)
-	})
 
-	t.Run("Quay context has buildman hostname. Certs do not contain buildman hostname. Operator should generate own.", func(t *testing.T) {
-		assert := assert.New(t)
-		// Buildman missing from certs, should generate own
-		recPublicKey, recPrivateKey, err := EnsureTLSFor(quayContextWithBuilder, quayRegistry, pubWithoutBuilder, privWithoutBuilder)
-		if err != nil {
-			t.Errorf(err.Error())
-		}
-		assert.NotEqual(recPublicKey, pubWithoutBuilder)
-		assert.NotEqual(recPrivateKey, privWithoutBuilder)
-	})
+		tlsCert, tlsKey, err := EnsureTLSFor(&quayContext, quayRegistry, test.providedCertKeyPair[0], test.providedCertKeyPair[1])
 
-	t.Run("Quay context has buildman hostname. Certs contain buildman hostname. Operator should not generate own.", func(t *testing.T) {
-		assert := assert.New(t)
-		// Buildman present in certs, should not generate
-		recPublicKey, recPrivateKey, err := EnsureTLSFor(quayContextWithBuilder, quayRegistry, pubWithBuilder, privWithBuilder)
-		if err != nil {
-			t.Errorf(err.Error())
-		}
-		assert.Equal(recPublicKey, pubWithBuilder)
-		assert.Equal(recPrivateKey, privWithBuilder)
-	})
+		assert.Equal(test.expectedErr, err, test.name)
 
-	t.Run("Quay context does not have buildman hostname. Certs do not contain buildman hostname. Operator should not generate own.", func(t *testing.T) {
-		assert := assert.New(t)
-		// Buildman not in certs, not in context, should not generate
-		recPublicKey, recPrivateKey, err := EnsureTLSFor(quayContextWithoutBuilder, quayRegistry, pubWithoutBuilder, privWithoutBuilder)
-		if err != nil {
-			t.Errorf(err.Error())
-		}
-		assert.Equal(recPublicKey, pubWithoutBuilder)
-		assert.Equal(recPrivateKey, privWithoutBuilder)
-	})
+		if test.expectedErr == nil {
+			if test.providedCertKeyPair[0] != nil && test.providedCertKeyPair[1] != nil {
+				assert.Equal(string(test.providedCertKeyPair[0]), string(tlsCert), test.name)
+				assert.Equal(string(test.providedCertKeyPair[1]), string(tlsKey), test.name)
+			}
 
-	t.Run("Quay context does not have buildman hostname. Certs contain buildman hostname. Operator should not generate own.", func(t *testing.T) {
-		assert := assert.New(t)
-		// Buildman not in certs, not in context, should not generate
-		recPublicKey, recPrivateKey, err := EnsureTLSFor(quayContextWithoutBuilder, quayRegistry, pubWithBuilder, privWithBuilder)
-		if err != nil {
-			t.Errorf(err.Error())
-		}
-		assert.Equal(recPublicKey, pubWithBuilder)
-		assert.Equal(recPrivateKey, privWithBuilder)
-	})
+			shared.ValidateCertPairWithHostname(tlsCert, tlsKey, test.serverHostname, fieldGroupNameFor("route"))
 
+			if test.buildManagerHostname != "" {
+				shared.ValidateCertPairWithHostname(tlsCert, tlsKey, test.buildManagerHostname, fieldGroupNameFor("route"))
+			}
+		}
+	}
 }