diff --git a/pkg/operator/controller/certificate-publisher/controller.go b/pkg/operator/controller/certificate-publisher/controller.go index fa1b90556e..d5af5b5f56 100644 --- a/pkg/operator/controller/certificate-publisher/controller.go +++ b/pkg/operator/controller/certificate-publisher/controller.go @@ -1,6 +1,10 @@ -// The certificate-publisher controller is responsible for publishing in-use -// certificates to the "router-certs" secret in the "openshift-config-managed" -// namespace. +// The certificate-publisher controller is responsible for publishing the +// certificate and key of the ingresscontroller for the cluster ingress domain +// to the "router-certs" secret in the "openshift-config-managed" namespace and +// for publishing the certificate for the default ingresscontroller to the +// "default-ingress-cert" configmap in the same namespace. Note that the +// "default" ingresscontroller is typically but not necessarily the +// ingresscontroller with the cluster ingress domain. package certificatepublisher import ( @@ -8,12 +12,14 @@ import ( "fmt" logf "github.com/openshift/cluster-ingress-operator/pkg/log" + "github.com/openshift/cluster-ingress-operator/pkg/manifests" "github.com/openshift/cluster-ingress-operator/pkg/operator/controller" "k8s.io/client-go/tools/record" corev1 "k8s.io/api/core/v1" + configv1 "github.com/openshift/api/config/v1" operatorv1 "github.com/openshift/api/operator/v1" "k8s.io/apimachinery/pkg/api/errors" @@ -47,7 +53,10 @@ type reconciler struct { } // New returns a new controller that publishes a "router-certs" secret in the -// openshift-config-managed namespace with all in-use default certificates. +// openshift-config-managed namespace with the certificate and key for the +// ingresscontroller for the cluster ingress domain, as well as a +// "default-ingress-cert" configmap with the certificate for the "default" +// ingresscontroller (which is usually the same ingresscontroller). func New(mgr manager.Manager, operatorNamespace, operandNamespace string) (runtimecontroller.Controller, error) { operatorCache := mgr.GetCache() reconciler := &reconciler{ @@ -63,8 +72,8 @@ func New(mgr manager.Manager, operatorNamespace, operandNamespace string) (runti } // Index ingresscontrollers over the default certificate name so that - // secretIsInUse and secretToIngressController can look up - // ingresscontrollers that reference the secret. + // secretToIngressController can look up ingresscontrollers that + // reference the secret. if err := operatorCache.IndexField(context.Background(), &operatorv1.IngressController{}, "defaultCertificateName", client.IndexerFunc(func(o client.Object) []string { secret := controller.RouterEffectiveDefaultCertificateSecretName(o.(*operatorv1.IngressController), operandNamespace) return []string{secret.Name} @@ -76,12 +85,7 @@ func New(mgr manager.Manager, operatorNamespace, operandNamespace string) (runti if err != nil { return nil, fmt.Errorf("failed to create informer for secrets: %v", err) } - if err := c.Watch(&source.Informer{Informer: secretsInformer}, handler.EnqueueRequestsFromMapFunc(reconciler.secretToIngressController), predicate.Funcs{ - CreateFunc: func(e event.CreateEvent) bool { return reconciler.secretIsInUse(e.Object) }, - DeleteFunc: func(e event.DeleteEvent) bool { return reconciler.secretIsInUse(e.Object) }, - UpdateFunc: func(e event.UpdateEvent) bool { return reconciler.secretIsInUse(e.ObjectNew) }, - GenericFunc: func(e event.GenericEvent) bool { return reconciler.secretIsInUse(e.Object) }, - }); err != nil { + if err := c.Watch(&source.Informer{Informer: secretsInformer}, handler.EnqueueRequestsFromMapFunc(reconciler.secretToIngressController)); err != nil { return nil, err } @@ -90,7 +94,9 @@ func New(mgr manager.Manager, operatorNamespace, operandNamespace string) (runti DeleteFunc: func(e event.DeleteEvent) bool { return reconciler.hasSecret(e.Object, e.Object) }, UpdateFunc: func(e event.UpdateEvent) bool { return reconciler.secretChanged(e.ObjectOld, e.ObjectNew) }, GenericFunc: func(e event.GenericEvent) bool { return reconciler.hasSecret(e.Object, e.Object) }, - }); err != nil { + }, predicate.NewPredicateFuncs(func(o client.Object) bool { + return reconciler.hasClusterIngressDomain(o) || isDefaultIngressController(o) + })); err != nil { return nil, err } @@ -100,14 +106,27 @@ func New(mgr manager.Manager, operatorNamespace, operandNamespace string) (runti // secretToIngressController maps a secret to a slice of reconcile requests, // one request per ingresscontroller that references the secret. func (r *reconciler) secretToIngressController(o client.Object) []reconcile.Request { - requests := []reconcile.Request{} - controllers, err := r.ingressControllersWithSecret(o.GetName()) - if err != nil { - log.Error(err, "failed to list ingresscontrollers for secret", "related", o.GetSelfLink()) + var ( + requests []reconcile.Request + list operatorv1.IngressControllerList + listOpts = client.MatchingFields(map[string]string{ + "defaultCertificateName": o.GetName(), + }) + ingressConfig configv1.Ingress + ) + if err := r.cache.List(context.Background(), &list, listOpts); err != nil { + log.Error(err, "failed to list ingresscontrollers for secret", "secret", o.GetName()) + return requests + } + if err := r.cache.Get(context.Background(), controller.IngressClusterConfigName(), &ingressConfig); err != nil { + log.Error(err, "failed to get ingresses.config.openshift.io", "name", controller.IngressClusterConfigName()) return requests } - for _, ic := range controllers { - log.Info("queueing ingresscontroller", "name", ic.Name, "related", o.GetSelfLink()) + for _, ic := range list.Items { + if ic.Status.Domain != ingressConfig.Spec.Domain { + continue + } + log.Info("queueing ingresscontroller", "name", ic.Name) request := reconcile.Request{ NamespacedName: types.NamespacedName{ Namespace: ic.Namespace, @@ -119,27 +138,6 @@ func (r *reconciler) secretToIngressController(o client.Object) []reconcile.Requ return requests } -// ingressControllersWithSecret returns the ingresscontrollers that reference -// the given secret. -func (r *reconciler) ingressControllersWithSecret(secretName string) ([]operatorv1.IngressController, error) { - controllers := &operatorv1.IngressControllerList{} - if err := r.cache.List(context.Background(), controllers, client.MatchingFields(map[string]string{"defaultCertificateName": secretName})); err != nil { - return nil, err - } - return controllers.Items, nil -} - -// secretIsInUse returns true if the given secret is referenced by some -// ingresscontroller. -func (r *reconciler) secretIsInUse(meta metav1.Object) bool { - controllers, err := r.ingressControllersWithSecret(meta.GetName()) - if err != nil { - log.Error(err, "failed to list ingresscontrollers for secret", "related", meta.GetSelfLink()) - return false - } - return len(controllers) > 0 -} - // hasSecret returns true if the effective default certificate secret for the // given ingresscontroller exists, false otherwise. func (r *reconciler) hasSecret(meta metav1.Object, o runtime.Object) bool { @@ -168,6 +166,26 @@ func (r *reconciler) secretChanged(old, new runtime.Object) bool { return oldSecret != newSecret || oldStatus != newStatus } +// hasClusterIngressDomain returns true if the effective domain for the given +// ingresscontroller is the cluster ingress domain. +func (r *reconciler) hasClusterIngressDomain(o client.Object) bool { + var ingressConfig configv1.Ingress + if err := r.cache.Get(context.Background(), controller.IngressClusterConfigName(), &ingressConfig); err != nil { + log.Error(err, "failed to get ingresses.config.openshift.io", "name", controller.IngressClusterConfigName()) + // Assume it might be a match. Better to reconcile an extra + // time than to miss an update. + return true + } + ic := o.(*operatorv1.IngressController) + return ic.Status.Domain == ingressConfig.Spec.Domain +} + +// isDefaultIngressController returns true if the given ingresscontroller is the +// "default" ingresscontroller. +func isDefaultIngressController(o client.Object) bool { + return o.GetNamespace() == controller.DefaultOperatorNamespace && o.GetName() == manifests.DefaultIngressControllerName +} + func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) (reconcile.Result, error) { log.Info("Reconciling", "request", request) @@ -181,9 +199,50 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( return reconcile.Result{}, fmt.Errorf("failed to list secrets: %v", err) } - if err := r.ensureRouterCertsGlobalSecret(secrets.Items, controllers.Items); err != nil { + var ingressConfig configv1.Ingress + if err := r.cache.Get(ctx, controller.IngressClusterConfigName(), &ingressConfig); err != nil { + return reconcile.Result{}, fmt.Errorf("failed to get ingresses.config.openshift.io %s: %v", controller.IngressClusterConfigName(), err) + } + + if err := r.ensureRouterCertsGlobalSecret(secrets.Items, controllers.Items, &ingressConfig); err != nil { return reconcile.Result{}, fmt.Errorf("failed to ensure global secret: %v", err) } + // We need to construct the CA bundle that can be used to verify the ingress used to serve the console and the oauth-server. + // In an operator maintained cluster, this is always `oc get -n openshift-ingress-operator ingresscontroller/default`, skip the rest and return here. + // TODO if network-edge wishes to expand the scope of the CA bundle (and you could legitimately see a need/desire to have one CA that verifies all ingress traffic). + // TODO this could be accomplished using union logic similar to the kube-apiserver's join of multiple CAs. + if request.NamespacedName.Namespace == controller.DefaultOperatorNamespace && request.NamespacedName.Name == manifests.DefaultIngressControllerName { + var defaultIngressController *operatorv1.IngressController + for i := range controllers.Items { + ic := &controllers.Items[i] + if isDefaultIngressController(ic) { + defaultIngressController = ic + break + } + } + if defaultIngressController == nil { + return reconcile.Result{}, fmt.Errorf("failed to lookup default ingresscontroller %s does not exist", manifests.DefaultIngressControllerName) + } + + var wildcardServingCertKeySecret *corev1.Secret + secretName := controller.RouterEffectiveDefaultCertificateSecretName(defaultIngressController, controller.DefaultOperandNamespace) + for i := range secrets.Items { + secret := &secrets.Items[i] + if secret.Namespace == secretName.Namespace && secret.Name == secretName.Name { + wildcardServingCertKeySecret = secret + break + } + } + if wildcardServingCertKeySecret == nil { + return reconcile.Result{}, fmt.Errorf("failed to lookup wildcard cert: secret %s does not exist", secretName) + } + + caBundle := string(wildcardServingCertKeySecret.Data["tls.crt"]) + if err := r.ensureDefaultIngressCertConfigMap(caBundle); err != nil { + return reconcile.Result{}, fmt.Errorf("failed to publish router CA: %w", err) + } + } + return reconcile.Result{}, nil } diff --git a/pkg/operator/controller/certificate/publish_ca.go b/pkg/operator/controller/certificate-publisher/publish_ca.go similarity index 99% rename from pkg/operator/controller/certificate/publish_ca.go rename to pkg/operator/controller/certificate-publisher/publish_ca.go index 4c9fe5a8fd..148ec6d856 100644 --- a/pkg/operator/controller/certificate/publish_ca.go +++ b/pkg/operator/controller/certificate-publisher/publish_ca.go @@ -1,4 +1,4 @@ -package certificate +package certificatepublisher import ( "context" diff --git a/pkg/operator/controller/certificate-publisher/publish_certs.go b/pkg/operator/controller/certificate-publisher/publish_certs.go index ea056aeac8..99e0086119 100644 --- a/pkg/operator/controller/certificate-publisher/publish_certs.go +++ b/pkg/operator/controller/certificate-publisher/publish_certs.go @@ -6,8 +6,10 @@ import ( "fmt" "reflect" + configv1 "github.com/openshift/api/config/v1" operatorv1 "github.com/openshift/api/operator/v1" "github.com/openshift/cluster-ingress-operator/pkg/operator/controller" + "github.com/openshift/cluster-ingress-operator/pkg/util/ingresscontroller" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" @@ -16,8 +18,8 @@ import ( // ensureRouterCertsGlobalSecret will create, update, or delete the global // certificates secret as appropriate. -func (r *reconciler) ensureRouterCertsGlobalSecret(secrets []corev1.Secret, ingresses []operatorv1.IngressController) error { - desired, err := desiredRouterCertsGlobalSecret(secrets, ingresses, r.operandNamespace) +func (r *reconciler) ensureRouterCertsGlobalSecret(secrets []corev1.Secret, ingresses []operatorv1.IngressController, ingressConfig *configv1.Ingress) error { + desired, err := desiredRouterCertsGlobalSecret(secrets, ingresses, r.operandNamespace, ingressConfig.Spec.Domain) if err != nil { return err } @@ -56,55 +58,84 @@ func (r *reconciler) ensureRouterCertsGlobalSecret(secrets []corev1.Secret, ingr // desiredRouterCertsGlobalSecret returns the desired router-certs global // secret. -func desiredRouterCertsGlobalSecret(secrets []corev1.Secret, ingresses []operatorv1.IngressController, operandNamespace string) (*corev1.Secret, error) { - if len(ingresses) == 0 || len(secrets) == 0 { - return nil, nil - } - - nameToSecret := map[string]*corev1.Secret{} - for i, certSecret := range secrets { - nameToSecret[certSecret.Name] = &secrets[i] - } +func desiredRouterCertsGlobalSecret(secrets []corev1.Secret, ingresses []operatorv1.IngressController, operandNamespace, clusterIngressDomain string) (*corev1.Secret, error) { + for i := range ingresses { + // The authentication operator only requires the certificate and + // key for the ingresscontroller that handles the cluster + // ingress domain, and publishing certificates for all + // ingresscontrollers could cause the secret's size to exceed + // the maximum secret size of 1 mebibyte. See + // . + if ingresses[i].Status.Domain != clusterIngressDomain { + continue + } - ingressToSecret := map[*operatorv1.IngressController]*corev1.Secret{} - for i, ingress := range ingresses { - // Check if ingress.Spec.DefaultCertificate is an available secret - // in the secrets slice. If it is not, attempt to fall back to the - // operator generated default certificate. If ingress.Spec.DefaultCertificate - // is updated to point to a non-existent secret, the certificate controller - // will not delete the operator generated default certificate. - // See https://bugzilla.redhat.com/show_bug.cgi?id=1887441 - if defaultCert := ingress.Spec.DefaultCertificate; defaultCert != nil { - if secret, ok := nameToSecret[defaultCert.Name]; ok { - ingressToSecret[&ingresses[i]] = secret - continue - } + // Multiple ingresscontrollers can have the cluster ingress + // domain, but only one can be admitted. If this + // ingresscontroller hasn't been admitted, it should be ignored + // so that we use the default certificate of an + // ingresscontroller that has been admitted. + if !ingresscontroller.IsAdmitted(&ingresses[i]) { + continue } - name := controller.RouterOperatorGeneratedDefaultCertificateSecretName(&ingress, operandNamespace) - if secret, ok := nameToSecret[name.Name]; ok { - ingressToSecret[&ingresses[i]] = secret + + cert := getDefaultCertificateSecretForIngressController(&ingresses[i], secrets, operandNamespace) + if cert == nil { + break } - } - name := controller.RouterCertsGlobalSecretName() - globalSecret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: name.Name, - Namespace: name.Namespace, - }, - Data: map[string][]byte{}, + globalCertName := controller.RouterCertsGlobalSecretName() + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: globalCertName.Name, + Namespace: globalCertName.Namespace, + }, + Data: map[string][]byte{ + ingresses[i].Status.Domain: bytes.Join([][]byte{ + cert.Data["tls.crt"], + cert.Data["tls.key"], + }, nil), + }, + }, nil } - for ingress, certSecret := range ingressToSecret { - if len(ingress.Status.Domain) == 0 { - continue + + return nil, nil +} + +// getDefaultCertificateSecretForIngressController returns the appropriate +// secret to use for the given ingresscontroller out of the provided list of +// secrets. If the ingresscontroller does not specify a secret or specifies a +// secret that doesn't exist, the operator-generated default certificate is +// returned. +// +// Note that if ingress.Spec.DefaultCertificate is updated to point to a +// non-existent secret, the certificate controller does not delete the +// operator-generated default certificate, so it is safe to fall back to using +// it. See . +// +// Note also that defaultCertName can be the same as customCertName; see +// . +func getDefaultCertificateSecretForIngressController(ic *operatorv1.IngressController, secrets []corev1.Secret, operandNamespace string) *corev1.Secret { + var ( + defaultCertName = controller.RouterOperatorGeneratedDefaultCertificateSecretName(ic, operandNamespace) + customCertName = ic.Spec.DefaultCertificate + defaultCert, customCert *corev1.Secret + ) + for i := range secrets { + if customCertName != nil && customCertName.Name == secrets[i].Name { + customCert = &secrets[i] } - pem := bytes.Join([][]byte{ - certSecret.Data["tls.crt"], - certSecret.Data["tls.key"], - }, nil) - globalSecret.Data[ingress.Status.Domain] = pem + if defaultCertName.Name == secrets[i].Name { + defaultCert = &secrets[i] + } + } + // Prefer any custom certificate, and fall back to the default + // certificate if no custom certificate was specified or the specified + // one was not found. + if customCert != nil { + return customCert } - return globalSecret, nil + return defaultCert } // currentRouterCertsGlobalSecret returns the current router-certs global diff --git a/pkg/operator/controller/certificate-publisher/publish_certs_test.go b/pkg/operator/controller/certificate-publisher/publish_certs_test.go index 84ea06ee13..214f4e8dbf 100644 --- a/pkg/operator/controller/certificate-publisher/publish_certs_test.go +++ b/pkg/operator/controller/certificate-publisher/publish_certs_test.go @@ -5,64 +5,23 @@ import ( "testing" operatorv1 "github.com/openshift/api/operator/v1" + "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/ingress" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) // newSecret returns a secret with the specified name and with data fields -// "tls.crt" and "tls.key" containing valid PEM-encoded certificate and private -// key, respectively. Note that the certificate and key are valid only in the -// sense that they respect PEM encoding, not that they have any particular -// subject etc. +// "tls.crt" and "tls.key" set to the secret's name. Note that the values for +// "tls.crt" and "tls.key" are not valid PEM data. func newSecret(name string) corev1.Secret { - const ( - // defaultCert is a PEM-encoded certificate. - defaultCert = `-----BEGIN CERTIFICATE----- -MIIDIjCCAgqgAwIBAgIBBjANBgkqhkiG9w0BAQUFADCBoTELMAkGA1UEBhMCVVMx -CzAJBgNVBAgMAlNDMRUwEwYDVQQHDAxEZWZhdWx0IENpdHkxHDAaBgNVBAoME0Rl -ZmF1bHQgQ29tcGFueSBMdGQxEDAOBgNVBAsMB1Rlc3QgQ0ExGjAYBgNVBAMMEXd3 -dy5leGFtcGxlY2EuY29tMSIwIAYJKoZIhvcNAQkBFhNleGFtcGxlQGV4YW1wbGUu -Y29tMB4XDTE2MDExMzE5NDA1N1oXDTI2MDExMDE5NDA1N1owfDEYMBYGA1UEAxMP -d3d3LmV4YW1wbGUuY29tMQswCQYDVQQIEwJTQzELMAkGA1UEBhMCVVMxIjAgBgkq -hkiG9w0BCQEWE2V4YW1wbGVAZXhhbXBsZS5jb20xEDAOBgNVBAoTB0V4YW1wbGUx -EDAOBgNVBAsTB0V4YW1wbGUwgZ8wDQYJKoZIhvcNAQEBBQADgY0AMIGJAoGBAM0B -u++oHV1wcphWRbMLUft8fD7nPG95xs7UeLPphFZuShIhhdAQMpvcsFeg+Bg9PWCu -v3jZljmk06MLvuWLfwjYfo9q/V+qOZVfTVHHbaIO5RTXJMC2Nn+ACF0kHBmNcbth -OOgF8L854a/P8tjm1iPR++vHnkex0NH7lyosVc/vAgMBAAGjDTALMAkGA1UdEwQC -MAAwDQYJKoZIhvcNAQEFBQADggEBADjFm5AlNH3DNT1Uzx3m66fFjqqrHEs25geT -yA3rvBuynflEHQO95M/8wCxYVyuAx4Z1i4YDC7tx0vmOn/2GXZHY9MAj1I8KCnwt -Jik7E2r1/yY0MrkawljOAxisXs821kJ+Z/51Ud2t5uhGxS6hJypbGspMS7OtBbw7 -8oThK7cWtCXOldNF6ruqY1agWnhRdAq5qSMnuBXuicOP0Kbtx51a1ugE3SnvQenJ -nZxdtYUXvEsHZC/6bAtTfNh+/SwgxQJuL2ZM+VG3X2JIKY8xTDui+il7uTh422lq -wED8uwKl+bOj6xFDyw4gWoBxRobsbFaME8pkykP1+GnKDberyAM= ------END CERTIFICATE----- -` - // defaultKey is a PEM-encoded private key. - defaultKey = `-----BEGIN RSA PRIVATE KEY----- -MIICWwIBAAKBgQDNAbvvqB1dcHKYVkWzC1H7fHw+5zxvecbO1Hiz6YRWbkoSIYXQ -EDKb3LBXoPgYPT1grr942ZY5pNOjC77li38I2H6Pav1fqjmVX01Rx22iDuUU1yTA -tjZ/gAhdJBwZjXG7YTjoBfC/OeGvz/LY5tYj0fvrx55HsdDR+5cqLFXP7wIDAQAB -AoGAfE7P4Zsj6zOzGPI/Izj7Bi5OvGnEeKfzyBiH9Dflue74VRQkqqwXs/DWsNv3 -c+M2Y3iyu5ncgKmUduo5X8D9To2ymPRLGuCdfZTxnBMpIDKSJ0FTwVPkr6cYyyBk -5VCbc470pQPxTAAtl2eaO1sIrzR4PcgwqrSOjwBQQocsGAECQQD8QOra/mZmxPbt -bRh8U5lhgZmirImk5RY3QMPI/1/f4k+fyjkU5FRq/yqSyin75aSAXg8IupAFRgyZ -W7BT6zwBAkEA0A0ugAGorpCbuTa25SsIOMxkEzCiKYvh0O+GfGkzWG4lkSeJqGME -keuJGlXrZNKNoCYLluAKLPmnd72X2yTL7wJARM0kAXUP0wn324w8+HQIyqqBj/gF -Vt9Q7uMQQ3s72CGu3ANZDFS2nbRZFU5koxrggk6lRRk1fOq9NvrmHg10AQJABOea -pgfj+yGLmkUw8JwgGH6xCUbHO+WBUFSlPf+Y50fJeO+OrjqPXAVKeSV3ZCwWjKT4 -9viXJNJJ4WfF0bO/XwJAOMB1wQnEOSZ4v+laMwNtMq6hre5K8woqteXICoGcIWe8 -u3YLAbyW/lHhOCiZu2iAI8AbmXem9lW6Tr7p/97s0w== ------END RSA PRIVATE KEY----- -` - ) return corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: name, }, Data: map[string][]byte{ - "tls.crt": []byte(defaultCert), - "tls.key": []byte(defaultKey), + "tls.crt": []byte(name), + "tls.key": []byte(name), }, } } @@ -70,12 +29,20 @@ u3YLAbyW/lHhOCiZu2iAI8AbmXem9lW6Tr7p/97s0w== // newIngressController returns a new ingresscontroller with the specified name, // default certificate secret name (or nil if empty), and ingress domain, for // use as a test input. -func newIngressController(name, defaultCertificateSecretName, domain string) operatorv1.IngressController { +func newIngressController(name, defaultCertificateSecretName, domain string, admitted bool) operatorv1.IngressController { + admittedStatus := operatorv1.ConditionFalse + if admitted { + admittedStatus = operatorv1.ConditionTrue + } ingresscontroller := operatorv1.IngressController{ ObjectMeta: metav1.ObjectMeta{ Name: name, }, Status: operatorv1.IngressControllerStatus{ + Conditions: []operatorv1.OperatorCondition{{ + Type: ingress.IngressControllerAdmittedConditionType, + Status: admittedStatus, + }}, Domain: domain, }, } @@ -98,17 +65,51 @@ func TestDesiredRouterCertsGlobalSecret(t *testing.T) { } var ( defaultCert = newSecret("router-certs-default") - defaultCI = newIngressController("default", "", "apps.my.devcluster.openshift.com") - ci1 = newIngressController("ci1", "s1", "dom1") - ci2 = newIngressController("ci2", "s2", "dom2") - s1 = newSecret("s1") - s2 = newSecret("s2") - // data has the PEM for defaultCert, s1, and s2 (which all have - // the same certificate and key). - data = bytes.Join([][]byte{ - s1.Data["tls.crt"], - s1.Data["tls.key"], + // defaultICWithDefaultCertUnspecified is an ingresscontroller + // named "default" that does not specify a default certificate. + // The operator should use the operator-generated default + // certificate (the "default default certificate") in this case. + defaultICWithDefaultCertUnspecified = newIngressController("default", "", "apps.my.devcluster.openshift.com", true) + + // defaultICWithDefaultCertSetToDefault is an ingresscontroller + // named "default" that specifies an explicit reference for a + // default certificate secret, where that secret is the same + // default one that the operator generates when none is + // specified (the "default default certificate"). + defaultICWithDefaultCertSetToDefault = newIngressController("default", "router-certs-default", "apps.my.devcluster.openshift.com", true) + + customDefaultCert = newSecret("custom-router-certs-default") + + // defaultICWithDefaultCertSetToCustom is an ingresscontroller + // named "default" that specifies a reference for a default + // certificate secret where that secret is a custom one (a + // "custom default certificate"). + defaultICWithDefaultCertSetToCustom = newIngressController("default", "custom-router-certs-default", "apps.my.devcluster.openshift.com", true) + + // customICWithClusterIngressDomain is an ingresscontroller + // named "custom" that specifies a reference for a default + // certificate secret where that secret is a custom one. The + // ingresscontroller also specifies the cluster ingress domain + // (which is usually owned by the "default" ingresscontroller. + customICWithClusterIngressDomain = newIngressController("custom", "custom-router-certs-default", "apps.my.devcluster.openshift.com", true) + + // invalidCustomICWithClusterIngressDomain is the same as + // customICWithClusterIngressDomain except that it has not been + // admitted by the operator. + invalidCustomICWithClusterIngressDomain = newIngressController("custom", "custom-router-certs-default", "apps.my.devcluster.openshift.com", false) + + ic1 = newIngressController("ic1", "s1", "dom1", true) + ic2 = newIngressController("ic2", "s2", "dom2", true) + s1 = newSecret("s1") + s2 = newSecret("s2") + defaultCertData = bytes.Join([][]byte{ + defaultCert.Data["tls.crt"], + defaultCert.Data["tls.key"], + }, nil) + customDefaultCertData = bytes.Join([][]byte{ + customDefaultCert.Data["tls.crt"], + customDefaultCert.Data["tls.key"], }, nil) ) testCases := []struct { @@ -117,14 +118,92 @@ func TestDesiredRouterCertsGlobalSecret(t *testing.T) { output testOutputs }{ { - description: "default configuration", + description: "default certificate, implicit", + inputs: testInputs{ + []operatorv1.IngressController{defaultICWithDefaultCertUnspecified}, + []corev1.Secret{defaultCert}, + }, + output: testOutputs{ + &corev1.Secret{ + Data: map[string][]byte{"apps.my.devcluster.openshift.com": defaultCertData}, + }, + }, + }, + // spec.defaultCertificate.name can specify the name of the + // operator-generated certificate; see + // . + { + description: "default certificate, explicit", + inputs: testInputs{ + []operatorv1.IngressController{defaultICWithDefaultCertSetToDefault}, + []corev1.Secret{defaultCert}, + }, + output: testOutputs{ + &corev1.Secret{ + Data: map[string][]byte{"apps.my.devcluster.openshift.com": defaultCertData}, + }, + }, + }, + { + description: "custom certificate", + inputs: testInputs{ + []operatorv1.IngressController{defaultICWithDefaultCertSetToCustom}, + []corev1.Secret{defaultCert, customDefaultCert}, + }, + output: testOutputs{ + &corev1.Secret{ + Data: map[string][]byte{"apps.my.devcluster.openshift.com": customDefaultCertData}, + }, + }, + }, + { + description: "custom certificate, with secrets having the custom one and then default one", + inputs: testInputs{ + []operatorv1.IngressController{defaultICWithDefaultCertSetToCustom}, + []corev1.Secret{customDefaultCert, defaultCert}, + }, + output: testOutputs{ + &corev1.Secret{ + Data: map[string][]byte{"apps.my.devcluster.openshift.com": customDefaultCertData}, + }, + }, + }, + // Fall back to the operator-generated default certificate if + // the specified secret doesn't exist; see + // . + { + description: "missing custom certificate", inputs: testInputs{ - []operatorv1.IngressController{defaultCI}, + []operatorv1.IngressController{defaultICWithDefaultCertSetToCustom}, []corev1.Secret{defaultCert}, }, output: testOutputs{ &corev1.Secret{ - Data: map[string][]byte{"apps.my.devcluster.openshift.com": data}, + Data: map[string][]byte{"apps.my.devcluster.openshift.com": defaultCertData}, + }, + }, + }, + { + description: "custom ingresscontroller with the cluster ingress domain", + inputs: testInputs{ + []operatorv1.IngressController{customICWithClusterIngressDomain}, + []corev1.Secret{customDefaultCert}, + }, + output: testOutputs{ + &corev1.Secret{ + Data: map[string][]byte{"apps.my.devcluster.openshift.com": customDefaultCertData}, + }, + }, + }, + { + description: "default certificate and custom ingresscontroller that conflicts on domain", + inputs: testInputs{ + []operatorv1.IngressController{invalidCustomICWithClusterIngressDomain, defaultICWithDefaultCertUnspecified}, + []corev1.Secret{defaultCert, customDefaultCert}, + }, + output: testOutputs{ + &corev1.Secret{ + Data: map[string][]byte{"apps.my.devcluster.openshift.com": defaultCertData}, }, }, }, @@ -139,7 +218,7 @@ func TestDesiredRouterCertsGlobalSecret(t *testing.T) { { description: "no secrets", inputs: testInputs{ - []operatorv1.IngressController{ci1}, + []operatorv1.IngressController{defaultICWithDefaultCertUnspecified}, []corev1.Secret{}, }, output: testOutputs{nil}, @@ -147,62 +226,57 @@ func TestDesiredRouterCertsGlobalSecret(t *testing.T) { { description: "missing secret", inputs: testInputs{ - []operatorv1.IngressController{ci1, ci2}, - []corev1.Secret{s1}, - }, - output: testOutputs{ - &corev1.Secret{ - Data: map[string][]byte{"dom1": data}, - }, + []operatorv1.IngressController{defaultICWithDefaultCertUnspecified}, + []corev1.Secret{s1, s2}, }, + output: testOutputs{nil}, }, { description: "extra secret", inputs: testInputs{ - []operatorv1.IngressController{ci2}, - []corev1.Secret{s1, s2}, + []operatorv1.IngressController{defaultICWithDefaultCertUnspecified, ic2}, + []corev1.Secret{s1, defaultCert, s2}, }, output: testOutputs{ &corev1.Secret{ - Data: map[string][]byte{"dom2": data}, + Data: map[string][]byte{"apps.my.devcluster.openshift.com": defaultCertData}, }, }, }, { description: "perfect match", inputs: testInputs{ - []operatorv1.IngressController{ci1, ci2}, - []corev1.Secret{s1, s2}, + []operatorv1.IngressController{defaultICWithDefaultCertUnspecified, ic1, ic2}, + []corev1.Secret{defaultCert, s1, s2}, }, output: testOutputs{ &corev1.Secret{ - Data: map[string][]byte{ - "dom1": data, - "dom2": data, - }, + Data: map[string][]byte{"apps.my.devcluster.openshift.com": defaultCertData}, }, }, }, } for _, tc := range testCases { - expected := tc.output.secret - actual, err := desiredRouterCertsGlobalSecret(tc.inputs.secrets, tc.inputs.ingresses, "openshift-ingress") - if err != nil { - t.Errorf("failed to get desired router-ca global secret: %v", err) - continue - } - if expected == nil || actual == nil { - if expected != nil { - t.Errorf("%q: expected %v, got nil", tc.description, expected) + t.Run(tc.description, func(t *testing.T) { + expected := tc.output.secret + actual, err := desiredRouterCertsGlobalSecret(tc.inputs.secrets, tc.inputs.ingresses, "openshift-ingress", "apps.my.devcluster.openshift.com") + if err != nil { + t.Errorf("failed to get desired router-ca global secret: %v", err) + } + if expected == nil || actual == nil { + if expected != nil { + t.Errorf("expected %v, got nil", expected) + } + if actual != nil { + t.Errorf("expected nil, got %v", actual) + } } - if actual != nil { - t.Errorf("%q: expected nil, got %v", tc.description, actual) + if expected != nil && actual != nil { + if !routerCertsSecretsEqual(expected, actual) { + t.Errorf("expected %v, got %v", expected, actual) + } } - continue - } - if !routerCertsSecretsEqual(expected, actual) { - t.Errorf("%q: expected %v, got %v", tc.description, expected, actual) - } + }) } } diff --git a/pkg/operator/controller/certificate/controller.go b/pkg/operator/controller/certificate/controller.go index 7d6d5cdf3d..2e2f3238eb 100644 --- a/pkg/operator/controller/certificate/controller.go +++ b/pkg/operator/controller/certificate/controller.go @@ -2,7 +2,6 @@ // // 1. Managing a CA for minting self-signed certs. // 2. Managing self-signed certificates for any ingresscontrollers which require them. -// 3. Publishing the CA to the openshift-config-managed namespace. package certificate import ( @@ -12,7 +11,6 @@ import ( logf "github.com/openshift/cluster-ingress-operator/pkg/log" "github.com/openshift/cluster-ingress-operator/pkg/operator/controller" - operatorcontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller" ingresscontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/ingress" "k8s.io/apimachinery/pkg/api/errors" @@ -22,7 +20,6 @@ import ( "k8s.io/client-go/tools/record" appsv1 "k8s.io/api/apps/v1" - corev1 "k8s.io/api/core/v1" operatorv1 "github.com/openshift/api/operator/v1" @@ -63,6 +60,8 @@ type reconciler struct { } func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) (reconcile.Result, error) { + log.Info("Reconciling", "request", request) + ca, err := r.ensureRouterCASecret() if err != nil { return reconcile.Result{}, fmt.Errorf("failed to ensure router CA: %v", err) @@ -108,23 +107,5 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( } } - // We need to construct the CA bundle that can be used to verify the ingress used to serve the console and the oauth-server. - // In an operator maintained cluster, this is always `oc get -n openshift-ingress-operator ingresscontroller/default`, skip the rest and return here. - // TODO if network-edge wishes to expand the scope of the CA bundle (and you could legitimately see a need/desire to have one CA that verifies all ingress traffic). - // TODO this could be accomplished using union logic similar to the kube-apiserver's join of multiple CAs. - if ingress == nil || ingress.Namespace != operatorcontroller.DefaultOperatorNamespace || ingress.Name != "default" { - return result, utilerrors.NewAggregate(errs) - } - - wildcardServingCertKeySecret := &corev1.Secret{} - if err := r.client.Get(ctx, controller.RouterEffectiveDefaultCertificateSecretName(ingress, operatorcontroller.DefaultOperandNamespace), wildcardServingCertKeySecret); err != nil { - errs = append(errs, fmt.Errorf("failed to lookup wildcard cert: %v", err)) - return result, utilerrors.NewAggregate(errs) - } - caBundle := string(wildcardServingCertKeySecret.Data["tls.crt"]) - if err := r.ensureDefaultIngressCertConfigMap(caBundle); err != nil { - errs = append(errs, fmt.Errorf("failed to publish router CA: %v", err)) - } - return result, utilerrors.NewAggregate(errs) } diff --git a/pkg/operator/controller/ingress/controller.go b/pkg/operator/controller/ingress/controller.go index b88089080f..fb9ef7a199 100644 --- a/pkg/operator/controller/ingress/controller.go +++ b/pkg/operator/controller/ingress/controller.go @@ -14,6 +14,7 @@ import ( "github.com/openshift/cluster-ingress-operator/pkg/manifests" operatorcontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller" routemetrics "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/route-metrics" + "github.com/openshift/cluster-ingress-operator/pkg/util/ingresscontroller" retryable "github.com/openshift/cluster-ingress-operator/pkg/util/retryableerror" "github.com/openshift/cluster-ingress-operator/pkg/util/slice" @@ -266,7 +267,7 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( // Admit if necessary. Don't process until admission succeeds. If admission is // successful, immediately re-queue to refresh state. - alreadyAdmitted := isAdmitted(ingress) + alreadyAdmitted := ingresscontroller.IsAdmitted(ingress) if !alreadyAdmitted || needsReadmission(ingress) { if err := r.admit(ingress, ingressConfig, platformStatus, dnsConfig, alreadyAdmitted); err != nil { switch err := err.(type) { @@ -375,15 +376,6 @@ func (r *reconciler) admit(current *operatorv1.IngressController, ingressConfig return nil } -func isAdmitted(ic *operatorv1.IngressController) bool { - for _, cond := range ic.Status.Conditions { - if cond.Type == IngressControllerAdmittedConditionType && cond.Status == operatorv1.ConditionTrue { - return true - } - } - return false -} - // needsReadmission returns a Boolean value indicating whether the given // ingresscontroller needs to be re-admitted. Re-admission is necessary in // order to revalidate mutable fields that are subject to admission checks. The @@ -773,7 +765,7 @@ func validateDomain(ic *operatorv1.IngressController) error { func validateDomainUniqueness(desired *operatorv1.IngressController, existing []operatorv1.IngressController) error { for i := range existing { current := existing[i] - if !isAdmitted(¤t) { + if !ingresscontroller.IsAdmitted(¤t) { continue } if desired.UID != current.UID && desired.Status.Domain == current.Status.Domain { diff --git a/pkg/util/ingresscontroller/ingresscontroller.go b/pkg/util/ingresscontroller/ingresscontroller.go new file mode 100644 index 0000000000..3a57aa3d43 --- /dev/null +++ b/pkg/util/ingresscontroller/ingresscontroller.go @@ -0,0 +1,28 @@ +package ingresscontroller + +import ( + operatorv1 "github.com/openshift/api/operator/v1" +) + +const ( + // ingressControllerAdmittedConditionType is the type for the + // IngressController status condition that indicates that the operator + // has validated and admitted the IngressController. + // + // TODO: Move the definition in this package and the one in the + // "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/ingress" + // package into a third package that the first two can import. + ingressControllerAdmittedConditionType = "Admitted" +) + +// IsAdmitted returns a Boolean value indicating whether the given +// ingresscontroller has been admitted, as indicated by its "Admitted" status +// condition. +func IsAdmitted(ic *operatorv1.IngressController) bool { + for _, cond := range ic.Status.Conditions { + if cond.Type == ingressControllerAdmittedConditionType && cond.Status == operatorv1.ConditionTrue { + return true + } + } + return false +} diff --git a/test/e2e/all_test.go b/test/e2e/all_test.go index ad5e2302b4..2f70de3d7e 100644 --- a/test/e2e/all_test.go +++ b/test/e2e/all_test.go @@ -23,8 +23,6 @@ func TestAll(t *testing.T) { t.Run("TestAWSELBConnectionIdleTimeout", TestAWSELBConnectionIdleTimeout) t.Run("TestClientTLS", TestClientTLS) t.Run("TestContainerLogging", TestContainerLogging) - t.Run("TestCreateIngressControllerThenSecret", TestCreateIngressControllerThenSecret) - t.Run("TestCreateSecretThenIngressController", TestCreateSecretThenIngressController) t.Run("TestCustomErrorpages", TestCustomErrorpages) t.Run("TestCustomIngressClass", TestCustomIngressClass) t.Run("TestDomainNotMatchingBase", TestDomainNotMatchingBase) @@ -102,7 +100,7 @@ func TestAll(t *testing.T) { t.Run("TestRouteHardStopAfterTestZeroLengthDuration", TestRouteHardStopAfterTestZeroLengthDuration) t.Run("TestRouteNbthreadIngressController", TestRouteNbthreadIngressController) t.Run("TestRouterCompressionOperation", TestRouterCompressionOperation) - t.Run("TestUpdateDefaultIngressController", TestUpdateDefaultIngressController) + t.Run("TestUpdateDefaultIngressControllerSecret", TestUpdateDefaultIngressControllerSecret) t.Run("TestCanaryRoute", TestCanaryRoute) t.Run("TestRouteHTTP2EnableAndDisableIngressConfig", TestRouteHTTP2EnableAndDisableIngressConfig) t.Run("TestRouteHardStopAfterEnableOnIngressConfig", TestRouteHardStopAfterEnableOnIngressConfig) diff --git a/test/e2e/certificate_publisher_test.go b/test/e2e/certificate_publisher_test.go deleted file mode 100644 index f6704a3215..0000000000 --- a/test/e2e/certificate_publisher_test.go +++ /dev/null @@ -1,115 +0,0 @@ -//go:build e2e -// +build e2e - -package e2e - -import ( - "context" - "testing" - "time" - - operatorv1 "github.com/openshift/api/operator/v1" - - "github.com/openshift/cluster-ingress-operator/pkg/operator/controller" - ingresscontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/ingress" - - corev1 "k8s.io/api/core/v1" - - "k8s.io/apimachinery/pkg/types" - "k8s.io/apimachinery/pkg/util/wait" - "k8s.io/apiserver/pkg/storage/names" -) - -// TestCreateIngressControllerThenSecret creates an ingresscontroller that -// references a secret that does not exist, then creates the secret and verifies -// that the operator updates the "router-certs" global secret. -func TestCreateIngressControllerThenSecret(t *testing.T) { - t.Parallel() - name := types.NamespacedName{Namespace: operatorNamespace, Name: names.SimpleNameGenerator.GenerateName("test-")} - ic := newPrivateController(name, name.Name+"."+dnsConfig.Spec.BaseDomain) - ic.Spec.DefaultCertificate = &corev1.LocalObjectReference{ - Name: name.Name, - } - if err := kclient.Create(context.TODO(), ic); err != nil { - t.Fatalf("failed to create ingresscontroller: %v", err) - } - defer assertIngressControllerDeleted(t, kclient, ic) - - conditions := []operatorv1.OperatorCondition{ - {Type: ingresscontroller.IngressControllerAdmittedConditionType, Status: operatorv1.ConditionTrue}, - } - err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, name, conditions...) - if err != nil { - t.Errorf("failed to observe expected conditions: %v", err) - } - - // Create the secret. - secret, err := createDefaultCertTestSecret(kclient, name.Name) - if err != nil { - t.Fatalf("failed to create secret: %v", err) - } - defer func() { - if err := kclient.Delete(context.TODO(), secret); err != nil { - t.Errorf("failed to delete secret: %v", err) - } - }() - - // Wait for the "router-certs" secret to be updated. - err = wait.PollImmediate(1*time.Second, 60*time.Second, func() (bool, error) { - globalSecret := &corev1.Secret{} - if err := kclient.Get(context.TODO(), controller.RouterCertsGlobalSecretName(), globalSecret); err != nil { - return false, nil - } - if _, ok := globalSecret.Data[ic.Spec.Domain]; !ok { - return false, nil - } - return true, nil - }) - if err != nil { - t.Fatalf("failed to observe updated global secret: %v", err) - } -} - -// TestCreateSecretThenIngressController creates a secret, then creates an -// ingresscontroller that references the secret and verifies that the operator -// updates the "router-certs" global secret. -func TestCreateSecretThenIngressController(t *testing.T) { - t.Parallel() - name := types.NamespacedName{Namespace: operatorNamespace, Name: names.SimpleNameGenerator.GenerateName("test-")} - - // Create the secret. - secret, err := createDefaultCertTestSecret(kclient, name.Name) - if err != nil { - t.Fatalf("failed to create secret: %v", err) - } - defer func() { - if err := kclient.Delete(context.TODO(), secret); err != nil { - t.Errorf("failed to delete secret: %v", err) - } - }() - - // Create the ingresscontroller. - ic := newPrivateController(name, name.Name+"."+dnsConfig.Spec.BaseDomain) - ic.Spec.DefaultCertificate = &corev1.LocalObjectReference{ - Name: name.Name, - } - if err := kclient.Create(context.TODO(), ic); err != nil { - t.Fatalf("failed to create ingresscontroller: %v", err) - } - defer assertIngressControllerDeleted(t, kclient, ic) - - // Wait for the "router-certs" secret to be updated. - err = wait.PollImmediate(1*time.Second, 60*time.Second, func() (bool, error) { - globalSecret := &corev1.Secret{} - if err := kclient.Get(context.TODO(), controller.RouterCertsGlobalSecretName(), globalSecret); err != nil { - return false, nil - } - if _, ok := globalSecret.Data[ic.Spec.Domain]; !ok { - return false, nil - } - return true, nil - }) - if err != nil { - t.Fatalf("failed to observe updated global secret: %v", err) - } -} diff --git a/test/e2e/operator_test.go b/test/e2e/operator_test.go index 5a5554f2f5..72f8937fde 100644 --- a/test/e2e/operator_test.go +++ b/test/e2e/operator_test.go @@ -464,10 +464,31 @@ func TestProxyProtocolAPI(t *testing.T) { } } -// NOTE: This test will mutate the default ingresscontroller. +// TestUpdateDefaultIngressControllerSecret verifies that the operator properly +// updates the "router-default" deployment, the "default-ingress-cert" +// configmap, and the "router-certs" secret when the default ingresscontroller's +// default- certificate secret reference is updated. // -// TODO: Find a way to do this test without mutating the default ingress? -func TestUpdateDefaultIngressController(t *testing.T) { +// First, the test verifies that the default ingresscontroller has the cluster +// ingress domain and that the router-default deployment is using the secret +// reference that the ingresscontroller is initially configured with. +// +// Next, the test updates the default ingresscontroller to reference a +// default-certificate secret that does not exist and verifies that the operator +// does not update the configmap or router-certs secret. +// +// Next, the test creates the referenced default-certificate secret and verifies +// that the operator updates the deployment, configmap, and router-certs secret. +// +// Finally, the test restores the original default-certificate secret reference +// and verifies that the operator updates the deployment, configmap, and +// router-certs secret to use a new, operator-generated default certificate. +// +// This is a serial test because it modifies the default ingresscontroller. The +// test needs to verify that the operator updates the default-ingress-cert +// configmap and router-certs secret correctly, and the operator updates these +// only when the default ingresscontroller's default certificate changes. +func TestUpdateDefaultIngressControllerSecret(t *testing.T) { ic := &operatorv1.IngressController{} if err := kclient.Get(context.TODO(), defaultName, ic); err != nil { t.Fatalf("failed to get default ingresscontroller: %v", err) @@ -476,11 +497,26 @@ func TestUpdateDefaultIngressController(t *testing.T) { t.Fatalf("failed to observe expected conditions: %v", err) } + // Verify that the default ingresscontroller has the cluster ingress + // domain. + ingressConfig := &configv1.Ingress{} + if err := kclient.Get(context.TODO(), clusterConfigName, ingressConfig); err != nil { + t.Fatalf("failed to get the cluster ingress config: %v", err) + } + if ic.Status.Domain != ingressConfig.Spec.Domain { + t.Fatalf("default ingresscontroller's domain %q does not match the cluster ingress domain %q", ic.Status.Domain, ingressConfig.Spec.Domain) + } + defaultIngressCAConfigmap := &corev1.ConfigMap{} if err := kclient.Get(context.TODO(), controller.DefaultIngressCertConfigMapName(), defaultIngressCAConfigmap); err != nil { t.Fatalf("failed to get CA certificate configmap: %v", err) } + routerCertsSecret := &corev1.Secret{} + if err := kclient.Get(context.TODO(), controller.RouterCertsGlobalSecretName(), routerCertsSecret); err != nil { + t.Fatalf("failed to get router-certs secret: %v", err) + } + // Verify that the deployment uses the secret name specified in the // ingress controller, or the default if none is set, and store the // secret name (if any) so we can reset it at the end of the test. @@ -498,27 +534,65 @@ func TestUpdateDefaultIngressController(t *testing.T) { expectedSecretName, deployment.Spec.Template.Spec.Volumes[0].Secret.SecretName) } - // Update the ingress controller and wait for the deployment to match. - secret, err := createDefaultCertTestSecret(kclient, names.SimpleNameGenerator.GenerateName("test-")) - if err != nil { - t.Fatalf("creating default cert test secret: %v", err) + // Update the ingresscontroller to reference a non-existent secret. + secretName := names.SimpleNameGenerator.GenerateName("test-") + if err := updateIngressControllerSpecWithRetryOnConflict(t, defaultName, timeout, func(spec *operatorv1.IngressControllerSpec) { + spec.DefaultCertificate = &corev1.LocalObjectReference{Name: secretName} + }); err != nil { + t.Fatalf("failed to update default ingress controller: %v", err) } + // One of the last steps in this test reverts the previous update, but + // the test also needs to revert the update if it fails midway. If the + // test reaches the end, the following update is redundant but harmless. defer func() { - if err := kclient.Delete(context.TODO(), secret); err != nil { - t.Errorf("failed to delete test secret: %v", err) + if err := updateIngressControllerSpecWithRetryOnConflict(t, defaultName, timeout, func(spec *operatorv1.IngressControllerSpec) { + spec.DefaultCertificate = originalSecret + }); err != nil { + t.Fatalf("failed to reset default ingresscontroller: %v", err) } }() - if err := updateIngressControllerSpecWithRetryOnConflict(t, defaultName, timeout, func(spec *operatorv1.IngressControllerSpec) { - spec.DefaultCertificate = &corev1.LocalObjectReference{Name: secret.Name} - }); err != nil { - t.Fatalf("failed to update default ingress controller: %v", err) + // Verify that the default-ingress-cert configmap and router-certs + // secret remain unchanged. (The operator does update the router + // deployment with the new secret, but the kubelet volume manager does + // not reflect the update as long as the referenced secret does not + // exist.) If there is no update after 20 seconds, assume everything is + // fine. + previousDefaultIngressCAConfigmap := defaultIngressCAConfigmap.DeepCopy() + previousRouterCertsSecret := routerCertsSecret.DeepCopy() + wait.PollImmediate(1*time.Second, timeout, func() (bool, error) { + if err := kclient.Get(context.TODO(), controller.DefaultIngressCertConfigMapName(), defaultIngressCAConfigmap); err != nil { + t.Fatalf("failed to get CA certificate configmap: %v", err) + } + if defaultIngressCAConfigmap.Data["ca-bundle.crt"] != previousDefaultIngressCAConfigmap.Data["ca-bundle.crt"] { + t.Fatalf("observed an unexpected update to the CA certificate configmap\nold ca-bundle.crt: %v\nnew ca-bundle.crt: %v", previousDefaultIngressCAConfigmap.Data["ca-bundle.crt"], defaultIngressCAConfigmap.Data["ca-bundle.crt"]) + } + + if err := kclient.Get(context.TODO(), controller.RouterCertsGlobalSecretName(), routerCertsSecret); err != nil { + t.Fatalf("failed to get the router-certs secret: %v", err) + } + if !reflect.DeepEqual(routerCertsSecret.Data, previousRouterCertsSecret.Data) { + t.Fatalf("observed an unexpected update to the router-certs secret\nold: %v\nnew: %v", previousRouterCertsSecret, routerCertsSecret) + } + + return false, nil + }) + + // Create the secret and wait for the deployment to match. + secret, err := createDefaultCertTestSecret(kclient, secretName) + if err != nil { + t.Fatalf("failed to create secret %s: %v", secretName, err) } + defer func() { + if err := kclient.Delete(context.TODO(), secret); err != nil { + t.Errorf("failed to delete secret %s: %v", secretName, err) + } + }() - name := types.NamespacedName{Namespace: deployment.Namespace, Name: deployment.Name} - err = wait.PollImmediate(1*time.Second, 15*time.Second, func() (bool, error) { - if err := kclient.Get(context.TODO(), name, deployment); err != nil { - t.Logf("failed to get deployment %s: %v", name, err) + // Verify that the deployment uses the new certificate. + err = wait.PollImmediate(1*time.Second, timeout, func() (bool, error) { + if err := kclient.Get(context.TODO(), controller.RouterDeploymentName(ic), deployment); err != nil { + t.Logf("failed to get deployment %s: %v", controller.RouterDeploymentName(ic), err) return false, nil } if deployment.Spec.Template.Spec.Volumes[0].Secret.SecretName != secret.Name { @@ -530,9 +604,8 @@ func TestUpdateDefaultIngressController(t *testing.T) { t.Fatalf("failed to observe updated deployment: %v", err) } - // Wait for the default ingress configmap to be updated - previousDefaultIngressCAConfigmap := defaultIngressCAConfigmap.DeepCopy() - err = wait.PollImmediate(1*time.Second, 10*time.Second, func() (bool, error) { + // Verify that the default ingress configmap gets updated. + err = wait.PollImmediate(1*time.Second, timeout, func() (bool, error) { if err := kclient.Get(context.TODO(), controller.DefaultIngressCertConfigMapName(), defaultIngressCAConfigmap); err != nil { t.Logf("failed to get CA config map %s: %v", controller.DefaultIngressCertConfigMapName(), err) return false, nil @@ -546,6 +619,21 @@ func TestUpdateDefaultIngressController(t *testing.T) { t.Fatalf("failed to observe update of default ingress CA certificate configmap: %v", err) } + // Verify that the router-certs secret gets updated. + err = wait.PollImmediate(1*time.Second, timeout, func() (bool, error) { + if err := kclient.Get(context.TODO(), controller.RouterCertsGlobalSecretName(), routerCertsSecret); err != nil { + t.Logf("failed to get the router-certs secret: %v", err) + return false, nil + } + if reflect.DeepEqual(routerCertsSecret.Data, previousRouterCertsSecret.Data) { + return false, nil + } + return true, nil + }) + if err != nil { + t.Fatalf("failed to observe update of the router-certs secret: %v", err) + } + // Reset .spec.defaultCertificate to its original value. if err := updateIngressControllerSpecWithRetryOnConflict(t, defaultName, timeout, func(spec *operatorv1.IngressControllerSpec) { spec.DefaultCertificate = originalSecret @@ -553,9 +641,25 @@ func TestUpdateDefaultIngressController(t *testing.T) { t.Fatalf("failed to reset default ingresscontroller: %v", err) } - // Wait for the default ingress configmap to be updated back to the original + // Verify that the default router deployment gets restored to the + // original secret reference. + err = wait.PollImmediate(1*time.Second, timeout, func() (bool, error) { + if err := kclient.Get(context.TODO(), controller.RouterDeploymentName(ic), deployment); err != nil { + t.Logf("failed to get default router deployment: %v", err) + return false, nil + } + if deployment.Spec.Template.Spec.Volumes[0].Secret.SecretName != expectedSecretName { + return false, nil + } + return true, nil + }) + if err != nil { + t.Fatalf("failed to observe restored deployment: %v", err) + } + + // Verify that the default ingress configmap gets updated. previousDefaultIngressCAConfigmap = defaultIngressCAConfigmap.DeepCopy() - err = wait.PollImmediate(1*time.Second, 10*time.Second, func() (bool, error) { + err = wait.PollImmediate(1*time.Second, timeout, func() (bool, error) { if err := kclient.Get(context.TODO(), controller.DefaultIngressCertConfigMapName(), defaultIngressCAConfigmap); err != nil { t.Logf("failed to get CA config map %s: %v", controller.DefaultIngressCertConfigMapName(), err) return false, nil @@ -569,6 +673,27 @@ func TestUpdateDefaultIngressController(t *testing.T) { t.Logf("secret content=%v", string(secret.Data["tls.crt"])) t.Fatalf("failed to observe update of default ingress CA certificate configmap: %v\noriginal=%v\ncurrent=%v", err, previousDefaultIngressCAConfigmap.Data["ca-bundle.crt"], defaultIngressCAConfigmap.Data["ca-bundle.crt"]) } + + // Verify that the router-certs secret gets updated. + previousRouterCertsSecret = routerCertsSecret.DeepCopy() + wait.PollImmediate(1*time.Second, timeout, func() (bool, error) { + if err := kclient.Get(context.TODO(), controller.RouterCertsGlobalSecretName(), routerCertsSecret); err != nil { + t.Logf("failed to get secret %s: %v", controller.DefaultIngressCertConfigMapName(), err) + return false, nil + } + if reflect.DeepEqual(routerCertsSecret.Data, previousRouterCertsSecret.Data) { + return false, nil + } + return true, nil + }) + if err != nil { + t.Fatalf("failed to observe update of the router-certs secret: %v\noriginal=%v\ncurrent=%v", err, previousRouterCertsSecret, routerCertsSecret) + } + + // Verify that the ingresscontroller status is healthy. + if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, defaultName, availableConditionsForIngressControllerWithLoadBalancer...); err != nil { + t.Fatalf("failed to observe expected conditions: %v", err) + } } // TestIngressControllerScale exercises a simple scale up/down scenario. This