Add TLS adherance feature gate#2680
Add TLS adherance feature gate#2680richardsonnick wants to merge 13 commits intoopenshift:masterfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Hello @richardsonnick! Some important instructions when contributing to openshift/api: |
📝 WalkthroughWalkthroughThis pull request introduces a new TLSAdherence field to the APIServer configuration specification. The change adds a TLSAdherencePolicy type with two enum values: LegacyExternalAPIServerComponentsOnly and StrictAllComponents. The field is wired through the Go type definitions, CRD schemas, generated OpenAPI documentation, and feature gate configurations. A cluster-wide validation rule prevents removal once set. The feature is registered as a feature gate and enabled in DevPreviewNoUpgrade and TechPreviewNoUpgrade modes. Test coverage is provided for valid creations, updates, removal attempts, and unsupported values. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
openshift-ci
bot
added
the
size/XL
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@config/v1/types_apiserver.go`:
- Around line 65-84: Update the TLSAdherence field documentation to include the
exclusion note from the CRD manifests: explicitly state that Kubelet and
IngressController are excluded from tlsAdherence control and instead use their
own TLS config (KubeletConfig and IngressController CRs). Edit the comment above
the TLSAdherence TLSAdherencePolicy field (the docblock that documents
TLSAdherence) to append a short sentence noting this exclusion so generated API
docs reflect the same caveat as the CRD manifests.
In `@features.md`:
- Line 78: The table row containing the "TLSAdherence" feature is missing a
space before the first pipe after the feature name, violating markdownlint table
spacing; edit the row that starts with "TLSAdherence" and insert a single space
before the following "|" so it reads "TLSAdherence |" (preserving the rest of
the cells exactly) to conform to the configured table style.
In `@openapi/generated_openapi/zz_generated.openapi.go`:
- Around line 12447-12461: Add a kubebuilder enum validation for the Curves
field so only supported TLS curve identifiers are accepted: add a comment marker
like +kubebuilder:validation:Enum=X25519;P-256;P-384;P-521 immediately above the
Curves []string field in the struct in config/v1/types_tlssecurityprofile.go
(mirror the pattern used for TLSProtocolVersion), then re-run
controller-gen/openapi generation to update
openapi/generated_openapi/zz_generated.openapi.go so the "curves" schema
includes the enum restriction.
🧹 Nitpick comments (5)
config/v1/types_tlssecurityprofile.go (1)
169-178: Consider adding listType=atomic for Curves for SSA consistency.
This mirrors theCipherslist semantics and avoids any unexpected merge behavior.🔧 Proposed tweak
// curves: // - X25519 // - P-256 // + // +listType=atomic Curves []string `json:"curves,omitempty"`openapi/generated_openapi/zz_generated.openapi.go (1)
9102-9108: Add enum validation fortlsAdherenceto match the documented valid values.The schema only declares a free-form string even though the description defines just
"Legacy"and"Strict". That means invalid values will pass admission and only be handled at runtime. Consider adding kubebuilder enum (and, if desired, a default) on the source type so the generated OpenAPI enforces the allowed values.[Suggested fix (apply in
config/v1/types_apiserver.go, not this generated file):]🔧 Example annotation update
// +optional +// +kubebuilder:validation:Enum=Legacy;Strict +// +kubebuilder:default=Legacy TLSAdherence TLSAdherencePolicy `json:"tlsAdherence,omitempty"`payload-manifests/crds/0000_10_config-operator_01_apiservers-Default.crd.yaml (1)
259-270: Consider addingx-kubernetes-list-typeannotation for consistency.The
curvesfield is missing thex-kubernetes-list-typeannotation that theciphersfield has (line 258 showsx-kubernetes-list-type: atomicfor ciphers). For consistency and to ensure proper list merge behavior, consider adding the same annotation.📝 Suggested fix
curves: description: |- curves is used to specify the elliptic curves that are used during the TLS handshake. Operators may remove entries their operands do not support. For example, to use X25519 and P-256 (yaml): curves: - X25519 - P-256 items: type: string type: array + x-kubernetes-list-type: atomicpayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml (1)
287-298: Same annotation consistency issue as Default CRD.The
curvesfield here also lacks thex-kubernetes-list-type: atomicannotation thatciphershas. See the comment on the Default CRD for the suggested fix.payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml (1)
355-366: Same annotation consistency issue.The
curvesfield here also lacksx-kubernetes-list-type: atomic. This is consistent across all the CRD files in this PR but differs from theciphersfield pattern.
| "curves": { | ||
| SchemaProps: spec.SchemaProps{ | ||
| Description: "curves is used to specify the elliptic curves that are used during the TLS handshake. Operators may remove entries their operands do not support. For example, to use X25519 and P-256 (yaml):\n\n curves:\n - X25519\n - P-256", | ||
| Type: []string{"array"}, | ||
| Items: &spec.SchemaOrArray{ | ||
| Schema: &spec.Schema{ | ||
| SchemaProps: spec.SchemaProps{ | ||
| Default: "", | ||
| Type: []string{"string"}, | ||
| Format: "", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🌐 Web query:
Go 1.21 1.22 crypto/tls supported curve names CurveID identifiers
💡 Result:
In Go 1.21 and Go 1.22, crypto/tls exposes these supported (named group) CurveID constants:
| TLS group / curve name | Go constant | CurveID (uint16) |
|---|---|---|
| NIST P-256 (secp256r1) | tls.CurveP256 |
23 |
| NIST P-384 (secp384r1) | tls.CurveP384 |
24 |
| NIST P-521 (secp521r1) | tls.CurveP521 |
25 |
| X25519 | tls.X25519 |
29 |
These are the CurveID identifiers defined by crypto/tls for those versions. [1]
Sources:
[1] https://go.googlesource.com/go/+/go1.14.8/src/crypto/tls/common.go
🏁 Script executed:
# First, find the source code that generates this OpenAPI schema
# The file is auto-generated, so we need to find what generates it
fd -t f "*.go" | head -20 | xargs grep -l "curves is used to specify" 2>/dev/null | head -5Repository: openshift/api
Length of output: 340
🏁 Script executed:
# Search for the source field definition
rg -A 5 -B 5 "curves.*TLS.*handshake" --type goRepository: openshift/api
Length of output: 3653
🏁 Script executed:
# Check for OpenAPI generation tooling and look for the source type
find . -name "*openapi*" -o -name "*swagger*" | grep -v generated | head -10Repository: openshift/api
Length of output: 427
🏁 Script executed:
# Look for struct definitions with a "curves" field that might be the source
rg "Curves\s*\[\]" --type go | head -20Repository: openshift/api
Length of output: 359
🏁 Script executed:
# Read the source file that defines the Curves field
cat -n config/v1/types_tlssecurityprofile.goRepository: openshift/api
Length of output: 11992
🏁 Script executed:
# Look for validation rules or constants in the codebase
rg "enum.*curve|Curve.*enum|CurveID|X25519|P-256|P-384|P-521" --type go config/v1/ | head -30Repository: openshift/api
Length of output: 568
🏁 Script executed:
# Search for validation struct tags or validation rules in similar fields
rg "json:.*omitempty.*validation|kubebuilder:validation" --type go config/v1/types_tlssecurityprofile.go -A 3 -B 3Repository: openshift/api
Length of output: 490
🏁 Script executed:
# Check how other string arrays are validated in the codebase
rg '\[\]string.*json' --type go config/v1/ -A 2 | grep -A 2 "kubebuilder" | head -40Repository: openshift/api
Length of output: 39
Add validation for curves field to restrict entries to supported TLS curve identifiers.
The Curves field in config/v1/types_tlssecurityprofile.go (line 178) accepts any string with no validation, allowing invalid curve names to be accepted during config validation but fail later at TLS handshake time. Add a +kubebuilder:validation:Enum marker similar to the pattern used for TLSProtocolVersion to enforce only supported curves: X25519, P-256, P-384, P-521 (as supported by Go 1.21+ crypto/tls).
🤖 Prompt for AI Agents
In `@openapi/generated_openapi/zz_generated.openapi.go` around lines 12447 -
12461, Add a kubebuilder enum validation for the Curves field so only supported
TLS curve identifiers are accepted: add a comment marker like
+kubebuilder:validation:Enum=X25519;P-256;P-384;P-521 immediately above the
Curves []string field in the struct in config/v1/types_tlssecurityprofile.go
(mirror the pattern used for TLSProtocolVersion), then re-run
controller-gen/openapi generation to update
openapi/generated_openapi/zz_generated.openapi.go so the "curves" schema
includes the enum restriction.
config/v1/types_apiserver.go
Outdated
| // When set to "Legacy" (the default), components attempt to honor the configured TLS profile | ||
| // but may fall back to their individual defaults if conflicts arise. This mode is intended for | ||
| // clusters that need to maintain compatibility with existing configurations during migration. |
There was a problem hiding this comment.
I thought Legacy was going to essentially mean "only the externally exposed API server components", and Strict will mean "ALL cluster components with the exception of Kubelet-related and Ingress-related servers"?
I also think we should update GoDoc for the TLSSecurityProfile field, which currently says "for externally exposed servers". With TLSAdherence in the mix, what the profile is for depends on the TLSAdherence value.
But that should doc change should also only apply when the feature gate is enabled. I think @JoelSpeed said there's a way to do that?
There was a problem hiding this comment.
I also wonder if we should improve the naming of the enums to more accurately describe what they do? For example:
- LegacyExternalAPIServerComponentsOnly
- StrictAllComponents
Or something along those lines?
There was a problem hiding this comment.
I think these names are far better. Going with LegacyExternalAPIServerComponentsOnly and StrictAllComponents
| // curves is used to specify the elliptic curves that are used during | ||
| // the TLS handshake. Operators may remove entries their operands do | ||
| // not support. For example, to use X25519 and P-256 (yaml): | ||
| // | ||
| // curves: | ||
| // - X25519 | ||
| // - P-256 | ||
| Curves []string `json:"curves,omitempty"` |
There was a problem hiding this comment.
Based on my read of openshift/enhancements#1894, it seems like very few components currently handle TLS curve configuration.
And I don't think we will be able to promote this full set of API changes unless components handle all of MinTLSVersion, CipherSuites, and Curves.
I am worried that the upstream's lack of curves support might take some time and would therefore block the rest of the feature around honoring the min TLS and cipher suites configuration.
Should we remove Curves from this PR or configure it to be present with a separate feature gate?
There was a problem hiding this comment.
Yeah that should not be here. It has since been removed. This PR applies solely to the adherence fate.
| // | ||
| // curves: | ||
| // - X25519 | ||
| // - P-256 |
There was a problem hiding this comment.
If/when we do add Curves field, it needs to be introduced behind a feature gate.
joelanford
left a comment
joelanford
left a comment
There was a problem hiding this comment.
Curves should be removed or included behind a feature gate. My understanding is that we can't add a field to the Default CRD without first going through TechPreviewNoUpgrade.
|
Adding to @joelanford 's comment - I'd prefer we keep the curves field entirely separate here to keep this well scoped. IIRC there is already a PR that is looking to add the curves field. |
richardsonnick
force-pushed
the
tls-adherance-fate
branch
from
0cc2223 to
f627b61
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml`:
- Around line 295-318: The CRD docs claim tlsAdherence defaults to
"LegacyExternalAPIServerComponentsOnly" but the schema lacks a default; add the
kubebuilder default tag to the APIServer Go field (the tlsAdherence field on the
APIServer type) as +kubebuilder:default=LegacyExternalAPIServerComponentsOnly so
the generated CRD includes the default, and run codegen to update the YAML; also
ensure operators consuming the APIServer resource handle omitted/empty
tlsAdherence by treating it as Legacy until the default is live. Additionally,
add x-kubernetes-list-type: atomic to the curves list (to match ciphers) in the
generated CRD so both lists have consistent list-type semantics.
🧹 Nitpick comments (1)
payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
352-364: Consider markingcurvesas an atomic list for consistency withciphers.
ciphersis explicitly atomic;curveswould benefit from the same list-type to avoid merge ambiguity in SSA. Prefer adding+listType=atomicon the Go field and regenerating manifests.♻️ Suggested manifest shape
curves: description: |- curves is used to specify the elliptic curves that are used during the TLS handshake. Operators may remove entries their operands do not support. For example, to use X25519 and P-256 (yaml): curves: - X25519 - P-256 items: type: string type: array + x-kubernetes-list-type: atomic
payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml
Show resolved
Hide resolved
richardsonnick
force-pushed
the
tls-adherance-fate
branch
from
f627b61 to
c7aecd0
Compare
| // | ||
| // When omitted, the default value is "LegacyExternalAPIServerComponentsOnly". | ||
| // +openshift:enable:FeatureGate=TLSAdherence | ||
| // +optional |
There was a problem hiding this comment.
We need to configure validation on this field, and set the default.
// +kubebuilder:validation:items:Enum:=LegacyExternalAPIServerComponentsOnly;StrictAllComponents
// +kubebuilder:default:=LegacyExternalAPIServerComponentsOnly
There was a problem hiding this comment.
Oh I see you are defining the Enum validation on the type, which is fine. Let's also include the +kubebuilder:default marker there as well.
There was a problem hiding this comment.
Hmm, reading the tlsAdherence description again, it seems like you are wanting it to be possible for an empty string to be allowed to be persisted for this field in etcd, which would allow us in theory to change the default semantic later in the future from legacy to strict without a change in the actual tlsAdherence value.
In practice, I think the problem with that idea is that each component essentially has their own implementation of this API and we'd have to do a really good job coordinating a semantic switch from legacy to strict.
What do folks think about:
- Actually writing
LegacyExternalAPIServerComponentsOnlyto etcd by default. - Telling implementers they specifically only need to look for
StrictAllComponentsand will not have to worry about the possibility of""with changing semantics later. - When want to set
StrictAllComponentsas the new default, we make sure newly installed clusters get that, and we potentially block upgrades of clusters that still useLegacyExternalAPIServerComponentsOnly? Or maybe there's some other way to encourage users to switch toStrictAllComponents?
There was a problem hiding this comment.
For configuration APIs we generally try to avoid setting the default via +kubebuilder:default marker so that we can change the default behavior of the "no-opinion" mechanism.
In this case, I don't think we will be doing that but I still wouldn't use the +kubebuilder:default marker because then we won't be able to distinguish whether or not the user intentionally set the field or if we defaulted it on their behalf and you technically get locked into that default being a contractual default.
There was a problem hiding this comment.
so that we can change the default behavior of the "no-opinion" mechanism.
Thanks, I was hoping you'd have some good advice here. It is the intent to eventually make all components honor the cluster-wide default, so I supposed we'll just have to deal with trying to coordinate a semantic change across a bunch of components at once when the time comes.
To facilitate that, I'd suggest a library-go function somehow like this:
// in configv1
const TLSAdherenceNoOpinion = ""func ShouldAllComponentsAdhere(tlsAdherence configv1.TLSAdherence) bool {
if tlsAdherence == configv1.TLSAdherenceNoOpinion || tlsAdherence == configv1.LegacyExternalAPIServerComponentsOnly {
return false
}
return true
}And then when we want to change the semantic, we'd update that function to:
func ShouldAllComponentsAdhere(tlsAdherence configv1.TLSAdherence) bool {
if tlsAdherence == configv1.LegacyExternalAPIServerComponentsOnly {
return false
}
return true
}There was a problem hiding this comment.
That aligns with what I envisioned.
Can we make sure to update the EP with a mention of this kind of check/library that implementors are expected to use?
There was a problem hiding this comment.
Added a mention of this helper function.
Also created a draft diff in library-go to add this function: openshift/library-go#2114
Will need to land this first to get the TLSAdherencePolicy type into library-go
.../zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-Default.crd.yaml
Show resolved
Hide resolved
|
Overall, I think this is looking pretty good for TechPreview. I think the main thing is providing clarity to implementers. Which is "use existing TLS configuration settings when Not sure what @everettraven thinks, but I think we could potentially work out the finer details of the defaulting and descriptions in follow-ups if we agree on the necessary bits to get implementers going. |
joelanford
dismissed
their stale review
The TLS Curves changes have been removed.
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Overall, this looks pretty good. Have a handful of comments
config/v1/types_apiserver.go
Outdated
| // Note: The Kubelet and IngressController components are excluded from tlsAdherence control | ||
| // as they have their own dedicated TLS configuration mechanisms via KubeletConfig and | ||
| // IngressController CRs respectively. |
There was a problem hiding this comment.
Once we settle on this in the EP, we should update this.
My current expectation is that this isn't an exclusion but rather a preference scenario where, if specified, the Kubelet / Ingress Controller components will be configured with what is specified in their existing APIs.
There was a problem hiding this comment.
I think that makes sense. It positions apiserver as the truly cluster-wide default. And it is still aligned with the fact that other components may have their own special configuration knobs that override the default.
There was a problem hiding this comment.
I think this makes sense. Updated this to leave out the exclusion language and instead present the behavior as a "preference"
| // | ||
| // When omitted, the default value is "LegacyExternalAPIServerComponentsOnly". | ||
| // +openshift:enable:FeatureGate=TLSAdherence | ||
| // +optional |
There was a problem hiding this comment.
For configuration APIs we generally try to avoid setting the default via +kubebuilder:default marker so that we can change the default behavior of the "no-opinion" mechanism.
In this case, I don't think we will be doing that but I still wouldn't use the +kubebuilder:default marker because then we won't be able to distinguish whether or not the user intentionally set the field or if we defaulted it on their behalf and you technically get locked into that default being a contractual default.
config/v1/types_apiserver.go
Outdated
| // This field is optional. When omitted, it is interpreted as "no opinion" and the cluster | ||
| // defaults to LegacyExternalAPIServerComponentsOnly behavior. Future versions of OpenShift |
There was a problem hiding this comment.
We have some standard terminology we like to use here for consistency. As an example:
api/config/v1/types_network.go
Lines 244 to 246 in 6186972
We should follow this convention as well so we aren't locked into a contractual default in case we want to do something like change the default behavior to strict but still give admins an option to specify that they want to stay on the legacy mode.
| contactPerson("joelanford"). | ||
| productScope(ocpSpecific). | ||
| enhancementPR("https://github.com/openshift/enhancements/pull/1910"). | ||
| enableIn(configv1.DevPreviewNoUpgrade, configv1.TechPreviewNoUpgrade). |
There was a problem hiding this comment.
Are we targeting for this to be the same maturity level for both standalone OCP and HyperShift (HCP)?
HyperShift does embed the APIServer configuration spec in https://github.com/openshift/hypershift/blob/825484eb33d14b4ab849b428d134582320655fcf/api/hypershift/v1beta1/hostedcluster_types.go#L1957-L1961 and if we aren't careful they might end up defaulting this new field to GA.
Regardless, I think we may want a sibling PR to ensure that a feature-gate entry at the appropriate level is added to https://github.com/openshift/hypershift/tree/main/api/hypershift/v1beta1/featuregates to prevent an accidental GA. We've been bitten by this in the past and it causes all kinds of headaches, so its probably best to avoid that here.
There was a problem hiding this comment.
Added a featuregate in hypershift that adds TLSAdherence to:
- The disabled list in the Default feature gate files
- The enabled list in the TechPreviewNoUpgrade feature gate files
| // Once set, this field may be changed to a different value. | ||
| // +openshift:enable:FeatureGate=TLSAdherence | ||
| // +optional | ||
| TLSAdherence TLSAdherencePolicy `json:"tlsAdherence,omitempty"` |
There was a problem hiding this comment.
In a recent call we discussed the fact that we might need different TLSSecurityProfile structs for APIServer vs Ingress. If we had separate structs, would this field still be a sibling of the TLSSecurityProfile, or would it be a member of the TLSSecurityProfile?
There was a problem hiding this comment.
My understanding is that this adherence field is only applicable for the APIServer type because it will be the central configuration type.
Ingress/Kubelet specific configurations would be overrides to the central configuration and Ingress and Kubelet would be expected to strictly adhere to those.
@joelanford @richardsonnick please correct me if I'm wrong here.
There was a problem hiding this comment.
This is my understanding as well. The ingress/kubelet configurations are overrides to the (central authoritative) apiserver type
There was a problem hiding this comment.
I understand that relationship, that wasn't actually the key part of my question
If we had separate structs, would this field still be a sibling of the TLSSecurityProfile, or would it be a member of the TLSSecurityProfile?
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml`:
- Around line 295-328: The CRD description promises a default for tlsAdherence
but the schema lacks one; add a kubebuilder default tag to the Go API so the
generated CRD enforces "LegacyExternalAPIServerComponentsOnly". Update the
APIServerSpec struct's tlsAdherence field in the Go type (type APIServerSpec,
field tlsAdherence) with a kubebuilder default marker (e.g.
+kubebuilder:default=LegacyExternalAPIServerComponentsOnly), re-run the CRD
generation (controller-gen / repo-specific generation task) to regenerate
payload-manifests/crds so the YAML contains the default, and commit the
regenerated CRD.
| tlsAdherence: | ||
| description: |- | ||
| tlsAdherence controls which components in the cluster adhere to the TLS security profile | ||
| configured on this APIServer resource. | ||
|
|
||
| Valid values are "LegacyExternalAPIServerComponentsOnly" and "StrictAllComponents". | ||
|
|
||
| When set to "LegacyExternalAPIServerComponentsOnly", only the externally exposed | ||
| API server components (kube-apiserver, openshift-apiserver, oauth-apiserver) honor the configured | ||
| TLS profile. Other components continue to use their individual TLS configurations. | ||
|
|
||
| When set to "StrictAllComponents", all components must honor the configured TLS profile | ||
| unless they have a component-specific TLS configuration that overrides it. | ||
| This mode is recommended for security-conscious deployments and is required for | ||
| certain compliance frameworks. | ||
|
|
||
| Note: The Kubelet and IngressController components are excluded from tlsAdherence control | ||
| as they have their own dedicated TLS configuration mechanisms via KubeletConfig and | ||
| IngressController CRs respectively. These component-specific configurations take precedence | ||
| over the cluster-wide tlsSecurityProfile when set. | ||
|
|
||
| Components that encounter an unknown value for tlsAdherence should treat it as "StrictAllComponents" | ||
| and log a warning to ensure forward compatibility while defaulting to the more secure behavior. | ||
|
|
||
| This field is optional. When omitted, it is interpreted as "no opinion" and the cluster | ||
| defaults to LegacyExternalAPIServerComponentsOnly behavior. Future versions of OpenShift | ||
| may require explicitly setting this field to "StrictAllComponents" before upgrading. | ||
| Administrators are encouraged to migrate to "StrictAllComponents" proactively. | ||
|
|
||
| Once set, this field may be changed to a different value. | ||
| enum: | ||
| - LegacyExternalAPIServerComponentsOnly | ||
| - StrictAllComponents | ||
| type: string |
There was a problem hiding this comment.
Add schema default to match the “omitted defaults to Legacy…” contract.
The description promises a default, but the schema doesn’t set one. This leaves behavior dependent on consumer-side fallback. Please add a default (via the Go type annotation and regen) to make the CRD enforce the documented contract.
✅ Suggested schema change (generated output)
tlsAdherence:
description: |-
...
+ default: LegacyExternalAPIServerComponentsOnly
enum:
- LegacyExternalAPIServerComponentsOnly
- StrictAllComponents
type: string📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tlsAdherence: | |
| description: |- | |
| tlsAdherence controls which components in the cluster adhere to the TLS security profile | |
| configured on this APIServer resource. | |
| Valid values are "LegacyExternalAPIServerComponentsOnly" and "StrictAllComponents". | |
| When set to "LegacyExternalAPIServerComponentsOnly", only the externally exposed | |
| API server components (kube-apiserver, openshift-apiserver, oauth-apiserver) honor the configured | |
| TLS profile. Other components continue to use their individual TLS configurations. | |
| When set to "StrictAllComponents", all components must honor the configured TLS profile | |
| unless they have a component-specific TLS configuration that overrides it. | |
| This mode is recommended for security-conscious deployments and is required for | |
| certain compliance frameworks. | |
| Note: The Kubelet and IngressController components are excluded from tlsAdherence control | |
| as they have their own dedicated TLS configuration mechanisms via KubeletConfig and | |
| IngressController CRs respectively. These component-specific configurations take precedence | |
| over the cluster-wide tlsSecurityProfile when set. | |
| Components that encounter an unknown value for tlsAdherence should treat it as "StrictAllComponents" | |
| and log a warning to ensure forward compatibility while defaulting to the more secure behavior. | |
| This field is optional. When omitted, it is interpreted as "no opinion" and the cluster | |
| defaults to LegacyExternalAPIServerComponentsOnly behavior. Future versions of OpenShift | |
| may require explicitly setting this field to "StrictAllComponents" before upgrading. | |
| Administrators are encouraged to migrate to "StrictAllComponents" proactively. | |
| Once set, this field may be changed to a different value. | |
| enum: | |
| - LegacyExternalAPIServerComponentsOnly | |
| - StrictAllComponents | |
| type: string | |
| tlsAdherence: | |
| description: |- | |
| tlsAdherence controls which components in the cluster adhere to the TLS security profile | |
| configured on this APIServer resource. | |
| Valid values are "LegacyExternalAPIServerComponentsOnly" and "StrictAllComponents". | |
| When set to "LegacyExternalAPIServerComponentsOnly", only the externally exposed | |
| API server components (kube-apiserver, openshift-apiserver, oauth-apiserver) honor the configured | |
| TLS profile. Other components continue to use their individual TLS configurations. | |
| When set to "StrictAllComponents", all components must honor the configured TLS profile | |
| unless they have a component-specific TLS configuration that overrides it. | |
| This mode is recommended for security-conscious deployments and is required for | |
| certain compliance frameworks. | |
| Note: The Kubelet and IngressController components are excluded from tlsAdherence control | |
| as they have their own dedicated TLS configuration mechanisms via KubeletConfig and | |
| IngressController CRs respectively. These component-specific configurations take precedence | |
| over the cluster-wide tlsSecurityProfile when set. | |
| Components that encounter an unknown value for tlsAdherence should treat it as "StrictAllComponents" | |
| and log a warning to ensure forward compatibility while defaulting to the more secure behavior. | |
| This field is optional. When omitted, it is interpreted as "no opinion" and the cluster | |
| defaults to LegacyExternalAPIServerComponentsOnly behavior. Future versions of OpenShift | |
| may require explicitly setting this field to "StrictAllComponents" before upgrading. | |
| Administrators are encouraged to migrate to "StrictAllComponents" proactively. | |
| Once set, this field may be changed to a different value. | |
| default: LegacyExternalAPIServerComponentsOnly | |
| enum: | |
| - LegacyExternalAPIServerComponentsOnly | |
| - StrictAllComponents | |
| type: string |
🤖 Prompt for AI Agents
In
`@payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml`
around lines 295 - 328, The CRD description promises a default for tlsAdherence
but the schema lacks one; add a kubebuilder default tag to the Go API so the
generated CRD enforces "LegacyExternalAPIServerComponentsOnly". Update the
APIServerSpec struct's tlsAdherence field in the Go type (type APIServerSpec,
field tlsAdherence) with a kubebuilder default marker (e.g.
+kubebuilder:default=LegacyExternalAPIServerComponentsOnly), re-run the CRD
generation (controller-gen / repo-specific generation task) to regenerate
payload-manifests/crds so the YAML contains the default, and commit the
regenerated CRD.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml`:
- Around line 316-317: The description for tlsAdherence is ambiguous about
“unknown values” — update the explanatory text (the paragraph referencing
tlsAdherence and StrictAllComponents) to clarify that unknown values cannot
occur under the current enum-validated schema and would only appear if the CRD
were extended in a future version; instruct components to treat any unrecognized
tlsAdherence (e.g., introduced by a newer CRD version) as "StrictAllComponents"
and log a warning for forward compatibility, rather than implying unknown values
are currently accepted.
payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml
Show resolved
Hide resolved
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
A few things I missed on the first round
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@config/v1/types_apiserver.go`:
- Around line 271-275: The TLSAdherencePolicy enum validation currently omits
the empty/"no opinion" value while the code defines TLSAdherencePolicyNoOpinion
= "" and docs mention setting the field to empty; update the kubebuilder enum
tag on type TLSAdherencePolicy to include the empty string (""), or
alternatively remove the guidance/constant TLSAdherencePolicyNoOpinion and
adjust docs to disallow empty—ensure changes touch the TLSAdherencePolicy type
declaration and related constants so generated CRDs include the intended allowed
values.
openshift-merge-robot
added
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml`:
- Around line 227-260: The CRD promises a default of
"LegacyExternalAPIServerComponentsOnly" for tlsAdherence but the schema lacks
it; add a kubebuilder default tag to the Go API type (e.g., the tlsAdherence
field on the APIServerSpec / struct field TLSAdherence) like `//
+kubebuilder:default=LegacyExternalAPIServerComponentsOnly` (or the equivalent
tag on TLSAdherence) and then regenerate the CRD (controller-tools/openapi) so
the generated payload-manifests/crds YAML includes the enum default entry;
ensure the field name referenced matches the struct field (TLSAdherence /
tlsAdherence) used by code generation.
payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
Show resolved
Hide resolved
richardsonnick
force-pushed
the
tls-adherance-fate
branch
from
2627c0b to
4d96508
Compare
openshift-merge-robot
removed
the
needs-rebase
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
A few things I noticed:
- Let's make sure we are attaching this feature to the correct Jira component.
- It looks like the payload manifests that were generated are missing adding the
tlsAdherencefield to the DPNU and TPNU manifests. - Integration tests are failing due to some kind of generation issue - let's make sure you are rebased on the latest
masterbranch changes and regenerate files. Let me know if there are persistent issues here and I'll do some digging.
| mustRegister() | ||
|
|
||
| FeatureGateTLSAdherence = newFeatureGate("TLSAdherence"). | ||
| reportProblemsToJiraComponent("kube-apiserver"). |
There was a problem hiding this comment.
I had missed this originally, but is this the appropriate Jira component to be reporting issues to?
I would expect this to be the team implementing and co-ordinating the work for this feature because this is the component that any bugs identified with this feature should be routed to.
If you feel strongly this should be routed to the kube-apiserver component, lets get an ack from the staff engineers/management of the control plane team that they are OK with fielding bugs related to this feature.
There was a problem hiding this comment.
For 1 and 2, it looks like completely bonked the rebase this morning. Apologies. Should be fixed now.
For 3: kube-apiserver may be the wrong component. I don't really have any strong convictions on who should own bugs for this. Does "Networking" fit the bill better? The TLSProfile.curves fate is assigned to "Networking".
Co-authored-by: Bryce Palmer <everettraven@gmail.com>
richardsonnick
force-pushed
the
tls-adherance-fate
branch
from
4d7e3a0 to
e014765
Compare
|
@richardsonnick: The following tests failed, say
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. |
6 participants
User description
Add TLSAdherence feature gate for cluster-wide TLS configuration
This PR introduces the
TLSAdherencefeature gate and adds thetlsAdherencefield to theAPIServerconfig resource (apiserver.config.openshift.io/v1).Summary
The
tlsAdherencefield controls how strictly components in the cluster adhere to the TLS security profile configured on the APIServer resource.Valid values:
Feature Gate Details
TLSAdherenceDevPreviewNoUpgrade,TechPreviewNoUpgradeRelated