feat: Maintain ingress serving cert for HostedCluster ingress - #9132
Open
deads2k wants to merge 5 commits into
Open
feat: Maintain ingress serving cert for HostedCluster ingress#9132deads2k wants to merge 5 commits into
deads2k wants to merge 5 commits into
Conversation
Add a DefaultCertificate field to IngressOperatorSpec that allows users to reference a TLS secret in the HostedCluster namespace containing a custom default serving certificate for the ingress controller. When set, this certificate replaces the auto-generated wildcard certificate. The field mirrors the upstream OpenShift IngressControllerSpec DefaultCertificate field and follows the same contract: the referenced secret must contain tls.crt and tls.key. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Verify N-1/N+1 JSON round-trip safety of the new DefaultCertificate field: old code ignores the unknown field, new code deserializes old data with nil DefaultCertificate. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add reconcileIngressDefaultCertSync to copy the user-provided TLS secret from the HostedCluster namespace to the HCP namespace. The ensureReferencedResourceAnnotation call drives re-reconciliation when the source secret is updated, enabling certificate rotation. Registered as a non-critical Phase 6b sync operation alongside SSHKeySync and other optional secret syncs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When DefaultCertificate is set in IngressOperatorSpec, HCCO reads the user-provided secret from the HCP namespace instead of the CPO-generated wildcard cert. The data is written into the same default-ingress-cert secret in the guest cluster's openshift-ingress namespace, so the IngressController spec needs no changes. The CPO continues generating the wildcard cert as a fallback and for the CA trust chain. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add v2 Ginkgo test (ValidateIngressDefaultCertificateTest) that verifies the default-ingress-cert secret in the hosted cluster's openshift-ingress namespace contains tls.crt and tls.key when DefaultCertificate is configured. Skips when not configured. Add v1 test utilities (ValidateIngressDefaultCertificate, EnsureIngressDefaultCertificate) following the existing pattern for EndpointPublishingStrategy validation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
Contributor
📝 WalkthroughWalkthroughAdds an optional 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: deads2k The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
area/api
openshift-ci
Bot
added
area/cli
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
hypershift-operator/controllers/hostedcluster/ingress_default_cert_sync_test.go (1)
20-27: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a test case for
OperatorConfigurationset butIngressOperatornil.The two no-op cases (Lines 29-38, 40-52) both exercise the same code path (
OperatorConfiguration == nil), since a zero-valueHostedClusterSpecalready has a nilOperatorConfiguration. The intermediate nil check (hcluster.Spec.OperatorConfiguration.IngressOperator == nil, Line 2101 inhostedcluster_controller.go) is not directly exercised by any case.♻️ Suggested additional test case
+ { + name: "When IngressOperator is nil, it should be a no-op", + hcluster: &hyperv1.HostedCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + Namespace: "clusters", + }, + Spec: hyperv1.HostedClusterSpec{ + OperatorConfiguration: &hyperv1.OperatorConfiguration{ + IngressOperator: nil, + }, + }, + }, + expectError: false, + expectSync: false, + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hypershift-operator/controllers/hostedcluster/ingress_default_cert_sync_test.go` around lines 20 - 27, Add a test case to TestReconcileIngressDefaultCertSync where HostedCluster.Spec.OperatorConfiguration is initialized but OperatorConfiguration.IngressOperator is nil, and assert the expected no-op behavior. Keep the existing nil-OperatorConfiguration case unchanged so the intermediate IngressOperator nil guard is exercised separately.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/hypershift/v1beta1/operator.go`:
- Line 309: The defaultCertificate field in api/hypershift/v1beta1/operator.go
must use an API CEL rule requiring a non-empty name whenever the reference is
non-nil, with envtest coverage for an empty reference. In
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go,
update the default-certificate reconciliation logic to return/report an error
for a non-nil reference with an empty name instead of serving the generated
wildcard certificate.
In
`@control-plane-operator/hostedclusterconfigoperator/controllers/resources/ingress/params_test.go`:
- Around line 321-347: Rename the test case in the ingress parameters table from
“DefaultCertificate set via OperatorConfiguration” to a stable “When … it should
…” description, such as “When DefaultCertificate is configured it should
propagate it to ingress parameters.” Leave the test inputs and expected values
unchanged.
In
`@control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go`:
- Around line 1430-1432: Add focused unit tests for the certificate-source
selection logic surrounding sourceCert in the reconciliation flow: verify custom
certificate copying, fallback to the CPO-generated wildcard certificate when no
custom source is provided, and propagation of custom-secret retrieval failures.
Add these cases in the relevant resources controller test suite rather than
relying on params_test.go, which only covers parameter propagation.
In `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go`:
- Around line 2094-2137: Update reconcileIngressDefaultCertSync to always sync
the referenced source secret to the fixed
manifests.IngressDefaultIngressControllerCert().Name destination, and handle a
removed or changed DefaultCertificate.Name by deleting the previously synced
control-plane secret so stale TLS private key material does not remain.
---
Nitpick comments:
In
`@hypershift-operator/controllers/hostedcluster/ingress_default_cert_sync_test.go`:
- Around line 20-27: Add a test case to TestReconcileIngressDefaultCertSync
where HostedCluster.Spec.OperatorConfiguration is initialized but
OperatorConfiguration.IngressOperator is nil, and assert the expected no-op
behavior. Keep the existing nil-OperatorConfiguration case unchanged so the
intermediate IngressOperator nil guard is exercised separately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3b7f5413-cb5d-48f0-a7f1-76ede80f12a2
⛔ Files ignored due to path filters (40)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.go,!**/zz_generated*api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/EtcdSharding.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/EtcdSharding.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**client/applyconfiguration/hypershift/v1beta1/ingressoperatorspec.gois excluded by!client/**cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamldocs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mddocs/content/reference/api.mdis excluded by!docs/content/reference/api.mdvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/operator.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated*
📒 Files selected for processing (9)
api/hypershift/v1beta1/operator.goapi/hypershift/v1beta1/operator_test.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/ingress/params.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/ingress/params_test.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/hostedcluster/ingress_default_cert_sync_test.gotest/e2e/util/util_ingress_operator_configuration.gotest/e2e/v2/tests/hosted_cluster_ingress_test.go
| // signed by the cluster's root CA. | ||
| // | ||
| // +optional | ||
| DefaultCertificate *corev1.LocalObjectReference `json:"defaultCertificate,omitempty"` |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject an empty configured certificate reference.
defaultCertificate: {} deserializes to a non-nil reference with an empty name. The reconciliation branch then treats it as absent and serves the generated wildcard certificate, rather than rejecting the invalid custom-certificate configuration.
api/hypershift/v1beta1/operator.go#L309-L309: add API CEL validation requiring a non-emptynamewheneverdefaultCertificateis present, with envtest coverage.control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go#L1433-L1442: defensively report an error for a non-nil empty reference instead of falling back silently.
As per coding guidelines, “For new validation in this module, use CEL rules on the API types instead of admission webhooks.”
📍 Affects 2 files
api/hypershift/v1beta1/operator.go#L309-L309(this comment)control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go#L1433-L1442
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/hypershift/v1beta1/operator.go` at line 309, The defaultCertificate field
in api/hypershift/v1beta1/operator.go must use an API CEL rule requiring a
non-empty name whenever the reference is non-nil, with envtest coverage for an
empty reference. In
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go,
update the default-certificate reconciliation logic to return/report an error
for a non-nil reference with an empty name instead of serving the generated
wildcard certificate.
Source: Coding guidelines
Comment on lines
+321
to
+347
| { | ||
| name: "DefaultCertificate set via OperatorConfiguration", | ||
| args: args{ | ||
| hcp: &hyperv1.HostedControlPlane{ | ||
| Spec: hyperv1.HostedControlPlaneSpec{ | ||
| OperatorConfiguration: &hyperv1.OperatorConfiguration{ | ||
| IngressOperator: &hyperv1.IngressOperatorSpec{ | ||
| DefaultCertificate: &corev1.LocalObjectReference{ | ||
| Name: "my-custom-cert", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| want: &IngressParams{ | ||
| IngressSubdomain: "apps.", | ||
| Replicas: 1, | ||
| IsPrivate: false, | ||
| IBMCloudUPI: false, | ||
| AWSNLB: false, | ||
| LoadBalancerScope: v1.ExternalLoadBalancer, | ||
| DefaultCertificate: &corev1.LocalObjectReference{ | ||
| Name: "my-custom-cert", | ||
| }, | ||
| }, | ||
| }, |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the required test-case naming format.
Rename this case to a stable “When … it should …” description, e.g. When DefaultCertificate is configured it should propagate it to ingress parameters.
As per coding guidelines, “Always use ‘When ... it should ...’ format for describing test cases when creating unit tests.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@control-plane-operator/hostedclusterconfigoperator/controllers/resources/ingress/params_test.go`
around lines 321 - 347, Rename the test case in the ingress parameters table
from “DefaultCertificate set via OperatorConfiguration” to a stable “When … it
should …” description, such as “When DefaultCertificate is configured it should
propagate it to ingress parameters.” Leave the test inputs and expected values
unchanged.
Source: Coding guidelines
Comment on lines
+1430
to
+1432
| // When the user provides a custom default certificate, use it as the source. | ||
| // Otherwise, fall back to the CPO-generated wildcard cert. | ||
| var sourceCert *corev1.Secret |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add focused coverage for certificate-source selection.
Cover custom-source copy, generated-wildcard fallback, and custom-secret retrieval failure. params_test.go only verifies parameter propagation, not this reconciliation behavior.
As per coding guidelines, “Unit test any code changes and additions.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go`
around lines 1430 - 1432, Add focused unit tests for the certificate-source
selection logic surrounding sourceCert in the reconciliation flow: verify custom
certificate copying, fallback to the CPO-generated wildcard certificate when no
custom source is provided, and propagation of custom-secret retrieval failures.
Add these cases in the relevant resources controller test suite rather than
relying on params_test.go, which only covers parameter propagation.
Source: Coding guidelines
Comment on lines
+2094
to
+2137
| // reconcileIngressDefaultCertSync syncs the user-provided ingress default | ||
| // certificate secret from the HostedCluster namespace to the control plane namespace. | ||
| func (r *HostedClusterReconciler) reconcileIngressDefaultCertSync( | ||
| ctx context.Context, hcluster *hyperv1.HostedCluster, createOrUpdate upsert.CreateOrUpdateFN, | ||
| controlPlaneNamespace string, | ||
| ) error { | ||
| if hcluster.Spec.OperatorConfiguration == nil || | ||
| hcluster.Spec.OperatorConfiguration.IngressOperator == nil || | ||
| hcluster.Spec.OperatorConfiguration.IngressOperator.DefaultCertificate == nil || | ||
| len(hcluster.Spec.OperatorConfiguration.IngressOperator.DefaultCertificate.Name) == 0 { | ||
| return nil | ||
| } | ||
| secretName := hcluster.Spec.OperatorConfiguration.IngressOperator.DefaultCertificate.Name | ||
| var src corev1.Secret | ||
| if err := r.Client.Get(ctx, client.ObjectKey{Namespace: hcluster.Namespace, Name: secretName}, &src); err != nil { | ||
| return fmt.Errorf("failed to get ingress default certificate secret %s: %w", secretName, err) | ||
| } | ||
| if err := ensureReferencedResourceAnnotation(ctx, r.Client, hcluster.Name, &src); err != nil { | ||
| return fmt.Errorf("failed to set referenced resource annotation: %w", err) | ||
| } | ||
| dest := &corev1.Secret{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Namespace: controlPlaneNamespace, | ||
| Name: secretName, | ||
| }, | ||
| } | ||
| _, err := createOrUpdate(ctx, r.Client, dest, func() error { | ||
| if _, ok := src.Data[corev1.TLSCertKey]; !ok { | ||
| return fmt.Errorf("ingress default certificate secret %q must have a %s key", secretName, corev1.TLSCertKey) | ||
| } | ||
| if _, ok := src.Data[corev1.TLSPrivateKeyKey]; !ok { | ||
| return fmt.Errorf("ingress default certificate secret %q must have a %s key", secretName, corev1.TLSPrivateKeyKey) | ||
| } | ||
| dest.Type = corev1.SecretTypeTLS | ||
| if dest.Data == nil { | ||
| dest.Data = map[string][]byte{} | ||
| } | ||
| dest.Data[corev1.TLSCertKey] = src.Data[corev1.TLSCertKey] | ||
| dest.Data[corev1.TLSPrivateKeyKey] = src.Data[corev1.TLSPrivateKeyKey] | ||
| return nil | ||
| }) | ||
| return err | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how the ingress params/HCCO layer looks up the synced default-certificate secret name in the CP namespace.
rg -n -C5 'DefaultCertificate' control-plane-operator/hostedclusterconfigoperator/controllers/resources/ingress/params.go
# Check whether any generic stale-secret GC exists for referenced-resource secrets.
rg -n -C3 'referencedResourceAnnotationPrefix|PruneStale|orphan' hypershift-operator/controllers/hostedcluster/hostedcluster_controller.goRepository: openshift/hypershift
Length of output: 2557
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the sync function and nearby reconciliation/cleanup logic.
sed -n '2060,2165p' hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
# Inspect how ingress parameters consume the default certificate reference.
sed -n '1,140p' control-plane-operator/hostedclusterconfigoperator/controllers/resources/ingress/params.go
# Inspect referenced-resource annotation handling around enqueue/cleanup.
sed -n '500,580p' hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
sed -n '5610,5665p' hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
sed -n '3988,4012p' hypershift-operator/controllers/hostedcluster/hostedcluster_controller.goRepository: openshift/hypershift
Length of output: 14942
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C4 'DefaultCertificate|default certificate|tls secret|TLS' control-plane-operator/hostedclusterconfigoperator/controllers/resources/ingress
rg -n -C4 'LocalObjectReference|SecretName|secretName' control-plane-operator/hostedclusterconfigoperator/controllers/resources/ingressRepository: openshift/hypershift
Length of output: 50376
Delete stale synced ingress TLS secrets when the source reference changes
reconcileIngressDefaultCertSync copies the source secret into the control-plane namespace using DefaultCertificate.Name, but the ingress controller consumes the fixed manifests.IngressDefaultIngressControllerCert().Name secret. If the source reference changes or is removed, the old control-plane secret — including private key material — stays behind indefinitely. Sync to the fixed destination name, or delete the previous secret when the reference changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go`
around lines 2094 - 2137, Update reconcileIngressDefaultCertSync to always sync
the referenced source secret to the fixed
manifests.IngressDefaultIngressControllerCert().Name destination, and handle a
removed or changed DefaultCertificate.Name by deleting the previously synced
control-plane secret so stale TLS private key material does not remain.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9132 +/- ##
==========================================
+ Coverage 44.51% 44.58% +0.07%
==========================================
Files 774 774
Lines 96997 97057 +60
==========================================
+ Hits 43179 43274 +95
+ Misses 50830 50790 -40
- Partials 2988 2993 +5
... and 6 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Contributor
|
@deads2k: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
deads2k
changed the title
bennerv
reviewed
Jul 27, 2026
| EndpointPublishingStrategy *operatorv1.EndpointPublishingStrategy `json:"endpointPublishingStrategy,omitempty"` | ||
|
|
||
| // defaultCertificate is a reference to a secret in the HostedCluster namespace | ||
| // that contains the default certificate served by the ingress controller. |
Member
There was a problem hiding this comment.
Do we want to specify it's for the default ingress controller?
Suggested change
| // that contains the default certificate served by the ingress controller. | |
| // that contains the default certificate served by the default ingress controller. |
4 tasks
muraee
reviewed
Jul 28, 2026
| dest := &corev1.Secret{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Namespace: controlPlaneNamespace, | ||
| Name: secretName, |
Contributor
There was a problem hiding this comment.
I think we should sync to a fixed secret name and clean it up when the reference is removed from the API.
Contributor
Author
There was a problem hiding this comment.
I think we should sync to a fixed secret name and clean it up when the reference is removed from the API.
What name would you like?
muraee
reviewed
Jul 28, 2026
| }) | ||
|
|
||
| report.execute("IngressDefaultCertSync", nonCritical, func() error { | ||
| return r.reconcileIngressDefaultCertSync(ctx, hcluster, createOrUpdate, controlPlaneNamespace.Name) |
Contributor
There was a problem hiding this comment.
reconcileIngressDefaultCertSync should be also called in https://github.com/openshift/hypershift/blob/main/hypershift-operator/controllers/hostedcluster/reconcile_legacy.go
JoelSpeed
reviewed
Jul 28, 2026
| // signed by the cluster's root CA. | ||
| // | ||
| // +optional | ||
| DefaultCertificate *corev1.LocalObjectReference `json:"defaultCertificate,omitempty"` |
Contributor
There was a problem hiding this comment.
Prefer not to use imported corev1 types for object references, please use a local dedicated reference type instead.
See for example SecretReference in etcdbackup_types.go
When doing so, make sure the name field is +required, preventing defaultCertificate: {} which is currently valid per the API definition here. Once you've done that, you won't need the pointer and can switch omitempty for omitzero
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Our ingress/router is ~synonymous with OpenShift and is used to serve our webconsole. This places it in a special situation where it must logically exist on newly created clusters. Rather than try to re-orient a successful part of our product in the market, we will instead allow the serving certificate only to be managed by a service provider using the API provided by Hypershift. This change adds a field to specify a default serving certificate and have it injected into the HostedCluster kube-apiserver.
Summary by CodeRabbit
New Features
Bug Fixes