From acb2b11d8c3fbe2dbc2e4fc4320b8f09d3419511 Mon Sep 17 00:00:00 2001 From: Alec Merdler Date: Thu, 18 Mar 2021 12:46:31 -0700 Subject: [PATCH] remove need for internal k8s service hosts in provided cert/key pair --- apis/quay/v1/quayregistry_types.go | 1 + controllers/quay/quayregistry_controller.go | 3 +- kustomize/base/quay.service.yaml | 2 +- pkg/configure/configure.go | 2 +- pkg/kustomize/secrets.go | 35 ++-- pkg/kustomize/secrets_test.go | 207 ++++++++++++-------- 6 files changed, 148 insertions(+), 102 deletions(-) diff --git a/apis/quay/v1/quayregistry_types.go b/apis/quay/v1/quayregistry_types.go index 96434785a..3dc7e7af7 100644 --- a/apis/quay/v1/quayregistry_types.go +++ b/apis/quay/v1/quayregistry_types.go @@ -61,6 +61,7 @@ var allComponents = []ComponentKind{ var requiredComponents = []ComponentKind{ ComponentPostgres, ComponentObjectStorage, + ComponentRoute, } // QuayRegistrySpec defines the desired state of QuayRegistry. diff --git a/controllers/quay/quayregistry_controller.go b/controllers/quay/quayregistry_controller.go index 3bd86a831..2f498f71d 100644 --- a/controllers/quay/quayregistry_controller.go +++ b/controllers/quay/quayregistry_controller.go @@ -233,7 +233,7 @@ func (r *QuayRegistryReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error updatedQuay.Status.Conditions = v1.RemoveCondition(updatedQuay.Status.Conditions, v1.ConditionTypeRolloutBlocked) for _, component := range updatedQuay.Spec.Components { - contains, err := kustomize.ContainsComponentConfig(userProvidedConfig, component.Kind) + contains, err := kustomize.ContainsComponentConfig(userProvidedConfig, component) if err != nil { updatedQuay, err = r.updateWithCondition(&quay, v1.ConditionTypeRolloutBlocked, metav1.ConditionTrue, v1.ConditionReasonConfigInvalid, err.Error()) if err != nil { @@ -463,6 +463,7 @@ func (r *QuayRegistryReconciler) createOrUpdateObject(ctx context.Context, obj k immutableResources := map[string]bool{ schema.GroupVersionKind{Group: "batch", Version: "v1", Kind: "Job"}.String(): true, + schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Service"}.String(): true, } log := r.Log.WithValues( diff --git a/kustomize/base/quay.service.yaml b/kustomize/base/quay.service.yaml index c4c8d7d28..df5ba6c01 100644 --- a/kustomize/base/quay.service.yaml +++ b/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 diff --git a/pkg/configure/configure.go b/pkg/configure/configure.go index 25b74480a..2d97f05e3 100644 --- a/pkg/configure/configure.go +++ b/pkg/configure/configure.go @@ -74,7 +74,7 @@ func ReconfigureHandler(k8sClient client.Client) func(w http.ResponseWriter, r * // Infer managed/unmanaged components from the given `config.yaml`. newComponents := []v1.Component{} for _, component := range quay.Spec.Components { - contains, err := kustomize.ContainsComponentConfig(reconfigureRequest.Config, component.Kind) + contains, err := kustomize.ContainsComponentConfig(reconfigureRequest.Config, component) if err != nil { log.Error(err, "failed to check `config.yaml` for component fieldgroup", "component", component.Kind) diff --git a/pkg/kustomize/secrets.go b/pkg/kustomize/secrets.go index 9984a3c5b..8b855b937 100644 --- a/pkg/kustomize/secrets.go +++ b/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,12 +167,8 @@ 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. @@ -182,10 +176,13 @@ func EnsureTLSFor(ctx *quaycontext.QuayRegistryContext, quay *v1.QuayRegistry, t 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()) } } @@ -195,10 +192,10 @@ func EnsureTLSFor(ctx *quaycontext.QuayRegistryContext, quay *v1.QuayRegistry, t // ContainsComponentConfig accepts a full `config.yaml` and determines if it contains // the fieldgroup for the given component by comparing it with the fieldgroup defaults. // TODO: Replace this with function from `config-tool` library once implemented. -func ContainsComponentConfig(fullConfig map[string]interface{}, component v1.ComponentKind) (bool, error) { +func ContainsComponentConfig(fullConfig map[string]interface{}, component v1.Component) (bool, error) { fields := []string{} - switch component { + switch component.Kind { case v1.ComponentClair: fields = (&securityscanner.SecurityScannerFieldGroup{}).Fields() case v1.ComponentPostgres: @@ -213,16 +210,20 @@ func ContainsComponentConfig(fullConfig map[string]interface{}, component v1.Com case v1.ComponentMirror: fields = (&repomirror.RepoMirrorFieldGroup{}).Fields() case v1.ComponentRoute: - for _, field := range (&hostsettings.HostSettingsFieldGroup{}).Fields() { - // SERVER_HOSTNAME is a special field which we allow when using managed `route` component. - if field != "SERVER_HOSTNAME" { - fields = append(fields, field) + fields = (&hostsettings.HostSettingsFieldGroup{}).Fields() + + if component.Managed { + for i, field := range fields { + // SERVER_HOSTNAME is a special field which we allow when using managed `route` component. + if field == "SERVER_HOSTNAME" { + fields = append(fields[:i], fields[i+1:]...) + } } } case v1.ComponentMonitoring: return false, nil default: - panic("unknown component: " + component) + panic("unknown component: " + component.Kind) } // FIXME: Only checking for the existance of a single field diff --git a/pkg/kustomize/secrets_test.go b/pkg/kustomize/secrets_test.go index 7a3373de5..d6f612e46 100644 --- a/pkg/kustomize/secrets_test.go +++ b/pkg/kustomize/secrets_test.go @@ -1,9 +1,8 @@ package kustomize import ( - "net" + "fmt" "net/url" - "strings" "testing" "github.com/stretchr/testify/assert" @@ -192,6 +191,7 @@ func TestFieldGroupFor(t *testing.T) { var containsComponentConfigTests = []struct { name string component v1.ComponentKind + managed bool rawConfig string expected bool expectedError error @@ -199,6 +199,7 @@ var containsComponentConfigTests = []struct { { "ClairContains", "clair", + true, `FEATURE_SECURITY_SCANNER: true`, true, nil, @@ -206,6 +207,7 @@ var containsComponentConfigTests = []struct { { "ClairDoesNotContain", "clair", + true, ``, false, nil, @@ -213,6 +215,7 @@ var containsComponentConfigTests = []struct { { "PostgresContains", "postgres", + true, `DB_URI: postgresql://test-quay-database:postgres@test-quay-database:5432/test-quay-database`, true, nil, @@ -220,6 +223,7 @@ var containsComponentConfigTests = []struct { { "PostgresDoesNotContain", "postgres", + true, ``, false, nil, @@ -227,6 +231,7 @@ var containsComponentConfigTests = []struct { { "RedisContains", "redis", + true, `BUILDLOGS_REDIS: host: test-quay-redis `, @@ -236,6 +241,7 @@ var containsComponentConfigTests = []struct { { "RedisDoesNotContain", "redis", + true, ``, false, nil, @@ -243,6 +249,7 @@ var containsComponentConfigTests = []struct { { "ObjectStorageContains", "objectstorage", + true, `DISTRIBUTED_STORAGE_PREFERENCE: - local_us `, @@ -252,6 +259,7 @@ var containsComponentConfigTests = []struct { { "ObjectStorageDoesNotContain", "objectstorage", + true, ``, false, nil, @@ -259,6 +267,7 @@ var containsComponentConfigTests = []struct { { "MirrorContains", "mirror", + true, `FEATURE_REPO_MIRROR: true`, true, nil, @@ -266,6 +275,7 @@ var containsComponentConfigTests = []struct { { "MirrorDeosNotContain", "mirror", + true, ``, false, nil, @@ -273,6 +283,7 @@ var containsComponentConfigTests = []struct { { "RouteContains", "route", + true, `PREFERRED_URL_SCHEME: http`, true, nil, @@ -280,6 +291,7 @@ var containsComponentConfigTests = []struct { { "RouteContainsServerHostname", "route", + true, `SERVER_HOSTNAME: registry.skynet.com`, false, nil, @@ -287,13 +299,31 @@ var containsComponentConfigTests = []struct { { "RouteDoesNotContain", "route", + true, + ``, + false, + nil, + }, + { + "RouteUnmanagedDoesNotContain", + "route", + false, ``, false, nil, }, + { + "RouteUnmanagedContainsServerHostname", + "route", + false, + `SERVER_HOSTNAME: registry.skynet.com`, + true, + nil, + }, { "HorizontalPodAutoscalerDoesNotContain", "horizontalpodautoscaler", + true, ``, false, nil, @@ -308,7 +338,7 @@ func TestContainsComponentConfig(t *testing.T) { err := yaml.Unmarshal([]byte(test.rawConfig), &fullConfig) assert.Nil(err, test.name) - contains, err := ContainsComponentConfig(fullConfig, test.component) + contains, err := ContainsComponentConfig(fullConfig, v1.Component{Kind: test.component, Managed: test.managed}) if test.expectedError != nil { assert.NotNil(err, test.name) @@ -319,94 +349,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")) + } + } + } }