diff --git a/pkg/manifests/assets/canary/service.yaml b/pkg/manifests/assets/canary/service.yaml index 0b1b97d791..72470dfcf2 100644 --- a/pkg/manifests/assets/canary/service.yaml +++ b/pkg/manifests/assets/canary/service.yaml @@ -2,10 +2,8 @@ # Specific values are applied at runtime kind: Service apiVersion: v1 -metadata: - # name and namespace are set at runtime. - annotations: - service.beta.openshift.io/serving-cert-secret-name: canary-serving-cert +metadata: {} + # metadata values are set at runtime spec: type: ClusterIP ports: diff --git a/pkg/operator/controller/canary-certificate/controller.go b/pkg/operator/controller/canary-certificate/controller.go new file mode 100644 index 0000000000..0f20eac158 --- /dev/null +++ b/pkg/operator/controller/canary-certificate/controller.go @@ -0,0 +1,255 @@ +package canarycertificate + +import ( + "context" + "fmt" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + logf "github.com/openshift/cluster-ingress-operator/pkg/log" + "github.com/openshift/cluster-ingress-operator/pkg/manifests" + + operatorcontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller" + + operatorv1 "github.com/openshift/api/operator/v1" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/tools/record" + + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/manager" + "sigs.k8s.io/controller-runtime/pkg/predicate" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + "sigs.k8s.io/controller-runtime/pkg/source" +) + +const ( + canaryCertControllerName = "canary_certificate_controller" +) + +var ( + log = logf.Logger.WithName(canaryCertControllerName) +) + +// Config holds all the things necessary for the controller to run. +type Config struct { + OperatorNamespace string + OperandNamespace string +} + +// reconciler handles the actual canary certificate reconciliation logic in +// response to events. +type reconciler struct { + config Config + client client.Client + recorder record.EventRecorder +} + +// New creates the canary certificate controller +// +// The canary certificate controller mirrors the default ingress controller's +// certificate into the canary's namespace. It watches the canary's certificate +// and the default ingress controller's certificate. +func New(mgr manager.Manager, config Config) (controller.Controller, error) { + operatorCache := mgr.GetCache() + reconciler := &reconciler{ + config: config, + client: mgr.GetClient(), + recorder: mgr.GetEventRecorderFor(canaryCertControllerName), + } + c, err := controller.New(canaryCertControllerName, mgr, controller.Options{Reconciler: reconciler}) + if err != nil { + return nil, err + } + // Watch for updates on the canary's certificate. + isCanaryCert := predicate.NewPredicateFuncs(func(o client.Object) bool { + return o.GetName() == operatorcontroller.CanaryCertificateName().Name && o.GetNamespace() == operatorcontroller.CanaryCertificateName().Namespace + }) + // Also watch for updates on the default ingress controller's certificate. + // Because the default ingress controller's certificate name can be set at + // runtime, a Get() needs to be done to determine the correct certificate. + isDefaultIngressCert := predicate.NewPredicateFuncs(func(o client.Object) bool { + if o.GetNamespace() != config.OperandNamespace { + return false + } + + defaultICName := types.NamespacedName{ + Namespace: config.OperatorNamespace, + Name: manifests.DefaultIngressControllerName, + } + defaultIC := &operatorv1.IngressController{} + if err := reconciler.client.Get(context.Background(), defaultICName, defaultIC); err != nil { + log.Error(err, "Failed to get default IngressController") + return false + } + + defaultCertName := operatorcontroller.RouterEffectiveDefaultCertificateSecretName(defaultIC, config.OperandNamespace) + + return o.GetName() == defaultCertName.Name + }) + // Regardless of which certificate changed, this controller only has one + // reconcile target: the canary certificate's secret. + enqueueRequestForCanaryCertificate := handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, a client.Object) []reconcile.Request { + return []reconcile.Request{{NamespacedName: operatorcontroller.CanaryCertificateName()}} + }) + if err := c.Watch(source.Kind[client.Object](operatorCache, &corev1.Secret{}, enqueueRequestForCanaryCertificate, predicate.Or(isCanaryCert, isDefaultIngressCert))); err != nil { + return nil, err + } + return c, nil +} + +// Reconcile ensures the canary's certificate mirrors the default ingress +// controller's certificate. +func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) (reconcile.Result, error) { + log.Info("Reconciling", "request", request) + result := reconcile.Result{} + + if _, _, err := r.ensureCanaryCertificate(ctx); err != nil { + return result, fmt.Errorf("failed to ensure canary certificate: %w", err) + } + + return result, nil +} + +// ensureCanaryCertificate ensures the canary certificate secret exists, and +// that it uses the same certificate as the default ingress controller. +func (r *reconciler) ensureCanaryCertificate(ctx context.Context) (bool, *corev1.Secret, error) { + defaultIngressControllerName := types.NamespacedName{ + Namespace: r.config.OperatorNamespace, + Name: manifests.DefaultIngressControllerName, + } + defaultIngressController := &operatorv1.IngressController{} + if err := r.client.Get(ctx, defaultIngressControllerName, defaultIngressController); err != nil { + return false, nil, err + } + defaultCertName := operatorcontroller.RouterEffectiveDefaultCertificateSecretName(defaultIngressController, r.config.OperandNamespace) + defaultCert := &corev1.Secret{} + if err := r.client.Get(ctx, defaultCertName, defaultCert); err != nil { + return false, nil, err + } + canaryDaemonSet := &appsv1.DaemonSet{} + if err := r.client.Get(ctx, operatorcontroller.CanaryDaemonSetName(), canaryDaemonSet); err != nil { + return false, nil, err + } + haveCert, current, err := r.currentCanaryCertificate(ctx) + if err != nil { + return false, nil, err + } + trueVar := true + ownerRef := metav1.OwnerReference{ + APIVersion: "apps/v1", + Kind: "DaemonSet", + Name: canaryDaemonSet.Name, + UID: canaryDaemonSet.UID, + Controller: &trueVar, + } + desired := desiredCanaryCertificate(ownerRef, defaultCert) + + switch { + case !haveCert: + if err := r.createCanaryCertificate(ctx, desired); err != nil { + return false, nil, err + } + return r.currentCanaryCertificate(ctx) + case haveCert: + if updated, err := r.updateCanaryCertificate(ctx, current, desired); err != nil { + return true, current, err + } else if updated { + return r.currentCanaryCertificate(ctx) + } + } + return true, current, nil +} + +// currentCanaryCertificate returns the current canary certificate secret, if it exists. +func (r *reconciler) currentCanaryCertificate(ctx context.Context) (bool, *corev1.Secret, error) { + currentCanaryCert := &corev1.Secret{} + if err := r.client.Get(ctx, operatorcontroller.CanaryCertificateName(), currentCanaryCert); err != nil { + if errors.IsNotFound(err) { + return false, nil, nil + } + return false, nil, err + } + return true, currentCanaryCert, nil + +} + +// desiredCanaryCertificate returns the desired canary certificate secret, based +// on the default ingress controller's certificate. +func desiredCanaryCertificate(canaryOwnerRef metav1.OwnerReference, defaultIngressCertificate *corev1.Secret) *corev1.Secret { + canaryCertName := operatorcontroller.CanaryCertificateName() + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: canaryCertName.Name, + Namespace: canaryCertName.Namespace, + OwnerReferences: []metav1.OwnerReference{canaryOwnerRef}, + }, + // No validation should be done here on Data or Type. The canary should + // accurately reflect the state of the default ingress controller, so + // either validation needs to be done at the ingresscontroller level, or + // invalid certificates will be detected by the canary at runtime. + Data: defaultIngressCertificate.Data, + Type: defaultIngressCertificate.Type, + } +} + +// createCanaryCertificate creates the canary certificate, or returns an error +// if it cannot. +func (r *reconciler) createCanaryCertificate(ctx context.Context, certificate *corev1.Secret) error { + if err := r.client.Create(ctx, certificate); err != nil { + return err + } + + r.recorder.Event(certificate, "Normal", "CreatedCanaryCertificate", "created canary certificate") + return nil +} + +// updateCanaryCertificate updates the canary certificate if the desired version +// differs from the current one, and returns an error if it cannot. +func (r *reconciler) updateCanaryCertificate(ctx context.Context, current, desired *corev1.Secret) (bool, error) { + changed, updated := canaryCertificateChanged(current, desired) + if !changed { + return false, nil + } + if err := r.client.Update(ctx, updated); err != nil { + return false, err + } + r.recorder.Event(updated, "Normal", "UpdatedCanaryCertificate", "updated canary certificate") + return true, nil +} + +// canaryCertificateChanged returns an updated certificate secret if the current +// secret differs from the desired one. +func canaryCertificateChanged(current, desired *corev1.Secret) (bool, *corev1.Secret) { + changed := false + updated := current.DeepCopy() + + if !cmp.Equal(current.OwnerReferences, desired.OwnerReferences) { + updated.OwnerReferences = desired.OwnerReferences + changed = true + } + if !cmp.Equal(current.Data, desired.Data) { + updated.Data = desired.Data + changed = true + } + if current.Type != desired.Type { + updated.Type = desired.Type + changed = true + } + if !cmp.Equal(current.Annotations, desired.Annotations, cmpopts.EquateEmpty()) { + updated.Annotations = desired.Annotations + changed = true + } + if !cmp.Equal(current.Labels, desired.Labels, cmpopts.EquateEmpty()) { + updated.Labels = desired.Labels + changed = true + } + + return changed, updated +} diff --git a/pkg/operator/controller/canary-certificate/controller_test.go b/pkg/operator/controller/canary-certificate/controller_test.go new file mode 100644 index 0000000000..bbceb51324 --- /dev/null +++ b/pkg/operator/controller/canary-certificate/controller_test.go @@ -0,0 +1,104 @@ +package canarycertificate + +import ( + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func Test_canaryCertificateChanged(t *testing.T) { + trueVar := true + testCases := []struct { + description string + mutate func(*corev1.Secret) + expect bool + }{ + { + description: "no change", + mutate: func(_ *corev1.Secret) {}, + expect: false, + }, + { + description: "new owner ref added", + mutate: func(certSecret *corev1.Secret) { + newOwnerRef := metav1.OwnerReference{ + APIVersion: "apps/v1", + Kind: "Service", + Name: "random-service", + UID: "bazquux", + Controller: &trueVar, + } + certSecret.OwnerReferences = append(certSecret.OwnerReferences, newOwnerRef) + }, + expect: true, + }, + { + description: "cert data changed", + mutate: func(certSecret *corev1.Secret) { + certSecret.Data = map[string][]byte{ + "foo": []byte("barbaz"), + } + }, + expect: true, + }, + { + description: "cert type changed", + mutate: func(certSecret *corev1.Secret) { + certSecret.Type = corev1.SecretTypeBasicAuth + }, + expect: true, + }, + { + description: "annotation added", + mutate: func(certSecret *corev1.Secret) { + if certSecret.Annotations == nil { + certSecret.Annotations = map[string]string{} + } + certSecret.Annotations["asdf"] = "asdfasdfasdf" + }, + expect: true, + }, + { + description: "label added", + mutate: func(certSecret *corev1.Secret) { + if certSecret.Labels == nil { + certSecret.Labels = map[string]string{} + } + certSecret.Labels["newlabel"] = "asdfasdf" + }, + expect: true, + }, + } + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + defaultICSecret := &corev1.Secret{ + Data: map[string][]byte{ + "foo": []byte("bar"), + "baz": []byte("quux"), + }, + Type: corev1.SecretTypeOpaque, + } + ownerRef := metav1.OwnerReference{ + APIVersion: "apps/v1", + Kind: "DaemonSet", + Name: "ingress-canary", + UID: "foobar", + Controller: &trueVar, + } + original := desiredCanaryCertificate(ownerRef, defaultICSecret) + mutated := original.DeepCopy() + tc.mutate(mutated) + if changed, updated := canaryCertificateChanged(original, mutated); changed != tc.expect { + t.Errorf("expect canaryCertificateChanged to be %t, got %t", tc.expect, changed) + } else if changed { + if updatedChanged, _ := canaryCertificateChanged(original, updated); !updatedChanged { + t.Error("canaryCertificateChanged reported changes but did not make any update") + } + if changedAgain, _ := canaryCertificateChanged(mutated, updated); changedAgain { + t.Error("canaryCertificateChanged does not behave as a fixed point function") + } + } + }) + } +} diff --git a/pkg/operator/controller/canary/service_test.go b/pkg/operator/controller/canary/service_test.go index 440d2fa691..2245f86fcd 100644 --- a/pkg/operator/controller/canary/service_test.go +++ b/pkg/operator/controller/canary/service_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/openshift/cluster-ingress-operator/pkg/manifests" "github.com/openshift/cluster-ingress-operator/pkg/operator/controller" @@ -54,10 +55,8 @@ func Test_desiredCanaryService(t *testing.T) { t.Errorf("expected service owner references %#v, but got %#v", expectedOwnerRefs, service.OwnerReferences) } - expectedAnnotations := map[string]string{ - "service.beta.openshift.io/serving-cert-secret-name": "canary-serving-cert", - } - if !cmp.Equal(service.Annotations, expectedAnnotations) { + expectedAnnotations := map[string]string{} + if !cmp.Equal(service.Annotations, expectedAnnotations, cmpopts.EquateEmpty()) { t.Errorf("expected service annotations to be %q, but got %q", expectedAnnotations, service.Annotations) } @@ -90,6 +89,9 @@ func Test_canaryServiceChanged(t *testing.T) { { description: "changed annotation", mutate: func(service *corev1.Service) { + if service.Annotations == nil { + service.Annotations = map[string]string{} + } service.Annotations["foo"] = "bar" }, expected: true, diff --git a/pkg/operator/controller/names.go b/pkg/operator/controller/names.go index 749bbf8a23..be72197005 100644 --- a/pkg/operator/controller/names.go +++ b/pkg/operator/controller/names.go @@ -292,6 +292,13 @@ func CanaryRouteName() types.NamespacedName { } } +func CanaryCertificateName() types.NamespacedName { + return types.NamespacedName{ + Namespace: DefaultCanaryNamespace, + Name: "canary-serving-cert", + } +} + func IngressClassName(ingressControllerName string) types.NamespacedName { return types.NamespacedName{Name: "openshift-" + ingressControllerName} } diff --git a/pkg/operator/operator.go b/pkg/operator/operator.go index 80fa6efa60..ff5457f84b 100644 --- a/pkg/operator/operator.go +++ b/pkg/operator/operator.go @@ -28,6 +28,7 @@ import ( operatorconfig "github.com/openshift/cluster-ingress-operator/pkg/operator/config" operatorcontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller" canarycontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/canary" + canarycertcontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/canary-certificate" certcontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/certificate" certpublishercontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/certificate-publisher" clientcacontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/clientca-configmap" @@ -289,6 +290,14 @@ func New(config operatorconfig.Config, kubeConfig *rest.Config) (*Operator, erro }); err != nil { return nil, fmt.Errorf("failed to create canary controller: %v", err) } + + // Set up the canary certificate controller + if _, err := canarycertcontroller.New(mgr, canarycertcontroller.Config{ + OperatorNamespace: config.Namespace, + OperandNamespace: operatorcontroller.DefaultOperandNamespace, + }); err != nil { + return nil, fmt.Errorf("failed to create canary cert controller: %w", err) + } } // Set up the route metrics controller.