Uh oh!
There was an error while loading. Please reload this page.
DPTP-5101: Add E2E test for the TLSAdherence openshift/api field - #31309
DPTP-5101: Add E2E test for the TLSAdherence openshift/api field#31309richardsonnick wants to merge 1 commit into
Conversation
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughNew extended apiserver tests add TLSAdherence feature-gate gating, validate ChangesTLSAdherence Integration Tests
TLS Minimum Versions Context Setup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/extended/apiserver/tls_adherence.go`:
- Around line 120-131: The test assertion in the It block does not account for
the optional spec.tlsAdherence field being unset. Since the field is marked as
optional with omitempty and the test comment acknowledges "when set", the
apiServer.Spec.TLSAdherence value may be nil, causing the BeElementOf assertion
to fail even when the state is valid. Either guard the assertion with a nil
check that conditionally validates only when the field is set, or add the
nil/zero value to the validValues slice so that an unset field is considered a
valid state.
🪄 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: 328d6dfb-fe5e-4dfc-a5f3-811d6c04d0e2
📒 Files selected for processing (1)
test/extended/apiserver/tls_adherence.go
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| g.It("[FeatureGate:TLSAdherence] should have TLSAdherence listed as enabled in featuregate/cluster status [apigroup:config.openshift.io]", func(ctx context.Context) { | ||
| fg, err := oc.AdminConfigClient().ConfigV1().FeatureGates().Get(ctx, "cluster", metav1.GetOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "failed to get featuregate/cluster") | ||
| found := false | ||
| for _, featureGateValues := range fg.Status.FeatureGates { | ||
| for _, enabledGate := range featureGateValues.Enabled { | ||
| if enabledGate.Name == tlsAdherenceFeatureGateName { | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
| if found { | ||
| break | ||
| } | ||
| } | ||
| o.Expect(found).To(o.BeTrue(), | ||
| "TLSAdherence must appear in featuregate/cluster .status.featureGates[].enabled[].name") | ||
| }) |
There was a problem hiding this comment.
Can't hurt to verify this, but I don't think this should count toward the 5 tests, the intent of which are more functional rather than verifying assumptions.
| // Test 5 – verify that no cluster operator is degraded when the TLSAdherence feature gate is active. | ||
| g.It("[FeatureGate:TLSAdherence] should not have any degraded cluster operators [apigroup:config.openshift.io]", func(ctx context.Context) { | ||
| coList, err := oc.AdminConfigClient().ConfigV1().ClusterOperators().List(ctx, metav1.ListOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "failed to list clusteroperators") | ||
| for _, co := range coList.Items { | ||
| for _, condition := range co.Status.Conditions { | ||
| if condition.Type == configv1.OperatorDegraded && condition.Status == configv1.ConditionTrue { | ||
| g.Fail(fmt.Sprintf("cluster operator %q is degraded: %s: %s", | ||
| co.Name, condition.Reason, condition.Message)) | ||
| } | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
I think there is a different kind of test that we can implement that essentially continuously monitors the cluster to verify an invariant is never violated. The way this test is implemented, you'll only get a point in time verification, which would likely result in false negatives.
Scheduling required tests: |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Scheduling required tests: |
e65c04c to
14d023aCompareScheduling required tests: |
14d023a to
f72bc40Comparerichardsonnick
commented
Jun 30, 2026
/test ? |
richardsonnick
commented
Jun 30, 2026
/test e2e-aws-ovn-single-node-techpreview |
richardsonnick
commented
Jun 30, 2026
/test e2e-gcp-ovn-techpreview |
f72bc40 to
a910371CompareThere was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/extended/apiserver/tls_adherence.go (1)
198-210: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueVerify Go's short-circuit case evaluation here is intentional and safe.
The
case original.Spec.TLSSecurityProfile == nil, original.Spec.TLSSecurityProfile.Type == ...:pattern relies on Go evaluating case expressions left-to-right and stopping at the first match, which prevents a nil dereference whenTLSSecurityProfileis nil. This is correct per the Go spec but is a subtle pattern worth a one-line comment for future maintainers, since it's easy to mistakenly "fix" by reordering.🤖 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 `@test/extended/apiserver/tls_adherence.go` around lines 198 - 210, Add a brief inline comment in the switch over original.Spec.TLSSecurityProfile explaining that the nil check must come first because Go evaluates case expressions left-to-right, so the Type access is safe only after the nil case short-circuits; keep this near the tlsShouldWork/tlsShouldNotWork setup in tls adherence test logic so future readers do not reorder the cases.
🤖 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 `@test/extended/apiserver/tls_adherence.go`:
- Around line 184-196: The cleanup in the APIServer TLS adherence test does not
actually restore the documented fallback when original.Spec.TLSAdherence was
unset, so the cluster can remain at StrictAllComponents after the test. Update
the DeferCleanup logic in tls_adherence.go to explicitly set the APIServer spec
back to LegacyAdheringComponentsOnly when the original value was empty, and only
skip the update when that fallback is already present; keep the existing restore
path for the APIServer client update using original.Spec.TLSAdherence and the
APIServers().Update flow.
---
Nitpick comments:
In `@test/extended/apiserver/tls_adherence.go`:
- Around line 198-210: Add a brief inline comment in the switch over
original.Spec.TLSSecurityProfile explaining that the nil check must come first
because Go evaluates case expressions left-to-right, so the Type access is safe
only after the nil case short-circuits; keep this near the
tlsShouldWork/tlsShouldNotWork setup in tls adherence test logic so future
readers do not reorder the cases.
🪄 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: 967b6fb4-7f0d-4fb5-a7a4-7f1da3dc89df
📒 Files selected for processing (2)
test/extended/apiserver/tls.gotest/extended/apiserver/tls_adherence.go
✅ Files skipped from review due to trivial changes (1)
- test/extended/apiserver/tls.go
Uh oh!
There was an error while loading. Please reload this page.
Scheduling required tests: |
a910371 to
b1744bdCompareScheduling required tests: |
richardsonnick
commented
Jul 1, 2026
/test ci/prow/e2e-vsphere-ovn |
1 similar comment
richardsonnick
commented
Jul 1, 2026
/test ci/prow/e2e-vsphere-ovn |
richardsonnick
commented
Jul 6, 2026
/test ci/prow/e2e-aws-csi |
richardsonnick
commented
Jul 6, 2026
/test ci/prow/e2e-vsphere-ovn |
richardsonnick
commented
Jul 21, 2026
/test e2e-aws-csi |
richardsonnick
commented
Jul 21, 2026
/retest-required |
richardsonnick
commented
Jul 21, 2026
/retest-required |
richardsonnick
commented
Jul 21, 2026
/test e2e-aws-ovn-microshift |
richardsonnick
commented
Jul 21, 2026
/retest-required |
1 similar comment
richardsonnick
commented
Jul 21, 2026
/retest-required |
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: cee20b8
New tests seen in this PR at sha: cee20b8
|
richardsonnick
commented
Jul 21, 2026
/retest-required |
richardsonnick
commented
Jul 22, 2026
/assign smg247 |
richardsonnick
commented
Jul 22, 2026
@smg247 Hey could you take a look at this PR for us? |
Uh oh!
There was an error while loading. Please reload this page.
| g.It("[FeatureGate:TLSAdherence] should reject an invalid spec.tlsAdherence value on apiservers/cluster [apigroup:config.openshift.io]", func(ctx context.Context) { | ||
| current, err := oc.AdminConfigClient().ConfigV1().APIServers().Get(ctx, "cluster", metav1.GetOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "failed to get apiservers/cluster") | ||
| desired := current.DeepCopy() | ||
| desired.Spec.TLSAdherence = configv1.TLSAdherencePolicy("InvalidValue") | ||
| _, err = oc.AdminConfigClient().ConfigV1().APIServers().Update(ctx, desired, metav1.UpdateOptions{ | ||
| DryRun: []string{metav1.DryRunAll}, | ||
| }) | ||
| o.Expect(err).To(o.HaveOccurred(), | ||
| "apiservers/cluster should reject an invalid spec.tlsAdherence value") | ||
| o.Expect(k8serrors.IsInvalid(err)).To(o.BeTrue(), | ||
| "error should be a 422 Invalid, got: %v", err) | ||
| }) | ||
| g.It("[FeatureGate:TLSAdherence] should accept and reflect all valid spec.tlsAdherence values on apiservers/cluster [apigroup:config.openshift.io]", func(ctx context.Context) { | ||
| validValues := []configv1.TLSAdherencePolicy{ | ||
| configv1.TLSAdherencePolicyStrictAllComponents, | ||
| configv1.TLSAdherencePolicyLegacyAdheringComponentsOnly, | ||
| } | ||
| for _, value := range validValues { | ||
| current, err := oc.AdminConfigClient().ConfigV1().APIServers().Get(ctx, "cluster", metav1.GetOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "failed to get apiservers/cluster") | ||
| desired := current.DeepCopy() | ||
| desired.Spec.TLSAdherence = value | ||
| result, err := oc.AdminConfigClient().ConfigV1().APIServers().Update(ctx, desired, metav1.UpdateOptions{ | ||
| DryRun: []string{metav1.DryRunAll}, | ||
| }) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), | ||
| "apiservers/cluster should accept spec.tlsAdherence=%s", value) | ||
| o.Expect(result.Spec.TLSAdherence).To( | ||
| o.Equal(value), | ||
| "apiservers/cluster should reflect spec.tlsAdherence=%s", value) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Aren't these already covered by integration tests in the openshift/api repo?
E2E tests are significantly more expensive to run than those, so I would avoid duplicating existing integration tests for an API.
If the integration tests don't exist for the API, let's make sure we add them in openshift/api.
There was a problem hiding this comment.
Yes this already verified in: https://github.com/richardsonnick/api/blob/b1be6747a0a7198e8f79555c137b91fa4b3753f2/config/v1/tests/apiservers.config.openshift.io/TLSAdherence.yaml
Removing these tests.
There was a problem hiding this comment.
Added 2 runtime tests that need a live cluster to replace these:
- Omitted value — confirms components still enforce the profile as
LegacyAdheringComponentsOnlywhen tlsAdherence is unset. - Override precedence — confirms IngressController's explicit override wins over the cluster-wide StrictAllComponents profile.
Uh oh!
There was an error while loading. Please reload this page.
| // checkLegacyAdheringComponents verifies that kube-apiserver and the OpenShift | ||
| // API servers honour the cluster-wide TLS profile at the wire level. | ||
| checkLegacyAdheringComponents := func() { | ||
| for _, target := range []struct{ name, namespace, port string }{ | ||
| {"apiserver", "openshift-kube-apiserver", "443"}, | ||
| {"api", "openshift-apiserver", "443"}, | ||
| {"api", "openshift-oauth-apiserver", "443"}, | ||
| } { |
There was a problem hiding this comment.
Is this an exhaustive list of components that must support both modes prior to promotion?
| candidates := []nonLegacyComponent{ | ||
| { | ||
| name: "cluster-control-plane-machine-set-operator", | ||
| namespace: "openshift-machine-api", | ||
| serviceName: "control-plane-machine-set-operator", | ||
| webhookPort: "9443", | ||
| }, | ||
| { | ||
| name: "cluster-baremetal-operator", | ||
| namespace: "openshift-machine-api", | ||
| serviceName: "baremetal-operator-webhook-service", | ||
| webhookPort: "9443", | ||
| }, | ||
| } |
There was a problem hiding this comment.
Is this an exhaustive list of components that must support StrictAllComponents prior to promotion?
There was a problem hiding this comment.
No there is no defined exhaustive list.
The closest thing that exists is the "Scope and Component Expectations" section, which says all core components honor the cluster-wide profile by default, with exceptions for the Ingress Controller, Kubelet, Routes and Gateway Controller supporting an optional per-component override.
There was a problem hiding this comment.
Would it be possible for us to define this list and ensure they are all meeting the criteria via this testing?
There was a problem hiding this comment.
I can expand the list of components under test here, but this would require keeping track of the exposed ports of each component, which can become flaky/stale as components change. There's already a periodic-tls13-adherence job (runs every 72h) using openshift/tls-scanner in prow CI that provisions a cluster with tlsAdherence: StrictAllComponents and discovers and scans every open port on every pod in the cluster. This test uniquely verifies the live LegacyAdheringComponentsOnly → StrictAllComponents transition and behavioral change and adding every component here would be a duplicate harder to maintain version of tls-scanner's coverage.
There was a problem hiding this comment.
ACK.
Is there a test name associated with that job that we can track stability of as part of the promotion process?
Would be helpful to have signal into what components are/are not updated yet before we promote the feature gate.
There was a problem hiding this comment.
This test tracks stability periodically against StrictAllComponents: periodic-ci-openshift-tls-scanner-main-periodic-tls13-adherence. Failures in this test surface which components are not abiding by the tlsAdherence flag properly (there are still a couple of components that are non-compliant). The signal is per-component, so it emits a junit testcase per each pod/port/service.
Here's a list of the current non-compliant components:
- loki-promtail
- cluster-baremetal-operator
- console
cee20b8 to
7984f30CompareScheduling required tests: |
7bc8efb to
6279f4eCompare6279f4e to
a618f7bComparesmg247
commented
Jul 23, 2026
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: joelanford, richardsonnick, smg247 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Scheduling required tests: |
richardsonnick
commented
Jul 24, 2026
/test e2e-gcp-ovn-upgrade |
richardsonnick
commented
Jul 24, 2026
/test e2e-aws-ovn-serial-2of2 |
1 similar comment
richardsonnick
commented
Jul 24, 2026
/test e2e-aws-ovn-serial-2of2 |
@richardsonnick: 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. |
@richardsonnick: This pull request references DPTP-5101 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
Adds E2E tests for the TLSAdherence openshift/api field. This is a requirement for GA.
Summary by CodeRabbit
spec.tlsAdherencebehavior.spec.tlsAdherencevalues are rejected and valid values (StrictAllComponents,LegacyAdheringComponentsOnly) are accepted.