Skip to content

CNTRLPLANE-4150: feat: Maintain ingress serving cert for HostedCluster ingress #9132

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

Merged
openshift-merge-bot merged 7 commits into from
Sep 11, 2026
Merged

CNTRLPLANE-4150: feat: Maintain ingress serving cert for HostedCluster ingress #9132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2d05470
feat(api): add ingress defaultCertificate field and synced condition
deads2k Sep 4, 2026
aa8e527
chore(api): regenerate CRDs, clients, deepcopy, and vendor
deads2k Sep 4, 2026
d346163
feat(hypershift-operator): sync ingress default certificate and repor…
deads2k Sep 4, 2026
408113b
feat(control-plane-operator): use user-provided ingress default certi…
deads2k Sep 4, 2026
004ae9b
test(e2e): add ingress default certificate coverage
deads2k Sep 4, 2026
01dee99
docs: regenerate API reference for ingress default certificate
deads2k Sep 4, 2026
57221be
test(e2e): verify ingress certificates are not copied when ingress is…
deads2k Sep 8, 2026
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
22 changes: 22 additions & 0 deletions api/hypershift/v1beta1/hostedcluster_conditions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,18 @@ const (
// remove the annotation.
// **False** (reason AsExpected) means no deprecated configuration is in use.
HostedClusterConfigurationDeprecated ConditionType = "HostedClusterConfigurationDeprecated"

// IngressDefaultCertificateSynced indicates whether the user-provided default
// ingress certificate referenced by
// spec.operatorConfiguration.ingressOperator.defaultCertificate has been
// synced from the HostedCluster namespace into the control plane namespace.
// **True** means the referenced Secret was found, contains tls.crt and tls.key,
// and its data was synced.
// **False** means the referenced Secret is missing or malformed; in that case
// the previously synced certificate (or the auto-generated wildcard certificate)
// keeps serving and the HostedCluster does not become degraded.
// The condition is absent when no defaultCertificate is configured.
IngressDefaultCertificateSynced ConditionType = "IngressDefaultCertificateSynced"

@ironcladlou ironcladlou Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Synced might be useful, but would the user also care for a high level signal as to whether the default ingress controller is serving a user-defined cert vs. the auto-generated wildcard at the high level? Sorry if that's already exposed in another way, I forget

@deads2k deads2k Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Synced might be useful, but would the user also care for a high level signal as to whether the default ingress controller is serving a user-defined cert vs. the auto-generated wildcard at the high level? Sorry if that's already exposed in another way, I forget

To my knowledge, not even exposed by the ingress operation in OCP.

)

// Reasons for PublicEndpointExposed condition.
Expand Down Expand Up @@ -361,6 +373,16 @@ const (

ReconcileErrorReason = "ReconcileError"

// IngressDefaultCertificateInvalidReason is used when the referenced default
// ingress certificate Secret exists but does not contain the required tls.crt
// and tls.key entries.
IngressDefaultCertificateInvalidReason = "InvalidCertificateSecret"

// IngressDefaultCertificatePlatformNotSupportedReason is used when a default
// ingress certificate is configured on a platform whose ingress controller does
// not consume it (e.g. IBM Cloud), so the certificate is intentionally not synced.
IngressDefaultCertificatePlatformNotSupportedReason = "PlatformNotSupported"

CloudResourcesCleanupSkippedReason = "CloudResourcesCleanupSkipped"

CloudResourcesDeletionTimedOutReason = "CloudResourcesDeletionTimedOut"
Expand Down
42 changes: 42 additions & 0 deletions api/hypershift/v1beta1/operator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,4 +349,46 @@ type IngressOperatorSpec struct {
// +kubebuilder:pruning:PreserveUnknownFields
// +kubebuilder:validation:Type=object
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 default ingress controller.
// When Routes don't specify their own certificate, defaultCertificate is used.
//
// The secret must contain the following keys and data:
// tls.crt: certificate file contents
// tls.key: key file contents
//
// When set, this certificate replaces the auto-generated wildcard certificate
// that is normally created by the control plane operator. The secret is synced
// from the HostedCluster namespace to the control plane, and then propagated
// to the hosted cluster's openshift-ingress namespace.
//
// When the referenced secret is updated, the new certificate data is
// automatically propagated to the hosted cluster.
Comment on lines +366 to +367

@muraee muraee Sep 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is not entirely true. The guest default-ingress-cert Secret is reconciled each HCCO resync, but ReconcileDefaultIngressController returns early on day-2 (if ingressController.ResourceVersion != ""), so a guest cluster-admin can edit IngressController.spec.defaultCertificate and HyperShift won't revert it

@deads2k deads2k Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is not entirely true. The guest default-ingress-cert Secret is reconciled each HCCO resync, but ReconcileDefaultIngressController returns early on day-2 (if ingressController.ResourceVersion != ""), so a guest cluster-admin can edit IngressController.spec.defaultCertificate and HyperShift won't revert it

Pretty sure the secret itself is still automatically propagated into the default location. The default location is simply unused. Do you have preference for different wording?

@muraee muraee Sep 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

true, I just think we need to make this clear, something like:

// Note: a cluster-admin in the hosted cluster can override the default ingress
// controller's certificate directly. That override takes precedence
// and the certificate referenced here is no longer served.

//
// When not set, the control plane operator generates a wildcard certificate
// signed by the cluster's root CA.
//
// Note: a cluster-admin in the hosted cluster can override the default ingress
// controller's certificate directly. That override takes precedence and the
// certificate referenced here is no longer served.
//
// +optional

@ironcladlou ironcladlou Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just some driveby feedback having just seen all this for the first time, not requesting any changes: I personally found the verbiage clear but potentially ambiguous with regards to the functional outcome depending on the state of the field... the preamble is written in the way that makes the most sense to me as a user (emphasis mine):

// defaultCertificate is a reference to a secret in the HostedCluster namespace
// that contains the default certificate served by the default ingress controller.

The functional intent of the field as I understand it is to drive what certificate is served by the default ingress controller, and that line above makes it clear to me. But the rest of the description about what happens under certain conditions changes the framing to talk about how the secret is synced to the hosted cluster, wildcard generation, etc. Which is all useful detail about what happens mechanically, but from a user's perspective, it seems like it boils down to whether this certificate or the auto-generated wildcard certificate is served by the ingress controller. Everything else feels like implementation details. Just my 0.02

@muraee muraee Sep 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1, I think some sentences document internal plumbing (the two-hop sync + specific internal namespace names) rather than user-observable behavior

DefaultCertificate IngressDefaultCertificateReference `json:"defaultCertificate,omitzero"`
}

// IngressDefaultCertificateReference contains a reference to a TLS Secret
// in the HostedCluster namespace used as the default serving certificate
// for the ingress controller.
type IngressDefaultCertificateReference struct {
// name is the name of the Secret containing tls.crt and tls.key.
// The Secret must exist in the same namespace as the HostedCluster.
// name must be a valid DNS subdomain name (RFC 1123): it must contain only
// lowercase alphanumeric characters, '-' or '.', and start and end with an
// alphanumeric character.
// +required
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=253
// +kubebuilder:validation:XValidation:rule="self.matches('^[a-z0-9]([-a-z0-9]*[a-z0-9])?([.][a-z0-9]([-a-z0-9]*[a-z0-9])?)*$')",message="name must be a valid DNS subdomain name: contain no more than 253 characters, contain only lowercase alphanumeric characters, '-' or '.', and start and end with an alphanumeric character"
Name string `json:"name,omitempty"`
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
91 changes: 91 additions & 0 deletions api/hypershift/v1beta1/operator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
package v1beta1

import (
"encoding/json"
"testing"

operatorv1 "github.com/openshift/api/operator/v1"
)

// ingressOperatorSpecNMinus1 represents the previous version of IngressOperatorSpec
// without the DefaultCertificate field.
type ingressOperatorSpecNMinus1 struct {
EndpointPublishingStrategy json.RawMessage `json:"endpointPublishingStrategy,omitempty"` //nolint:kubeapilinter

@bryan-cox bryan-cox Aug 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟡 Medium — the round-trip is vacuous as written; the repo's own precedent shows the fix.

This file follows nodepool_types_test.go in this same package — same nMinus1 stand-in, same table shape, same SerializationCompatibility naming. Good pattern to have copied. But it drops the one thing that makes the pattern meaningful.

nodepool_types_test.go asserts a non-empty expectation, so sibling fields are genuinely proven to survive:

nMinus1Result: nodePoolAutoScalingNMinus1{Min: 3, Max: 5},

Both cases here leave it empty:

nMinus1Result: ingressOperatorSpecNMinus1{},

ingressOperatorSpecNMinus1 has exactly one sibling field (EndpointPublishingStrategy), and it's unset in both fixtures — so a round-trip that silently dropped it would still pass. That's precisely the property the enhancement's Serialization Compatibility section asks for: "can safely unmarshal and re-marshal … without losing other fields."

Populating EndpointPublishingStrategy with non-empty raw JSON and asserting it survives would close this, and would match the precedent.

Two things I checked and am not raising: round-tripping IngressOperatorSpec rather than a full HostedCluster matches nodepool_types_test.go, which round-trips NodePoolAutoScaling — repo convention, and only the enhancement's wording says "HostedCluster". And a Go unit test is the right vehicle here; the CRD testsuites cover what the API server enforces, which is a different layer.

Genuinely open question, not a finding: is the //nolint:kubeapilinter needed? I couldn't tell whether kube-api-linter runs over _test.go files.

}

func TestIngressOperatorSpecSerializationCompatibility(t *testing.T) {
tests := []struct {
name string
current IngressOperatorSpec
expectedJSON string
nMinus1Result ingressOperatorSpecNMinus1
}{
{
name: "When DefaultCertificate is zero it should be omitted and N-1 should deserialize cleanly",
current: IngressOperatorSpec{},
expectedJSON: `{}`,
nMinus1Result: ingressOperatorSpecNMinus1{},
},
{
name: "When DefaultCertificate is set it should serialize and N-1 should ignore it",
current: IngressOperatorSpec{
DefaultCertificate: IngressDefaultCertificateReference{
Name: "my-cert",
},
},
expectedJSON: `{"defaultCertificate":{"name":"my-cert"}}`,
nMinus1Result: ingressOperatorSpecNMinus1{},
},
{
name: "When N-1 data carries EndpointPublishingStrategy it should survive the round-trip into N",
current: IngressOperatorSpec{},
expectedJSON: `{}`,
nMinus1Result: ingressOperatorSpecNMinus1{
EndpointPublishingStrategy: json.RawMessage(`{"type":"LoadBalancerService"}`),
},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
data, err := json.Marshal(tt.current)
if err != nil {
t.Fatalf("failed to marshal current struct: %v", err)
}
if string(data) != tt.expectedJSON {
t.Errorf("unexpected JSON output: got %s, want %s", string(data), tt.expectedJSON)
}

// N -> N-1: old code should ignore the unknown DefaultCertificate field
var nMinus1 ingressOperatorSpecNMinus1
if err := json.Unmarshal(data, &nMinus1); err != nil {
t.Fatalf("N-1 failed to unmarshal JSON from N: %v", err)
}

// N-1 -> N: data from old code should deserialize into new struct with zero DefaultCertificate
nMinus1Data, err := json.Marshal(tt.nMinus1Result)
if err != nil {
t.Fatalf("failed to marshal N-1 struct: %v", err)
}
var roundTrip IngressOperatorSpec
if err := json.Unmarshal(nMinus1Data, &roundTrip); err != nil {
t.Fatalf("N failed to unmarshal JSON from N-1: %v", err)
}
if roundTrip.DefaultCertificate.Name != "" {
t.Errorf("expected DefaultCertificate to be zero after N-1 round-trip, got %+v", roundTrip.DefaultCertificate)
}

// Sibling fields written by N-1 must survive into N unchanged; otherwise
// the round-trip would silently drop data the enhancement requires to be
// preserved.
if len(tt.nMinus1Result.EndpointPublishingStrategy) > 0 {
if roundTrip.EndpointPublishingStrategy == nil {
t.Errorf("expected EndpointPublishingStrategy to survive N-1 -> N round-trip, got nil")
} else if roundTrip.EndpointPublishingStrategy.Type != operatorv1.LoadBalancerServiceStrategyType {
t.Errorf("expected EndpointPublishingStrategy.Type %q to survive N-1 -> N round-trip, got %q",
operatorv1.LoadBalancerServiceStrategyType, roundTrip.EndpointPublishingStrategy.Type)
}
}
})
}
}
16 changes: 16 additions & 0 deletions api/hypershift/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

44 changes: 44 additions & 0 deletions ...erated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3515,6 +3515,50 @@ spec:
ingressOperator specifies the configuration for the Ingress Operator in the hosted cluster.
This allows configuring how the default ingress controller endpoints are published.
properties:
defaultCertificate:
description: |-
defaultCertificate is a reference to a secret in the HostedCluster namespace
that contains the default certificate served by the default ingress controller.
When Routes don't specify their own certificate, defaultCertificate is used.

The secret must contain the following keys and data:
tls.crt: certificate file contents
tls.key: key file contents

When set, this certificate replaces the auto-generated wildcard certificate
that is normally created by the control plane operator. The secret is synced
from the HostedCluster namespace to the control plane, and then propagated
to the hosted cluster's openshift-ingress namespace.

When the referenced secret is updated, the new certificate data is
automatically propagated to the hosted cluster.

When not set, the control plane operator generates a wildcard certificate
signed by the cluster's root CA.

Note: a cluster-admin in the hosted cluster can override the default ingress
controller's certificate directly. That override takes precedence and the
certificate referenced here is no longer served.
properties:
name:
description: |-
name is the name of the Secret containing tls.crt and tls.key.
The Secret must exist in the same namespace as the HostedCluster.
name must be a valid DNS subdomain name (RFC 1123): it must contain only
lowercase alphanumeric characters, '-' or '.', and start and end with an
alphanumeric character.
maxLength: 253
minLength: 1
type: string
x-kubernetes-validations:
- message: 'name must be a valid DNS subdomain name: contain
no more than 253 characters, contain only lowercase
alphanumeric characters, ''-'' or ''.'', and start
and end with an alphanumeric character'
rule: self.matches('^[a-z0-9]([-a-z0-9]*[a-z0-9])?([.][a-z0-9]([-a-z0-9]*[a-z0-9])?)*$')
required:
- name
type: object
endpointPublishingStrategy:
description: |-
endpointPublishingStrategy is used to publish the default ingress controller endpoints.
Expand Down
44 changes: 44 additions & 0 deletions ...egated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3506,6 +3506,50 @@ spec:
ingressOperator specifies the configuration for the Ingress Operator in the hosted cluster.
This allows configuring how the default ingress controller endpoints are published.
properties:
defaultCertificate:
description: |-
defaultCertificate is a reference to a secret in the HostedCluster namespace
that contains the default certificate served by the default ingress controller.
When Routes don't specify their own certificate, defaultCertificate is used.

The secret must contain the following keys and data:
tls.crt: certificate file contents
tls.key: key file contents

When set, this certificate replaces the auto-generated wildcard certificate
that is normally created by the control plane operator. The secret is synced
from the HostedCluster namespace to the control plane, and then propagated
to the hosted cluster's openshift-ingress namespace.

When the referenced secret is updated, the new certificate data is
automatically propagated to the hosted cluster.

When not set, the control plane operator generates a wildcard certificate
signed by the cluster's root CA.

Note: a cluster-admin in the hosted cluster can override the default ingress
controller's certificate directly. That override takes precedence and the
certificate referenced here is no longer served.
properties:
name:
description: |-
name is the name of the Secret containing tls.crt and tls.key.
The Secret must exist in the same namespace as the HostedCluster.
name must be a valid DNS subdomain name (RFC 1123): it must contain only
lowercase alphanumeric characters, '-' or '.', and start and end with an
alphanumeric character.
maxLength: 253
minLength: 1
type: string
x-kubernetes-validations:
- message: 'name must be a valid DNS subdomain name: contain
no more than 253 characters, contain only lowercase
alphanumeric characters, ''-'' or ''.'', and start
and end with an alphanumeric character'
rule: self.matches('^[a-z0-9]([-a-z0-9]*[a-z0-9])?([.][a-z0-9]([-a-z0-9]*[a-z0-9])?)*$')
required:
- name
type: object
endpointPublishingStrategy:
description: |-
endpointPublishingStrategy is used to publish the default ingress controller endpoints.
Expand Down
44 changes: 44 additions & 0 deletions ...manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3525,6 +3525,50 @@ spec:
ingressOperator specifies the configuration for the Ingress Operator in the hosted cluster.
This allows configuring how the default ingress controller endpoints are published.
properties:
defaultCertificate:
description: |-
defaultCertificate is a reference to a secret in the HostedCluster namespace
that contains the default certificate served by the default ingress controller.
When Routes don't specify their own certificate, defaultCertificate is used.

The secret must contain the following keys and data:
tls.crt: certificate file contents
tls.key: key file contents

When set, this certificate replaces the auto-generated wildcard certificate
that is normally created by the control plane operator. The secret is synced
from the HostedCluster namespace to the control plane, and then propagated
to the hosted cluster's openshift-ingress namespace.

When the referenced secret is updated, the new certificate data is
automatically propagated to the hosted cluster.

When not set, the control plane operator generates a wildcard certificate
signed by the cluster's root CA.

Note: a cluster-admin in the hosted cluster can override the default ingress
controller's certificate directly. That override takes precedence and the
certificate referenced here is no longer served.
properties:
name:
description: |-
name is the name of the Secret containing tls.crt and tls.key.
The Secret must exist in the same namespace as the HostedCluster.
name must be a valid DNS subdomain name (RFC 1123): it must contain only
lowercase alphanumeric characters, '-' or '.', and start and end with an
alphanumeric character.
maxLength: 253
minLength: 1
type: string
x-kubernetes-validations:
- message: 'name must be a valid DNS subdomain name: contain
no more than 253 characters, contain only lowercase
alphanumeric characters, ''-'' or ''.'', and start
and end with an alphanumeric character'
rule: self.matches('^[a-z0-9]([-a-z0-9]*[a-z0-9])?([.][a-z0-9]([-a-z0-9]*[a-z0-9])?)*$')
required:
- name
type: object
endpointPublishingStrategy:
description: |-
endpointPublishingStrategy is used to publish the default ingress controller endpoints.
Expand Down
Loading
Loading