Skip to content

OCPBUGS-9037, OCPBUGS-64565: Use cluster wildcard certificate for ingress canary #1155

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
rfredette wants to merge 1 commit into from
Closed

OCPBUGS-9037, OCPBUGS-64565: Use cluster wildcard certificate for ingress canary #1155

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/manifests/assets/canary/daemonset.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ spec:
volumes:
- name: cert
secret:
secretName: canary-serving-cert
# secret name is set at runtime
defaultMode: 0420
updateStrategy:
type: RollingUpdate
Expand Down
6 changes: 2 additions & 4 deletions pkg/manifests/assets/canary/service.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
24 changes: 22 additions & 2 deletions pkg/operator/controller/canary/daemonset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,23 @@ import (
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"

operatorv1 "github.com/openshift/api/operator/v1"
"github.com/openshift/cluster-ingress-operator/pkg/manifests"
"github.com/openshift/cluster-ingress-operator/pkg/operator/controller"

appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/types"
)

// ensureCanaryDaemonSet ensures the canary daemonset exists
func (r *reconciler) ensureCanaryDaemonSet() (bool, *appsv1.DaemonSet, error) {
desired := desiredCanaryDaemonSet(r.config.CanaryImage)
secretName, err := r.canarySecretName(controller.CanaryDaemonSetName().Namespace)
if err != nil {
return false, nil, err
}
desired := desiredCanaryDaemonSet(r.config.CanaryImage, secretName.Name)
haveDs, current, err := r.currentCanaryDaemonSet()
if err != nil {
return false, nil, err
Expand Down Expand Up @@ -80,7 +86,7 @@ func (r *reconciler) updateCanaryDaemonSet(current, desired *appsv1.DaemonSet) (

// desiredCanaryDaemonSet returns the desired canary daemonset read in
// from manifests
func desiredCanaryDaemonSet(canaryImage string) *appsv1.DaemonSet {
func desiredCanaryDaemonSet(canaryImage, secretName string) *appsv1.DaemonSet {
daemonset := manifests.CanaryDaemonSet()
name := controller.CanaryDaemonSetName()
daemonset.Name = name.Name
Expand All @@ -97,6 +103,8 @@ func desiredCanaryDaemonSet(canaryImage string) *appsv1.DaemonSet {
daemonset.Spec.Template.Spec.Containers[0].Image = canaryImage
daemonset.Spec.Template.Spec.Containers[0].Command = []string{"ingress-operator", CanaryHealthcheckCommand}

daemonset.Spec.Template.Spec.Volumes[0].Secret.SecretName = secretName

return daemonset
}

Expand Down Expand Up @@ -196,3 +204,15 @@ func cmpTolerations(a, b corev1.Toleration) bool {
}
return true
}

func (r *reconciler) canarySecretName(namespace string) (types.NamespacedName, error) {
defaultIC := operatorv1.IngressController{}
defaultICName := types.NamespacedName{
Name: manifests.DefaultIngressControllerName,
Namespace: r.config.Namespace,
}
if err := r.client.Get(context.TODO(), defaultICName, &defaultIC); err != nil {
return types.NamespacedName{}, err
}
return controller.RouterEffectiveDefaultCertificateSecretName(&defaultIC, namespace), nil
}
22 changes: 20 additions & 2 deletions pkg/operator/controller/canary/daemonset_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ import (
func Test_desiredCanaryDaemonSet(t *testing.T) {
// canaryImageName is the ingress-operator image
canaryImageName := "openshift/origin-cluster-ingress-operator:latest"
daemonset := desiredCanaryDaemonSet(canaryImageName)
certSecretName := "test_secret_name"
daemonset := desiredCanaryDaemonSet(canaryImageName, certSecretName)

expectedDaemonSetName := controller.CanaryDaemonSetName()

Expand Down Expand Up @@ -83,6 +84,23 @@ func Test_desiredCanaryDaemonSet(t *testing.T) {
if !cmp.Equal(tolerations, expectedTolerations) {
t.Errorf("expected daemonset tolerations to be %v, but got %v", expectedTolerations, tolerations)
}

volumes := daemonset.Spec.Template.Spec.Volumes
secretMode := int32(0420)
Copy link
Copy Markdown
Contributor

@candita candita Dec 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know why the defaultMode has to be set for the test but not in the function desiredCanaryDaemonSet?

Copy link
Copy Markdown
Contributor Author

@rfredette rfredette Dec 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm setting the default mode on the expected result here. Default mode is set in the daemonset manifest here for the desired daemonset

expectedVolumes := []corev1.Volume{
{
Name: "cert",
VolumeSource: corev1.VolumeSource{
Secret: &corev1.SecretVolumeSource{
SecretName: certSecretName,
DefaultMode: &secretMode,
},
},
},
}
if !cmp.Equal(volumes, expectedVolumes) {
t.Errorf("expected daemonset volumes to be %v, but got %v", expectedVolumes, volumes)
}
}

func Test_canaryDaemonsetChanged(t *testing.T) {
Expand Down Expand Up @@ -229,7 +247,7 @@ func Test_canaryDaemonsetChanged(t *testing.T) {

for _, tc := range testCases {
t.Run(tc.description, func(t *testing.T) {
original := desiredCanaryDaemonSet("")
original := desiredCanaryDaemonSet("", "foobar")
mutated := original.DeepCopy()
tc.mutate(mutated)
if changed, updated := canaryDaemonSetChanged(original, mutated); changed != tc.expect {
Expand Down
10 changes: 6 additions & 4 deletions pkg/operator/controller/canary/service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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,
Expand Down
44 changes: 44 additions & 0 deletions pkg/operator/controller/certificate/controller.go
View file
Open in desktop
Copy link
Copy Markdown
Contributor

@Miciah Miciah Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment at the top of this file should be updated to document that the controller copies the certificate for the canary... though the more I think about it, the more I think we should have a separate controller for this purpose.

Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"time"

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"
ingresscontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/ingress"

Expand All @@ -20,6 +21,7 @@ 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"

Expand Down Expand Up @@ -105,6 +107,48 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
if _, err := r.ensureDefaultCertificateForIngress(ca, deployment.Namespace, deploymentRef, ingress); err != nil {
errs = append(errs, fmt.Errorf("failed to ensure default cert for %s: %v", ingress.Name, err))
}
// The ingress canary verifies that the default ingress controller is functioning. Since it uses a
// passthrough route, we mirror the default ingress controller's certificate in the canary to detect any
// issues with that certificate, so if the default controller's cert is updated, update the canary's cert as
// well.
if ingress.Name == manifests.DefaultIngressControllerName {
log.Info("Ensuring canary certificate")
daemonset := &appsv1.DaemonSet{}
err = r.client.Get(ctx, controller.CanaryDaemonSetName(), daemonset)
Comment on lines +110 to +117
Copy link
Copy Markdown
Contributor

@Miciah Miciah Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a watch on secrets so that we update the canary's copy when the cluster-admin updates the original secret? Maybe re-using this controller isn't the best approach. I suppose we can refactor later on as a follow-up.

if err != nil {
if errors.IsNotFound(err) {
// All ingresses should have a deployment, so this one may not have been
// created yet. Retry after a reasonable amount of time.
Comment on lines +120 to +121
Copy link
Copy Markdown
Contributor

@Miciah Miciah Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment looks like copypasta.

log.Info("Canary daemonset not found; will retry default cert sync")
result.RequeueAfter = 5 * time.Second
} else {
errs = append(errs, fmt.Errorf("failed to get daemonset: %w", err))
}
} else {
defaultCertName := controller.RouterEffectiveDefaultCertificateSecretName(ingress, deployment.Namespace)
defaultCert := &corev1.Secret{}
if err := r.client.Get(ctx, defaultCertName, defaultCert); err != nil {
errs = append(errs, fmt.Errorf("failed to get certificate for canary: %w", err))
}
canaryCert := defaultCert.DeepCopy()
Copy link
Copy Markdown
Contributor

@Miciah Miciah Dec 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We really only care about defaultCert.Data and defaultCert.SecretType, right?

I suppose using DeepCopy will work as you stomp ObjectMeta and the API server doesn't send StringData; using DeepCopy just does some extra (unnecessary) work.

canaryCertName := controller.RouterEffectiveDefaultCertificateSecretName(ingress, daemonset.Namespace)
Copy link
Copy Markdown
Contributor

@Miciah Miciah Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should work, but is there a reason to copy the name from the effective certificate? If you used a static name, you could get rid of canarySecretName in pkg/operator/controller/canary/daemonset.go and simplify the logic a bit.

Copy link
Copy Markdown
Contributor

@Miciah Miciah Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One consequence of copying the name is that you can end up with multiple secrets in the canary namespace if IngressController.spec.defaultCertificate is updated.

For example, the TestUpdateDefaultIngressControllerSecret test updates the default certificate from the default "router-certs-default" secret to a "test-xyz" secret (where "xyz" is a randomly generated suffix) and then reverts the default certificate back to "router-certs-default". As a consequence the CI artifacts have both a "router-certs-default" secret and a "test-xyz" secret in the canary namespace.

If the canary daemonset were ever deleted, the owner reference would cause these secrets to be cleaned up, but otherwise you can accumulate these secrets. This isn't a major problem, but it does create some unnecessary cruft.

Copy link
Copy Markdown
Contributor

@Miciah Miciah Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well... I wonder whether the issue referenced at #1155 (comment) will prevent garbage collection from working properly?

canaryRef := metav1.OwnerReference{
APIVersion: "apps/v1",
Kind: "Daemonset",
Copy link
Copy Markdown
Contributor

@Miciah Miciah Dec 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The kind is miscapitalized here:

Suggested change
Kind: "Daemonset",
Kind: "DaemonSet",

Does using "Daemonset" actually work? I don't know how forgiving the API server [or garbage collector] is.

Name: daemonset.Name,
UID: daemonset.UID,
Controller: &trueVar,
}
canaryCert.ObjectMeta = metav1.ObjectMeta{
Name: canaryCertName.Name,
Namespace: canaryCertName.Namespace,
OwnerReferences: []metav1.OwnerReference{canaryRef},
}
if err := r.client.Create(ctx, canaryCert); err != nil {
Copy link
Copy Markdown
Contributor

@candita candita Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be way off, but I'm not sure why you can't just store the name of the default cert instead of creating a copy of it? I defer to @Miciah on this.

Copy link
Copy Markdown
Contributor

@Miciah Miciah Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to copy the secret's data so that the canary application uses the exact certificate that is used as the default IngressController's default certificate. That is, we specifically need canaryCert.Data to match defaultCert.Data (and it makes sense to set canaryCert.Type to defaultCert.Type as well). The name doesn't matter, other than that the copy needs to be in the same namespace as the canary daemonset and needs to match whatever name the canary daemonset specifies in its volume.

errs = append(errs, fmt.Errorf("failed to ensure certificate for canary: %w", err))
}
}
}
}
}

Expand Down
4 changes: 2 additions & 2 deletions pkg/operator/controller/certificate/default_cert.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,14 @@ func (r *reconciler) ensureDefaultCertificateForIngress(caSecret *corev1.Secret,
if deleted, err := r.deleteRouterDefaultCertificate(current); err != nil {
return true, fmt.Errorf("failed to delete default certificate: %v", err)
} else if deleted {
r.recorder.Eventf(ci, "Normal", "DeletedDefaultCertificate", "Deleted default wildcard certificate %q", current.Name)
r.recorder.Eventf(ci, "Normal", "DeletedDefaultCertificate", "Deleted default wildcard certificate %q in namespace %q", current.Name, current.Namespace)
return false, nil
}
case wantCert && !haveCert:
if created, err := r.createRouterDefaultCertificate(desired); err != nil {
return false, fmt.Errorf("failed to create default certificate: %v", err)
} else if created {
r.recorder.Eventf(ci, "Normal", "CreatedDefaultCertificate", "Created default wildcard certificate %q", desired.Name)
r.recorder.Eventf(ci, "Normal", "CreatedDefaultCertificate", "Created default wildcard certificate %q in namespace %q", desired.Name, desired.Namespace)
return true, nil
}
case wantCert && haveCert:
Expand Down