WIP: Add TLS profile compliance e2e tests - #761
Conversation
Add comprehensive e2e test suite for ingress-node-firewall TLS profile compliance automation with feature gate detection support. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
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:
WalkthroughThe change adds OpenShift TLSAdherence configuration helpers, TLS profile compliance utilities, cluster stabilization checks, OpenShift functional test contexts, diagnostics, documentation, and dependency updates. Only the Modern profile ingress-node-firewall metrics test is active; StrictAllComponents and Custom contexts remain skipped. ChangesTLS adherence and compliance testing
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant FunctionalTests
participant TLSHelpers
participant OpenShiftAPIs
participant TLSCompliance
participant NetworkEndpoint
FunctionalTests->>TLSHelpers: Enable TLSAdherence and configure Modern profile
TLSHelpers->>OpenShiftAPIs: Update FeatureGate and APIServer
TLSHelpers->>OpenShiftAPIs: Wait for cluster stabilization
FunctionalTests->>TLSCompliance: Verify ingress-node-firewall endpoint
TLSCompliance->>NetworkEndpoint: Test allowed TLS protocol
TLSCompliance->>NetworkEndpoint: Test rejected TLS protocol
NetworkEndpoint-->>FunctionalTests: Return compliance result
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 3 warnings)
✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: weliang1 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: 8
🧹 Nitpick comments (7)
test/e2e/tls/REUSABLE_CODE_REFERENCE.md (1)
7-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove personal local filesystem paths from the committed reference doc.
These source references point to a specific contributor's home directory (
/home/weliang/...). They are not resolvable by anyone else and needlessly expose personal machine layout in a shared repository.📝 Proposed fix
-- `/home/weliang/claude-workspace/repository/origin/test/extended/apiserver/tls.go` -- `/home/weliang/Documents/RedHat/Documents/Release/TLS-profile-compliance/Automation/auto-tls-test-simple.sh` +- `origin/test/extended/apiserver/tls.go` (upstream OpenShift origin repository) +- Internal TLS-profile-compliance automation script (see release documentation)🤖 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/e2e/tls/REUSABLE_CODE_REFERENCE.md` around lines 7 - 8, Remove the contributor-specific absolute filesystem paths from REUSABLE_CODE_REFERENCE.md and replace them with repository-relative references or other portable source identifiers, preserving the references to tls.go and auto-tls-test-simple.sh.test/e2e/functional/tests/e2e.go (3)
1165-1274: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate the repeated client-creation boilerplate.
Each of the 5
It()blocks in "Modern TLS Profile with StrictAllComponents" repeats the samekubernetes.NewForConfig/configv1client.NewForConfigconstruction. Move this into a sharedBeforeEachat theContext("Modern TLS Profile with StrictAllComponents", ...)level and reference the resulting clients from eachIt().🤖 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/e2e/functional/tests/e2e.go` around lines 1165 - 1274, Add a shared BeforeEach within the “Modern TLS Profile with StrictAllComponents” context to create and validate the Kubernetes and config clients once, storing them in context-scoped variables. Remove the repeated kubernetes.NewForConfig and configv1client.NewForConfig blocks from each It test and reuse those variables in the existing TLS verification calls.
1276-1297: 🎯 Functional Correctness | 🔵 TrivialCustom TLS Profile tests are empty, and the shared setup only configures Modern.
These 5
It()blocks are// TODO: Implementationstubs with no assertions; they currently always pass without testing anything. Also note that the parentBeforeEach(Lines 1147-1163) unconditionally callstls.EnableTLSAdherence, which hardcodes the Modern profile. When these are implemented, they will need their own setup (for example,tls.EnableTLSAdherenceWithProfile(..., "Custom", ...)) rather than relying on the shared Modern-profileBeforeEach.Do you want me to draft the Custom-profile implementations, or open a tracking issue for this TODO?
🤖 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/e2e/functional/tests/e2e.go` around lines 1276 - 1297, Implement all five Custom TLS Profile tests under “Custom TLS Profile” with real assertions instead of TODO stubs, and add per-test setup that applies the Custom profile via the existing TLS profile helper (for example, EnableTLSAdherenceWithProfile) rather than relying on the parent Modern-profile BeforeEach. Ensure each test targets its named component: ingress-node-firewall, multus-cni, ovn-kubernetes, cluster-network-operator, and openshift-network-console.
1146-1298: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider moving "TLS Profile Compliance" out of the "Disruption" Context.
This new
Context("TLS Profile Compliance", ...)is nested inside the pre-existingContext("Disruption", ...), which otherwise covers daemon/controller-manager restart resilience. TLS profile compliance is unrelated to disruption testing; placing it as a sibling top-levelContextwould make the suite easier to navigate.🤖 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/e2e/functional/tests/e2e.go` around lines 1146 - 1298, Move the “TLS Profile Compliance” Context out of the existing “Disruption” Context and make it a sibling top-level Context in the surrounding test suite. Preserve its BeforeEach setup, Modern TLS tests, and Custom TLS Profile tests unchanged while adjusting only the surrounding block structure.test/e2e/tls/verify_config.sh (1)
4-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winA single failed
oc getaborts the rest of the verification script.
set -euo pipefailcombined with unguardedoc get ... && echo ""chains means any single failing check (for example, a missingFeatureGateor transient API error) stops the script before it reaches the MCP, operator, and node checks. A diagnostics script is more useful if it reports as many sections as possible even when one check fails.🔧 Proposed fix: tolerate individual check failures
echo "1. Checking FeatureGate 'cluster':" echo " Spec.FeatureSet:" -oc get featuregate cluster -o jsonpath='{.spec.featureSet}' && echo "" +oc get featuregate cluster -o jsonpath='{.spec.featureSet}' 2>/dev/null && echo "" || echo " (unavailable)"Apply similarly to the other unguarded
oc getcalls.🤖 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/e2e/tls/verify_config.sh` around lines 4 - 23, Update the verification commands in verify_config.sh, including the FeatureGate and APIServer checks and the corresponding MCP, operator, and node checks, so an individual oc get failure is tolerated and the script continues through all sections while still displaying successful values and blank-line separators. Preserve the overall strict shell settings and apply the same guarded pattern consistently to every unguarded oc get call.test/e2e/tls/tls_compliance.go (1)
84-124: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConfirm
InsecureSkipVerify: trueis intentional for these TLS probes.Every
tls.Configbuilt inGetExpectedTLSConfigsand used byDirectTLSTest/CheckTLSConnectionsetsInsecureSkipVerify: true, flagged repeatedly by static analysis (CWE-295, improper certificate validation). This is plausible here since the goal is testing negotiated protocol version against in-cluster endpoints whose certs the test client has no CA for, but it is worth an explicit code comment stating that rationale so future readers (and scanners) don't mistake it for an oversight.As per coding guidelines, "Flag usage of weak cryptographic algorithms ... and non-constant-time comparison of secrets or tokens" for Go files; certificate-validation bypass falls under this same TLS-security scrutiny.
Also applies to: 261-293
🤖 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/e2e/tls/tls_compliance.go` around lines 84 - 124, Add an explicit comment in GetExpectedTLSConfigs documenting that InsecureSkipVerify is intentional for these in-cluster TLS probes, since they validate negotiated protocol versions without a trusted CA rather than certificate identity. Cover both the working and failing tls.Config constructions, including the configurations used by DirectTLSTest and CheckTLSConnection, without changing their behavior.Source: Coding guidelines
test/e2e/tls/tls.go (1)
278-295: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace all five
wait.PollImmediatecalls withwait.PollUntilContextTimeout.k8s.io/apimachinery v0.36.2still exportswait.PollImmediate, but marks it deprecated and schedules it for removal. Pass the callback context to Kubernetes API calls.🤖 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/e2e/tls/tls.go` around lines 278 - 295, Replace each of the five wait.PollImmediate calls in the TLS test helpers, including the shown MCP polling callback, with wait.PollUntilContextTimeout using the existing callback context and equivalent interval and timeout behavior. Update callback signatures to accept context as required and pass that context to Kubernetes API calls such as MachineConfigPools().List; preserve the existing polling conditions and results.
🤖 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/e2e/functional/tests/e2e.go`:
- Around line 1146-1163: Add an exported cleanup/revert function in
test/e2e/tls/tls.go that restores the original cluster-scoped FeatureGate and
APIServer TLS settings, preserving state captured before EnableTLSAdherence. In
test/e2e/functional/tests/e2e.go lines 1146-1163, add matching Ginkgo teardown
after the TLS Profile Compliance setup and invoke the revert function. In
test/e2e/tls/tls_test.go lines 14-29, add equivalent teardown invoking the same
function; do not leave either test destructive against shared clusters.
- Around line 1191-1273: Add meaningful failure-context messages to both
client-creation assertions in each test: “should verify multus-cni TLS
compliance,” “should verify ovn-kubernetes TLS compliance,” “should verify
cluster-network-operator TLS compliance,” and “should verify
openshift-network-console TLS compliance.” Update the Kubernetes client and
config client Expect(err).NotTo(HaveOccurred()) calls to identify which client
creation failed, matching the contextual messages used by the preceding TLS
compliance test.
In `@test/e2e/tls/README.md`:
- Around line 144-146: Correct the usage sample by replacing the split
`tls.IsTLSAdherence Enabled` call with the valid `tls.IsTLSAdherenceEnabled`
function name, leaving its existing configClient argument and error handling
unchanged.
In `@test/e2e/tls/tls_compliance.go`:
- Around line 440-458: Update execInPod to create a timeout-bound context and
use exec.CommandContext for the oc exec invocation, matching the existing
30-second timeout used by ForwardPortAndExecute and
ForwardPortToResourceAndExecute. Preserve the current stdout/stderr collection
and return behavior while ensuring stalled oc processes are terminated.
- Around line 362-375: Update the logging statement after the TLS profile switch
to avoid dereferencing apiserver.Spec.TLSSecurityProfile when it is nil. Derive
a safe profile label for the nil case, while preserving the existing profile
type output for non-nil TLSSecurityProfile values, and use that label in the
“Testing with profile” log.
In `@test/e2e/tls/tls.go`:
- Around line 174-191: Update patchFeatureGate and patchAPIServerTLSProfile to
perform their Get-and-Update operations inside retry.RetryOnConflict, refetching
the singleton resource and applying the TLS changes on each retry. Preserve the
existing error wrapping and return success only after the update completes
without a conflict.
- Around line 103-127: Update the MCP rollout flow around areAllMCPsComplete,
waitForMCPRolloutStart, and waitForAllMCPsComplete to capture each MCP’s
pre-patch configuration and require the target post-patch configuration before
treating it as complete. Ensure completion requires the configuration to have
changed from the captured baseline and Updated=True; do not rely on
Status.ObservedGeneration == Generation alone, while preserving the no-rollout
path when the target configuration is already present.
In `@test/e2e/tls/USAGE.md`:
- Around line 34-268: The documentation advertises exported
WaitForClusterStability but tls.go only provides unexported
verifyClusterStability, so documented usage cannot compile. Implement and export
WaitForClusterStability(client *testclient.ClientSet) error in tls.go by reusing
the existing cluster-stability logic, or revise the documentation to reference
the available API; keep all examples and comparison tables consistent with the
chosen API.
---
Nitpick comments:
In `@test/e2e/functional/tests/e2e.go`:
- Around line 1165-1274: Add a shared BeforeEach within the “Modern TLS Profile
with StrictAllComponents” context to create and validate the Kubernetes and
config clients once, storing them in context-scoped variables. Remove the
repeated kubernetes.NewForConfig and configv1client.NewForConfig blocks from
each It test and reuse those variables in the existing TLS verification calls.
- Around line 1276-1297: Implement all five Custom TLS Profile tests under
“Custom TLS Profile” with real assertions instead of TODO stubs, and add
per-test setup that applies the Custom profile via the existing TLS profile
helper (for example, EnableTLSAdherenceWithProfile) rather than relying on the
parent Modern-profile BeforeEach. Ensure each test targets its named component:
ingress-node-firewall, multus-cni, ovn-kubernetes, cluster-network-operator, and
openshift-network-console.
- Around line 1146-1298: Move the “TLS Profile Compliance” Context out of the
existing “Disruption” Context and make it a sibling top-level Context in the
surrounding test suite. Preserve its BeforeEach setup, Modern TLS tests, and
Custom TLS Profile tests unchanged while adjusting only the surrounding block
structure.
In `@test/e2e/tls/REUSABLE_CODE_REFERENCE.md`:
- Around line 7-8: Remove the contributor-specific absolute filesystem paths
from REUSABLE_CODE_REFERENCE.md and replace them with repository-relative
references or other portable source identifiers, preserving the references to
tls.go and auto-tls-test-simple.sh.
In `@test/e2e/tls/tls_compliance.go`:
- Around line 84-124: Add an explicit comment in GetExpectedTLSConfigs
documenting that InsecureSkipVerify is intentional for these in-cluster TLS
probes, since they validate negotiated protocol versions without a trusted CA
rather than certificate identity. Cover both the working and failing tls.Config
constructions, including the configurations used by DirectTLSTest and
CheckTLSConnection, without changing their behavior.
In `@test/e2e/tls/tls.go`:
- Around line 278-295: Replace each of the five wait.PollImmediate calls in the
TLS test helpers, including the shown MCP polling callback, with
wait.PollUntilContextTimeout using the existing callback context and equivalent
interval and timeout behavior. Update callback signatures to accept context as
required and pass that context to Kubernetes API calls such as
MachineConfigPools().List; preserve the existing polling conditions and results.
In `@test/e2e/tls/verify_config.sh`:
- Around line 4-23: Update the verification commands in verify_config.sh,
including the FeatureGate and APIServer checks and the corresponding MCP,
operator, and node checks, so an individual oc get failure is tolerated and the
script continues through all sections while still displaying successful values
and blank-line separators. Preserve the overall strict shell settings and apply
the same guarded pattern consistently to every unguarded oc get call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| Context("TLS Profile Compliance", func() { | ||
| BeforeEach(func() { | ||
| // Skip TLS compliance tests on vanilla Kubernetes (OpenShift-only feature) | ||
| if !tls.IsOpenShiftCluster(testclient.Client) { | ||
| Skip("TLS Profile Compliance testing requires OpenShift cluster with config.openshift.io APIs") | ||
| } | ||
|
|
||
| // Enable TLSAdherence feature gate and wait for complete cluster stability | ||
| // This runs ONCE before all TLS compliance tests (both Modern and Custom profiles) | ||
| // It does EVERYTHING: | ||
| // - Patches feature gate to enable TLSAdherence | ||
| // - Waits for MCP rollout (start + complete) | ||
| // - Waits for all cluster operators to settle | ||
| // - Waits for all nodes to be ready and stable | ||
| // - Verifies TLSAdherence is active in feature gate status | ||
| err := tls.EnableTLSAdherence(testclient.Client) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
No cleanup path exists for the cluster-wide TLSAdherence/TLS-profile mutation. Both call sites enable TLSAdherence and set the Modern TLS profile on the cluster-scoped FeatureGate and APIServer resources, but test/e2e/tls/tls.go provides no exported function to revert these settings, so neither caller can restore prior cluster state after tests finish.
test/e2e/functional/tests/e2e.go#L1146-L1163: add anAfterEach/AfterAll(or suite-level teardown) that restores the pre-testFeatureGateandAPIServerTLS settings once a revert function is added totls.go.test/e2e/tls/tls_test.go#L14-L29: add equivalent teardown logic (or clearly document that this test is destructive and not safe to run against shared/persistent clusters).
As per coding guidelines, "Ensure Ginkgo tests use BeforeEach/AfterEach for setup and cleanup; flag tests that create resources without cleanup, especially cluster-scoped resources."
📍 Affects 2 files
test/e2e/functional/tests/e2e.go#L1146-L1163(this comment)test/e2e/tls/tls_test.go#L14-L29
🤖 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/e2e/functional/tests/e2e.go` around lines 1146 - 1163, Add an exported
cleanup/revert function in test/e2e/tls/tls.go that restores the original
cluster-scoped FeatureGate and APIServer TLS settings, preserving state captured
before EnableTLSAdherence. In test/e2e/functional/tests/e2e.go lines 1146-1163,
add matching Ginkgo teardown after the TLS Profile Compliance setup and invoke
the revert function. In test/e2e/tls/tls_test.go lines 14-29, add equivalent
teardown invoking the same function; do not leave either test destructive
against shared clusters.
Source: Coding guidelines
| configClient, _ := configv1client.NewForConfig(client.Config) | ||
| enabled, err := tls.IsTLSAdherence Enabled(configClient) | ||
| if err != nil { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the broken function name in the usage sample.
tls.IsTLSAdherence Enabled(configClient) has a stray space, splitting IsTLSAdherenceEnabled into two tokens. Copying this sample produces invalid Go code.
📝 Proposed fix
-enabled, err := tls.IsTLSAdherence Enabled(configClient)
+enabled, err := tls.IsTLSAdherenceEnabled(configClient)📝 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.
| configClient, _ := configv1client.NewForConfig(client.Config) | |
| enabled, err := tls.IsTLSAdherence Enabled(configClient) | |
| if err != nil { | |
| configClient, _ := configv1client.NewForConfig(client.Config) | |
| enabled, err := tls.IsTLSAdherenceEnabled(configClient) | |
| if err != nil { |
🤖 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/e2e/tls/README.md` around lines 144 - 146, Correct the usage sample by
replacing the split `tls.IsTLSAdherence Enabled` call with the valid
`tls.IsTLSAdherenceEnabled` function name, leaving its existing configClient
argument and error handling unchanged.
| switch { | ||
| case apiserver.Spec.TLSSecurityProfile == nil, apiserver.Spec.TLSSecurityProfile.Type == configv1.TLSProfileIntermediateType: | ||
| tlsWorkVersion = "tls1_2" | ||
| tlsFailVersion = "tls1_1" | ||
| case apiserver.Spec.TLSSecurityProfile.Type == configv1.TLSProfileModernType: | ||
| tlsWorkVersion = "tls1_3" | ||
| tlsFailVersion = "tls1_2" | ||
| default: | ||
| tlsWorkVersion = "tls1_2" | ||
| tlsFailVersion = "tls1" | ||
| } | ||
|
|
||
| log.Printf("Testing with profile: %s (TLS %s should work, TLS %s should fail)", | ||
| apiserver.Spec.TLSSecurityProfile.Type, tlsWorkVersion, tlsFailVersion) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Nil pointer dereference when TLSSecurityProfile is nil.
The switch at Line 362-372 safely handles apiserver.Spec.TLSSecurityProfile == nil using Go's left-to-right case short-circuit. But Line 374-375 dereferences apiserver.Spec.TLSSecurityProfile.Type again, outside that guard. When TLSSecurityProfile is nil (a normal state for a cluster that has not set an explicit TLS profile), this line panics.
🐛 Proposed fix
+ var profileType configv1.TLSProfileType
+ if apiserver.Spec.TLSSecurityProfile != nil {
+ profileType = apiserver.Spec.TLSSecurityProfile.Type
+ }
+
log.Printf("Testing with profile: %s (TLS %s should work, TLS %s should fail)",
- apiserver.Spec.TLSSecurityProfile.Type, tlsWorkVersion, tlsFailVersion)
+ profileType, tlsWorkVersion, tlsFailVersion)📝 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.
| switch { | |
| case apiserver.Spec.TLSSecurityProfile == nil, apiserver.Spec.TLSSecurityProfile.Type == configv1.TLSProfileIntermediateType: | |
| tlsWorkVersion = "tls1_2" | |
| tlsFailVersion = "tls1_1" | |
| case apiserver.Spec.TLSSecurityProfile.Type == configv1.TLSProfileModernType: | |
| tlsWorkVersion = "tls1_3" | |
| tlsFailVersion = "tls1_2" | |
| default: | |
| tlsWorkVersion = "tls1_2" | |
| tlsFailVersion = "tls1" | |
| } | |
| log.Printf("Testing with profile: %s (TLS %s should work, TLS %s should fail)", | |
| apiserver.Spec.TLSSecurityProfile.Type, tlsWorkVersion, tlsFailVersion) | |
| var profileType configv1.TLSProfileType | |
| if apiserver.Spec.TLSSecurityProfile != nil { | |
| profileType = apiserver.Spec.TLSSecurityProfile.Type | |
| } | |
| log.Printf("Testing with profile: %s (TLS %s should work, TLS %s should fail)", | |
| profileType, tlsWorkVersion, tlsFailVersion) |
🤖 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/e2e/tls/tls_compliance.go` around lines 362 - 375, Update the logging
statement after the TLS profile switch to avoid dereferencing
apiserver.Spec.TLSSecurityProfile when it is nil. Derive a safe profile label
for the nil case, while preserving the existing profile type output for non-nil
TLSSecurityProfile values, and use that label in the “Testing with profile” log.
| func execInPod(client kubernetes.Interface, namespace, podName, containerName string, command []string) (string, error) { | ||
| // Note: This is a simplified version. In production, you would use: | ||
| // - k8s.io/client-go/tools/remotecommand | ||
| // - Proper SPDY/WebSocket connection | ||
| // For now, we'll use oc exec via shell | ||
|
|
||
| args := []string{"exec", "-n", namespace, podName, "-c", containerName, "--"} | ||
| args = append(args, command...) | ||
|
|
||
| cmd := exec.Command("oc", args...) | ||
| var stdout, stderr strings.Builder | ||
| cmd.Stdout = &stdout | ||
| cmd.Stderr = &stderr | ||
|
|
||
| err := cmd.Run() | ||
| output := stdout.String() + stderr.String() | ||
|
|
||
| return output, err | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a timeout to execInPod's oc exec call.
execInPod runs exec.Command("oc", args...) with cmd.Run() and no context or deadline. If the oc exec connection itself stalls (for example, an API server network issue), this call blocks indefinitely; the timeout 3 embedded in the caller's command only bounds the process running inside the container, not the oc exec process on the test runner. ForwardPortAndExecute and ForwardPortToResourceAndExecute already use exec.CommandContext with a 30-second timeout for the same reason.
🔧 Proposed fix
-func execInPod(client kubernetes.Interface, namespace, podName, containerName string, command []string) (string, error) {
+func execInPod(client kubernetes.Interface, namespace, podName, containerName string, command []string) (string, error) {
+ ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+ defer cancel()
args := []string{"exec", "-n", namespace, podName, "-c", containerName, "--"}
args = append(args, command...)
- cmd := exec.Command("oc", args...)
+ cmd := exec.CommandContext(ctx, "oc", args...)📝 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.
| func execInPod(client kubernetes.Interface, namespace, podName, containerName string, command []string) (string, error) { | |
| // Note: This is a simplified version. In production, you would use: | |
| // - k8s.io/client-go/tools/remotecommand | |
| // - Proper SPDY/WebSocket connection | |
| // For now, we'll use oc exec via shell | |
| args := []string{"exec", "-n", namespace, podName, "-c", containerName, "--"} | |
| args = append(args, command...) | |
| cmd := exec.Command("oc", args...) | |
| var stdout, stderr strings.Builder | |
| cmd.Stdout = &stdout | |
| cmd.Stderr = &stderr | |
| err := cmd.Run() | |
| output := stdout.String() + stderr.String() | |
| return output, err | |
| } | |
| func execInPod(client kubernetes.Interface, namespace, podName, containerName string, command []string) (string, error) { | |
| // Note: This is a simplified version. In production, you would use: | |
| // - k8s.io/client-go/tools/remotecommand | |
| // - Proper SPDY/WebSocket connection | |
| // For now, we'll use oc exec via shell | |
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | |
| defer cancel() | |
| args := []string{"exec", "-n", namespace, podName, "-c", containerName, "--"} | |
| args = append(args, command...) | |
| cmd := exec.CommandContext(ctx, "oc", args...) | |
| var stdout, stderr strings.Builder | |
| cmd.Stdout = &stdout | |
| cmd.Stderr = &stderr | |
| err := cmd.Run() | |
| output := stdout.String() + stderr.String() | |
| return output, err | |
| } |
🤖 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/e2e/tls/tls_compliance.go` around lines 440 - 458, Update execInPod to
create a timeout-bound context and use exec.CommandContext for the oc exec
invocation, matching the existing 30-second timeout used by
ForwardPortAndExecute and ForwardPortToResourceAndExecute. Preserve the current
stdout/stderr collection and return behavior while ensuring stalled oc processes
are terminated.
| // Step 3: Wait for MCP rollout to start (or verify already complete) | ||
| log.Printf("Step 3: Checking MCP status") | ||
|
|
||
| // First check if MCPs are already complete | ||
| mcpsAlreadyComplete, err := areAllMCPsComplete(machineConfigClient) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to check MCP status: %w", err) | ||
| } | ||
|
|
||
| if mcpsAlreadyComplete { | ||
| log.Printf("✓ All MCPs are already updated (no rollout needed)") | ||
| } else { | ||
| log.Printf("Waiting for MCP rollout to start") | ||
| if err := waitForMCPRolloutStart(machineConfigClient, 5*time.Minute); err != nil { | ||
| return err | ||
| } | ||
| log.Printf("✓ MCP rollout started") | ||
|
|
||
| // Step 4: Wait for all MCPs to complete | ||
| log.Printf("Step 4: Waiting for MCP rollout to complete") | ||
| if err := waitForAllMCPsComplete(machineConfigClient, 30*time.Minute); err != nil { | ||
| return err | ||
| } | ||
| log.Printf("✓ All MCPs updated successfully") | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate file map ---'
ast-grep outline test/e2e/tls/tls.go --match areAllMCPsComplete --view expanded || true
printf '%s\n' '--- relevant source ---'
sed -n '1,220p' test/e2e/tls/tls.go
printf '%s\n' '--- MCP helper definitions and call sites ---'
rg -n -C 8 'areAllMCPsComplete|waitForMCPRolloutStart|waitForAllMCPsComplete|TLSAdherence|FeatureGate|APIServer' test/e2e/tls .Repository: openshift/ingress-node-firewall
Length of output: 50388
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- tls.go size and relevant ranges ---'
wc -l test/e2e/tls/tls.go
rg -n -C 12 'areAllMCPsComplete|waitForMCPRolloutStart|waitForAllMCPsComplete|TLSAdherence|FeatureGate|APIServer' test/e2e/tls/tls.go
printf '%s\n' '--- repository call sites, excluding vendor ---'
rg -n -g '!vendor/**' -g '!*generated*' 'areAllMCPsComplete|waitForMCPRolloutStart|waitForAllMCPsComplete|TLSAdherence|tlsAdherence' .
printf '%s\n' '--- MCP type definitions and status fields in vendored APIs ---'
rg -n -C 8 'type MachineConfigPoolStatus|ObservedGeneration|Updated.*bool|MachineConfigPool' vendor/github.com/openshift/api/machineconfiguration/v1/types.go vendor/github.com/openshift/client-go/machineconfiguration/clientset/versioned/typed/machineconfiguration/v1 2>/dev/null | head -n 240Repository: openshift/ingress-node-firewall
Length of output: 50387
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- helper implementations ---'
sed -n '241,360p' test/e2e/tls/tls.go
printf '%s\n' '--- TLS-related machine configuration references ---'
rg -n -g '!vendor/**' -g '!*generated*' -i -C 5 'tlsadherence|tls adherence|tlssecurityprofile|MachineConfig|machine config' pkg test | head -n 500
printf '%s\n' '--- commit and dependency context ---'
sed -n '1,120p' go.mod
rg -n -g '!vendor/**' 'openshift/api|machine-config|machineconfiguration' go.mod go.sumRepository: openshift/ingress-node-firewall
Length of output: 40300
🌐 Web query:
OpenShift TLSAdherence feature gate MachineConfigPool rollout Machine Config Operator
💡 Result:
The TLSAdherence feature gate in OpenShift allows administrators to control how strictly cluster components adhere to the centralized TLS security profile configured on the APIServer resource [1][2]. Key details regarding this feature and its interaction with the Machine Config Operator (MCO): 1. Purpose: When the TLSAdherence feature gate is enabled, it exposes the tlsAdherence field on the apiserver.config.openshift.io/v1 APIServer resource [1][2]. This field allows users to set a policy—such as LegacyAdheringComponentsOnly or StrictAllComponents—that dictates whether components must strictly follow the configured TLS profile [3][4][2]. 2. Machine Config Operator (MCO) Role: The Machine Config Operator manages the application of configuration changes to nodes. When FeatureGates are enabled or modified, the MCO is responsible for orchestrating the resulting changes across the cluster [5][6]. Enabling the TLSAdherence feature gate triggers the MCO to create new machine configurations and update the MachineConfigPools [5]. During this rollout, scheduling on each affected node is typically disabled while the configuration is applied [5]. 3. Status: The TLSAdherence feature was initially introduced as a Technology Preview feature [2][7]. It has since been promoted to General Availability (GA) and is included in default feature sets in newer OpenShift versions [1][3]. 4. Usage: While the feature gate is cluster-wide, specific operators or components (such as the cert-manager Operator) may also have their own CR-level settings that require the cluster-wide TLSAdherence feature gate to be enabled to function in strict modes [7]. Top results: [5][1][3][2][7]
Citations:
- 1: HPCASE-362: Promote TLSAdherence feature gate to GA api#2883
- 2: Add TLS adherence feature gate api#2680
- 3: openshift/api@8182f05...54a3998
- 4: openshift/api@165d38f
- 5: https://docs.okd.io/4.22/nodes/clusters/nodes-cluster-enabling-features.html
- 6: https://search.help.openshift.com/cache/?docId=1531f476751e4a95a83369552bac90d1&hq=cli
- 7: https://docs.redhat.com/en/documentation/openshift_container_platform/4.22/html/security_and_compliance/cert-manager-operator-for-red-hat-openshift
🌐 Web query:
site:docs.openshift.com TLSAdherence StrictAllComponents MachineConfigPool
💡 Result:
Based on available technical documentation for OpenShift Container Platform, there is no standard configuration parameter or field named TLSAdherence or StrictAllComponents associated with MachineConfigPool resources [1][2][3]. It is possible that these terms refer to custom labels, annotations, or internal configurations specific to a particular environment or a third-party add-on rather than core OpenShift Machine Config Operator (MCO) functionality [1][4]. In OpenShift, TLS security profiles are typically managed via the KubeletConfig custom resource, which uses a machineConfigPoolSelector to target specific node pools [5][3]. The MachineConfigPool resource itself is primarily used to manage node configurations, upgrades, and status reporting [1][6][3]. If you are encountering these terms in a specific error message or configuration requirement, it is recommended to check: 1. Custom operator documentation if you are using specialized security or compliance tooling [7]. 2. Your cluster's custom resource definitions (CRDs) to see if these fields have been added by a custom extension. 3. Organizational policy or internal documentation if this is an environment-specific naming convention.
Citations:
- 1: https://docs.openshift.com/container-platform/4.18/machine_configuration/index.html
- 2: https://docs.openshift.com/container-platform/4.13/rest_api/machine_apis/machineconfigpool-machineconfiguration-openshift-io-v1.html
- 3: https://docs.openshift.com/container-platform/4.17/post_installation_configuration/node-tasks.html
- 4: https://docs.openshift.com/container-platform/4.18/machine_configuration/machine-configs-configure.html
- 5: https://docs.openshift.com/container-platform/4.11/security/tls-security-profiles.html
- 6: https://docs.openshift.com/container-platform/4.15/post_installation_configuration/machine-configuration-tasks.html
- 7: https://docs.openshift.com/container-platform/4.20/security/cert_manager_operator/cert-manager-operator-uninstall.html
🌐 Web query:
site:github.com/openshift/machine-config-operator TLSAdherence
💡 Result:
There is no universally defined technical standard or cybersecurity term known as "TLSAdherence." Based on current technical documentation and industry practices, it is likely that this term is a misspelling or a colloquial reference to "TLS Security Profile" (often referred to as tlsSecurityProfile) or a general requirement for "TLS Adherence" (compliance) within specific software architectures, such as Red Hat OpenShift [1][2][3]. In the context of OpenShift and similar container orchestration platforms, tlsSecurityProfile is a configuration parameter used to enforce consistent Transport Layer Security (TLS) settings, such as minimum TLS versions and permitted cipher suites, across various components like the API server, kubelet, and CRI-O [1][3][4]. Organizations use these profiles to ensure their infrastructure adheres to specific security mandates by selecting from predefined levels (e.g., Old, Intermediate, Modern) [1][3]. If you encountered this term in a specific log file, configuration parameter, or security audit report, it may be a custom internal identifier or a shorthand used by a specific vendor or security compliance tool to denote whether a system correctly follows defined TLS security policies.
Citations:
- 1: MCO-1202: MCO-1203: MCO-1204: MCO-1205: MCO-1213: Implementing tlsSecurityProfile for MCO machine-config-operator#4435
- 2: OCPNODE-464: add tls security profile support machine-config-operator#2324
- 3: http://github.com/openshift/machine-config-operator/pull/5672
- 4: OCPBUGS-35800: kubelet config controller does not always propogate tlsSecurityProfile machine-config-operator#4512
🌐 Web query:
site:github.com/openshift/machine-config-operator FeatureGate MachineConfigPool feature gates controller
💡 Result:
In the context of the OpenShift Machine Config Operator (MCO), there is no single controller named "FeatureGate MachineConfigPool." Instead, the MachineConfigPool (MCP) and its associated controllers—such as the Kubelet Config controller and the Container Runtime Config controller—utilize a centralized feature gate mechanism to reconcile configurations based on the cluster's enabled features [1][2][3]. Key aspects of how feature gates and the MCO interact include: 1. Centralized Feature Gate Handling: The MCO uses a wrapper around the OpenShift featuregates.FeatureGateAccess interface [4]. This allows different controllers to check if a specific feature is enabled (e.g., features.Enabled(configv1.FeatureGate...)) without needing to manually handle errors during feature gate fetching [3][4]. 2. Controller Reconciliation: Controllers like the Kubelet Config controller monitor changes in FeatureGate custom resources [1]. When a feature gate's state changes, the controller is notified—often via a FeatureGate informer or a change handler—and triggers a reconciliation loop [1][2][5]. During this process, the controller inspects all existing MachineConfigPools (MCPs) to apply or update configurations (such as kubelet settings or container runtime configurations) that are dependent on the feature's status [2][3]. 3. Kubelet Config and MCP Interaction: For Kubelet configuration, the controller fetches the current feature gate set and generates a "FeatureMap." It then iterates through all defined MCPs to ensure that the generated MachineConfig objects for each pool correctly reflect the required feature-gated settings [2]. 4. Implementation Detail: Because these controllers watch FeatureGate resources, updates to features can cause widespread updates across node pools. It is important to note that historically, ensuring these controllers react properly to additions or changes in MCPs themselves has required registering specific event handlers on the MCP informer to ensure timely reconciliation [6]. In summary, feature gates act as a configuration toggle that the MCO controllers evaluate during their reconciliation loops to determine which features should be active on which MachineConfigPools [2][3][7].
Citations:
- 1: https://github.com/openshift/machine-config-operator/blob/v4.2.0-alpha.0/pkg/controller/kubelet-config/kubelet_config_controller.go
- 2: https://github.com/openshift/machine-config-operator/blob/v4.2.0-alpha.0/pkg/controller/kubelet-config/kubelet_config_features.go
- 3: https://github.com/openshift/machine-config-operator/blob/master/pkg/controller/container-runtime-config/container_runtime_config_controller.go
- 4: MCO-1661: Feature Gate handling rework machine-config-operator#5029
- 5: openshift/machine-config-operator@bf9193a
- 6: The controller doesn't generate
%s-generated-kubeletconfig-%sfiles for new MCPs machine-config-operator#5521 - 7: openshift/machine-config-operator@baa37ae
🌐 Web query:
OpenShift 4.22 TLSAdherence feature gate machine config operator rollout
💡 Result:
In OpenShift Container Platform 4.22, the TLSAdherence feature gate is enabled by default [1][2]. This feature introduces the tlsAdherence field within the APIServer configuration (apiserver.config.openshift.io/v1), which allows cluster administrators to enforce how strictly individual cluster components must adhere to the centralized TLS security profile [3][4][5]. The tlsAdherence setting supports two primary policies: 1. LegacyAdheringComponentsOnly (default): Maintains backward-compatible behavior where components continue to honor the cluster-wide TLS profile if they are already designed to do so, while other components may continue to use their own individual TLS configurations [5]. 2. StrictAllComponents: Enforces that all components must honor the configured cluster-wide TLS security profile unless they possess an explicit, component-specific override [5]. Regarding the Machine Config Operator (MCO) rollout: The MCO has been updated to integrate with the global cluster TLS settings [6]. The operator monitors the APIServer object for updates to TLS settings, including the tlsAdherence policy [7][8]. When these TLS configurations are modified, the MCO reconciles the changes by updating the relevant component manifests, such as those for the kube-rbac-proxy or kubelet configurations [6][7]. If a change necessitates updates to node-level configurations, the MCO triggers a MachineConfig rollout to the affected node pools to ensure the new TLS settings are applied across the cluster [6][7]. Administrators can verify the propagation of these settings by observing the MachineConfig status and inspecting the TLS configurations on nodes [7].
Citations:
- 1: https://amd64.ocp.releases.ci.openshift.org/releasetag/4.22.0?from=4.21.18
- 2: https://openshift-release.apps.ci.l2s4.p1.openshiftapps.com/releasetag/4.22.0?from=4.22.0-ec.3
- 3: Add TLS adherence feature gate api#2680
- 4: HPCASE-362: Promote TLSAdherence feature gate to GA api#2883
- 5: openshift/api@5c75e62
- 6: MCO-1202: MCO-1203: MCO-1204: MCO-1205: MCO-1213: Implementing tlsSecurityProfile for MCO machine-config-operator#4435
- 7: OCPBUGS-35800: kubelet config controller does not always propogate tlsSecurityProfile machine-config-operator#4512
- 8: https://amd64.ocp.releases.ci.openshift.org/releasestream/4-stable/release/4.22.2
🏁 Script executed:
#!/bin/bash
set -eu
for n in 4435 4512 5029; do
printf '%s\n' "--- MCO PR $n metadata ---"
curl -fsSL "https://api.github.com/repos/openshift/machine-config-operator/pulls/$n" |
jq '{number,title,state,merged_at,html_url,body}'
done
printf '%s\n' '--- MCO repository references to TLS terms on the current default branch ---'
curl -fsSL 'https://api.github.com/repos/openshift/machine-config-operator/git/trees/master?recursive=1' |
jq -r '.tree[] | select(.type=="blob") | .path' |
grep -Ei 'tls|kubelet|feature' | head -n 200Repository: openshift/ingress-node-firewall
Length of output: 529
🏁 Script executed:
#!/bin/bash
set -eu
for n in 4435 4512; do
printf '%s\n' "--- files changed by MCO PR $n ---"
curl -fsSL "https://api.github.com/repos/openshift/machine-config-operator/pulls/$n/files?per_page=100" |
jq -r '.[] | [.filename, (.status // ""), (.patch // "")] | `@tsv`' |
sed -n '1,240p'
doneRepository: openshift/ingress-node-firewall
Length of output: 341
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '350,445p' test/e2e/tls/tls.go
printf '%s\n' '--- MCP condition semantics in vendored API ---'
sed -n '511,575p' vendor/github.com/openshift/api/machineconfiguration/v1/types.go
printf '%s\n' '--- documentation statements about whether this helper always rolls out ---'
sed -n '1,90p' test/e2e/tls/CHANGES.md
sed -n '1,75p' test/e2e/tls/USAGE.mdRepository: openshift/ingress-node-firewall
Length of output: 11374
🏁 Script executed:
#!/bin/bash
set -eu
fetch() {
url="$1"
printf '%s\n' "--- $url ---"
body="$(curl -fsSL "$url" || true)"
printf '%s\n' "$body" | grep -Ein -C 4 'tlsadherence|tls adherence|tlssecurityprofile|featuregate|machineconfig' | head -n 240 || true
}
fetch 'https://raw.githubusercontent.com/openshift/machine-config-operator/master/pkg/controller/kubelet-config/kubelet_config_features.go'
fetch 'https://raw.githubusercontent.com/openshift/machine-config-operator/master/pkg/controller/kubelet-config/kubelet_config_controller.go'
fetch 'https://raw.githubusercontent.com/openshift/machine-config-operator/master/pkg/controller/container-runtime-config/container_runtime_config_controller.go'
printf '%s\n' '--- direct PR pages, filtered for rollout and TLS terms ---'
for n in 4435 4512; do
curl -fsSL "https://github.com/openshift/machine-config-operator/pull/$n" |
sed 's/<[^>]*>/ /g' |
grep -Eio '.{0,100}(tlsadherence|tls security|machineconfig|rollout).{0,180}' |
head -n 80 || true
doneRepository: openshift/ingress-node-firewall
Length of output: 50387
🏁 Script executed:
#!/bin/bash
set -eu
for n in 4435 4512; do
printf '%s\n' "--- MCO PR $n patch summary ---"
curl -fsSL "https://github.com/openshift/machine-config-operator/pull/$n.patch" |
grep -E '^(From |Subject:|diff --git|\+\+\+|--- )' |
head -n 180
printf '%s\n' "--- MCO PR $n TLS/adherence patch lines ---"
curl -fsSL "https://github.com/openshift/machine-config-operator/pull/$n.patch" |
grep -Ein -C 8 'tlsadherence|tls adherence|tlssecurityprofile|machineconfig rollout|rollout' |
head -n 260 || true
doneRepository: openshift/ingress-node-firewall
Length of output: 28666
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- deterministic model of areAllMCPsComplete ---'
python3 - <<'PY'
def are_all_complete(mcps):
for mcp in mcps:
if mcp["machine_count"] == 0:
continue
updated = any(c == ("Updated", True) for c in mcp["conditions"])
updating = any(c == ("Updating", True) for c in mcp["conditions"])
if not updated or updating:
return False
return True
stale = [{
"machine_count": 3,
"conditions": [("Updated", True), ("Updating", False)],
}]
print("stale pre-reconcile status:", are_all_complete(stale))
print("reconciling status:", are_all_complete([{
"machine_count": 3,
"conditions": [("Updated", False), ("Updating", True)],
}]))
PY
printf '%s\n' '--- current MCO APIServer event and feature reconciliation paths ---'
curl -fsSL 'https://raw.githubusercontent.com/openshift/machine-config-operator/master/pkg/controller/kubelet-config/kubelet_config_controller.go' |
sed -n '138,235p;400,455p'
printf '%s\n' '--- TLSAdherence feature-gate declarations in repository API ---'
rg -n -C 3 'TLSAdherence' vendor/github.com/openshift/api/config/v1 vendor/github.com/openshift/api/featuresRepository: openshift/ingress-node-firewall
Length of output: 12984
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- MCO APIServer update handler ---'
curl -fsSL 'https://raw.githubusercontent.com/openshift/machine-config-operator/master/pkg/controller/kubelet-config/kubelet_config_controller.go' |
sed -n '228,275p'
printf '%s\n' '--- MCO generated config inputs ---'
curl -fsSL 'https://raw.githubusercontent.com/openshift/machine-config-operator/master/pkg/controller/kubelet-config/kubelet_config_controller.go' |
sed -n '590,635p'
curl -fsSL 'https://raw.githubusercontent.com/openshift/machine-config-operator/master/pkg/controller/kubelet-config/kubelet_config_features.go' |
sed -n '55,130p'
printf '%s\n' '--- local helper and patch ordering ---'
sed -n '76,128p' test/e2e/tls/tls.go
sed -n '241,273p' test/e2e/tls/tls.goRepository: openshift/ingress-node-firewall
Length of output: 9106
Wait for the target MCP configuration, not only current conditions.
A TLSAdherence enablement does not always require a rollout, but areAllMCPsComplete can accept stale pre-patch status when a rollout is required. Capture each MCP’s pre-patch configuration and wait for a changed configuration before accepting Updated=True. Status.ObservedGeneration == Generation alone is insufficient.
🤖 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/e2e/tls/tls.go` around lines 103 - 127, Update the MCP rollout flow
around areAllMCPsComplete, waitForMCPRolloutStart, and waitForAllMCPsComplete to
capture each MCP’s pre-patch configuration and require the target post-patch
configuration before treating it as complete. Ensure completion requires the
configuration to have changed from the captured baseline and Updated=True; do
not rely on Status.ObservedGeneration == Generation alone, while preserving the
no-rollout path when the target configuration is already present.
| func patchFeatureGate(ctx context.Context, configClient configv1client.Interface, fg *configv1.FeatureGate) error { | ||
| if fg.Spec.FeatureSet == configv1.CustomNoUpgrade && fg.Spec.CustomNoUpgrade != nil { | ||
| // Add to existing CustomNoUpgrade | ||
| fg.Spec.CustomNoUpgrade.Enabled = append(fg.Spec.CustomNoUpgrade.Enabled, "TLSAdherence") | ||
| } else { | ||
| // Set CustomNoUpgrade with TLSAdherence | ||
| fg.Spec.FeatureSet = configv1.CustomNoUpgrade | ||
| fg.Spec.CustomNoUpgrade = &configv1.CustomFeatureGates{ | ||
| Enabled: []configv1.FeatureGateName{"TLSAdherence"}, | ||
| } | ||
| } | ||
|
|
||
| _, err := configClient.ConfigV1().FeatureGates().Update(ctx, fg, metav1.UpdateOptions{}) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to update featuregate: %w", err) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Add conflict retry to the FeatureGate and APIServer updates.
patchFeatureGate and patchAPIServerTLSProfile both Get then Update the same singleton "cluster" resources that other in-cluster controllers frequently update (status subresource writes still bump resourceVersion). Without conflict retry, a concurrent status update between the Get and Update calls causes Update to fail with a 409 Conflict, and the whole enablement flow returns an error instead of retrying.
🔧 Proposed fix using retry.RetryOnConflict
+ "k8s.io/client-go/util/retry"
...
func patchFeatureGate(ctx context.Context, configClient configv1client.Interface, fg *configv1.FeatureGate) error {
- if fg.Spec.FeatureSet == configv1.CustomNoUpgrade && fg.Spec.CustomNoUpgrade != nil {
- // Add to existing CustomNoUpgrade
- fg.Spec.CustomNoUpgrade.Enabled = append(fg.Spec.CustomNoUpgrade.Enabled, "TLSAdherence")
- } else {
- // Set CustomNoUpgrade with TLSAdherence
- fg.Spec.FeatureSet = configv1.CustomNoUpgrade
- fg.Spec.CustomNoUpgrade = &configv1.CustomFeatureGates{
- Enabled: []configv1.FeatureGateName{"TLSAdherence"},
- }
- }
-
- _, err := configClient.ConfigV1().FeatureGates().Update(ctx, fg, metav1.UpdateOptions{})
- if err != nil {
- return fmt.Errorf("failed to update featuregate: %w", err)
- }
- return nil
+ return retry.RetryOnConflict(retry.DefaultRetry, func() error {
+ latest, err := configClient.ConfigV1().FeatureGates().Get(ctx, "cluster", metav1.GetOptions{})
+ if err != nil {
+ return fmt.Errorf("failed to refresh featuregate: %w", err)
+ }
+ if latest.Spec.FeatureSet == configv1.CustomNoUpgrade && latest.Spec.CustomNoUpgrade != nil {
+ latest.Spec.CustomNoUpgrade.Enabled = append(latest.Spec.CustomNoUpgrade.Enabled, "TLSAdherence")
+ } else {
+ latest.Spec.FeatureSet = configv1.CustomNoUpgrade
+ latest.Spec.CustomNoUpgrade = &configv1.CustomFeatureGates{
+ Enabled: []configv1.FeatureGateName{"TLSAdherence"},
+ }
+ }
+ _, err = configClient.ConfigV1().FeatureGates().Update(ctx, latest, metav1.UpdateOptions{})
+ return err
+ })
}Apply the same pattern to patchAPIServerTLSProfile.
Also applies to: 193-239
🤖 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/e2e/tls/tls.go` around lines 174 - 191, Update patchFeatureGate and
patchAPIServerTLSProfile to perform their Get-and-Update operations inside
retry.RetryOnConflict, refetching the singleton resource and applying the TLS
changes on each retry. Preserve the existing error wrapping and return success
only after the update completes without a conflict.
| ### 2. `WaitForClusterStability()` - ⭐ REUSABLE Cluster Stability Wait | ||
|
|
||
| **This is the MOST REUSABLE function** - use it after ANY disruptive cluster operation! | ||
|
|
||
| ```go | ||
| func WaitForClusterStability(client *testclient.ClientSet) error | ||
| ``` | ||
|
|
||
| **Use cases:** | ||
| - ✅ After changing TLS profiles | ||
| - ✅ After enabling/disabling ANY feature gate | ||
| - ✅ After modifying cluster configuration | ||
| - ✅ After ANY operation that triggers MCP updates | ||
|
|
||
| **What it does:** | ||
| 1. Waits for MCP rollout to start (5 min timeout) | ||
| 2. Waits for all MCPs to complete (30 min timeout) | ||
| 3. Waits for all cluster operators to settle (30 min timeout) | ||
| 4. Waits for all nodes to be ready (10 min timeout) | ||
|
|
||
| #### Example 1: After Changing TLS Profile | ||
|
|
||
| ```go | ||
| It("should apply Modern TLS profile", func() { | ||
| // Change TLS profile | ||
| err := oc.Run("patch").Args("apiserver", "cluster", "--type=merge", | ||
| "-p", `{"spec":{"tlsSecurityProfile":{"type":"Modern","modern":{}}}}`).Execute() | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| // Wait for cluster stability | ||
| err = tls.WaitForClusterStability(testclient.Client) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| // Now cluster is stable and ready for TLS compliance tests | ||
| // ... your test logic here ... | ||
| }) | ||
| ``` | ||
|
|
||
| #### Example 2: After Enabling TechPreviewNoUpgrade | ||
|
|
||
| ```go | ||
| It("should enable TechPreview features", func() { | ||
| // Enable TechPreviewNoUpgrade | ||
| err := oc.Run("patch").Args("tls", "cluster", "--type=merge", | ||
| "-p", `{"spec":{"featureSet":"TechPreviewNoUpgrade"}}`).Execute() | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| // Wait for cluster stability | ||
| err = tls.WaitForClusterStability(testclient.Client) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| // Cluster is now stable with TechPreview features enabled | ||
| }) | ||
| ``` | ||
|
|
||
| #### Example 3: In Your TLS Tests | ||
|
|
||
| ```go | ||
| Context("TLS Profile Compliance", func() { | ||
| BeforeEach(func() { | ||
| // Enable TLSAdherence feature gate | ||
| err := tls.EnableTLSAdherence(testclient.Client) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| }) | ||
|
|
||
| Context("Modern TLS Profile", func() { | ||
| BeforeEach(func() { | ||
| // Apply Modern TLS profile | ||
| err := oc.Run("patch").Args("apiserver", "cluster", "--type=merge", | ||
| "-p", `{"spec":{"tlsSecurityProfile":{"type":"Modern"}}}`).Execute() | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| // Wait for cluster stability after TLS profile change | ||
| err = tls.WaitForClusterStability(testclient.Client) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| }) | ||
|
|
||
| It("should verify ingress-node-firewall TLS compliance", func() { | ||
| // Test TLS compliance | ||
| }) | ||
| }) | ||
|
|
||
| Context("Custom TLS Profile", func() { | ||
| BeforeEach(func() { | ||
| // Apply Custom TLS profile | ||
| err := oc.Run("patch").Args("apiserver", "cluster", "--type=merge", | ||
| "-p", `{"spec":{"tlsSecurityProfile":{"type":"Custom","custom":{"minTLSVersion":"VersionTLS12"}}}}`).Execute() | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| // Wait for cluster stability after TLS profile change | ||
| err = tls.WaitForClusterStability(testclient.Client) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| }) | ||
|
|
||
| It("should verify ingress-node-firewall TLS compliance", func() { | ||
| // Test TLS compliance | ||
| }) | ||
| }) | ||
| }) | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ### 3. `IsTLSAdherenceEnabled()` - Quick Check | ||
|
|
||
| Check if TLSAdherence is enabled without waiting. | ||
|
|
||
| ```go | ||
| func IsTLSAdherenceEnabled(configClient configv1client.Interface) (bool, error) | ||
| ``` | ||
|
|
||
| **Usage:** | ||
| ```go | ||
| configClient, _ := configv1client.NewForConfig(client.Config) | ||
| enabled, err := tls.IsTLSAdherenceEnabled(configClient) | ||
| if err != nil { | ||
| log.Fatalf("Failed to check TLSAdherence: %v", err) | ||
| } | ||
|
|
||
| if enabled { | ||
| log.Println("TLSAdherence is enabled") | ||
| } else { | ||
| log.Println("TLSAdherence is NOT enabled") | ||
| } | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## Comparison: When to Use Which Function | ||
|
|
||
| | Function | Use Case | Enables Feature Gate? | Waits for Stability? | | ||
| |----------|----------|----------------------|---------------------| | ||
| | `EnableTLSAdherence()` | Enable TLSAdherence feature gate | ✅ Yes | ✅ Yes | | ||
| | `WaitForClusterStability()` | After ANY cluster config change | ❌ No | ✅ Yes | | ||
| | `IsTLSAdherenceEnabled()` | Quick check without waiting | ❌ No | ❌ No | | ||
|
|
||
| --- | ||
|
|
||
| ## Why `WaitForClusterStability()` is Reusable | ||
|
|
||
| ### Problem It Solves: | ||
|
|
||
| Many cluster configuration changes are **disruptive** and trigger: | ||
| 1. MCP rollouts (nodes restart) | ||
| 2. Operator reconciliation | ||
| 3. Pod restarts | ||
| 4. Configuration propagation | ||
|
|
||
| **Without waiting for stability**, tests will: | ||
| - ❌ Run against unstable cluster | ||
| - ❌ Get flaky results | ||
| - ❌ Experience false positives/negatives | ||
|
|
||
| ### Solution: | ||
|
|
||
| `WaitForClusterStability()` provides a **single, reusable** function that properly waits for: | ||
| - ✅ All MCPs to complete rollout | ||
| - ✅ All operators to be healthy | ||
| - ✅ All nodes to be ready | ||
|
|
||
| ### When to Use It: | ||
|
|
||
| **After ANY of these operations:** | ||
| - Changing feature gates | ||
| - Changing TLS profiles | ||
| - Modifying APIServer configuration | ||
| - Updating cluster-wide settings | ||
| - Any `oc patch` on cluster-scoped resources that triggers MCO | ||
|
|
||
| ### When NOT to Use It: | ||
|
|
||
| **Do NOT use after:** | ||
| - Creating pods/deployments (not cluster-wide) | ||
| - Namespaced resource changes | ||
| - Operations that don't affect nodes/operators | ||
|
|
||
| --- | ||
|
|
||
| ## Logging Output | ||
|
|
||
| ### `EnableTLSAdherence()` | ||
|
|
||
| ``` | ||
| === Starting TLSAdherence feature gate enablement === | ||
| Step 1: Patching FeatureGate to enable TLSAdherence | ||
| ✓ Feature gate patched successfully | ||
| === Waiting for cluster stability === | ||
| Step 1: Waiting for MCP rollout to start | ||
| ✓ MCP rollout started | ||
| Step 2: Waiting for MCP rollout to complete | ||
| ✓ MCP master: 3/3 machines updated | ||
| ✓ MCP worker: 2/2 machines updated | ||
| ✓ All MCPs updated successfully | ||
| Step 3: Waiting for all cluster operators to settle | ||
| ✓ All cluster operators settled | ||
| Step 4: Waiting for all nodes to be ready and stable | ||
| ✓ All nodes are ready and stable | ||
| === Cluster is stable === | ||
| Step 6: Verifying TLSAdherence is active in feature gate status | ||
| ✓ TLSAdherence is active in feature gate status | ||
| === TLSAdherence feature gate successfully enabled and cluster is stable === | ||
| ``` | ||
|
|
||
| ### `WaitForClusterStability()` | ||
|
|
||
| ``` | ||
| === Waiting for cluster stability === | ||
| Step 1: Waiting for MCP rollout to start | ||
| ✓ MCP rollout started | ||
| Step 2: Waiting for MCP rollout to complete | ||
| ✓ MCP master: 3/3 machines updated | ||
| ✓ MCP worker: 2/2 machines updated | ||
| ✓ All MCPs updated successfully | ||
| Step 3: Waiting for all cluster operators to settle | ||
| ✓ All cluster operators settled | ||
| Step 4: Waiting for all nodes to be ready and stable | ||
| ✓ All nodes are ready and stable | ||
| === Cluster is stable === | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## Timeouts | ||
|
|
||
| | Step | Timeout | Interval | | ||
| |------|---------|----------| | ||
| | MCP Rollout Start | 5 minutes | 10 seconds | | ||
| | MCP Rollout Complete | 30 minutes per MCP | 30 seconds | | ||
| | Operators Settle | 30 minutes | 10 seconds | | ||
| | Nodes Ready | 10 minutes | 30 seconds | | ||
| | TLSAdherence Active | 15 minutes | 15 seconds | | ||
|
|
||
| **Typical execution time:** 10-20 minutes | ||
| **Maximum time:** ~75 minutes (all timeouts exhausted) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
WaitForClusterStability() is documented but not implemented.
This file documents WaitForClusterStability(client *testclient.ClientSet) error as "the MOST REUSABLE function" with multiple usage examples (lines 34-268). The provided test/e2e/tls/tls.go does not define this function; only EnableTLSAdherence, EnableTLSAdherenceWithProfile, IsTLSAdherenceEnabled, and the unexported verifyClusterStability exist. Any code written against this guide fails to compile.
Either implement and export WaitForClusterStability() in tls.go, or remove/rewrite this section to reference the functions that actually exist.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 216-216: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 239-239: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/e2e/tls/USAGE.md` around lines 34 - 268, The documentation advertises
exported WaitForClusterStability but tls.go only provides unexported
verifyClusterStability, so documented usage cannot compile. Implement and export
WaitForClusterStability(client *testclient.ClientSet) error in tls.go by reusing
the existing cluster-stability logic, or revise the documentation to reference
the available API; keep all examples and comparison tables consistent with the
chosen API.
Implement intelligent container discovery in TLS compliance tests to handle variations in pod container structures across different OCP versions and configurations. Changes: - Add findContainerForPort() helper to discover containers by port - Enhance VerifyTLSComplianceInPod() with auto-discovery fallback - Validate container existence before using hardcoded names - Provide clear warnings when auto-discovery is triggered This fixes test failures when container names don't match expected values (e.g., CNO pod structure differences between environments). Benefits: - Resilient to container name changes across OCP versions - Self-healing when container names don't match expectations - Backward compatible with existing test code - Better diagnostic output for debugging Tested: - All Modern TLS Profile tests pass (5/5) - Auto-discovery verified with intentionally wrong container names - Works with multus, ovn-kubernetes, CNO, and network-console Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/e2e/tls/tls_compliance.go (1)
388-402: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winNil pointer dereference persists at
apiserver.Spec.TLSSecurityProfile.Type.The switch at Line 388-399 safely handles
apiserver.Spec.TLSSecurityProfile == nilthrough Go's left-to-right case short-circuit. Line 402 dereferencesapiserver.Spec.TLSSecurityProfile.Typeagain, outside that guard. WhenTLSSecurityProfileis nil, a normal state for a cluster with no explicit TLS profile, this line panics. This was flagged in a past review on an earlier commit of this file and remains unresolved.🐛 Proposed fix
+ var profileType configv1.TLSProfileType + if apiserver.Spec.TLSSecurityProfile != nil { + profileType = apiserver.Spec.TLSSecurityProfile.Type + } + log.Printf("Testing with profile: %s (TLS %s should work, TLS %s should fail)", - apiserver.Spec.TLSSecurityProfile.Type, tlsWorkVersion, tlsFailVersion) + profileType, tlsWorkVersion, tlsFailVersion)🤖 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/e2e/tls/tls_compliance.go` around lines 388 - 402, Update the logging expression after the TLS profile switch to avoid dereferencing apiserver.Spec.TLSSecurityProfile when it is nil. Use a nil-safe profile label while preserving the existing tlsWorkVersion and tlsFailVersion values selected by the switch.
🧹 Nitpick comments (1)
test/e2e/tls/tls_compliance.go (1)
417-462: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRedundant pod fetches during container auto-discovery.
The loop at Line 418-423 iterates
pods.Items, which already contains fullPodobjects from the earlierListcall, but discards everything except the name. Line 434 then issues anotherGetcall for the same pod, andfindContainerForPort(Line 434-461 paths) may issue yet anotherGetfor the same pod. Reuse thePodobject already retrieved byListinstead of re-fetching it.// Test the first running pod var testPod string var testPodObj *corev1.Pod for i := range pods.Items { if pods.Items[i].Status.Phase == "Running" { testPod = pods.Items[i].Name testPodObj = &pods.Items[i] break } }Then use
testPodObj.Spec.Containersdirectly instead of re-fetching at Line 434, and pass the container list to a variant offindContainerForPortthat accepts a*corev1.Podinstead of fetching it again.🤖 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/e2e/tls/tls_compliance.go` around lines 417 - 462, Reuse the running Pod object selected from pods.Items in the container-discovery flow instead of fetching it again. Track a *corev1.Pod alongside testPod, inspect testPodObj.Spec.Containers directly in the containerName validation branch, and update findContainerForPort or add a pod-based variant so discovery uses that object without issuing another Get call.
🤖 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/e2e/tls/tls_compliance.go`:
- Around line 350-376: Update findContainerForPort to create a context with the
established 30-second timeout before calling
k8sClient.CoreV1().Pods(namespace).Get, and ensure the context is canceled
appropriately. Pass this bounded context to the Kubernetes API call while
preserving the existing error handling and container-selection logic.
---
Duplicate comments:
In `@test/e2e/tls/tls_compliance.go`:
- Around line 388-402: Update the logging expression after the TLS profile
switch to avoid dereferencing apiserver.Spec.TLSSecurityProfile when it is nil.
Use a nil-safe profile label while preserving the existing tlsWorkVersion and
tlsFailVersion values selected by the switch.
---
Nitpick comments:
In `@test/e2e/tls/tls_compliance.go`:
- Around line 417-462: Reuse the running Pod object selected from pods.Items in
the container-discovery flow instead of fetching it again. Track a *corev1.Pod
alongside testPod, inspect testPodObj.Spec.Containers directly in the
containerName validation branch, and update findContainerForPort or add a
pod-based variant so discovery uses that object without issuing another Get
call.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5f40c272-7531-4a64-bfb4-8a7f0615f887
📒 Files selected for processing (3)
test/e2e/functional/tests/e2e.gotest/e2e/tls/tls.gotest/e2e/tls/tls_compliance.go
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/functional/tests/e2e.go
- test/e2e/tls/tls.go
| // findContainerForPort finds the container name that serves a specific port in a pod | ||
| func findContainerForPort(k8sClient kubernetes.Interface, namespace, podName, port string) (string, error) { | ||
| ctx := context.Background() | ||
| pod, err := k8sClient.CoreV1().Pods(namespace).Get(ctx, podName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to get pod %s/%s: %w", namespace, podName, err) | ||
| } | ||
|
|
||
| // Check each container for the specified port | ||
| for _, container := range pod.Spec.Containers { | ||
| for _, p := range container.Ports { | ||
| if fmt.Sprintf("%d", p.ContainerPort) == port { | ||
| log.Printf("Auto-discovered container '%s' serving port %s in pod %s/%s", container.Name, port, namespace, podName) | ||
| return container.Name, nil | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // If no container explicitly declares the port, try the first container | ||
| // (some containers serve ports without declaring them) | ||
| if len(pod.Spec.Containers) > 0 { | ||
| log.Printf("Warning: No container explicitly declares port %s, using first container '%s'", port, pod.Spec.Containers[0].Name) | ||
| return pod.Spec.Containers[0].Name, nil | ||
| } | ||
|
|
||
| return "", fmt.Errorf("no container found serving port %s in pod %s/%s", port, namespace, podName) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a timeout to findContainerForPort's Kubernetes API call.
findContainerForPort creates ctx := context.Background() with no deadline before calling k8sClient.CoreV1().Pods(namespace).Get. If the API server stalls, this call blocks indefinitely and the test hangs. Other parts of this file already use a 30-second timeout context for similar operations (ForwardPortAndExecute, ForwardPortToResourceAndExecute, per past review).
🔧 Proposed fix
func findContainerForPort(k8sClient kubernetes.Interface, namespace, podName, port string) (string, error) {
- ctx := context.Background()
+ ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+ defer cancel()
pod, err := k8sClient.CoreV1().Pods(namespace).Get(ctx, podName, metav1.GetOptions{})As per path instructions, "**/*.go... context.Context for cancellation and timeouts."
📝 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.
| // findContainerForPort finds the container name that serves a specific port in a pod | |
| func findContainerForPort(k8sClient kubernetes.Interface, namespace, podName, port string) (string, error) { | |
| ctx := context.Background() | |
| pod, err := k8sClient.CoreV1().Pods(namespace).Get(ctx, podName, metav1.GetOptions{}) | |
| if err != nil { | |
| return "", fmt.Errorf("failed to get pod %s/%s: %w", namespace, podName, err) | |
| } | |
| // Check each container for the specified port | |
| for _, container := range pod.Spec.Containers { | |
| for _, p := range container.Ports { | |
| if fmt.Sprintf("%d", p.ContainerPort) == port { | |
| log.Printf("Auto-discovered container '%s' serving port %s in pod %s/%s", container.Name, port, namespace, podName) | |
| return container.Name, nil | |
| } | |
| } | |
| } | |
| // If no container explicitly declares the port, try the first container | |
| // (some containers serve ports without declaring them) | |
| if len(pod.Spec.Containers) > 0 { | |
| log.Printf("Warning: No container explicitly declares port %s, using first container '%s'", port, pod.Spec.Containers[0].Name) | |
| return pod.Spec.Containers[0].Name, nil | |
| } | |
| return "", fmt.Errorf("no container found serving port %s in pod %s/%s", port, namespace, podName) | |
| } | |
| // findContainerForPort finds the container name that serves a specific port in a pod | |
| func findContainerForPort(k8sClient kubernetes.Interface, namespace, podName, port string) (string, error) { | |
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | |
| defer cancel() | |
| pod, err := k8sClient.CoreV1().Pods(namespace).Get(ctx, podName, metav1.GetOptions{}) | |
| if err != nil { | |
| return "", fmt.Errorf("failed to get pod %s/%s: %w", namespace, podName, err) | |
| } | |
| // Check each container for the specified port | |
| for _, container := range pod.Spec.Containers { | |
| for _, p := range container.Ports { | |
| if fmt.Sprintf("%d", p.ContainerPort) == port { | |
| log.Printf("Auto-discovered container '%s' serving port %s in pod %s/%s", container.Name, port, namespace, podName) | |
| return container.Name, nil | |
| } | |
| } | |
| } | |
| // If no container explicitly declares the port, try the first container | |
| // (some containers serve ports without declaring them) | |
| if len(pod.Spec.Containers) > 0 { | |
| log.Printf("Warning: No container explicitly declares port %s, using first container '%s'", port, pod.Spec.Containers[0].Name) | |
| return pod.Spec.Containers[0].Name, nil | |
| } | |
| return "", fmt.Errorf("no container found serving port %s in pod %s/%s", port, namespace, podName) | |
| } |
🤖 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/e2e/tls/tls_compliance.go` around lines 350 - 376, Update
findContainerForPort to create a context with the established 30-second timeout
before calling k8sClient.CoreV1().Pods(namespace).Get, and ensure the context is
canceled appropriately. Pass this bounded context to the Kubernetes API call
while preserving the existing error handling and container-selection logic.
Source: Path instructions
Implement comprehensive Custom TLS Profile testing for OpenShift networking components with configurable minTLSVersion and cipher suites. New Features: - ConfigureCustomTLSProfile() helper function - Sets Custom TLS profile with minTLSVersion and custom ciphers - Waits for MCP rollout and cluster stability - Verifies APIServer configuration Enhanced TLS Compliance Verification: - Smart detection of Custom TLS profile type - Dual-version testing for minTLSVersion=VersionTLS12 * Both TLS 1.2 and 1.3 should succeed * TLS 1.1 should fail - Profile-specific logging and diagnostics Test Implementation: - BeforeEach configures Custom TLS profile - Tests ingress-node-firewall-daemon metrics endpoint - Uses daemon container (has openssl) - Validates TLS 1.2, 1.3, and 1.1 behavior Custom Profile Configuration: - minTLSVersion: VersionTLS12 - Ciphers: TLS 1.3 + TLS 1.2 secure ciphers * TLS_AES_128_GCM_SHA256 * TLS_AES_256_GCM_SHA384 * TLS_CHACHA20_POLY1305_SHA256 * ECDHE-RSA-AES128-GCM-SHA256 * ECDHE-ECDSA-AES128-GCM-SHA256 - tlsAdherence: StrictAllComponents Workflow: 1. Configure Custom TLS profile 2. Wait for MCP rollout (if needed) 3. Wait for cluster operators to settle 4. Wait for nodes to be ready 5. Verify APIServer configuration 6. Test TLS compliance (1.2, 1.3, 1.1) Benefits: - Validates compatibility mode (TLS 1.2 + 1.3) - Tests custom cipher suite enforcement - Ensures adherence policy compliance - Provides detailed diagnostics TODO: Add tests for multus, ovn-kubernetes, CNO, network-console Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/e2e/tls/tls_compliance.go (1)
398-418: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReturn an error for an unrecognized
MinTLSVersion.The
defaultbranch at Line 411 and theCustom == nilbranch at Line 415 both fall back totls1_2/tls1_1. If a test configuresVersionTLS10or misspells the version, the helper silently tests the wrong protocol pair and reports success. Return an explicit error for these two cases so the test fails with a clear cause.🤖 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/e2e/tls/tls_compliance.go` around lines 398 - 418, The Custom TLS profile handling silently accepts unsupported or missing MinTLSVersion values. Update the minVersion switch and the Custom == nil branch to return an explicit error for unrecognized or absent configuration, while preserving the existing TLS 1.2 and TLS 1.3 behavior.test/e2e/functional/tests/e2e.go (1)
1288-1294: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRemove the TLS 1.3 names from the custom cipher list. OpenShift documents these suites as non-configurable and always enabled when TLS 1.3 is negotiated. The test checks protocol versions, not cipher selection, so these entries do not affect its assertions.
🤖 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/e2e/functional/tests/e2e.go` around lines 1288 - 1294, Update the custom ciphers list in the e2e TLS configuration to remove the TLS 1.3 suites TLS_AES_128_GCM_SHA256, TLS_AES_256_GCM_SHA384, and TLS_CHACHA20_POLY1305_SHA256, while retaining the configurable TLS 1.2 cipher entries used by the test.
🤖 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/e2e/functional/tests/e2e.go`:
- Around line 1277-1299: Update the Custom TLS setup around
ConfigureCustomTLSProfile to run once per Ginkgo context by guarding it with a
context-local sync.Once and invoking the configuration only from that
once-protected path. Convert the four related TODO It specs to PIt while they
remain unimplemented, preserving the active test coverage and existing TLS
configuration values.
In `@test/e2e/tls/tls.go`:
- Around line 335-341: Update verifyAPIServerTLSConfiguration to read back
Spec.TLSSecurityProfile.Custom and compare both MinTLSVersion and Ciphers
against the requested values before reporting success. Add a context.WithTimeout
around the context initialized near the function’s API calls, ensure it is
canceled, and use the derived context for every API request. Keep the existing
adherence-policy and profile-type verification intact.
---
Nitpick comments:
In `@test/e2e/functional/tests/e2e.go`:
- Around line 1288-1294: Update the custom ciphers list in the e2e TLS
configuration to remove the TLS 1.3 suites TLS_AES_128_GCM_SHA256,
TLS_AES_256_GCM_SHA384, and TLS_CHACHA20_POLY1305_SHA256, while retaining the
configurable TLS 1.2 cipher entries used by the test.
In `@test/e2e/tls/tls_compliance.go`:
- Around line 398-418: The Custom TLS profile handling silently accepts
unsupported or missing MinTLSVersion values. Update the minVersion switch and
the Custom == nil branch to return an explicit error for unrecognized or absent
configuration, while preserving the existing TLS 1.2 and TLS 1.3 behavior.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f9afb250-c966-416a-8373-6fdb75e90ed8
📒 Files selected for processing (3)
test/e2e/functional/tests/e2e.gotest/e2e/tls/tls.gotest/e2e/tls/tls_compliance.go
| BeforeEach(func() { | ||
| // Configure Custom TLS profile with minTLSVersion=VersionTLS12 | ||
| // and custom cipher before running Custom profile tests | ||
| // This will: | ||
| // - Set Custom TLS profile with minTLSVersion=VersionTLS12 | ||
| // - Set custom ciphers (e.g., TLS_AES_128_GCM_SHA256) | ||
| // - Set tlsAdherence=StrictAllComponents | ||
| // - Wait for MCP rollout (if needed) | ||
| // - Wait for all cluster operators to settle | ||
| // - Wait for all nodes to be ready | ||
| minTLSVersion := "VersionTLS12" | ||
| ciphers := []string{ | ||
| "TLS_AES_128_GCM_SHA256", | ||
| "TLS_AES_256_GCM_SHA384", | ||
| "TLS_CHACHA20_POLY1305_SHA256", | ||
| "ECDHE-RSA-AES128-GCM-SHA256", | ||
| "ECDHE-ECDSA-AES128-GCM-SHA256", | ||
| } | ||
| tlsAdherencePolicy := "StrictAllComponents" | ||
|
|
||
| err := tls.ConfigureCustomTLSProfile(testclient.Client, minTLSVersion, ciphers, tlsAdherencePolicy) | ||
| Expect(err).NotTo(HaveOccurred(), "Failed to configure Custom TLS profile") | ||
| }) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Determine the Ginkgo major version used by the repository.
rg -n 'onsi/ginkgo' go.mod
rg -n --type=go 'onsi/ginkgo' test/e2e/functional/tests/e2e.goRepository: openshift/ingress-node-firewall
Length of output: 256
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- e2e context ---'
sed -n '1190,1335p' test/e2e/functional/tests/e2e.go
printf '%s\n' '--- TLS helper references ---'
rg -n --type=go 'ConfigureCustomTLSProfile|30(\s|\.)*min|10(\s|\.)*min|MCP rollout|cluster operators|nodes' .
printf '%s\n' '--- test declarations and TODOs in the relevant area ---'
sed -n '1240,1340p' test/e2e/functional/tests/e2e.go | rg -n 'Describe|Context|BeforeEach|BeforeAll|Ordered|It\(|PIt\(|TODO'Repository: openshift/ingress-node-firewall
Length of output: 50388
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- remaining Custom TLS Profile tests ---'
sed -n '1315,1375p' test/e2e/functional/tests/e2e.go
printf '%s\n' '--- ConfigureCustomTLSProfile implementation ---'
sed -n '240,345p' test/e2e/tls/tls.go
printf '%s\n' '--- timeout helper implementations ---'
sed -n '450,570p' test/e2e/tls/tls.go
printf '%s\n' '--- Ginkgo lifecycle and setup symbols ---'
rg -n --type=go 'BeforeEach|BeforeSuite|AfterEach|AfterSuite|ConfigureCustomTLSProfile|Custom TLS Profile|sync\.Once|PIt\(' test/e2e/functional/tests test/e2e/tlsRepository: openshift/ingress-node-firewall
Length of output: 12096
Run Custom TLS setup once per context.
ConfigureCustomTLSProfile runs in BeforeEach for all five It specs. Each call can wait for MCP rollout, operators, and nodes, adding several hours to CI. Use a context-local sync.Once with Ginkgo v1, and mark the four TODO specs as PIt until implemented.
🤖 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/e2e/functional/tests/e2e.go` around lines 1277 - 1299, Update the Custom
TLS setup around ConfigureCustomTLSProfile to run once per Ginkgo context by
guarding it with a context-local sync.Once and invoking the configuration only
from that once-protected path. Convert the four related TODO It specs to PIt
while they remain unimplemented, preserving the active test coverage and
existing TLS configuration values.
| // Step 7: Verify APIServer TLS configuration | ||
| log.Println("Step 7: Verifying APIServer Custom TLS profile configuration") | ||
| err = verifyAPIServerTLSConfiguration(ctx, configClient, "Custom", tlsAdherencePolicy) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to verify APIServer TLS configuration: %w", err) | ||
| } | ||
| log.Printf("✓ APIServer Custom TLS profile (minTLSVersion=%s) and tlsAdherence=%s verified", minTLSVersion, tlsAdherencePolicy) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The verification does not cover minTLSVersion or the ciphers.
verifyAPIServerTLSConfiguration receives only the profile type "Custom" and the adherence policy. The log at Line 341 states that minTLSVersion is verified. A cluster that applies a different MinTLSVersion or a different cipher list still passes this step. Read back Spec.TLSSecurityProfile.Custom and compare MinTLSVersion and Ciphers with the requested values.
Also, ctx at Line 259 has no deadline. Every API call in this function can block without bound. Use context.WithTimeout.
As per path instructions, "**/*.go... context.Context for cancellation and timeouts."
🤖 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/e2e/tls/tls.go` around lines 335 - 341, Update
verifyAPIServerTLSConfiguration to read back Spec.TLSSecurityProfile.Custom and
compare both MinTLSVersion and Ciphers against the requested values before
reporting success. Add a context.WithTimeout around the context initialized near
the function’s API calls, ensure it is canceled, and use the derived context for
every API request. Keep the existing adherence-policy and profile-type
verification intact.
Source: Path instructions
Restructure test contexts to avoid double TLS profile rollouts that cause timeouts, and enhance Custom profile support for port-forward based testing. Test Structure Fixes: - Move Modern profile setup into Modern context BeforeEach - Keep Custom profile setup in Custom context BeforeEach - Avoids Modern→Custom double rollout (20+ minutes each) - Each profile context now independently configures its profile ConfigureCustomTLSProfile Enhancements: - Add TLSAdherence feature gate enablement - Check if feature gate already enabled - Add TLSAdherence verification step - Update step numbering (1-9) - Matches EnableTLSAdherence workflow GetExpectedTLSConfigs Enhancements: - Add full Custom TLS profile support - Handle minTLSVersion=VersionTLS12 (both 1.2 and 1.3 work) - Handle minTLSVersion=VersionTLS13 (only 1.3 works) - Provide detailed description for Custom profiles - Distinguish from Old profile handling Test Implementation: - Use VerifyTLSComplianceForPods for ingress-node-firewall - Port-forward approach (pods don't have openssl) - Works with Custom profile detection - Tests TLS 1.2, 1.3, and 1.1 correctly Performance Impact: - Before: Modern setup (15min) + Custom setup (15min) = 30min - After: Only Custom setup (15min) = 15min - Test now fits within 20-minute timeout Benefits: - Tests complete within timeout window - No redundant cluster rollouts - Better test isolation - Clearer test organization - Full Custom profile support Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
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/e2e/tls/tls_compliance.go`:
- Around line 114-172: Update the custom-profile switch in the TLS compliance
helper to explicitly handle VersionTLS10, VersionTLS11, VersionTLS12, and
VersionTLS13, with VersionTLS11 allowing TLS 1.1–1.3 and rejecting TLS 1.0;
define an appropriate positive-only expectation for VersionTLS10 because no
lower protocol exists. Apply the identical mapping in VerifyTLSComplianceInPod,
and consolidate the repeated TLS 1.2+ setup into shared logic while preserving
descriptions and compliance outcomes.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 95b328d4-5ae3-4139-94b4-1d38b2d57a1d
📒 Files selected for processing (3)
test/e2e/functional/tests/e2e.gotest/e2e/tls/tls.gotest/e2e/tls/tls_compliance.go
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/functional/tests/e2e.go
- test/e2e/tls/tls.go
| case profile.Type == configv1.TLSProfileCustomType: | ||
| // Custom: Check minTLSVersion from profile | ||
| if profile.Custom != nil { | ||
| minVersion := string(profile.Custom.MinTLSVersion) | ||
| switch minVersion { | ||
| case "VersionTLS12": | ||
| // TLS 1.2+: Both 1.2 and 1.3 should work, TLS 1.1 should fail | ||
| shouldWork = &tls.Config{ | ||
| MinVersion: tls.VersionTLS12, | ||
| MaxVersion: tls.VersionTLS13, | ||
| InsecureSkipVerify: true, | ||
| } | ||
| shouldNotWork = &tls.Config{ | ||
| MinVersion: tls.VersionTLS11, | ||
| MaxVersion: tls.VersionTLS11, | ||
| InsecureSkipVerify: true, | ||
| } | ||
| description = "Custom profile (minTLSVersion=VersionTLS12): TLS 1.2+ should work, TLS 1.1 should fail" | ||
| case "VersionTLS13": | ||
| // TLS 1.3 only: TLS 1.3 should work, TLS 1.2 should fail | ||
| shouldWork = &tls.Config{ | ||
| MinVersion: tls.VersionTLS13, | ||
| MaxVersion: tls.VersionTLS13, | ||
| InsecureSkipVerify: true, | ||
| } | ||
| shouldNotWork = &tls.Config{ | ||
| MinVersion: tls.VersionTLS12, | ||
| MaxVersion: tls.VersionTLS12, | ||
| InsecureSkipVerify: true, | ||
| } | ||
| description = "Custom profile (minTLSVersion=VersionTLS13): TLS 1.3 only, TLS 1.2 should fail" | ||
| default: | ||
| // Default to TLS 1.2+ | ||
| shouldWork = &tls.Config{ | ||
| MinVersion: tls.VersionTLS12, | ||
| MaxVersion: tls.VersionTLS13, | ||
| InsecureSkipVerify: true, | ||
| } | ||
| shouldNotWork = &tls.Config{ | ||
| MinVersion: tls.VersionTLS11, | ||
| MaxVersion: tls.VersionTLS11, | ||
| InsecureSkipVerify: true, | ||
| } | ||
| description = "Custom profile: TLS 1.2+ should work, TLS 1.1 should fail" | ||
| } | ||
| } else { | ||
| // No custom config, default to TLS 1.2+ | ||
| shouldWork = &tls.Config{ | ||
| MinVersion: tls.VersionTLS12, | ||
| MaxVersion: tls.VersionTLS13, | ||
| InsecureSkipVerify: true, | ||
| } | ||
| shouldNotWork = &tls.Config{ | ||
| MinVersion: tls.VersionTLS11, | ||
| MaxVersion: tls.VersionTLS11, | ||
| InsecureSkipVerify: true, | ||
| } | ||
| description = "Custom profile: TLS 1.2+ should work, TLS 1.1 should fail" | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate file ---'
sed -n '80,185p' test/e2e/tls/tls_compliance.go
printf '%s\n' '--- TLS version declarations and uses ---'
rg -n --glob '*.{go,yaml,yml,json}' 'VersionTLS10|VersionTLS11|VersionTLS12|VersionTLS13|minTLSVersion|TLSProfileCustomType' .
printf '%s\n' '--- module metadata ---'
find . -maxdepth 3 -type f \( -name 'go.mod' -o -name 'go.sum' \) -printRepository: openshift/ingress-node-firewall
Length of output: 19118
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- custom-profile compliance path ---'
sed -n '400,610p' test/e2e/tls/tls_compliance.go
printf '%s\n' '--- TLS connection helpers ---'
rg -n 'func .*TLS|shouldWork|shouldNotWork|GetExpectedTLSConfigs|testTLS|tls.Dial|DialWith' test/e2e/tls/tls_compliance.go test/e2e/tls
printf '%s\n' '--- API enum and profile semantics ---'
sed -n '220,270p' vendor/github.com/openshift/api/config/v1/types_tlssecurityprofile.go
sed -n '300,360p' vendor/github.com/openshift/api/config/v1/types_tlssecurityprofile.goRepository: openshift/ingress-node-firewall
Length of output: 20162
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- direct TLS checks and callers ---'
sed -n '230,380p' test/e2e/tls/tls_compliance.go
printf '%s\n' '--- tests for GetExpectedTLSConfigs ---'
rg -n -C 8 'GetExpectedTLSConfigs|CheckTLSConnection|DirectTLSTest|VersionTLS11|VersionTLS10' test/e2e/tls --glob '*_test.go' --glob '*.go'
printf '%s\n' '--- Go tool availability ---'
command -v go || true
go version 2>/dev/null || trueRepository: openshift/ingress-node-firewall
Length of output: 18098
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
p = Path("test/e2e/tls/tls_compliance.go").read_text()
start = p.index("case profile.Type == configv1.TLSProfileCustomType:")
end = p.index("\n\tdefault:", start)
custom = p[start:end]
for version in ("VersionTLS10", "VersionTLS11", "VersionTLS12", "VersionTLS13"):
m = re.search(rf'case "{version}":(.*?)(?=\n\t\t\tcase |\n\t\t\tdefault:|\n\t\t}})', custom, re.S)
if m:
body = m.group(1)
mins = re.findall(r'(?:MinVersion|MaxVersion):\s+tls\.(VersionTLS\d+)', body)
print(version, mins)
else:
print(version, "no explicit case; default mapping")
print("API enum:")
api = Path("vendor/github.com/openshift/api/config/v1/types_tlssecurityprofile.go").read_text()
enum = re.search(r'type TLSProtocolVersion string(.*?)(?:// TLSProfiles)', api, re.S).group(1)
print(" ".join(re.findall(r'VersionTLS1[013]', enum)))
PY
cat >/tmp/tls_version_probe.go <<'EOF'
package main
import (
"crypto/tls"
"fmt"
)
func main() {
fmt.Printf("TLS constants: 1.0=%#04x 1.1=%#04x 1.2=%#04x 1.3=%#04x\n",
tls.VersionTLS10, tls.VersionTLS11, tls.VersionTLS12, tls.VersionTLS13)
for _, v := range []uint16{tls.VersionTLS10, tls.VersionTLS11, tls.VersionTLS12, tls.VersionTLS13} {
c := &tls.Config{MinVersion: v, MaxVersion: tls.VersionTLS13}
fmt.Printf("min=%#04x max=%#04x\n", c.MinVersion, c.MaxVersion)
}
fmt.Println("No TLS protocol constant is lower than VersionTLS10.")
}
EOF
go run /tmp/tls_version_probe.go
rm -f /tmp/tls_version_probe.goRepository: openshift/ingress-node-firewall
Length of output: 707
Handle every valid custom TLS minimum version.
VersionTLS11 currently falls into the VersionTLS12 default and causes a false compliance failure. Add an explicit TLS 1.1 expectation: TLS 1.1–1.3 should work, and TLS 1.0 should fail. Handle valid VersionTLS10 separately because no lower TLS version exists for the negative check. Apply the same mapping to VerifyTLSComplianceInPod and consolidate the duplicate TLS 1.2+ blocks.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 120-124: TLS certificate verification is disabled by setting InsecureSkipVerify: true on the tls.Config. This makes the connection vulnerable to man-in-the-middle attacks because the server's certificate chain and host name are not validated. Remove InsecureSkipVerify (or set it to false) and provide a proper RootCAs pool to trust custom certificates instead.
Context: tls.Config{
MinVersion: tls.VersionTLS12,
MaxVersion: tls.VersionTLS13,
InsecureSkipVerify: true,
}
Note: [CWE-295] Improper Certificate Validation.
(tls-insecure-skip-verify-go)
[warning] 125-129: TLS certificate verification is disabled by setting InsecureSkipVerify: true on the tls.Config. This makes the connection vulnerable to man-in-the-middle attacks because the server's certificate chain and host name are not validated. Remove InsecureSkipVerify (or set it to false) and provide a proper RootCAs pool to trust custom certificates instead.
Context: tls.Config{
MinVersion: tls.VersionTLS11,
MaxVersion: tls.VersionTLS11,
InsecureSkipVerify: true,
}
Note: [CWE-295] Improper Certificate Validation.
(tls-insecure-skip-verify-go)
[warning] 133-137: TLS certificate verification is disabled by setting InsecureSkipVerify: true on the tls.Config. This makes the connection vulnerable to man-in-the-middle attacks because the server's certificate chain and host name are not validated. Remove InsecureSkipVerify (or set it to false) and provide a proper RootCAs pool to trust custom certificates instead.
Context: tls.Config{
MinVersion: tls.VersionTLS13,
MaxVersion: tls.VersionTLS13,
InsecureSkipVerify: true,
}
Note: [CWE-295] Improper Certificate Validation.
(tls-insecure-skip-verify-go)
[warning] 138-142: TLS certificate verification is disabled by setting InsecureSkipVerify: true on the tls.Config. This makes the connection vulnerable to man-in-the-middle attacks because the server's certificate chain and host name are not validated. Remove InsecureSkipVerify (or set it to false) and provide a proper RootCAs pool to trust custom certificates instead.
Context: tls.Config{
MinVersion: tls.VersionTLS12,
MaxVersion: tls.VersionTLS12,
InsecureSkipVerify: true,
}
Note: [CWE-295] Improper Certificate Validation.
(tls-insecure-skip-verify-go)
[warning] 146-150: TLS certificate verification is disabled by setting InsecureSkipVerify: true on the tls.Config. This makes the connection vulnerable to man-in-the-middle attacks because the server's certificate chain and host name are not validated. Remove InsecureSkipVerify (or set it to false) and provide a proper RootCAs pool to trust custom certificates instead.
Context: tls.Config{
MinVersion: tls.VersionTLS12,
MaxVersion: tls.VersionTLS13,
InsecureSkipVerify: true,
}
Note: [CWE-295] Improper Certificate Validation.
(tls-insecure-skip-verify-go)
[warning] 151-155: TLS certificate verification is disabled by setting InsecureSkipVerify: true on the tls.Config. This makes the connection vulnerable to man-in-the-middle attacks because the server's certificate chain and host name are not validated. Remove InsecureSkipVerify (or set it to false) and provide a proper RootCAs pool to trust custom certificates instead.
Context: tls.Config{
MinVersion: tls.VersionTLS11,
MaxVersion: tls.VersionTLS11,
InsecureSkipVerify: true,
}
Note: [CWE-295] Improper Certificate Validation.
(tls-insecure-skip-verify-go)
[warning] 160-164: TLS certificate verification is disabled by setting InsecureSkipVerify: true on the tls.Config. This makes the connection vulnerable to man-in-the-middle attacks because the server's certificate chain and host name are not validated. Remove InsecureSkipVerify (or set it to false) and provide a proper RootCAs pool to trust custom certificates instead.
Context: tls.Config{
MinVersion: tls.VersionTLS12,
MaxVersion: tls.VersionTLS13,
InsecureSkipVerify: true,
}
Note: [CWE-295] Improper Certificate Validation.
(tls-insecure-skip-verify-go)
[warning] 165-169: TLS certificate verification is disabled by setting InsecureSkipVerify: true on the tls.Config. This makes the connection vulnerable to man-in-the-middle attacks because the server's certificate chain and host name are not validated. Remove InsecureSkipVerify (or set it to false) and provide a proper RootCAs pool to trust custom certificates instead.
Context: tls.Config{
MinVersion: tls.VersionTLS11,
MaxVersion: tls.VersionTLS11,
InsecureSkipVerify: true,
}
Note: [CWE-295] Improper Certificate Validation.
(tls-insecure-skip-verify-go)
[warning] 125-129: The 'tls.Config' MinVersion is set below TLS 1.2 (TLS 1.0, TLS 1.1, or SSL 3.0). These protocols have known vulnerabilities (e.g. POODLE, BEAST) and are deprecated. Set 'MinVersion: tls.VersionTLS12' or, preferably, 'tls.VersionTLS13'.
Context: tls.Config{
MinVersion: tls.VersionTLS11,
MaxVersion: tls.VersionTLS11,
InsecureSkipVerify: true,
}
Note: [CWE-326] Inadequate Encryption Strength.
(tls-min-version-below-12-go)
[warning] 151-155: The 'tls.Config' MinVersion is set below TLS 1.2 (TLS 1.0, TLS 1.1, or SSL 3.0). These protocols have known vulnerabilities (e.g. POODLE, BEAST) and are deprecated. Set 'MinVersion: tls.VersionTLS12' or, preferably, 'tls.VersionTLS13'.
Context: tls.Config{
MinVersion: tls.VersionTLS11,
MaxVersion: tls.VersionTLS11,
InsecureSkipVerify: true,
}
Note: [CWE-326] Inadequate Encryption Strength.
(tls-min-version-below-12-go)
[warning] 165-169: The 'tls.Config' MinVersion is set below TLS 1.2 (TLS 1.0, TLS 1.1, or SSL 3.0). These protocols have known vulnerabilities (e.g. POODLE, BEAST) and are deprecated. Set 'MinVersion: tls.VersionTLS12' or, preferably, 'tls.VersionTLS13'.
Context: tls.Config{
MinVersion: tls.VersionTLS11,
MaxVersion: tls.VersionTLS11,
InsecureSkipVerify: true,
}
Note: [CWE-326] Inadequate Encryption Strength.
(tls-min-version-below-12-go)
🪛 OpenGrep (1.26.0)
[ERROR] 121-125: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
[ERROR] 121-125: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
[ERROR] 126-130: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
[ERROR] 126-130: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
[ERROR] 134-138: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
[ERROR] 134-138: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
[ERROR] 139-143: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
[ERROR] 139-143: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
[ERROR] 147-151: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
[ERROR] 147-151: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
[ERROR] 152-156: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
[ERROR] 152-156: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
[ERROR] 161-165: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
[ERROR] 161-165: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
[ERROR] 166-170: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
[ERROR] 166-170: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
🤖 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/e2e/tls/tls_compliance.go` around lines 114 - 172, Update the
custom-profile switch in the TLS compliance helper to explicitly handle
VersionTLS10, VersionTLS11, VersionTLS12, and VersionTLS13, with VersionTLS11
allowing TLS 1.1–1.3 and rejecting TLS 1.0; define an appropriate positive-only
expectation for VersionTLS10 because no lower protocol exists. Apply the
identical mapping in VerifyTLSComplianceInPod, and consolidate the repeated TLS
1.2+ setup into shared logic while preserving descriptions and compliance
outcomes.
Implement comprehensive TLS compliance testing for Modern TLS profile with LegacyAdheringComponentsOnly adherence policy. Changes: 1. New test structure with parent and child BeforeEach separation: - Parent BeforeEach: Enable TLSAdherence feature gate (runs once) - Child BeforeEach: Configure Modern TLS profile (per context) 2. New helper functions in test/e2e/tls/tls.go: - EnableTLSAdherenceFeatureGateOnly(): Enable feature gate only - ConfigureModernTLSProfileWithAdherence(): Configure TLS profile 3. Enhanced TLS compliance logic in test/e2e/tls/tls_compliance.go: - GetExpectedTLSConfigs() now accepts tlsAdherence parameter - Supports LegacyAdheringComponentsOnly policy detection - Dynamic TLS version description in log messages 4. Test context organization (test/e2e/functional/tests/e2e.go): - Modern TLS Profile with LegacyAdheringComponentsOnly (ACTIVE) - Modern TLS Profile with StrictAllComponents (SKIPPED) - Custom TLS Profile (SKIPPED) Test behavior for LegacyAdheringComponentsOnly: - TLS 1.3 connections: SHOULD WORK ✓ - TLS 1.2 connections: SHOULD WORK ✓ (legacy allowed) - TLS 1.1 connections: SHOULD FAIL ✓ Test execution: - Duration: ~2.7 seconds - Status: PASSED - Component: ingress-node-firewall-daemon:9301 Documentation: - MODERN_LEGACY_ADHERING_TEST_IMPLEMENTATION.md: Full implementation details - CORRECTED_TEST_STRUCTURE.md: Corrected test structure per requirements - TLS_TEST_STRUCTURE_FINAL.md: Final test organization and execution guide Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add Skip statements to non-TLS test contexts to ensure only TLS Profile Compliance tests execute. Skipped contexts: - IngressNodeFirewall: Main firewall functionality tests - Statistics: Statistics collection tests - Webhook: Webhook validation tests Active context: - TLS Profile Compliance - Modern TLS Profile with LegacyAdheringComponentsOnly (ACTIVE) - Modern TLS Profile with StrictAllComponents (SKIPPED) - Custom TLS Profile (SKIPPED) Test execution: - Total specs: 37 - TLS specs: 15 (3 contexts × 5 tests each) - Active specs: 5 (LegacyAdheringComponentsOnly only) - Skipped: 32 (22 non-TLS + 10 other TLS contexts) This ensures clean test execution focusing only on the LegacyAdheringComponentsOnly compliance testing. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/tls/tls_compliance.go (1)
292-305: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound both TLS handshakes.
If the peer accepts TCP but stalls during the TLS handshake, both
tls.Dialcalls can block without a deadline. Usetls.DialWithDialerwith a finite timeout ortls.Dialer.DialContextwith a deadline.🤖 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/e2e/tls/tls_compliance.go` around lines 292 - 305, Update both TLS connection attempts in the compliance test to use a finite connection/handshake deadline via tls.DialWithDialer or tls.Dialer.DialContext. Apply the bounded dialing consistently to the tlsShouldWork and tlsShouldNotWork calls while preserving their existing error handling and TLS configurations.Source: Path instructions
🤖 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 `@CORRECTED_TEST_STRUCTURE.md`:
- Around line 15-47: Correct the Ginkgo lifecycle and spec-count documentation:
in CORRECTED_TEST_STRUCTURE.md lines 15-47 and 241-253, remove “runs once” or
hook-skipping claims and state that BeforeEach runs before every matching It,
with parent hooks preceding child hooks; in
MODERN_LEGACY_ADHERING_TEST_IMPLEMENTATION.md lines 254-289 and
TLS_TEST_STRUCTURE_FINAL.md lines 104-111 and 145-147, document the four TODO
bodies as ordinary runnable It specs, reporting five runnable and zero pending
specs, or explicitly convert them to PIt/XIt and report one runnable and four
pending specs.
In `@MODERN_LEGACY_ADHERING_TEST_IMPLEMENTATION.md`:
- Around line 19-61: Update MODERN_LEGACY_ADHERING_TEST_IMPLEMENTATION.md lines
19-61 to document the active two-step setup: call
EnableTLSAdherenceFeatureGateOnly in the parent setup and
ConfigureModernTLSProfileWithAdherence in the child setup, including their
current source locations. Update TLS_TEST_STRUCTURE_FINAL.md lines 36-39 to
replace EnableTLSAdherenceWithProfile with the same two-step setup and current
helper names.
In `@test/e2e/functional/tests/e2e.go`:
- Around line 1178-1199: Update VerifyTLSComplianceForPods and its
CheckTLSConnection flow to test TLS 1.2 and TLS 1.3 in separate connections
using configurations where MinVersion equals MaxVersion for each requested
version. Assert both exact-version handshakes succeed, while retaining the TLS
1.1 connection as an expected rejection.
- Around line 1202-1216: Mark all eight TODO TLS coverage specs as pending by
changing their It declarations to PIt. Change the StrictAllComponents and Custom
TLS Profile contexts to PContext, and remove their nested Skip calls so pending
contexts do not execute the TLSAdherence-enabling BeforeEach.
In `@test/e2e/tls/tls_compliance.go`:
- Around line 286-287: Update CheckTLSConnection to construct endpoint using the
provided host via net.JoinHostPort(host, strconv.Itoa(port)) instead of
hardcoding localhost, preserving correct handling for non-local and IPv6 hosts.
- Around line 105-115: The Modern profile test in the shouldWork setup must
validate TLS 1.2 and TLS 1.3 as separate handshakes rather than using a single
TLS 1.2–1.3 range. Update the relevant CheckTLSConnection calls and TLS
configurations so each protocol version is tested explicitly, while preserving
the TLS 1.1 failure assertion.
In `@test/e2e/tls/tls.go`:
- Around line 49-74: Create a bounded timeout context in the TLS setup flow
instead of using context.Background(), defer its cancellation, and pass that
context through the direct FeatureGate API requests and helper calls such as
patchFeatureGate and verifyTLSAdherenceActive. Ensure all related control-plane
Get, Update, and polling operations use the same cancellable context.
---
Outside diff comments:
In `@test/e2e/tls/tls_compliance.go`:
- Around line 292-305: Update both TLS connection attempts in the compliance
test to use a finite connection/handshake deadline via tls.DialWithDialer or
tls.Dialer.DialContext. Apply the bounded dialing consistently to the
tlsShouldWork and tlsShouldNotWork calls while preserving their existing error
handling and TLS configurations.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f88e9436-48d1-4f6d-9b5f-9a0681ec686d
📒 Files selected for processing (6)
CORRECTED_TEST_STRUCTURE.mdMODERN_LEGACY_ADHERING_TEST_IMPLEMENTATION.mdTLS_TEST_STRUCTURE_FINAL.mdtest/e2e/functional/tests/e2e.gotest/e2e/tls/tls.gotest/e2e/tls/tls_compliance.go
| ├── BeforeEach() ────────────────────────────────────────────────┐ | ||
| │ │ │ | ||
| │ │ PARENT BEFOEREACH - Runs ONCE for ALL TLS contexts │ | ||
| │ │ │ | ||
| │ ├─ Step 1: Patch FeatureGate to enable TLSAdherence │ | ||
| │ │ Function: EnableTLSAdherenceFeatureGateOnly() │ | ||
| │ │ │ | ||
| │ └─ Step 2: Verify TLSAdherence is active in status │ | ||
| │ (15 min timeout) │ | ||
| │ │ | ||
| ├── Context("Modern TLS Profile with LegacyAdheringComponentsOnly") ⭐ | ||
| │ │ | ||
| │ ├── BeforeEach() ────────────────────────────────────────────┐ | ||
| │ │ │ │ | ||
| │ │ │ CHILD BEFOREEACH - Runs for LegacyAdhering tests only │ | ||
| │ │ │ │ | ||
| │ │ ├─ Step 1: Configure APIServer with Modern TLS profile │ | ||
| │ │ │ and tlsAdherence=LegacyAdheringComponentsOnly │ | ||
| │ │ │ Function: ConfigureModernTLSProfileWithAdherence() | ||
| │ │ │ │ | ||
| │ │ ├─ Step 2: Wait for MCP rollout to start │ | ||
| │ │ │ (5 min timeout) │ | ||
| │ │ │ │ | ||
| │ │ ├─ Step 3: Wait for all MCPs to complete │ | ||
| │ │ │ (30 min timeout) │ | ||
| │ │ │ │ | ||
| │ │ ├─ Step 4: Wait for cluster operators to settle │ | ||
| │ │ │ (30 min timeout) │ | ||
| │ │ │ │ | ||
| │ │ ├─ Step 5: Wait for nodes to be ready │ | ||
| │ │ │ (10 min timeout) │ | ||
| │ │ │ │ | ||
| │ │ └─ Step 6: Verify APIServer TLS configuration │ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'github.com/onsi/ginkgo' go.mod
rg -n -C 4 '\bBeforeEach\s*\(' test/e2e/functional/tests/e2e.goRepository: openshift/ingress-node-firewall
Length of output: 2968
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in CORRECTED_TEST_STRUCTURE.md MODERN_LEGACY_ADHERING_TEST_IMPLEMENTATION.md TLS_TEST_STRUCTURE_FINAL.md; do
echo "===== $f ====="
wc -l "$f"
done
echo "===== lifecycle sections ====="
sed -n '15,55p;235,260p' CORRECTED_TEST_STRUCTURE.md
sed -n '250,295p' MODERN_LEGACY_ADHERING_TEST_IMPLEMENTATION.md
sed -n '98,115p;140,150p' TLS_TEST_STRUCTURE_FINAL.md
echo "===== TLS spec declarations and skips ====="
sed -n '1140,1415p' test/e2e/functional/tests/e2e.go | \
rg -n -C 3 'Context\(|It\(|BeforeEach\(|Skip\('Repository: openshift/ingress-node-firewall
Length of output: 13074
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== pending-spec conventions ====="
rg -n -C 2 '\b(PIt|Pending|XIt|XContext|TODO: Implementation)\b' --glob '*.go' --glob '*.md' .
echo "===== registered TLS specs ====="
python3 - <<'PY'
from pathlib import Path
import re
p = Path("test/e2e/functional/tests/e2e.go")
text = p.read_text()
start = text.index('Context("TLS Profile Compliance"')
end = text.index('\n\tContext("Statistics"', start)
section = text[start:end]
contexts = re.findall(r'Context\("([^"]+)"', section)
its = re.findall(r'\bIt\("([^"]+)"', section)
skips = re.findall(r'\bSkip\("([^"]+)"', section)
print("TLS contexts:", contexts)
print("It declarations:", len(its))
for title in its:
print(" -", title)
print("Skip calls:", len(skips))
for reason in skips:
print(" -", reason)
PYRepository: openshift/ingress-node-firewall
Length of output: 37124
🌐 Web query:
Ginkgo v1.16.5 documentation BeforeEach runs before each spec pending PIt
💡 Result:
In Ginkgo v1.16.5, a BeforeEach block runs before each associated It block [1][2]. Ginkgo is designed such that specs are independent, and BeforeEach blocks are used to extract common setup code that must run for every It block within the scope of the container (Describe/Context/When) where the BeforeEach is defined [3][4][5]. Regarding "pending" specs (specs marked with XIt or PIt), these are skipped by the Ginkgo runner [4][6]. Because they are not executed, any associated BeforeEach blocks in their hierarchy are not run for those specific pending specs [3][4]. Key points regarding BeforeEach behavior: - Execution Hierarchy: If multiple BeforeEach blocks are defined in nested Describe or Context blocks, the outermost blocks run first, followed by the innermost blocks, leading up to the execution of the It block [1][3][4]. - Parallelism and Independence: To ensure specs can be shuffled, randomized, and run in parallel, Ginkgo requires each It block to be independent [3][5]. BeforeEach is the intended mechanism to ensure each spec has the necessary pristine state [3][4][5]. - JustBeforeEach: For further control, Ginkgo provides JustBeforeEach, which runs after all BeforeEach blocks but before the It block, often used for final configuration after the primary setup is complete [1][2].
Citations:
- 1: https://github.com/onsi/ginkgo/blob/v1.16.5/ginkgo_dsl.go
- 2: https://github.com/onsi/ginkgo/tree/v1.16.5
- 3: https://github.com/onsi/ginkgo/blob/master/docs/index.md
- 4: https://onsi.github.io/ginkgo/
- 5: Writting dependent Specs? onsi/ginkgo#246
- 6: https://github.com/onsi/ginkgo/blob/master/README.md
Correct the Ginkgo lifecycle and spec-count descriptions.
BeforeEachruns before every matchingIt; parent hooks run before child hooks. Remove allRUNS ONCElabels and the claim that later specs skip these hooks.- The four TODO bodies are ordinary
Itspecs, not pending specs. Document five runnable specs and zero pending specs, or replace them withPIt/XItand report one runnable spec plus four pending specs.
📍 Affects 3 files
CORRECTED_TEST_STRUCTURE.md#L15-L47(this comment)CORRECTED_TEST_STRUCTURE.md#L241-L253MODERN_LEGACY_ADHERING_TEST_IMPLEMENTATION.md#L254-L289TLS_TEST_STRUCTURE_FINAL.md#L104-L111TLS_TEST_STRUCTURE_FINAL.md#L145-L147
🤖 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 `@CORRECTED_TEST_STRUCTURE.md` around lines 15 - 47, Correct the Ginkgo
lifecycle and spec-count documentation: in CORRECTED_TEST_STRUCTURE.md lines
15-47 and 241-253, remove “runs once” or hook-skipping claims and state that
BeforeEach runs before every matching It, with parent hooks preceding child
hooks; in MODERN_LEGACY_ADHERING_TEST_IMPLEMENTATION.md lines 254-289 and
TLS_TEST_STRUCTURE_FINAL.md lines 104-111 and 145-147, document the four TODO
bodies as ordinary runnable It specs, reporting five runnable and zero pending
specs, or explicitly convert them to PIt/XIt and report one runnable and four
pending specs.
| **Location:** `test/e2e/functional/tests/e2e.go:1278` | ||
|
|
||
| Added a new test context parallel to the existing Modern and Custom TLS profile tests: | ||
|
|
||
| ```go | ||
| Context("Modern TLS Profile with LegacyAdheringComponentsOnly", func() { | ||
| BeforeEach(func() { | ||
| // Configure Modern TLS profile with LegacyAdheringComponentsOnly | ||
| // Steps executed: | ||
| // - Step 1: Patch FeatureGate to enable TLSAdherence | ||
| // - Step 2: Verify TLSAdherence is active in status (15 min timeout) | ||
| // - Step 3: Configure APIServer with Modern TLS profile | ||
| // - Step 4: Wait for MCP rollout to start (5 min timeout) | ||
| // - Step 5: Wait for all MCPs to complete (30 min timeout) | ||
| // - Step 6: Wait for cluster operators to settle (30 min timeout) | ||
| // - Step 7: Wait for nodes to be ready (10 min timeout) | ||
| // - Step 8: Verify APIServer TLS configuration | ||
|
|
||
| err := tls.EnableTLSAdherenceWithProfile( | ||
| testclient.Client, | ||
| "Modern", | ||
| "LegacyAdheringComponentsOnly" | ||
| ) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| }) | ||
|
|
||
| It("should verify ingress-node-firewall TLS compliance", func() { | ||
| // Test implementation with detailed steps | ||
| // - Step 1: Port-forward to pod (oc port-forward) | ||
| // - Step 2: Test TLS 1.3 connection (SHOULD work) | ||
| // - Step 3: Test TLS 1.2 connection (SHOULD work - legacy allowed) | ||
| // - Step 4: Test TLS 1.1 connection (SHOULD fail) | ||
|
|
||
| namespace := OperatorNameSpace | ||
| labelSelector := "app=ingress-node-firewall-daemon" | ||
| port := "9301" | ||
|
|
||
| err = tls.VerifyTLSComplianceForPods(configClient, k8sClient, namespace, labelSelector, port) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| }) | ||
|
|
||
| // Additional component tests (TODO)... | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace the obsolete one-step setup example.
The active implementation uses EnableTLSAdherenceFeatureGateOnly in the parent setup and ConfigureModernTLSProfileWithAdherence in the child setup. These documents still instruct readers to use EnableTLSAdherenceWithProfile.
MODERN_LEGACY_ADHERING_TEST_IMPLEMENTATION.md#L19-L61: document the current parent and child helper calls and current source locations.TLS_TEST_STRUCTURE_FINAL.md#L36-L39: replace the obsolete helper call with the current two-step setup.
📍 Affects 2 files
MODERN_LEGACY_ADHERING_TEST_IMPLEMENTATION.md#L19-L61(this comment)TLS_TEST_STRUCTURE_FINAL.md#L36-L39
🤖 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 `@MODERN_LEGACY_ADHERING_TEST_IMPLEMENTATION.md` around lines 19 - 61, Update
MODERN_LEGACY_ADHERING_TEST_IMPLEMENTATION.md lines 19-61 to document the active
two-step setup: call EnableTLSAdherenceFeatureGateOnly in the parent setup and
ConfigureModernTLSProfileWithAdherence in the child setup, including their
current source locations. Update TLS_TEST_STRUCTURE_FINAL.md lines 36-39 to
replace EnableTLSAdherenceWithProfile with the same two-step setup and current
helper names.
| It("should verify ingress-node-firewall TLS compliance", func() { | ||
| // This test verifies that ingress-node-firewall daemon metrics endpoint | ||
| // complies with Modern TLS profile with LegacyAdheringComponentsOnly | ||
| // Expected behavior (for legacy-tolerant policy): | ||
| // - Step 1: Port-forward to pod (oc port-forward) | ||
| // - Step 2: Test TLS 1.3 connection (SHOULD work) | ||
| // - Step 3: Test TLS 1.2 connection (SHOULD work - legacy allowed) | ||
| // - Step 4: Test TLS 1.1 connection (SHOULD fail) | ||
|
|
||
| k8sClient, err := kubernetes.NewForConfig(testclient.Client.Config) | ||
| Expect(err).NotTo(HaveOccurred(), "Failed to create Kubernetes client") | ||
|
|
||
| configClient, err := configv1client.NewForConfig(testclient.Client.Config) | ||
| Expect(err).NotTo(HaveOccurred(), "Failed to create config client") | ||
|
|
||
| namespace := OperatorNameSpace | ||
| labelSelector := "app=ingress-node-firewall-daemon" | ||
| port := "9301" | ||
|
|
||
| By(fmt.Sprintf("Testing TLS compliance for ingress-node-firewall daemon in %s on port %s", namespace, port)) | ||
| err = tls.VerifyTLSComplianceForPods(configClient, k8sClient, namespace, labelSelector, port) | ||
| Expect(err).NotTo(HaveOccurred(), "TLS compliance verification failed") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline test/e2e/tls/tls_compliance.go --items all --type function
rg -n -C 8 'GetExpectedTLSConfigs|CheckTLSConnection|VersionTLS11|VersionTLS12|VersionTLS13' test/e2e/tls/tls_compliance.goRepository: openshift/ingress-node-firewall
Length of output: 12453
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '84,190p' test/e2e/tls/tls_compliance.go
sed -n '284,337p' test/e2e/tls/tls_compliance.go
rg -n -C 5 'LegacyAdheringComponentsOnly|GetExpectedTLSConfigs|CheckTLSConnection|should work|TLS 1\.2' --glob '*.go' .Repository: openshift/ingress-node-firewall
Length of output: 50387
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
p = Path("test/e2e/tls/tls_compliance.go")
s = p.read_text()
# Read-only invariant check: the LegacyAdheringComponentsOnly success config
# spans TLS 1.2 through TLS 1.3, while the connection helper performs one
# handshake for that config and one exact-version rejection handshake.
block = re.search(
r'if tlsAdherence == "LegacyAdheringComponentsOnly".*?'
r'(?=\n\t\telse \{|\n\tcase profile\.Type)',
s,
re.S,
)
assert block, "LegacyAdheringComponentsOnly branch not found"
b = block.group(0)
m = re.search(
r'shouldWork = &tls\.Config\{\s*'
r'MinVersion:\s*tls\.VersionTLS12,\s*'
r'MaxVersion:\s*tls\.VersionTLS13,',
b,
)
assert m, "Expected TLS 1.2-1.3 success range not found"
helper = re.search(r'func CheckTLSConnection\(.*?\n\}', s, re.S)
assert helper, "CheckTLSConnection not found"
h = helper.group(0)
assert h.count('tls.Dial("tcp", endpoint,') == 2, "Expected two TLS handshakes"
assert "tlsShouldWork" in h and "tlsShouldNotWork" in h
assert "MinVersion == MaxVersion" not in h
print("LegacyAdheringComponentsOnly success range: TLS 1.2-TLS 1.3")
print("CheckTLSConnection handshakes: one success-config handshake and one rejection-config handshake")
print("No exact-version assertion exists in CheckTLSConnection")
print("A successful range handshake alone does not establish that TLS 1.2 was negotiated")
PYRepository: openshift/ingress-node-firewall
Length of output: 462
Test TLS 1.2 and TLS 1.3 separately.
CheckTLSConnection performs one successful handshake with a TLS 1.2–1.3 range and does not assert the negotiated version. A TLS 1.3-only endpoint can therefore pass without proving TLS 1.2 support. Use separate exact-version configurations with MinVersion == MaxVersion, and retain TLS 1.1 as a rejection test.
🤖 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/e2e/functional/tests/e2e.go` around lines 1178 - 1199, Update
VerifyTLSComplianceForPods and its CheckTLSConnection flow to test TLS 1.2 and
TLS 1.3 in separate connections using configurations where MinVersion equals
MaxVersion for each requested version. Assert both exact-version handshakes
succeed, while retaining the TLS 1.1 connection as an expected rejection.
| It("should verify multus-cni TLS compliance", func() { | ||
| // TODO: Implementation | ||
| }) | ||
|
|
||
| It("should verify ovn-kubernetes TLS compliance", func() { | ||
| // TODO: Implementation | ||
| }) | ||
|
|
||
| It("should verify cluster-network-operator TLS compliance", func() { | ||
| // TODO: Implementation | ||
| }) | ||
|
|
||
| It("should verify openshift-network-console TLS compliance", func() { | ||
| // TODO: Implementation | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'github.com/onsi/ginkgo' go.mod
rg -n -C 3 '\b(PIt|PContext|It|Context|Skip)\s*\(' test/e2e/functional/tests/e2e.goRepository: openshift/ingress-node-firewall
Length of output: 11139
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- TLS compliance block ---'
sed -n '1128,1420p' test/e2e/functional/tests/e2e.go
printf '%s\n' '--- Ginkgo pending API usage and module metadata ---'
rg -n '\b(PIt|PContext|FIt|FContext|Skip)\s*\(' --glob '*.go' .
sed -n '1,30p' go.mod
rg -n 'github.com/onsi/ginkgo v1\.16\.5|onsi/ginkgo' go.sumRepository: openshift/ingress-node-firewall
Length of output: 20109
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1128,1420p' test/e2e/functional/tests/e2e.go
rg -n '\b(PIt|PContext|FIt|FContext|Skip)\s*\(' --glob '*.go' .
sed -n '1,30p' go.mod
rg -n 'github.com/onsi/ginkgo v1\.16\.5|onsi/ginkgo' go.sumRepository: openshift/ingress-node-firewall
Length of output: 20027
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Ginkgo v1 execution semantics ---'
sed -n '40,145p' vendor/github.com/onsi/ginkgo/internal/specrunner/spec_runner.go
sed -n '40,140p' vendor/github.com/onsi/ginkgo/internal/spec/specs.go
sed -n '340,370p' vendor/github.com/onsi/ginkgo/ginkgo_dsl.go
sed -n '400,425p' vendor/github.com/onsi/ginkgo/ginkgo_dsl.go
printf '%s\n' '--- Read-only structural verifier ---'
python3 - <<'PY'
from pathlib import Path
import re
path = Path("test/e2e/functional/tests/e2e.go")
text = path.read_text()
todo_specs = re.findall(
r'(?m)^\s*It\("should verify (?:multus-cni|ovn-kubernetes|cluster-network-operator|openshift-network-console) TLS compliance", func\(\) \{\s*// TODO: Implementation\s*\n\s*\}\)',
text,
)
print(f"TODO compliance specs: {len(todo_specs)}")
for name in (
"Modern TLS Profile with StrictAllComponents",
"Custom TLS Profile",
):
match = re.search(
rf'Context\("{re.escape(name)}".*?BeforeEach\(func\(\) \{{(.*?)\n\s*\}}\)',
text,
re.S,
)
body = match.group(1) if match else ""
print(f"{name}: nested Skip calls: {len(re.findall(r'\\bSkip\\(', body))}")
vendor = Path("vendor/github.com/onsi/ginkgo/ginkgo_dsl.go").read_text()
for symbol in ("PIt", "PContext"):
print(f"{symbol} available in vendored Ginkgo: {bool(re.search(rf'func {symbol}\\(', vendor))}")
PYRepository: openshift/ingress-node-firewall
Length of output: 7553
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '40,145p' vendor/github.com/onsi/ginkgo/internal/specrunner/spec_runner.go
sed -n '40,140p' vendor/github.com/onsi/ginkgo/internal/spec/specs.go
sed -n '340,370p' vendor/github.com/onsi/ginkgo/ginkgo_dsl.go
sed -n '400,425p' vendor/github.com/onsi/ginkgo/ginkgo_dsl.go
python3 - <<'PY'
from pathlib import Path
import re
text = Path("test/e2e/functional/tests/e2e.go").read_text()
todo_specs = re.findall(
r'(?m)^\s*It\("should verify (?:multus-cni|ovn-kubernetes|cluster-network-operator|openshift-network-console) TLS compliance", func\(\) \{\s*// TODO: Implementation\s*\n\s*\}\)',
text,
)
print(f"TODO compliance specs: {len(todo_specs)}")
vendor = Path("vendor/github.com/onsi/ginkgo/ginkgo_dsl.go").read_text()
for symbol in ("PIt", "PContext"):
print(f"{symbol} available: {bool(re.search(rf'func {symbol}\\(', vendor))}")
PYRepository: openshift/ingress-node-firewall
Length of output: 7481
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Spec execution and pending-state handling ---'
rg -n -C 6 'func \(runner \*SpecRunner\) runSpecs|func \(spec \*Spec\) Run|Pending\(\)|SpecStatePending|FlagTypePending' \
vendor/github.com/onsi/ginkgo/internal/specrunner \
vendor/github.com/onsi/ginkgo/internal/spec \
vendor/github.com/onsi/ginkgo/internal
printf '%s\n' '--- Corrected structural verifier ---'
python3 - <<'PY'
from pathlib import Path
import re
text = Path("test/e2e/functional/tests/e2e.go").read_text()
todo_pattern = re.compile(
r'(?m)^\s*It\("should verify (?:multus-cni|ovn-kubernetes|cluster-network-operator|openshift-network-console) TLS compliance", func\(\) \{\s*// TODO: Implementation\s*\n\s*\}\)'
)
print("TODO compliance specs:", len(todo_pattern.findall(text)))
for name in (
"Modern TLS Profile with StrictAllComponents",
"Custom TLS Profile",
):
start = text.index(f'Context("{name}"')
end = text.index("\n\t\t\t})", start)
body = text[start:end]
print(name, "nested Skip calls:", len(re.findall(r"\bSkip\(", body)))
vendor = Path("vendor/github.com/onsi/ginkgo/ginkgo_dsl.go").read_text()
for symbol in ("PIt", "PContext"):
print(symbol, "available:", bool(re.search(r"func " + symbol + r"\(", vendor)))
PYRepository: openshift/ingress-node-firewall
Length of output: 24527
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 'func \(runner \*SpecRunner\) runSpecs|func \(spec \*Spec\) Run|Pending\(\)|SpecStatePending|FlagTypePending' \
vendor/github.com/onsi/ginkgo/internal/specrunner \
vendor/github.com/onsi/ginkgo/internal/spec \
vendor/github.com/onsi/ginkgo/internal
python3 - <<'PY'
from pathlib import Path
import re
text = Path("test/e2e/functional/tests/e2e.go").read_text()
todo_pattern = re.compile(
r'(?m)^\s*It\("should verify (?:multus-cni|ovn-kubernetes|cluster-network-operator|openshift-network-console) TLS compliance", func\(\) \{\s*// TODO: Implementation\s*\n\s*\}\)'
)
print("TODO compliance specs:", len(todo_pattern.findall(text)))
for name in (
"Modern TLS Profile with StrictAllComponents",
"Custom TLS Profile",
):
start = text.index(f'Context("{name}"')
end = text.index("\n\t\t\t})", start)
body = text[start:end]
print(name, "nested Skip calls:", len(re.findall(r"\bSkip\(", body)))
vendor = Path("vendor/github.com/onsi/ginkgo/ginkgo_dsl.go").read_text()
for symbol in ("PIt", "PContext"):
print(symbol, "available:", bool(re.search(r"func " + symbol + r"\(", vendor)))
PYRepository: openshift/ingress-node-firewall
Length of output: 24439
Mark all unimplemented TLS coverage as pending.
Change all eight TODO It specs to PIt. Change the StrictAllComponents and Custom TLS Profile contexts to PContext, and remove their nested Skip calls. The active TODO specs otherwise pass without assertions, while the nested Skip calls occur after the parent BeforeEach enables TLSAdherence.
🤖 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/e2e/functional/tests/e2e.go` around lines 1202 - 1216, Mark all eight
TODO TLS coverage specs as pending by changing their It declarations to PIt.
Change the StrictAllComponents and Custom TLS Profile contexts to PContext, and
remove their nested Skip calls so pending contexts do not execute the
TLSAdherence-enabling BeforeEach.
| func CheckTLSConnection(host string, port int, tlsShouldWork, tlsShouldNotWork *tls.Config) error { | ||
| endpoint := fmt.Sprintf("localhost:%d", port) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '250,330p' test/e2e/tls/tls_compliance.go
printf '\nCall sites:\n'
rg -n -C 3 'CheckTLSConnection\s*\(' .
printf '\nImports and relevant connection calls:\n'
sed -n '1,80p' test/e2e/tls/tls_compliance.go
rg -n -C 4 'endpoint|Dial|DialTLS|tlsShouldWork|tlsShouldNotWork' test/e2e/tls/tls_compliance.goRepository: openshift/ingress-node-firewall
Length of output: 12239
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
import re
p = Path("test/e2e/tls/tls_compliance.go")
s = p.read_text()
fn = re.search(
r'func CheckTLSConnection\(host string, port int, tlsShouldWork, tlsShouldNotWork \*tls\.Config\) error \{(?P<body>.*?)\n\}',
s,
re.S,
)
assert fn, "CheckTLSConnection declaration not found"
body = fn.group("body")
print("endpoint uses literal localhost:", 'fmt.Sprintf("localhost:%d", port)' in body)
print("endpoint uses host variable:", re.search(r'\bhost\b', body) is not None)
calls = re.findall(r'CheckTLSConnection\(\s*([^,\n]+)\s*,\s*([^,\n]+)', s)
print("call count:", len(calls))
for i, (host_arg, port_arg) in enumerate(calls, 1):
print(f"call {i}: host={host_arg.strip()} port={port_arg.strip()}")
PYRepository: openshift/ingress-node-firewall
Length of output: 375
Use host when constructing the endpoint.
CheckTLSConnection ignores its host parameter and always connects to localhost. Use net.JoinHostPort(host, strconv.Itoa(port)) so non-local and IPv6 hosts connect correctly.
🤖 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/e2e/tls/tls_compliance.go` around lines 286 - 287, Update
CheckTLSConnection to construct endpoint using the provided host via
net.JoinHostPort(host, strconv.Itoa(port)) instead of hardcoding localhost,
preserving correct handling for non-local and IPv6 hosts.
| ctx := context.Background() | ||
|
|
||
| log.Println("=== Enabling TLSAdherence Feature Gate ===") | ||
|
|
||
| // Step 1: Patch FeatureGate to enable TLSAdherence | ||
| log.Printf("Step 1: Patching FeatureGate to enable TLSAdherence") | ||
| fg, err := configClient.ConfigV1().FeatureGates().Get(ctx, "cluster", metav1.GetOptions{}) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get featuregate: %w", err) | ||
| } | ||
|
|
||
| // Check if already enabled | ||
| tlsFeatureAlreadyEnabled := isAlreadyEnabled(fg) | ||
| if tlsFeatureAlreadyEnabled { | ||
| log.Printf("✓ TLSAdherence feature gate already enabled") | ||
| } else { | ||
| // Enable TLSAdherence feature gate | ||
| if err := patchFeatureGate(ctx, configClient, fg); err != nil { | ||
| return err | ||
| } | ||
| log.Printf("✓ Feature gate patched successfully") | ||
| } | ||
|
|
||
| // Step 2: Verify TLSAdherence is active in status (15 min timeout) | ||
| log.Printf("Step 2: Verifying TLSAdherence is active in status (15 min timeout)") | ||
| if err := verifyTLSAdherenceActive(ctx, configClient); err != nil { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Add bounded contexts for control-plane requests.
context.Background() gives the direct API requests no deadline. A stalled Get or Update can block these setup flows indefinitely before or after the polling helpers run.
Create a timeout context, defer its cancellation, and pass it through the direct client calls and helper calls.
#!/bin/bash
set -euo pipefail
ast-grep outline test/e2e/tls/tls.go --items all --type function
sed -n '39,177p' test/e2e/tls/tls.go
rg -n -C 3 'context\.Background|context\.WithTimeout|FeatureGates\(\)\.Get|APIServers\(\)\.Get|\.Update\(' test/e2e/tlsAs per path instructions, "**/*.go: context.Context for cancellation and timeouts."
Also applies to: 107-175
🤖 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/e2e/tls/tls.go` around lines 49 - 74, Create a bounded timeout context
in the TLS setup flow instead of using context.Background(), defer its
cancellation, and pass that context through the direct FeatureGate API requests
and helper calls such as patchFeatureGate and verifyTLSAdherenceActive. Ensure
all related control-plane Get, Update, and polling operations use the same
cancellable context.
Source: Path instructions
- Update deployment to use internal OpenShift registry for images - Controller: image-registry.openshift-image-registry.svc:5000/.../controller - Daemon: image-registry.openshift-image-registry.svc:5000/.../daemon - Ensures images persist with NFS-backed PVC storage - Add tlsAdherence API field support validation in TLS tests - Check if apiserver.spec.tlsAdherence field is supported - Fail with clear error if field is not available in OpenShift version - Prevents silent failures when API field is unsupported Tested on OpenShift 5.0.0-0.ci-2026-08-03-055327 with full API support. TLS compliance test PASSED: Modern profile with LegacyAdheringComponentsOnly. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
@weliang1: 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. |
1 participant
TEST PR
Summary by CodeRabbit
New Features
Documentation
Tests