Add TLS Profile Compliance e2e test for CNO and networking components - #3074
Add TLS Profile Compliance e2e test for CNO and networking components#3074weliang1 wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
[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 |
Summary by CodeRabbit
WalkthroughAdds a Kubernetes e2e client and a TLS compliance test framework covering TLS profile configuration, cluster stabilization, shared scanner management, component verification, OVN database checks, and four profile scenarios. ChangesTLS profile compliance
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ComplianceTest
participant APIServer
participant OperatorWorkloads
participant SharedScannerPod
ComplianceTest->>APIServer: apply TLS profile or adherence
APIServer-->>ComplianceTest: profile state changes
ComplianceTest->>OperatorWorkloads: wait for stability and restart workloads
OperatorWorkloads-->>ComplianceTest: workloads ready
ComplianceTest->>SharedScannerPod: run TLS endpoint scans
SharedScannerPod-->>ComplianceTest: scan results
ComplianceTest->>OperatorWorkloads: verify TLS arguments and connections
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (5 errors, 3 warnings)
✅ Passed checks (16 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in :\n\tgithub.com/Masterminds/semver@v1.5.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/Masterminds/sprig/v3@v3.2.3: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/containernetworking/cni@v1.3.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/ghodss/yaml@v1.0.1-0.20190212211648-25d852aebe32: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/go-bindata/go-bindata@v3.1.2+incompatible: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/onsi/gomega@v1.39.1: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/ope ... [truncated 17377 characters] ... red in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/gengo/v2@v2.0.0-20251215205346-5ee0d033ba5b: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/kms@v0.35.2: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/kube-aggregator@v0.35.1: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tsigs.k8s.io/randfill@v1.0.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tsigs.k8s.io/structured-merge-diff/v6@v6.3.2: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n" 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: 12
🧹 Nitpick comments (5)
test/EGRESS_IP_HEALTH_SERVER_ANALYSIS.md (1)
165-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd languages to the fenced code blocks.
Use an appropriate language such as
text,console, oryamlafter each opening fence so markdownlint does not report MD040.
test/EGRESS_IP_HEALTH_SERVER_ANALYSIS.md#L165-L167: annotate the warning block.test/OVN_COMPONENTS_SHOULD_WE_TEST.md#L198-L200: annotate the warning block.This follows the supplied markdownlint findings.
🤖 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/EGRESS_IP_HEALTH_SERVER_ANALYSIS.md` around lines 165 - 167, Annotate the fenced warning blocks with an appropriate language identifier, such as text or console, in both test/EGRESS_IP_HEALTH_SERVER_ANALYSIS.md lines 165-167 and test/OVN_COMPONENTS_SHOULD_WE_TEST.md lines 198-200; no other content changes are needed.Source: Linters/SAST tools
test/e2e/tls/stability.go (1)
116-160: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
WaitForClusterOperatorsStableparsesoc get coby positional column.Relying on
awk '{if ($4 != "False")...}'for the Progressing column is brittle againstocoutput column changes/wrapping. This is acceptable for a test helper but worth noting. Not blocking.🤖 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/stability.go` around lines 116 - 160, Update WaitForClusterOperatorsStable to identify the Progressing condition by named or structured fields instead of assuming it is column 4 in oc output. Preserve the existing retry behavior, operator logging, and stable result when every operator has Progressing=False.test/e2e/tls/scanner.go (1)
57-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePin the scanner base image instead of
:latest.
ScannerImage = "...ubi9/ubi:latest"(utils.go) plus theclients/ocp/latestdownload make scanner builds non-reproducible and subject to silent breakage. Pin to a specific UBI9 digest/tag and a fixedocversion.🤖 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/scanner.go` at line 57, Update the scanner image configuration symbol ScannerImage in utils.go to use a specific immutable UBI9 digest or fixed tag instead of :latest, and change the clients/ocp/latest download reference to a fixed oc version. Keep both scanner build inputs pinned so builds remain reproducible.test/e2e/tls/waiting.go (2)
369-369: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLocal
errorsslice shadows the importederrorspackage.
var errors []stringshadowsk8s.io/apimachinery/pkg/api/errorswithinVerifyPodsHealthy. Harmless today (the package isn't referenced here), but it's a latent trap if error-handling is later added to this function. Rename to e.g.errMsgs.🤖 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/waiting.go` at line 369, Rename the local errors slice in VerifyPodsHealthy to errMsgs (or an equivalent non-conflicting name), and update all references within that function while preserving its existing behavior.
27-39: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffUse context-aware polling
wait.PollImmediateignoresctx, so these loops can keep running past cancellation/deadline even though the API calls already usectx. Switch thewaiting.go,stability.go, andscanner.gocall sites towait.PollUntilContextTimeout(ctx, ...)likehelpers.godoes.🤖 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/waiting.go` around lines 27 - 39, Replace the wait.PollImmediate call sites with wait.PollUntilContextTimeout in test/e2e/tls/waiting.go (lines 27-39), test/e2e/tls/stability.go (line 24), and test/e2e/tls/scanner.go (line 161), passing the existing ctx and preserving each poll interval, timeout, condition, and result handling. Follow the context-aware pattern used by helpers.go so cancellation and deadlines stop every polling loop.
🤖 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/client/client.go`:
- Around line 77-83: Update the client.New error handling in New to panic or
otherwise fail immediately with the returned error, matching the other failure
paths instead of returning nil. Preserve successful client assignment to
clientSet.Client and ensure init cannot leave the package-global Client unset
after construction failure.
In `@test/e2e/tls_profile_test.go`:
- Around line 46-58: Prevent Scenario 3 from calling VerifyStrictAdherence after
VerifyModernLegacyAdherence fails. Track the Scenario 2 result and either fail
fast or skip the Strict verification when Modern is not successfully
established, while preserving the existing failure reporting.
- Around line 29-34: Update the deferred cleanup block in the test to capture
the error returned by tlstest.RestoreBaselineProfile() and report any failure
through the test’s logging or failure mechanism instead of discarding it.
Preserve the existing cleanup order and scanner namespace cleanup behavior.
- Around line 17-20: Propagate a deadline-aware context from
test/e2e/tls_profile_test.go lines 17-20 through the TLS helpers and their
callers. Update test/e2e/tls/verify.go lines 17-35 and 306-320 and
test/e2e/tls/ovn_database.go lines 107-111 and 157-167 so retry sleeps terminate
on cancellation, and change queryOVNDBProtocol to use CommandContext with the
propagated context instead of osexec.Command.
In `@test/e2e/tls/ovn_database.go`:
- Around line 111-114: Update the error handling after CombinedOutput in the
relevant query function to avoid embedding raw command output in the returned
error. Replace it with a sanitized, truncated diagnostic while preserving the
container name and command error context.
- Around line 60-74: Update the NB and SB protocol validation around
queryOVNDBProtocol to parse ssl_protocols into its comma/space-delimited
protocol values and require an exact match with the expected protocol set,
rather than using strings.Contains. Preserve the existing query error handling
and mismatch reporting while rejecting additional or missing protocols.
In `@test/e2e/tls/scanner.go`:
- Around line 66-68: Update the scanner bootstrap around the oc download to
select the client archive for the node’s architecture instead of hardcoding
x86_64, including mappings for supported arm64, ppc64le, and s390x values.
Remove runtime dependence on mirror.openshift.com and other GitHub downloads by
using a pre-baked image or equivalent repository-provided artifacts for
disconnected CI.
In `@test/e2e/tls/test/TLS_TEST_FIX_SUMMARY.md`:
- Around line 194-214: The rerun guidance in TLS_TEST_FIX_SUMMARY.md uses the
obsolete TestMultusTLSProfileCompliance target and stale step numbering. Update
the command to run TestNetworkingTLSProfileCompliance, and revise the monitoring
and success-criteria references to reflect the added OVN database Step 4 and the
resulting TLS test steps.
In `@test/e2e/tls/waiting.go`:
- Around line 439-444: Update the readiness check around cmd.Run to construct
the endpoint with net.JoinHostPort(podIP, "9301") before formatting the openssl
command, ensuring both IPv4 and IPv6 pod addresses are valid while preserving
the existing timeout and error behavior.
In `@test/NETWORK_NODE_IDENTITY_TLS_ANALYSIS.md`:
- Around line 280-290: Fix both multi-line curl examples in the TLS analysis by
removing trailing spaces and inline comments after the continuation backslashes.
Move the StrictAllComponents behavior comments above each command or after the
complete command, preserving the intended TLS 1.2 failure and TLS 1.3 success
checks.
In `@test/OVN_COMPONENTS_TLS_COVERAGE_ANALYSIS.md`:
- Around line 9-15: Synchronize the four analyses with the implemented
VerifyOVNDatabaseTLS database checks: in
test/OVN_COMPONENTS_TLS_COVERAGE_ANALYSIS.md lines 9-15, remove the claim that
NB/SB TLS is entirely unverified; in test/EGRESS_IP_HEALTH_SERVER_ANALYSIS.md
lines 23-41, document the existing OVN database verification in current
coverage; in test/OVN_COMPONENTS_SHOULD_WE_TEST.md lines 146-180, replace the
recommendation to add database checks with their implemented status; and in
test/OVN_DATABASE_TLS_SCANNER_ANALYSIS.md lines 359-365, replace the future next
step with the implemented database-query approach. Keep the remaining uncovered
components clearly identified.
In `@test/TLS_TEST_REQUIREMENTS_UPDATE.md`:
- Around line 61-88: Update the Custom TLS Profile requirements in both
documented occurrences to include ECDHE-RSA-AES128-GCM-SHA256 alongside
TLS_AES_128_GCM_SHA256 for ovn, multus, and CNO. Ensure the verification
requirement and expectedArgs list contain both configured ciphers, while keeping
missingArgs empty.
---
Nitpick comments:
In `@test/e2e/tls/scanner.go`:
- Line 57: Update the scanner image configuration symbol ScannerImage in
utils.go to use a specific immutable UBI9 digest or fixed tag instead of
:latest, and change the clients/ocp/latest download reference to a fixed oc
version. Keep both scanner build inputs pinned so builds remain reproducible.
In `@test/e2e/tls/stability.go`:
- Around line 116-160: Update WaitForClusterOperatorsStable to identify the
Progressing condition by named or structured fields instead of assuming it is
column 4 in oc output. Preserve the existing retry behavior, operator logging,
and stable result when every operator has Progressing=False.
In `@test/e2e/tls/waiting.go`:
- Line 369: Rename the local errors slice in VerifyPodsHealthy to errMsgs (or an
equivalent non-conflicting name), and update all references within that function
while preserving its existing behavior.
- Around line 27-39: Replace the wait.PollImmediate call sites with
wait.PollUntilContextTimeout in test/e2e/tls/waiting.go (lines 27-39),
test/e2e/tls/stability.go (line 24), and test/e2e/tls/scanner.go (line 161),
passing the existing ctx and preserving each poll interval, timeout, condition,
and result handling. Follow the context-aware pattern used by helpers.go so
cancellation and deadlines stop every polling loop.
In `@test/EGRESS_IP_HEALTH_SERVER_ANALYSIS.md`:
- Around line 165-167: Annotate the fenced warning blocks with an appropriate
language identifier, such as text or console, in both
test/EGRESS_IP_HEALTH_SERVER_ANALYSIS.md lines 165-167 and
test/OVN_COMPONENTS_SHOULD_WE_TEST.md lines 198-200; no other content changes
are needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 28d8122e-7703-4bb1-a8a5-84e504624a2c
📒 Files selected for processing (21)
test/EGRESS_IP_HEALTH_SERVER_ANALYSIS.mdtest/NETWORK_NODE_IDENTITY_TLS_ANALYSIS.mdtest/OVN_COMPONENTS_SHOULD_WE_TEST.mdtest/OVN_COMPONENTS_TLS_COVERAGE_ANALYSIS.mdtest/OVN_DATABASE_TESTING_IMPLEMENTATION.mdtest/OVN_DATABASE_TLS_SCANNER_ANALYSIS.mdtest/TLS_REQUIREMENTS_CONFIRMED.mdtest/TLS_TEST_FINAL_REQUIREMENTS.mdtest/TLS_TEST_REQUIREMENTS_UPDATE.mdtest/e2e/client/client.gotest/e2e/tls/helpers.gotest/e2e/tls/logger.gotest/e2e/tls/ovn_database.gotest/e2e/tls/profiles.gotest/e2e/tls/scanner.gotest/e2e/tls/stability.gotest/e2e/tls/test/TLS_TEST_FIX_SUMMARY.mdtest/e2e/tls/utils.gotest/e2e/tls/verify.gotest/e2e/tls/waiting.gotest/e2e/tls_profile_test.go
| var ( | ||
| cs = testclient.Client | ||
| ctx = context.Background() | ||
| operatorNS = "openshift-multus" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files test/e2e/tls_profile_test.go test/e2e/tls/verify.go test/e2e/tls/ovn_database.go
printf '\n--- outline: test/e2e/tls_profile_test.go ---\n'
ast-grep outline test/e2e/tls_profile_test.go --view expanded || true
printf '\n--- outline: test/e2e/tls/verify.go ---\n'
ast-grep outline test/e2e/tls/verify.go --view expanded || true
printf '\n--- outline: test/e2e/tls/ovn_database.go ---\n'
ast-grep outline test/e2e/tls/ovn_database.go --view expanded || true
printf '\n--- tls_profile_test.go lines 1-120 ---\n'
sed -n '1,120p' test/e2e/tls_profile_test.go
printf '\n--- verify.go lines 1-420 ---\n'
sed -n '1,420p' test/e2e/tls/verify.go
printf '\n--- ovn_database.go lines 1-260 ---\n'
sed -n '1,260p' test/e2e/tls/ovn_database.goRepository: openshift/cluster-network-operator
Length of output: 29178
Propagate cancellation through the TLS test flow. test/e2e/tls_profile_test.go starts from context.Background(), and the retry loops in test/e2e/tls/verify.go and test/e2e/tls/ovn_database.go keep sleeping after cancellation. queryOVNDBProtocol also uses osexec.Command, so a timed-out run can keep blocking in oc exec. Thread a deadline-aware context through these helpers, make the sleeps cancellable, and switch the subprocess call to CommandContext.
📍 Affects 3 files
test/e2e/tls_profile_test.go#L17-L20(this comment)test/e2e/tls/verify.go#L17-L35test/e2e/tls/verify.go#L306-L320test/e2e/tls/ovn_database.go#L107-L111test/e2e/tls/ovn_database.go#L157-L167
🤖 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_profile_test.go` around lines 17 - 20, Propagate a
deadline-aware context from test/e2e/tls_profile_test.go lines 17-20 through the
TLS helpers and their callers. Update test/e2e/tls/verify.go lines 17-35 and
306-320 and test/e2e/tls/ovn_database.go lines 107-111 and 157-167 so retry
sleeps terminate on cancellation, and change queryOVNDBProtocol to use
CommandContext with the propagated context instead of osexec.Command.
Source: Path instructions
| 1. **Re-run Full Test Suite:** | ||
| ```bash | ||
| go test -timeout 90m -v ./test/e2e -run="TestMultusTLSProfileCompliance" \ | ||
| 2>&1 | tee /tmp/tls_test_output_fixed.log | ||
| ``` | ||
|
|
||
| 2. **Monitor for Step 3 Success:** | ||
| - Watch for "TLS args verification" to pass | ||
| - Confirm Steps 4-7 execute | ||
|
|
||
| 3. **Validate New OVN Code:** | ||
| - Verify Step 6 (OVN TLS 1.2 test) executes and fails as expected | ||
| - Verify Step 7 (OVN TLS 1.3 test) executes and succeeds as expected | ||
|
|
||
| ### Success Criteria | ||
|
|
||
| ✅ All 4 scenarios complete successfully | ||
| ✅ Step 3 passes in all scenarios | ||
| ✅ Steps 4-5 execute (Multus TLS connection tests) | ||
| ✅ Steps 6-7 execute (OVN-Kubernetes TLS connection tests - NEW) | ||
| ✅ Total test time: ~50-60 minutes (including stabilization waits) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the rerun command and step references.
TestMultusTLSProfileCompliance does not match the added TestNetworkingTLSProfileCompliance, so this command may run zero tests. The listed TLS-test steps are also stale after the OVN database Step 4 was added.
🤖 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/test/TLS_TEST_FIX_SUMMARY.md` around lines 194 - 214, The rerun
guidance in TLS_TEST_FIX_SUMMARY.md uses the obsolete
TestMultusTLSProfileCompliance target and stale step numbering. Update the
command to run TestNetworkingTLSProfileCompliance, and revise the monitoring and
success-criteria references to reflect the added OVN database Step 4 and the
resulting TLS test steps.
| ```bash | ||
| # Test TLS connection to webhook from inside same pod | ||
| oc exec -n openshift-network-node-identity network-node-identity-XXX -c webhook -- \ | ||
| curl -v --cacert /etc/webhook-cert/ca.crt \ | ||
| --tls-max 1.2 \ # Should fail with StrictAllComponents | ||
| https://127.0.0.1:9743/validate-pod | ||
|
|
||
| oc exec -n openshift-network-node-identity network-node-identity-XXX -c webhook -- \ | ||
| curl -v --cacert /etc/webhook-cert/ca.crt \ | ||
| --tlsv1.3 \ # Should succeed with StrictAllComponents | ||
| https://127.0.0.1:9743/validate-pod |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the broken multi-line curl examples.
The continuation backslashes are followed by spaces and inline comments, so the shell does not continue onto the URL line. Move comments above the command or place them after the completed command.
Proposed fix
- --tls-max 1.2 \ # Should fail with StrictAllComponents
- https://127.0.0.1:9743/validate-pod
+# Should fail with StrictAllComponents
+curl -v --cacert /etc/webhook-cert/ca.crt --tls-max 1.2 \
+ https://127.0.0.1:9743/validate-pod🤖 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/NETWORK_NODE_IDENTITY_TLS_ANALYSIS.md` around lines 280 - 290, Fix both
multi-line curl examples in the TLS analysis by removing trailing spaces and
inline comments after the continuation backslashes. Move the StrictAllComponents
behavior comments above each command or after the complete command, preserving
the intended TLS 1.2 failure and TLS 1.3 success checks.
Address all critical and important issues identified in PR openshift#3074 review: Critical Fixes: - Fix client.New error handling to panic instead of returning nil - Fix test scenario logic to skip Strict when Modern prerequisite fails - Fix IPv6 address formatting using net.JoinHostPort for dual-stack support - Fix TLS protocol validation to use exact set matching instead of substring - Add multi-architecture support for scanner (x86_64/arm64/ppc64le/s390x) - Replace wait.PollImmediate with context-aware wait.PollUntilContextTimeout Important Fixes: - Add error handling for RestoreBaselineProfile cleanup - Sanitize command output in error messages to avoid exposing internal endpoints - Rename errors variable to errMsgs to avoid shadowing errors package Changes: - test/e2e/client/client.go: Panic on client.New failure - test/e2e/tls_profile_test.go: Add Modern->Strict dependency + cleanup errors - test/e2e/tls/waiting.go: IPv6 support + variable rename + context-aware polling - test/e2e/tls/ovn_database.go: Exact protocol matching + sanitized errors - test/e2e/tls/scanner.go: Multi-arch support + context-aware polling - test/e2e/tls/stability.go: Context-aware polling - test/e2e/tls/utils.go: Code formatting - test/e2e/tls/verify.go: Code formatting Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This PR adds comprehensive end-to-end testing for TLS profile compliance across OpenShift networking components including CNO, Multus, and OVN-Kubernetes. Test Coverage: - Baseline TLS configuration (LegacyAdheringComponentsOnly) - Modern TLS Profile with LegacyAdheringComponentsOnly - Strict TLS Profile (StrictAllComponents) - Custom TLS Profile with specific ciphers Components Verified: - Cluster Network Operator (CNO) - Multus CNI DaemonSet - OVN-Kubernetes control-plane and node components - OVN NB/SB database TLS configuration Key Features: - Automated TLS scanner pod for connection testing - Container TLS arguments verification - OVN database TLS protocol validation - Cluster stability checks during profile transitions - Comprehensive error reporting and logging Test Infrastructure: - Reusable scanner pod with persistent namespace - Dynamic TLS configuration based on cluster profile - Retry logic for eventual consistency - Multi-architecture support (x86_64/arm64/ppc64le/s390x) - IPv6 and dual-stack networking support Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
fc68063 to
e5127d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/waiting.go`:
- Around line 21-25: Remove the process/history comments from the helper
declarations, including the “These were originally…” consolidation note and each
“Refactored: Added ctx parameter…” note near GetDeploymentWithRetry and the
corresponding helpers. Preserve comments that describe current behavior, such as
the retry-purpose documentation.
- Around line 116-119: Update the four getMCPConditionStatus calls in the
waiting loop to capture their returned errors instead of discarding them. Log
each failure, or aggregate and return the errors using the standard-library
errors package while preserving any existing Kubernetes errors alias, so
oc/auth/API failures are surfaced rather than treated as false conditions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f5c21130-67b7-4020-bb0c-fbea4c78a287
📒 Files selected for processing (11)
test/e2e/client/client.gotest/e2e/tls/helpers.gotest/e2e/tls/logger.gotest/e2e/tls/ovn_database.gotest/e2e/tls/profiles.gotest/e2e/tls/scanner.gotest/e2e/tls/stability.gotest/e2e/tls/utils.gotest/e2e/tls/verify.gotest/e2e/tls/waiting.gotest/e2e/tls_profile_test.go
🚧 Files skipped from review as they are similar to previous changes (10)
- test/e2e/tls/logger.go
- test/e2e/tls_profile_test.go
- test/e2e/tls/utils.go
- test/e2e/tls/stability.go
- test/e2e/client/client.go
- test/e2e/tls/ovn_database.go
- test/e2e/tls/profiles.go
- test/e2e/tls/helpers.go
- test/e2e/tls/verify.go
- test/e2e/tls/scanner.go
| // Deployment and DaemonSet helper functions | ||
| // These were originally in separate packages but consolidated here to follow the "minimal changes" golden rule | ||
|
|
||
| // GetDeploymentWithRetry retrieves a deployment with retry logic | ||
| // Refactored: Added ctx parameter for proper context propagation (was using context.Background()) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove process/history comments that don't add value.
Lines 22 and 25 (and the identical Refactored: Added ctx parameter... at Lines 43, 58, 76) describe the refactoring history rather than the code's behavior. They add noise without value and will go stale.
♻️ Suggested trim
-// Deployment and DaemonSet helper functions
-// These were originally in separate packages but consolidated here to follow the "minimal changes" golden rule
+// Deployment and DaemonSet helper functions
// GetDeploymentWithRetry retrieves a deployment with retry logic
-// Refactored: Added ctx parameter for proper context propagation (was using context.Background())
func GetDeploymentWithRetry(cs *testclient.ClientSet, ctx context.Context, namespace, name string, interval, timeout time.Duration) (*appsv1.Deployment, error) {Apply the same removal at Lines 43, 58, and 76.
As per coding guidelines: "Flag obvious comment slop... Fail unless comments add genuine value."
📝 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.
| // Deployment and DaemonSet helper functions | |
| // These were originally in separate packages but consolidated here to follow the "minimal changes" golden rule | |
| // GetDeploymentWithRetry retrieves a deployment with retry logic | |
| // Refactored: Added ctx parameter for proper context propagation (was using context.Background()) | |
| // Deployment and DaemonSet helper functions | |
| // GetDeploymentWithRetry retrieves a deployment with retry logic |
🤖 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/waiting.go` around lines 21 - 25, Remove the process/history
comments from the helper declarations, including the “These were originally…”
consolidation note and each “Refactored: Added ctx parameter…” note near
GetDeploymentWithRetry and the corresponding helpers. Preserve comments that
describe current behavior, such as the retry-purpose documentation.
Source: Coding guidelines
| masterUpdating, _ := getMCPConditionStatus("master", "Updating") | ||
| masterUpdated, _ := getMCPConditionStatus("master", "Updated") | ||
| workerUpdating, _ := getMCPConditionStatus("worker", "Updating") | ||
| workerUpdated, _ := getMCPConditionStatus("worker", "Updated") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Don't silently discard getMCPConditionStatus errors.
The four _ discards mean a failing oc invocation (auth loss, transient API error) is indistinguishable from "condition not True". The loop then reports misleading Updating/Updated status and keeps retrying until MCPReadyTimeout instead of surfacing the real failure. At minimum, capture and log the errors so failures are diagnosable.
🛠️ Example
- masterUpdating, _ := getMCPConditionStatus("master", "Updating")
- masterUpdated, _ := getMCPConditionStatus("master", "Updated")
- workerUpdating, _ := getMCPConditionStatus("worker", "Updating")
- workerUpdated, _ := getMCPConditionStatus("worker", "Updated")
+ masterUpdating, err1 := getMCPConditionStatus("master", "Updating")
+ masterUpdated, err2 := getMCPConditionStatus("master", "Updated")
+ workerUpdating, err3 := getMCPConditionStatus("worker", "Updating")
+ workerUpdated, err4 := getMCPConditionStatus("worker", "Updated")
+ if err := errors.Join(err1, err2, err3, err4); err != nil {
+ return false, fmt.Sprintf("Failed to read MCP conditions: %v", err), nil
+ }(errors.Join requires the stdlib errors package, not the k8s errors alias already imported — adjust accordingly.)
As per path instructions: "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/tls/waiting.go` around lines 116 - 119, Update the four
getMCPConditionStatus calls in the waiting loop to capture their returned errors
instead of discarding them. Log each failure, or aggregate and return the errors
using the standard-library errors package while preserving any existing
Kubernetes errors alias, so oc/auth/API failures are surfaced rather than
treated as false conditions.
Source: Path instructions
|
/test lint |
|
@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
This commit introduces a comprehensive end-to-end test suite to verify that the Cluster Network Operator (CNO) properly propagates TLS profile configurations from the cluster-wide APIServer resource to all managed networking components.
Test Coverage:
CNO-Managed Components Verified:
Test Workflow:
Implementation Details:
Test Structure: