[NE-2821] Implement e2e test for TLSAdherence on Gateway API - #31415
[NE-2821] Implement e2e test for TLSAdherence on Gateway API#31415rikatz wants to merge 4 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rikatz 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds Gateway API LoadBalancer TLS validation with prerequisite checks, resource lifecycle handling, TLS probing, and integration into disruptive API server TLS profile reconciliation. ChangesGateway TLS adherence validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TLSConfigTest
participant GatewayAPI
participant GatewayController
participant LoadBalancer
participant gatewayLBTarget
TLSConfigTest->>GatewayAPI: Create GatewayClass, Secret, and HTTPS Gateway
GatewayController-->>GatewayAPI: Set Programmed=True
GatewayAPI->>LoadBalancer: Provision external address
LoadBalancer-->>TLSConfigTest: Return hostname or IP
TLSConfigTest->>gatewayLBTarget: Add Gateway TLS target
gatewayLBTarget->>LoadBalancer: Test accepted and rejected TLS profiles
LoadBalancer-->>gatewayLBTarget: Return TLS success or failure
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/extended/tls/tls_observed_config.go (1)
1296-1313: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestore
APIServeradherence before the test finishes.When Gateway API TLS validation runs, this test sets
APIServer.spec.tlsAdherence = StrictAllComponentsbut leaves no cleanup path. SincetlsAdherencecannot be unset once set, re-running the same disruptive test or another test that needs a different adherence value on the same cluster can hit a permanent API validation error instead of a retryable 409 conflict. Capture and restore the compliance-affected adherence value, or avoid modifying the immutable field unless absolutely required.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/tls/tls_observed_config.go` around lines 1296 - 1313, Update the test cleanup around setAPIServerTLSProfile so any tlsAdherence value changed to StrictAllComponents is captured before modification and restored before the test completes. Preserve the existing NoOpinion behavior for callers that do not need adherence changes, and ensure restoration uses the same APIServer update path with conflict retry.
🧹 Nitpick comments (2)
test/extended/tls/gateway_tls.go (2)
230-256: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueEphemeral test private key isn't explicitly zeroized after use.
privateKey/pemKeyhold the private key material for the self-signed test cert and are only released via GC. The crypto-security guideline for files matching*tls*calls for zeroizing key material rather than relying on the garbage collector. Given this is a throwaway, non-sensitive test certificate, the practical risk is minimal, but flagging for guideline compliance.As per path instructions, "Zeroize key material after use (no garbage-collector reliance)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/tls/gateway_tls.go` around lines 230 - 256, Explicitly zeroize the private key material in the certificate-generation flow after it is no longer needed, including the `privateKey` and any mutable key bytes represented by `pemKey`, while preserving the existing Secret creation behavior and ensuring cleanup occurs on error paths as well.Source: Path instructions
125-134: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider
tls.Dialer.DialContextinstead oftls.DialWithDialer.
DialWithDialerusescontext.Background()internally and only respects the dialer's fixed 10s timeout; it ignores thectxparameter already passed intotestTLS. Switching to(&tls.Dialer{NetDialer: dialer, Config: cfg}).DialContext(ctx, "tcp", hostPort)would let the dial respect the caller's overall test deadline/cancellation in addition to the per-dial timeout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/tls/gateway_tls.go` around lines 125 - 134, Update testTLS to use a tls.Dialer configured with the existing dialer and expected TLS configuration, then call DialContext with the provided ctx, "tcp", and hostPort. Preserve the existing connection error reporting and close the returned connection as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/extended/tls/tls_observed_config.go`:
- Around line 512-521: Ensure the gatewayFixture returned by
setupGatewayTLSTarget is cleaned up regardless of whether setup succeeds or
returns an error. Register gatewayFixture.cleanup(ctx) immediately after
setupGatewayTLSTarget returns, before branching on err, while preserving the
existing error logging and successful target registration behavior.
---
Outside diff comments:
In `@test/extended/tls/tls_observed_config.go`:
- Around line 1296-1313: Update the test cleanup around setAPIServerTLSProfile
so any tlsAdherence value changed to StrictAllComponents is captured before
modification and restored before the test completes. Preserve the existing
NoOpinion behavior for callers that do not need adherence changes, and ensure
restoration uses the same APIServer update path with conflict retry.
---
Nitpick comments:
In `@test/extended/tls/gateway_tls.go`:
- Around line 230-256: Explicitly zeroize the private key material in the
certificate-generation flow after it is no longer needed, including the
`privateKey` and any mutable key bytes represented by `pemKey`, while preserving
the existing Secret creation behavior and ensuring cleanup occurs on error paths
as well.
- Around line 125-134: Update testTLS to use a tls.Dialer configured with the
existing dialer and expected TLS configuration, then call DialContext with the
provided ctx, "tcp", and hostPort. Preserve the existing connection error
reporting and close the returned connection as before.
🪄 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: 14ea05da-bef0-4fd0-b1f2-52e8ff5ddea2
📒 Files selected for processing (2)
test/extended/tls/gateway_tls.gotest/extended/tls/tls_observed_config.go
openshift-ci
Bot
added
the
ready-for-human-review
|
/test ? |
|
/testwith openshift/origin/main/e2e-gcp-ovn-techpreview-serial-1of2 openshift/cluster-ingress-operator#1480 |
|
/testwith openshift/origin/main/e2e-gcp-ovn-techpreview-serial-2of2 openshift/cluster-ingress-operator#1480 |
|
@rikatz, |
|
@rikatz, |
|
/testwith e2e-gcp-ovn-techpreview-serial-1of2 openshift/cluster-ingress-operator#1480 |
|
@rikatz, |
|
/testwith openshift/origin/main/e2e-gcp-ovn-techpreview-serial-1of2 openshift/cluster-ingress-operator#1480 |
|
@rikatz, |
|
/testwith openshift/origin/main/e2e-gcp-ovn-techpreview-serial-1of2 openshift/cluster-ingress-operator#1480 |
|
@gcs278, |
|
/testwith openshift/origin/main/e2e-gcp-ovn openshift/api#2883 openshift/cluster-ingress-operator#1480 |
|
/testwith openshift/origin/main/e2e-aws-ovn-serial-1of2 openshift/api#2883 openshift/cluster-ingress-operator#1480 |
|
@gcs278, |
|
/testwith openshift/origin/main/e2e-aws-ovn-serial-2of2 openshift/api#2883 openshift/cluster-ingress-operator#1480 |
|
@gcs278, |
|
/testwith openshift/origin/main/e2e-gcp-ovn-techpreview-serial openshift/cluster-ingress-operator#1480 |
|
/test e2e-aws-tls-observed-config |
|
/test periodic-ci-openshift-tls-scanner-main-periodic-tls13-conformance |
|
@rikatz: The specified target(s) for The following commands are available to trigger optional jobs: Use 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. |
|
/testwith openshift/origin/main/e2e-aws-tls-observed-config openshift/cluster-ingress-operator#1480 |
|
@rikatz: The following test 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. |
|
/testwith openshift/origin/main/e2e-aws-tls-observed-config openshift/cluster-ingress-operator#1480 |
|
/testwith openshift/origin/main/e2e-aws-tls-observed-config openshift/cluster-ingress-operator#1480,openshift/api#2883 |
|
/testwith openshift/origin/main/e2e-aws-tls-observed-config openshift/cluster-ingress-operator#1480 openshift/api#2883 |
Gateway API, implemented via Istio and Sail Library can now respect centralized TLS configuration from OpenShift. This centralized configuration is implemented and respected with TLSAdherence set as strict. The current change implements an e2e test for Gateway API when TLSAdherence is available and can be used by admins to also define that Gateways provisioned through OCP Gateway API must respect a specific TLS profile
42baf75 to
48acc8a
Compare
|
/testwith openshift/origin/main/e2e-aws-tls-observed-config openshift/cluster-ingress-operator#1480 openshift/api#2883 |
Code Review — Gateway API TLS Adherence e2e TestReviewed the full diff across both files. Well-structured test code with good documentation, proper cleanup patterns, and thoughtful platform/feature-gate gating. A few findings below. 🔴 Critical — TLS version floor mismatch (likely root cause of CI failure)The The problem: With Strict enforcement, the Gateway endpoint rejects TLS 1.1/1.2 connections — so Fix: The
Consider adding a Relevant code: // tls_observed_config.go — same config for all targets including Gateway:
waitErr := waitForTLSReconciliation(oc, configChangeCtx, false, targetsWithGateway, targetTLSConfig)
// gateway_tls.go — testTLS uses whatever it's given, no Strict floor awareness:
shouldWorkDialer := &tls.Dialer{NetDialer: netDialer, Config: expected.tlsShouldWork}🟠 Medium — Not IPv6-safe address formatting
hostPort := fmt.Sprintf("%s:%s", t.address, t.port)If the LB publishes an IPv6 address, this produces 🟠 Medium — Fragile Service naming convention
serviceName := createdGateway.Name + "-" + gatewayClassNameThis assumes the ingress controller names the per-Gateway LB Service as 🟠 Medium — Immutable
|
|
Note The previously reviewed commits are no longer reachable (likely due to a force-push or rebase), so CodeRabbit is performing a full review instead of an incremental one. This review may take a little longer. |
| // unset. This suite is [Disruptive] and runs on ephemeral CI | ||
| // clusters, so permanently setting StrictAllComponents for the | ||
| // Gateway wire-level check is acceptable. | ||
| adherence = configv1.TLSAdherencePolicyStrictAllComponents |
There was a problem hiding this comment.
I don't know if this is doable and will not break other things.
|
/testwith openshift/origin/main/e2e-aws-tls-observed-config openshift/cluster-ingress-operator#1480 openshift/api#2883 |
1 similar comment
|
/testwith openshift/origin/main/e2e-aws-tls-observed-config openshift/cluster-ingress-operator#1480 openshift/api#2883 |
|
/close this is not the rightplace to add it, suggestion is to make this change as part of CIO tls-scanner |
|
@rikatz: Closed this PR. 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. |
3 participants
Gateway API, implemented via Istio and Sail Library can now respect centralized TLS configuration from OpenShift.
This centralized configuration is implemented and respected with TLSAdherence set as strict.
The current change implements an e2e test for Gateway API when TLSAdherence is available and can be used by admins to also define that Gateways provisioned through OCP Gateway API must respect a specific TLS profile
Marking as draft while openshift/cluster-ingress-operator#1480 is not merged
Summary by CodeRabbit
New Features
Tests