CORENET-7243: Add TLS profile compliance e2e tests for ingress-node-firewall - #766
Conversation
|
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 PR adds OpenShift TLS profile compliance tests. The tests configure TLS adherence, wait for cluster stabilization, discover daemon pods, and validate TLS 1.2 and TLS 1.3 behavior. It also registers OpenShift API schemes and updates build and test settings. ChangesOpenShift TLS compliance
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant FunctionalTest
participant TLSHelpers
participant OpenShiftAPIs
participant ClusterResources
participant DaemonPod
participant MetricsEndpoint
FunctionalTest->>TLSHelpers: Configure TLS profile and adherence
TLSHelpers->>OpenShiftAPIs: Update FeatureGate and APIServer
TLSHelpers->>ClusterResources: Wait for pools, nodes, and operators
ClusterResources-->>TLSHelpers: Report stable state
FunctionalTest->>TLSHelpers: Verify daemon TLS compliance
TLSHelpers->>DaemonPod: Restart and await readiness
TLSHelpers->>DaemonPod: Execute TLS 1.3 and TLS 1.2 checks
DaemonPod->>MetricsEndpoint: Connect with selected TLS protocol
MetricsEndpoint-->>DaemonPod: Return protocol result
DaemonPod-->>TLSHelpers: Return validation result
TLSHelpers-->>FunctionalTest: Return compliance status
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
test/e2e/tls/tls.go (3)
49-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
errors.Asfor the sentinel error check.
IsTLSAdherenceNotSupporteduses a direct type assertion. It fails if the error is ever wrapped.ConfigureTLSProfileWithAdherencereturns the error unwrapped at Line 106 today, so the check works. It breaks silently if a future change addsfmt.Errorf("...: %w", err)on that path, and the test then fails instead of skipping.♻️ Proposed refactor
func IsTLSAdherenceNotSupported(err error) bool { - _, ok := err.(*TLSAdherenceNotSupportedError) - return ok + var target *TLSAdherenceNotSupportedError + return errors.As(err, &target) }Add
"errors"to the import block.🤖 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 - 52, Update IsTLSAdherenceNotSupported to use errors.As when matching TLSAdherenceNotSupportedError, adding the errors import as needed, so wrapped errors are recognized while preserving the existing boolean result.
79-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix the misleading step numbering and the inverted log message.
Two problems appear in this block. Line 79 logs "Step 2" but it is the first step; no "Step 1" exists. Line 84 tests
!featureGateEnabledand then logs "Feature gate enabled and cluster stabilized". The return value means "was already enabled", so the condition is correct but the name and the message read as contradictory.♻️ Proposed refactor
- log.Println("Step 2: Checking and enabling TLSAdherence feature gate") - featureGateEnabled, err := enableTLSAdherenceFeatureGate(ctx, c) + log.Println("Step 1: Checking and enabling TLSAdherence feature gate") + alreadyEnabled, err := enableTLSAdherenceFeatureGate(ctx, c) if err != nil { return err } - if !featureGateEnabled { + if !alreadyEnabled { log.Println(" Feature gate enabled and cluster stabilized") } - log.Printf("Step 3: Configuring APIServer with %s TLS profile and tlsAdherence=%s", tlsProfileType, tlsAdherencePolicy) + log.Printf("Step 2: Configuring APIServer with %s TLS profile and tlsAdherence=%s", tlsProfileType, tlsAdherencePolicy)Renumber the remaining steps at Lines 93, 99, and 104 accordingly.
🤖 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 79 - 86, In the TLS setup flow around enableTLSAdherenceFeatureGate, rename the returned flag to reflect that it indicates the feature gate was already enabled, and update the conditional log so it accurately describes the !already-enabled path. Change the current “Step 2” label to the first step, then renumber the subsequent step logs at the later checkpoints sequentially.
318-320: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAccept a
context.Contextparameter instead of creatingcontext.Background()internally.Seven helpers construct their own root context. The combined wait budgets are large:
MCPRolloutCompleteTimeoutandNodeStabilityTimeoutare 60 minutes each, andOperatorSettleTimeMinutesis 60. A caller cannot cancel these waits, so a Ginkgo interrupt cannot stop them early.ConfigureTLSProfileWithAdherencealready holds actxat Line 77 and can pass it down.Change each helper to take
ctx context.Contextas the first parameter and thread the caller's context through.As per path instructions: "context.Context for cancellation and timeouts".
Also applies to: 351-353, 372-374, 395-397, 472-474, 551-553, 654-658
🤖 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 318 - 320, Update all seven helper functions, including areAllMCPsComplete and the helpers at the referenced locations, to accept ctx context.Context as their first parameter instead of creating context.Background() internally. Thread the existing context from ConfigureTLSProfileWithAdherence through every helper call and use it for downstream operations, preserving the current wait behavior while allowing cancellation.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 `@Makefile`:
- Line 166: Align the TLS suite’s wait budgets with the 90-minute go test
timeout. Update MCPRolloutCompleteTimeout, NodeStabilityTimeout, and
OperatorSettleTimeMinutes in the TLS configuration flow, or raise the Makefile
timeout, so repeated waits in enableTLSAdherenceFeatureGate and
waitForClusterStability across all ConfigureTLSProfileWithAdherence profile
combinations complete within the suite timeout and preserve JUnit report
generation.
In `@test/e2e/functional/tests/e2e.go`:
- Around line 40-43: Update the TLS pod verification flow to use
OperatorNameSpace instead of OpenShiftNameSpace for both the status message and
verifier call, while leaving the daemonLabelSelector unchanged.
In `@test/e2e/tls/tls.go`:
- Around line 264-316: Add exported APIServer spec save/restore helpers and use
them in the functional suite’s AfterEach or AfterSuite to restore the original
TLS profile and TLSAdherence after patchAPIServerTLSProfile runs. Ensure cleanup
is registered before the test cases execute and still runs when tests fail.
Document at the suite invocation that patchFeatureGate changes FeatureSet to
CustomNoUpgrade and cannot be reversed, warning that the cluster must not be
reused for upgrade testing.
- Around line 599-623: Update the poll failure handling after
wait.PollUntilContextTimeout to wrap pollErr, not the closure-scoped err, when
constructing lastErr. Preserve the existing retry flow and include the delay
context while retaining the actual timeout or polling failure cause.
- Around line 709-718: Update testTLS12Connection’s expectReject branch to
retain and inspect the error returned by execCommandInPodWithRetry. Distinguish
expected curl/TLS handshake failure from pod-exec or transport setup failure,
returning the infrastructure error instead of treating empty output as a
successful rejection; preserve the existing rejection-output validation for a
completed curl invocation.
- Around line 540-549: Guard TLSSecurityProfile access in
determineTLSTestBehavior and the related log statement used by
VerifyIngressNodeFirewallTLSComplianceInPod. Treat a nil profile as unset
without dereferencing it, preserve the existing behavior for configured
profiles, and use an "<unset>" label when logging the profile type.
---
Nitpick comments:
In `@test/e2e/tls/tls.go`:
- Around line 49-52: Update IsTLSAdherenceNotSupported to use errors.As when
matching TLSAdherenceNotSupportedError, adding the errors import as needed, so
wrapped errors are recognized while preserving the existing boolean result.
- Around line 79-86: In the TLS setup flow around enableTLSAdherenceFeatureGate,
rename the returned flag to reflect that it indicates the feature gate was
already enabled, and update the conditional log so it accurately describes the
!already-enabled path. Change the current “Step 2” label to the first step, then
renumber the subsequent step logs at the later checkpoints sequentially.
- Around line 318-320: Update all seven helper functions, including
areAllMCPsComplete and the helpers at the referenced locations, to accept ctx
context.Context as their first parameter instead of creating
context.Background() internally. Thread the existing context from
ConfigureTLSProfileWithAdherence through every helper call and use it for
downstream operations, preserving the current wait behavior while allowing
cancellation.
🪄 Autofix
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: 8453c843-be80-49c6-9c63-5dbaee4f81b8
⛔ Files ignored due to path filters (29)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/config/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_cluster_version.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_crio_credential_provider_config.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_kmsencryption.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_network.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machineconfiguration/v1/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/types_internalreleaseimage.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/types_machineconfignode.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/types_machineosbuild.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/types_machineosconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/types_osimagestream.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/types_pinnedimageset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (5)
Makefilego.modtest/e2e/client/client.gotest/e2e/functional/tests/e2e.gotest/e2e/tls/tls.go
| func patchFeatureGate(ctx context.Context, c client.Client, fg *configv1.FeatureGate) error { | ||
| if fg.Spec.FeatureSet == configv1.CustomNoUpgrade && fg.Spec.CustomNoUpgrade != nil { | ||
| fg.Spec.CustomNoUpgrade.Enabled = append(fg.Spec.CustomNoUpgrade.Enabled, "TLSAdherence") | ||
| } else { | ||
| fg.Spec.FeatureSet = configv1.CustomNoUpgrade | ||
| fg.Spec.CustomNoUpgrade = &configv1.CustomFeatureGates{ | ||
| Enabled: []configv1.FeatureGateName{"TLSAdherence"}, | ||
| } | ||
| } | ||
|
|
||
| err := c.Update(ctx, fg) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to update featuregate: %w", err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func patchAPIServerTLSProfile(ctx context.Context, c client.Client, tlsProfileType string, tlsAdherencePolicy string) error { | ||
| apiserver := &configv1.APIServer{} | ||
| err := c.Get(ctx, types.NamespacedName{Name: "cluster"}, apiserver) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get apiserver: %w", err) | ||
| } | ||
|
|
||
| switch tlsProfileType { | ||
| case "Modern": | ||
| apiserver.Spec.TLSSecurityProfile = &configv1.TLSSecurityProfile{ | ||
| Type: configv1.TLSProfileModernType, | ||
| Modern: &configv1.ModernTLSProfile{}, | ||
| } | ||
| case "Intermediate": | ||
| apiserver.Spec.TLSSecurityProfile = &configv1.TLSSecurityProfile{ | ||
| Type: configv1.TLSProfileIntermediateType, | ||
| Intermediate: &configv1.IntermediateTLSProfile{}, | ||
| } | ||
| case "Old": | ||
| apiserver.Spec.TLSSecurityProfile = &configv1.TLSSecurityProfile{ | ||
| Type: configv1.TLSProfileOldType, | ||
| Old: &configv1.OldTLSProfile{}, | ||
| } | ||
| default: | ||
| return fmt.Errorf("unsupported TLS profile type: %s (must be Modern, Intermediate, or Old)", tlsProfileType) | ||
| } | ||
|
|
||
| apiserver.Spec.TLSAdherence = configv1.TLSAdherencePolicy(tlsAdherencePolicy) | ||
|
|
||
| err = c.Update(ctx, apiserver) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to update apiserver: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Restore the cluster state that these helpers mutate.
patchFeatureGate and patchAPIServerTLSProfile write cluster-scoped OpenShift configuration. Neither helper records the previous value, and the functional test at test/e2e/functional/tests/e2e.go Line 1166 only deletes the IngressNodeFirewallConfig. Two consequences follow.
First, patchFeatureGate sets Spec.FeatureSet to CustomNoUpgrade. On OpenShift this transition is not reversible and it blocks cluster upgrades. Any CI cluster that runs this suite is left unusable for upgrade testing.
Second, the last executed profile combination stays applied. The three test cases at test/e2e/functional/tests/e2e.go Lines 1177-1181 run in order, so the cluster is left on Modern with StrictAllComponents, which can break unrelated tests that run afterwards.
Add exported save and restore helpers for the APIServer spec, and call them from an AfterEach or AfterSuite. Document the CustomNoUpgrade side effect where the suite is invoked, because it cannot be undone.
Do you want me to generate the save/restore helpers and the cleanup wiring?
🤖 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 264 - 316, Add exported APIServer spec
save/restore helpers and use them in the functional suite’s AfterEach or
AfterSuite to restore the original TLS profile and TLSAdherence after
patchAPIServerTLSProfile runs. Ensure cleanup is registered before the test
cases execute and still runs when tests fail. Document at the suite invocation
that patchFeatureGate changes FeatureSet to CustomNoUpgrade and cannot be
reversed, warning that the cluster must not be reused for upgrade testing.
There was a problem hiding this comment.
Documents at /test/e2e/functional/tests/e2e.go:
// Cluster State Modifications (cannot be fully reverted):
// - FeatureGate/cluster: Changed to CustomNoUpgrade with TLSAdherence enabled (IRREVERSIBLE)
// - APIServer/cluster: TLS profile and adherence policy modified (reversible but not restored)
// - MachineConfigPools: Rolled out with new kubelet TLS configuration
There was a problem hiding this comment.
@weliang1, the documentation correctly describes the irreversible FeatureGate/cluster change.
The finding remains open. The comment also confirms that the test does not restore the reversible APIServer/cluster TLS profile and TLSAdherence policy. Add AfterEach or AfterSuite cleanup that restores the initial APIServer values.
✏️ Learnings added
Learnt from: weliang1
Repo: openshift/ingress-node-firewall PR: 766
File: test/e2e/tls/tls.go:255-307
Timestamp: 2026-08-12T13:51:27.197Z
Learning: In `test/e2e/functional/tests/e2e.go`, the TLS profile compliance tests change `FeatureGate/cluster` to `CustomNoUpgrade` with `TLSAdherence`, which is irreversible. They also change `APIServer/cluster` TLS profile and TLS adherence policy; these values are reversible but are not currently restored.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
weliang1
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@weliang1: This pull request references CORENET-7243 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/test ingress-node-firewall-e2e-metal-ipi |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/e2e/functional/tests/e2e.go (2)
1178-1182: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPropagate the TLS configuration cleanup error.
infwutils.DeleteIngressNodeFirewallConfigreturns an error, butAfterEachignores it. A failed deletion can leave cluster state behind and contaminate later TLS cases. Assert or otherwise propagate the returned error.As per path instructions:
**/*.go: Never ignore error returns.🤖 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 - 1182, Update the AfterEach cleanup around infwutils.DeleteIngressNodeFirewallConfig to handle its returned error instead of discarding it; assert or propagate the error while preserving the existing config != nil guard.Source: Path instructions
1162-1167: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRequire explicit opt-in for destructive TLS tests.
The OpenShift check allows these tests to run on any OpenShift cluster. The test then changes
FeatureGate.Spec.FeatureSetand API server TLS settings without restoring them. The warning comment does not preventmake test-functionalor a focused local run from changing a reusable cluster. Add an explicit opt-in or CI-only guard before configuring TLS.🤖 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 1162 - 1167, Add an explicit opt-in or CI-only guard in the TLS compliance test setup before any TLS configuration changes, alongside the existing OpenShift check in BeforeEach. Require that guard in addition to the OpenShift validation, and skip unless enabled so local or default functional runs cannot modify a reusable cluster.
🤖 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.
Outside diff comments:
In `@test/e2e/functional/tests/e2e.go`:
- Around line 1178-1182: Update the AfterEach cleanup around
infwutils.DeleteIngressNodeFirewallConfig to handle its returned error instead
of discarding it; assert or propagate the error while preserving the existing
config != nil guard.
- Around line 1162-1167: Add an explicit opt-in or CI-only guard in the TLS
compliance test setup before any TLS configuration changes, alongside the
existing OpenShift check in BeforeEach. Require that guard in addition to the
OpenShift validation, and skip unless enabled so local or default functional
runs cannot modify a reusable cluster.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 585bc14a-6370-45d8-b864-1ff2c5c55df2
📒 Files selected for processing (3)
Makefiletest/e2e/functional/tests/e2e.gotest/e2e/tls/tls.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/tls/tls.go
tpantelis
left a comment
tpantelis
left a comment
There was a problem hiding this comment.
Some comments in openshift/origin#31500 also apply here.
| log.Printf("=== Configuring %s TLS Profile with %s (started: %s) ===", | ||
| tlsProfileType, tlsAdherencePolicy, overallStartTime.Format("15:04:05")) | ||
|
|
||
| ctx := context.Background() |
There was a problem hiding this comment.
Multiple functions create context.Background() instead of accepting a context parameter, eg:
- areAllMCPsComplete
- waitForMCPRolloutStart
- waitForNodesStability
- findRunningPod
- WaitForDaemonPodsReady
Thread the context from here through the call stack to enable proper cancellation.
|
|
||
| ctx := context.Background() | ||
|
|
||
| log.Println("Step 2: Checking and enabling TLSAdherence feature gate") |
There was a problem hiding this comment.
Shouldn't this be Step 1?
| } | ||
|
|
||
| func IsTLSAdherenceNotSupported(err error) bool { | ||
| _, ok := err.(*TLSAdherenceNotSupportedError) |
There was a problem hiding this comment.
Use errors.As instead of type assertion in case the error is ever wrapped.
| _, ok := err.(*TLSAdherenceNotSupportedError) | |
| var target *TLSAdherenceNotSupportedError | |
| return errors.As(err, &target) |
| ctx := context.Background() | ||
|
|
||
| log.Println("Step 2: Checking and enabling TLSAdherence feature gate") | ||
| featureGateEnabled, err := enableTLSAdherenceFeatureGate(ctx, c) |
There was a problem hiding this comment.
featureGateEnabled actually means alreadyEnabled.
This commit addresses all review feedback from PR openshift#766: 1. Combine wasteful test setup (tpantelis) - Merge 3 separate It specs into single It spec - Reduces test time by ~2-4 hours (avoids redundant MCP rollouts) 2. Use configv1 typed constants (tpantelis) - Replace string literals with configv1.TLSProfileType - Replace string literals with configv1.TLSAdherencePolicy - Update all function signatures and comparisons 3. Fix pod readiness check (tpantelis) - Use podutil.IsPodReady() instead of only checking Phase==Running - Prevents race conditions by ensuring pod is actually ready 4. Simplify node readiness check (tpantelis) - Use slices.ContainsFunc() for cleaner code 5. Fix step numbering (tpantelis) - Renumber steps to start from 1 instead of 2 6. Use errors.As() for error type checking (tpantelis) - Replace type assertion with errors.As() - Future-proof for wrapped errors 7. Rename variable for clarity (tpantelis) - Rename featureGateEnabled to alreadyEnabled 8. Add [OCPFeatureGate:TLSAdherence][Serial] tags (CodeRabbit) - Ensures tests run on dedicated, disposable CI infrastructure - Update documentation to match openshift/origin#31500 pattern Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/e2e/functional/tests/e2e.go (1)
1192-1205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIsolate each TLS profile combination into its own Ginkgo spec.
Each profile currently runs inside one
It. If one profile fails, later profiles do not run. Register one spec pertlsProfileTestentry so the suite reports all three scenarios independently.🤖 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 1192 - 1205, Refactor the TLS compliance test around the current `It` block and `tlsProfiles` loop so each `tlsProfileTest` entry is registered as its own Ginkgo spec. Move the profile-specific configuration and verification steps into the per-profile spec while preserving the existing skip and failure behavior, allowing all profile scenarios to run independently.
🤖 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 `@Dockerfile`:
- Line 2: Pin all specified non-Red-Hat base images by digest: update the
builder image in Dockerfile and Dockerfile.daemon to
golang:1.26@sha256:7caba5286b4c3613a337b709c573047d8ae62ee76106647313b61e72b99f20af,
and update Dockerfile’s distroless static image to
gcr.io/distroless/static:nonroot@sha256:f7f8f729987ad0fdf6b05eeeae94b26e6a0f613bdf46feea7fc40f7bd72953e6.
In `@Dockerfile.daemon`:
- Line 1: Add a Docker HEALTHCHECK to Dockerfile.daemon using a command
available in the final image that verifies the daemon is ready, while preserving
the existing builder and runtime stages.
- Line 1: Update the Dockerfile daemon image and its deployment configuration to
create and use a supported non-root user for /usr/bin/daemon and
/usr/bin/syslog. Set the container security context to runAsNonRoot: true,
explicitly prevent UID 0, and grant only the capabilities required by the
daemon.
---
Nitpick comments:
In `@test/e2e/functional/tests/e2e.go`:
- Around line 1192-1205: Refactor the TLS compliance test around the current
`It` block and `tlsProfiles` loop so each `tlsProfileTest` entry is registered
as its own Ginkgo spec. Move the profile-specific configuration and verification
steps into the per-profile spec while preserving the existing skip and failure
behavior, allowing all profile scenarios to run independently.
🪄 Autofix
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: bd35713d-00e0-4efb-b969-aed57fe6769c
📒 Files selected for processing (4)
DockerfileDockerfile.daemontest/e2e/functional/tests/e2e.gotest/e2e/tls/tls.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/tls/tls.go
| @@ -1,5 +1,5 @@ | |||
| # Build the manager binary | |||
| FROM golang:1.25 as builder | |||
| FROM golang:1.26 as builder | |||
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Dockerfile ---'
sed -n '1,120p' Dockerfile
printf '%s\n' '--- Dockerfile.daemon ---'
sed -n '1,120p' Dockerfile.daemon
printf '%s\n' '--- image references ---'
rg -n '^\s*FROM\s+' Dockerfile Dockerfile.daemon
printf '%s\n' '--- repository status ---'
git status --shortRepository: openshift/ingress-node-firewall
Length of output: 1736
🏁 Script executed:
#!/bin/bash
set -eu
repo='library/golang'
tag='1.26'
token="$(curl -fsSL "https://auth.docker.io/token?service=registry.docker.io&scope=repository:${repo}:pull" | jq -r .token)"
curl -fsSL \
-H "Authorization: Bearer ${token}" \
-H 'Accept: application/vnd.oci.image.index.v1+json, application/vnd.docker.distribution.manifest.list.v2+json, application/vnd.docker.distribution.manifest.v2+json' \
-D /tmp/golang-headers \
"https://registry-1.docker.io/v2/${repo}/manifests/${tag}" \
-o /tmp/golang-manifest
printf '%s\n' '--- digest header ---'
awk 'BEGIN{IGNORECASE=1} /^docker-content-digest:/ {print}' /tmp/golang-headers
printf '%s\n' '--- manifest media type and digest ---'
jq -r '.mediaType // "single-manifest", (.manifests[]? | [.platform.os, .platform.architecture, .digest] | `@tsv`)' /tmp/golang-manifestRepository: openshift/ingress-node-firewall
Length of output: 1742
🏁 Script executed:
#!/bin/bash
set -eu
repo='distroless/static'
tag='nonroot'
token="$(curl -fsSL "https://gcr.io/v2/token?scope=repository:${repo}:pull&service=gcr.io" | jq -r .token)"
curl -fsSL \
-H "Authorization: Bearer ${token}" \
-H 'Accept: application/vnd.oci.image.index.v1+json, application/vnd.docker.distribution.manifest.list.v2+json, application/vnd.docker.distribution.manifest.v2+json' \
-D /tmp/distroless-headers \
"https://gcr.io/v2/${repo}/manifests/${tag}" \
-o /tmp/distroless-manifest
printf '%s\n' '--- digest header ---'
awk 'BEGIN{IGNORECASE=1} /^docker-content-digest:/ {print}' /tmp/distroless-headers
printf '%s\n' '--- manifest media type and platform digests ---'
jq -r '.mediaType // "single-manifest", (.manifests[]? | [.platform.os, .platform.architecture, .digest] | `@tsv`)' /tmp/distroless-manifestRepository: openshift/ingress-node-firewall
Length of output: 882
Pin every non-Red-Hat base image by digest.
Dockerfile#L2andDockerfile.daemon#L1: usegolang:1.26@sha256:7caba5286b4c3613a337b709c573047d8ae62ee76106647313b61e72b99f20af.Dockerfile#L24: usegcr.io/distroless/static:nonroot@sha256:f7f8f729987ad0fdf6b05eeeae94b26e6a0f613bdf46feea7fc40f7bd72953e6.
📍 Affects 2 files
Dockerfile#L2-L2(this comment)Dockerfile.daemon#L1-L1
🤖 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 `@Dockerfile` at line 2, Pin all specified non-Red-Hat base images by digest:
update the builder image in Dockerfile and Dockerfile.daemon to
golang:1.26@sha256:7caba5286b4c3613a337b709c573047d8ae62ee76106647313b61e72b99f20af,
and update Dockerfile’s distroless static image to
gcr.io/distroless/static:nonroot@sha256:f7f8f729987ad0fdf6b05eeeae94b26e6a0f613bdf46feea7fc40f7bd72953e6.
Source: Path instructions
| @@ -1,4 +1,4 @@ | |||
| FROM golang:1.25 AS builder | |||
| FROM golang:1.26 AS builder | |||
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Define a container health check.
Dockerfile.daemon has no HEALTHCHECK. Add a check that uses a command available in the final image and verifies daemon readiness.
As per path instructions, Dockerfiles must define HEALTHCHECK.
🧰 Tools
🪛 Trivy (0.72.0)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
🤖 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 `@Dockerfile.daemon` at line 1, Add a Docker HEALTHCHECK to Dockerfile.daemon
using a command available in the final image that verifies the daemon is ready,
while preserving the existing builder and runtime stages.
Source: Path instructions
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Dockerfile.daemon ---'
cat -n Dockerfile.daemon
printf '%s\n' '--- container-related files ---'
git ls-files | grep -Ei '(^|/)(Dockerfile|Containerfile|.*(compose|deployment|daemon|k8s|helm).*)' | head -200
printf '%s\n' '--- runtime-user and daemon references ---'
rg -n --hidden -S 'Dockerfile\.daemon|/usr/bin/daemon|USER[[:space:]]|runAsUser|runAsNonRoot|user:[[:space:]]*0|privileged|capAdd|HEALTHCHECK' . \
-g '!node_modules' -g '!dist' -g '!build' -g '!vendor' | head -300Repository: openshift/ingress-node-firewall
Length of output: 16396
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- daemon manifests and security contexts ---'
for f in bindata/manifests/daemon/daemonset.yaml manifests/stable/ingress-node-firewall.clusterserviceversion.yaml bundle/manifests/ingress-node-firewall.clusterserviceversion.yaml config/openshift/rbac.yaml config/openshift-olm/rbac.yaml openshift-ci/deploy_ingress_node_firewall.sh; do
if [ -f "$f" ]; then
printf '\n### %s\n' "$f"
rg -n -C 8 'securityContext|runAsUser|runAsNonRoot|privileged|capAdd|hostNetwork|hostPID|hostPath|serviceAccountName|image:' "$f" || true
fi
done
printf '%s\n' '--- daemon implementation and image variants ---'
ast-grep outline cmd/daemon/daemon.go
cat -n Dockerfile.daemon.openshift
rg -n -C 5 'CAP_|capability|privilege|root|/sys|bpf|iptables|netlink|setns|host' cmd/daemon config bindata manifests Dockerfile.daemon Dockerfile.daemon.openshiftRepository: openshift/ingress-node-firewall
Length of output: 50388
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
dockerfile = Path("Dockerfile.daemon").read_text().splitlines()
stage_starts = [i for i, line in enumerate(dockerfile) if re.match(r"^\s*FROM\b", line, re.I)]
final_start = stage_starts[-1]
final_stage = dockerfile[final_start:]
print("final_stage_base:", dockerfile[final_start])
print("final_stage_has_user:", any(re.match(r"^\s*USER\b", line, re.I) for line in final_stage))
print("final_stage_has_healthcheck:", any(re.match(r"^\s*HEALTHCHECK\b", line, re.I) for line in final_stage))
print("final_stage_cmds:", [line.strip() for line in final_stage if re.match(r"^\s*CMD\b", line, re.I)])
daemonset = Path("bindata/manifests/daemon/daemonset.yaml").read_text()
print("daemonset_has_security_context:", bool(re.search(r"^\s{10,}securityContext:", daemonset, re.M)))
print("daemonset_has_run_as_user_zero:", bool(re.search(r"runAsUser:\s*0\b", daemonset)))
print("daemonset_uses_host_network:", bool(re.search(r"^\s*hostNetwork:\s*true\b", daemonset, re.M)))
print("daemonset_uses_host_pid:", bool(re.search(r"^\s*hostPID:\s*true\b", daemonset, re.M)))
PYRepository: openshift/ingress-node-firewall
Length of output: 471
Run the daemon containers as non-root.
Dockerfile.daemon has no USER, so /usr/bin/daemon and /usr/bin/syslog run as UID 0 by default. Add a supported non-root user, set runAsNonRoot: true, and grant only the capabilities required by the daemon. Ensure the deployment does not permit UID 0.
🧰 Tools
🪛 Trivy (0.72.0)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
🤖 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 `@Dockerfile.daemon` at line 1, Update the Dockerfile daemon image and its
deployment configuration to create and use a supported non-root user for
/usr/bin/daemon and /usr/bin/syslog. Set the container security context to
runAsNonRoot: true, explicitly prevent UID 0, and grant only the capabilities
required by the daemon.
Sources: Path instructions, Linters/SAST tools
…irewall PR openshift#766 Add missing MCP rollout handling functions to make test more robust when enabling feature gates and applying TLS profile configurations. Following the proven pattern from openshift/ingress-node-firewall PR openshift#766. New functions: - waitForMCPRolloutStart(): Waits for MCP "Updating" condition before checking completion, avoiding race where we check status before rollout begins - areAllMCPsComplete(): Checks if MCPs are already stable before waiting, avoiding unnecessary waits Flow improvements: 1. After enabling TLSAdherence feature gate: - Wait for MCP rollout to START (10 min timeout) - If started, wait for completion (60 min timeout) - Prevents false positives from checking before rollout begins 2. After applying TLS profile configuration: - Check if MCPs are already complete - If not complete, wait for rollout start then completion - Avoids redundant waits when MCPs are already stable This eliminates race conditions where we might check MCP status too early before the rollout has begun, leading to false positives. Reference: openshift/ingress-node-firewall#766 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
/retest |
|
/retest-required |
|
Re: #766 (review) The |
|
Re: #766 (review) This was intentionally combined in commit 27f8f9d to avoid redundant MCP rollouts, which reduces test time by 2-4 hours. Each separate It() would trigger a new BeforeEach, causing additional 60+ minute MCP rollouts. The current approach prioritizes CI efficiency over test isolation, which is appropriate for these long-running destructive tests. For Dockerfile-related issues, these are legitimate security concerns but NOT related to my TLS test code. |
|
/lgtm |
|
FYI @weliang1 @tpantelis The test at test/e2e/functional/tests/e2e.go:1192 will break after the bug fix that removes the kube-rbac-proxy from daemon pods: [OCPFeatureGate:TLSAdherence][Serial] TLS Profile Compliance It calls tls.VerifyIngressNodeFirewallTLSComplianceInPod which execs curl into the kube-rbac-proxy container. After the fix removes that container, the exec fails with "container not found". Specifically, the failure chain would be:
Will fix the failing test as part of the bug fix but this PR needs to be merged before that. |
@asood-rh so just to confirm, we should merge this PR as-is? You're just warning about a change that will be needed to a future PR because of this? |
@danwinship That is correct this PR should merge and the test will pass without issue at this point. Nothing should hold this PR. |
|
/verified by CI |
openshift-ci-robot
added
the
verified
|
@weliang1: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/approve |
openshift-ci
Bot
added
the
approved
Implements comprehensive TLS compliance testing to verify that ingress-node-firewall-daemon adheres to OpenShift cluster-wide TLS security profiles (Intermediate, Modern) with different adherence policies (LegacyAdheringComponentsOnly, StrictAllComponents). Changes: - Add new test/e2e/tls/tls.go package with TLS compliance utilities: * ConfigureTLSProfileWithAdherence() - Configure cluster TLS profile * VerifyIngressNodeFirewallTLSComplianceInPod() - Verify daemon TLS compliance * TLS cipher suite validation against expected profiles * Machine Config Pool rollout monitoring * Feature gate management for TLSAdherence API - Add TLS Profile Compliance test suite in test/e2e/functional/tests/e2e.go: * Test Intermediate profile with LegacyAdheringComponentsOnly * Test Modern profile with LegacyAdheringComponentsOnly * Test Modern profile with StrictAllComponents * Auto-skip on non-OpenShift clusters or unsupported API versions - Update dependencies: * Add github.com/openshift/client-go for config API access * Update vendor with config v1alpha1, v1alpha2, and machineconfiguration APIs * Update go.mod and go.sum - Update environment configuration for daemon TLS settings Test Results: All 3 TLS compliance tests passed successfully: ✓ Intermediate TLS Profile with LegacyAdheringComponentsOnly ✓ Modern TLS Profile with LegacyAdheringComponentsOnly ✓ Modern TLS Profile with StrictAllComponents Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Implements comprehensive TLS compliance testing to verify that ingress-node-firewall-daemon adheres to OpenShift cluster-wide TLS security profiles (Intermediate, Modern) with different adherence policies (LegacyAdheringComponentsOnly, StrictAllComponents). Changes: - Add new test/e2e/tls/tls.go package with TLS compliance utilities: * ConfigureTLSProfileWithAdherence() - Configure cluster TLS profile * VerifyIngressNodeFirewallTLSComplianceInPod() - Verify daemon TLS compliance * TLS cipher suite validation against expected profiles * Machine Config Pool rollout monitoring * Feature gate management for TLSAdherence API * Refactored to use untyped controller-runtime client (no client-go dependency) - Add TLS Profile Compliance test suite in test/e2e/functional/tests/e2e.go: * Test Intermediate profile with LegacyAdheringComponentsOnly * Test Modern profile with LegacyAdheringComponentsOnly * Test Modern profile with StrictAllComponents * Auto-skip on non-OpenShift clusters or unsupported API versions - Update test/e2e/client/client.go: * Add OpenShift config v1 and machineconfiguration v1 to controller-runtime scheme * Enables untyped client access to OpenShift-specific APIs - Update dependencies: * Uses only github.com/openshift/api (no client-go dependency) * Removed 521 vendor files (~55,000 lines of unused code) * Update go.mod and go.sum - Update Makefile to increase test timeout to 90m (TLS tests require ~42m for MCP rollouts) Technical Implementation: - Uses controller-runtime's untyped client.Client for all Kubernetes API access - Avoids github.com/openshift/client-go dependency by using controller-runtime patterns - All OpenShift config resources accessed via controller-runtime client with proper scheme registration Test Results: All 3 TLS compliance tests passed successfully: ✓ Intermediate TLS Profile with LegacyAdheringComponentsOnly ✓ Modern TLS Profile with LegacyAdheringComponentsOnly ✓ Modern TLS Profile with StrictAllComponents Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit adds comprehensive end-to-end tests to validate TLS profile
compliance for the ingress-node-firewall daemon's kube-rbac-proxy endpoint.
Changes:
- Add test/e2e/tls/tls.go with TLS compliance testing utilities (795 lines)
* ConfigureTLSProfileWithAdherence: Configures cluster TLS profile
* VerifyIngressNodeFirewallTLSComplianceInPod: Validates TLS behavior
* Waits for MachineConfigPool rollouts and cluster stabilization
* Handles transient connection issues with retry logic
- Add TLS Profile Compliance test context to test/e2e/functional/tests/e2e.go
* Tests 3 profile/adherence combinations:
- Intermediate + LegacyAdheringComponentsOnly (TLS 1.2 & 1.3 work)
- Modern + LegacyAdheringComponentsOnly (TLS 1.2 & 1.3 work)
- Modern + StrictAllComponents (TLS 1.3 only, 1.2 rejected)
Test Implementation:
- Uses OpenShift TLSAdherence feature gate and APIServer configuration
- Verifies actual TLS protocol behavior via curl in pod exec
- Properly waits for cluster components (MCPs, operators, nodes) to stabilize
- Follows e2e golden rules (self-contained, no downstream references)
Code Quality:
- Modern Kubernetes 1.21+ APIs (wait.PollUntilContextTimeout)
- Uses standard utilities (podutil.IsPodReady)
- Zero dead code, zero duplications
- Proper error handling with context-aware operations
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit addresses all 6 CodeRabbit review comments to improve code quality, prevent bugs, and ensure correct test behavior. Fixes: 1. Increase functional test timeout from 90m to 180m - Aligns timeout with TLS test worst-case timing (60m waits in sequence) - Prevents go test SIGKILL that would lose JUnit reports - Addresses comment #3754883537 2. Fix namespace hardcoding bug - Remove hardcoded OpenShiftNameSpace constant - Use dynamic OperatorNameSpace variable (respects OO_INSTALL_NAMESPACE) - Fixes TLS test to work in all environments (CI, local dev, custom namespaces) - Addresses comment #3754883539 3. Document cluster state mutations - Add warning about FeatureGate.Spec.FeatureSet -> CustomNoUpgrade (irreversible) - Document APIServer TLS profile changes not being restored - Explain why restoration not implemented (ephemeral cluster, expensive, irreversible) - Warn local developers about permanent cluster modifications - Addresses comment #3754883543 4. Add nil pointer checks for TLSSecurityProfile - Guard determineTLSTestBehavior against nil TLSSecurityProfile - Guard log statement in VerifyIngressNodeFirewallTLSComplianceInPod - Prevents panics, makes exported functions defensive - Consistent with verifyAPIServerTLSProfile which already has nil check - Addresses comment #3754883546 5. Fix error wrapping in execCommandInPodWithRetry - Rename err -> findErr for clarity - Wrap pollErr (timeout error) instead of closure-scoped findErr - Fixes misleading error messages like "failed to find ready pod: %!w(<nil>)" - Proper Go error wrapping idiom with full context - Addresses comment #3754883566 6. Fix critical error discard bug in testTLS12Connection - Check error in TLS 1.2 rejection test instead of discarding with _ - Distinguish infrastructure failure (exec didn't run) from TLS rejection - Prevents FALSE POSITIVES where strictest assertion passes without testing - Modern + StrictAllComponents test can now properly fail if pod exec fails - Addresses comment #3754883574 Impact: - 4 bugs fixed (namespace, nil pointer, error wrapping, error discard) - 1 critical fix preventing false positives in most important assertion - 1 defensive improvement (timeout alignment) - 1 documentation enhancement (cluster state warning) All changes verified to compile successfully. Co-Authored-By: CodeRabbit AI <noreply@coderabbit.ai>
This commit addresses all review feedback from PR openshift#766: 1. Combine wasteful test setup (tpantelis) - Merge 3 separate It specs into single It spec - Reduces test time by ~2-4 hours (avoids redundant MCP rollouts) 2. Use configv1 typed constants (tpantelis) - Replace string literals with configv1.TLSProfileType - Replace string literals with configv1.TLSAdherencePolicy - Update all function signatures and comparisons 3. Fix pod readiness check (tpantelis) - Use podutil.IsPodReady() instead of only checking Phase==Running - Prevents race conditions by ensuring pod is actually ready 4. Simplify node readiness check (tpantelis) - Use slices.ContainsFunc() for cleaner code 5. Fix step numbering (tpantelis) - Renumber steps to start from 1 instead of 2 6. Use errors.As() for error type checking (tpantelis) - Replace type assertion with errors.As() - Future-proof for wrapped errors 7. Rename variable for clarity (tpantelis) - Rename featureGateEnabled to alreadyEnabled 8. Add [OCPFeatureGate:TLSAdherence][Serial] tags (CodeRabbit) - Ensures tests run on dedicated, disposable CI infrastructure - Update documentation to match openshift/origin#31500 pattern Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
TLS Test Changes: - Reorder TLS profile test cases for better coverage - Replace Intermediate+LegacyAdheringComponentsOnly with Intermediate+StrictAllComponents - Update MCP rollout logic to handle Intermediate profile with StrictAllComponents - New test order: Modern+Legacy, Modern+Strict, Intermediate+Strict Build Changes: - Bump Golang version from 1.25 to 1.26 in Dockerfiles Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit fixes three critical issues in the TLS profile compliance
e2e tests that caused failures when testing TLS 1.2 rejection:
1. Remove exit code 35 from transient error list
- Exit code 35 is an SSL protocol error (handshake failure), not
an infrastructure/network issue
- When testing TLS 1.2 rejection, this is the expected behavior
- Previously caused 3 retries with empty output and test failure
2. Add RestartDaemonPods() function
- Ensures kube-rbac-proxy containers pick up new TLS configuration
- Deletes old daemon pods and waits for new pods with different UIDs
- Critical for testing multiple TLS profiles in sequence
- Without this, pods retain previous TLS settings
3. Enhance TLS 1.2 rejection test logic
- Properly handle SSL errors when expecting connection rejection
- Check for SSL/alert messages in curl verbose output
- Return success when TLS handshake fails as expected
- Only fail if connection succeeds when it should be rejected
Test Results:
- Modern + LegacyAdheringComponentsOnly: PASS (TLS 1.2 & 1.3 work)
- Modern + StrictAllComponents: PASS (TLS 1.3 only, 1.2 rejected)
- Intermediate + StrictAllComponents: PASS (TLS 1.2 & 1.3 work)
Signed-off-by: Wei Liang <weliang@redhat.com>
Fix two test reliability issues in TLS 1.2 rejection verification: 1. Recognize exit codes 35/60 as valid TLS rejection When TLS 1.2 is correctly rejected by StrictAllComponents policy, curl exits with code 35 (SSL handshake failure). The test previously required non-empty output to recognize this as success, causing false failures when curl produced minimal output. 2. Add 30-second propagation delay after pod restart After restarting daemon pods, the new TLS configuration needs time to propagate to kube-rbac-proxy before testing can proceed reliably. Tested with all three profile combinations: - Modern + LegacyAdheringComponentsOnly: TLS 1.2/1.3 both allowed - Modern + StrictAllComponents: TLS 1.3 only, TLS 1.2 rejected - Intermediate + StrictAllComponents: TLS 1.2/1.3 both allowed Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
The 30-second wait was insufficient when transitioning between StrictAllComponents profiles (e.g., Modern→Intermediate). The daemon pods' kube-rbac-proxy containers need more time to pick up the new TLS configuration in strict mode. Fixes test failure in Intermediate + StrictAllComponents test case where TLS 1.2 was incorrectly rejected due to residual Modern profile enforcement. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…ness Replace the fixed 60-second sleep with an intelligent polling mechanism that waits for TLS configuration to actually propagate to restarted pods. Changes: - Add waitForTLSConfigurationReady() that polls every 5 seconds - Tests TLS 1.3 connection (supported by all profiles) to verify readiness - Timeout after 2 minutes if configuration doesn't propagate - Returns immediately once TLS is ready (no unnecessary waiting) Benefits: - More robust: verifies actual configuration state vs. arbitrary delay - More efficient: waits only as long as needed (typically 10-30 seconds) - Better error handling: explicit timeout with clear error message Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add -timeout 180m to go test commands in openshift-ci/run_e2e.sh to prevent test timeout failures. The TLS compliance tests require extended time for MachineConfigPool rollouts (up to 60 minutes), node stability checks (60 minutes), and operator settling (60 minutes), totaling ~130-140 minutes in worst case. The Makefile was already updated with this timeout in commit 7bb5452, but the CI script bypassed it and used Go's default 10-minute timeout, causing: panic: test timed out after 10m0s This fix aligns the CI script timeout with the Makefile configuration. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
c40a325 to
f0b5740
Compare
openshift-ci-robot
removed
the
verified
|
/lgtm |
|
/verified by weliang |
openshift-ci-robot
added
the
verified
|
@weliang1: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship, tpantelis, weliang1 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
The job of ci/prow/ingress-node-firewall-e2e-metal-ipi failed, but TLS case passed in: https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_ingress-node-firewall/766/pull-ci-openshift-ingress-node-firewall-master-ingress-node-firewall-e2e-metal-ipi/2087961678955155456/artifacts/ingress-node-firewall-e2e-metal-ipi/baremetalds-ingress-node-firewall-e2e-test/build-log.txt @danwinship, could you overwrite the job of "ingress-node-firewall-e2e-metal-ipi"? |
|
/test ingress-node-firewall-e2e-metal-ipi |
|
@asood-rh My TLS case passed in: https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_ingress-node-firewall/766/pull-ci-openshift-ingress-node-firewall-master-ingress-node-firewall-e2e-metal-ipi/2087961678955155456/artifacts/ingress-node-firewall-e2e-metal-ipi/baremetalds-ingress-node-firewall-e2e-test/build-log.txt. Other TLS case cause this job failed. We can overwrite this job to merge testing PR. |
|
/override ci/prow/ingress-node-firewall-e2e-metal-ipi |
|
@danwinship: Overrode contexts on behalf of danwinship: ci/prow/ingress-node-firewall-e2e-metal-ipi DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@weliang1: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
5 participants
Summary
This PR adds comprehensive end-to-end tests to validate TLS profile compliance for the ingress-node-firewall daemon's kube-rbac-proxy endpoint on port 9301.
What's Changed
New Files
ConfigureTLSProfileWithAdherence: Configures OpenShift cluster TLS profile and adherence policyVerifyIngressNodeFirewallTLSComplianceInPod: Validates actual TLS protocol behaviorModified Files
Testing
To run the tests:
make test-functional FOCUS='-ginkgo.focus="TLS Profile Compliance"'Requirements:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores