RHOAIENG-36997: Add comprehensive E2E tests for Gateway with OpenShift OAuth authentication #2753
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughEnables the Gateway e2e test and adds a GatewayTestCtx with hostname caching plus many public validation and helper methods for gateway, TLS, OAuth, auth-proxy, EnvoyFilter and routes; moves DataScienceCluster component-state update logic into a new TestContext method. (≈31 words) Changes
Sequence Diagram(s)sequenceDiagram
rect rgb(250,250,255)
participant Test as GatewayTestCtx
participant K8s as Kubernetes API
participant OAuth as OAuth/Auth-Proxy
participant Envoy as EnvoyFilter
participant Route as HTTPRoute/Gateway
end
Test->>K8s: Get cached gateway hostname (sync.Once)
K8s-->>Test: Hostname
alt Validate infra
Test->>K8s: Validate GatewayConfig / GatewayClass / TLS secrets / KubernetesGateway
K8s-->>Test: Resource states
end
Test->>OAuth: Validate OAuthClient & Secret
OAuth-->>Test: Client/secret present
Test->>K8s: Validate auth-proxy Deployment / Service / TLS / args
K8s-->>Test: Deployment & Service status
Test->>Envoy: Validate EnvoyFilter patches (ext_authz, Lua, clusters)
Envoy-->>Test: Patches observed
Test->>Route: Validate HTTPRoute acceptance, OAuth callback route, redirects
Route-->>Test: Route status / redirects
alt Running in-pod
Test->>K8s: Fetch service-account token
Test->>Route: Authenticated requests with token
Route-->>Test: 200 OK
else
Test->>Route: Unauthenticated requests -> expect OAuth redirect
Route-->>Test: 302/redirect
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (2)tests/e2e/test_context_test.go (4)
tests/e2e/gateway_test.go (9)
🪛 ast-grep (0.39.9)tests/e2e/gateway_test.go[warning] 568-571: MinVersion (missing-ssl-minversion-go) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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 |
GowthamShanmugam
force-pushed
the
gate_e2e_suit
branch
from
c8b15b9 to
e9c546f
Compare
![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)
tests/e2e/gateway_test.go (1)
733-749: Remove or integrate unused test function.The
ValidateKubeAuthProxySecretHashfunction is defined but never called. It's not included in thetestCasesslice at lines 74-83. Either add it to the test suite or remove it.If this validation should be part of the test suite, I can help integrate it. Should this be added as a new test case?
🧹 Nitpick comments (2)
tests/e2e/gateway_test.go (2)
74-86: Consider clarifying test case naming.The test case at line 82 is named "Validate unauthenticated access redirects to login", but the function
ValidateUnauthenticatedRedirect(lines 480-492) actually tests both unauthenticated and authenticated access flows. Consider renaming to "Validate authentication flows" for clarity.
527-562: Consider extracting hardcoded dashboard identifiers.The hardcoded strings "opendatahub" and "odh-dashboard" at lines 531-532, 537, 551 could be extracted as constants for better maintainability.
+const ( + dashboardNamespace = "opendatahub" + dashboardName = "odh-dashboard" +) + func (tc *GatewayTestCtx) waitForDashboardReady(t *testing.T) { t.Helper() - dashboardNamespace := "opendatahub" - dashboardRouteName := "odh-dashboard" + dashboardRouteName := dashboardName
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/e2e/controller_test.go(1 hunks)tests/e2e/gateway_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/e2e/controller_test.go (1)
api/services/v1alpha1/gateway_types.go (1)
GatewayServiceName(27-27)
tests/e2e/gateway_test.go (9)
api/services/v1alpha1/gateway_types.go (2)
GatewayInstanceName(30-30)GatewayConfig(105-111)internal/controller/services/gateway/gateway_support.go (16)
DefaultGatewayName(22-22)GatewayClassName(20-20)GatewayNamespace(19-19)AuthClientID(25-25)KubeAuthProxyName(29-29)KubeAuthProxyTLSName(31-31)KubeAuthProxySecretsName(30-30)OAuthCallbackRouteName(32-32)AuthProxyOAuth2Path(38-38)AuthProxyHTTPPort(35-35)AuthProxyHTTPSPort(36-36)AuthProxyMetricsPort(37-37)GatewayControllerName(21-21)EnvClientID(50-50)EnvClientSecret(51-51)EnvCookieSecret(52-52)tests/e2e/test_context_test.go (2)
TestContext(38-62)NewTestContext(76-103)tests/e2e/helper_test.go (1)
ExtractAndExpectValue(136-147)tests/e2e/resource_options_test.go (4)
WithMinimalObject(155-170)WithCondition(262-266)WithCustomErrorMsg(287-291)WithMutateFunc(241-245)pkg/cluster/gvk/gvk.go (12)
GatewayConfig(340-344)GatewayClass(346-350)Secret(160-164)Namespace(28-32)OAuthClient(370-374)Deployment(112-116)Service(172-176)HTTPRoute(364-368)Group(124-128)EnvoyFilter(328-332)KubernetesGateway(352-356)DataScienceCluster(64-68)pkg/utils/test/matchers/jq/jq_matcher.go (1)
Match(11-15)pkg/utils/test/testf/testf_support.go (1)
Transform(78-106)pkg/cluster/cluster_config.go (1)
GetDomain(97-114)
🪛 ast-grep (0.39.6)
tests/e2e/gateway_test.go
[warning] 659-662: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
// #nosec G402 -- e2e test environment requires skipping TLS verification for self-signed certificates
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🔇 Additional comments (6)
tests/e2e/gateway_test.go (5)
3-32: LGTM! Clean imports and dependencies.The imports are well-organized and appropriate for comprehensive E2E testing of Gateway with OAuth flows.
56-62: LGTM! Well-designed caching pattern.The use of
sync.Onceand cached hostname prevents redundant API calls while maintaining thread safety.
96-478: LGTM! Comprehensive validation coverage.The validation functions provide excellent E2E coverage with:
- Detailed resource existence and configuration checks
- Clear progress logging
- Descriptive error messages
- Consistent patterns using jq matchers
The thoroughness in
ValidateAuthProxyDeployment(lines 219-321) is particularly noteworthy, covering all critical deployment aspects.
672-706: LGTM! Well-designed conditional test execution.The helpers appropriately detect pod execution context and retrieve service account tokens only when available. The use of
#nosec G101is correct—these are standard Kubernetes paths, not hardcoded credentials. Clear logging helps understand when authenticated tests are skipped.
708-726: LGTM! Effective hostname caching with sync.Once.The caching pattern avoids repeated cluster API calls while ensuring thread-safe initialization. Error handling appropriately fails the test when cluster domain cannot be determined.
tests/e2e/controller_test.go (1)
135-143: LGTM! Gateway service testing correctly enabled.The uncommented line properly activates the comprehensive gateway test suite added in
gateway_test.go, integrating it with the existing Services test group.
| func (tc *GatewayTestCtx) createHTTPClient() *http.Client { | ||
| // cookiejar.New never errors with nil options, safe to ignore error | ||
| jar, _ := cookiejar.New(nil) | ||
|
|
||
| return &http.Client{ | ||
| Jar: jar, | ||
| Transport: &http.Transport{ | ||
| TLSClientConfig: &tls.Config{ | ||
| // #nosec G402 -- e2e test environment requires skipping TLS verification for self-signed certificates | ||
| InsecureSkipVerify: true, | ||
| }, | ||
| }, | ||
| // don't follow redirects so we can assert Location header | ||
| CheckRedirect: func(req *http.Request, via []*http.Request) error { | ||
| return http.ErrUseLastResponse | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
Add TLS MinVersion for defense-in-depth.
While this is a test environment with self-signed certificates, the TLS configuration should still specify a minimum TLS version for defense-in-depth. Static analysis correctly flagged this as CWE-327.
Transport: &http.Transport{
TLSClientConfig: &tls.Config{
+ MinVersion: tls.VersionTLS12,
// #nosec G402 -- e2e test environment requires skipping TLS verification for self-signed certificates
InsecureSkipVerify: true,
},
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (tc *GatewayTestCtx) createHTTPClient() *http.Client { | |
| // cookiejar.New never errors with nil options, safe to ignore error | |
| jar, _ := cookiejar.New(nil) | |
| return &http.Client{ | |
| Jar: jar, | |
| Transport: &http.Transport{ | |
| TLSClientConfig: &tls.Config{ | |
| // #nosec G402 -- e2e test environment requires skipping TLS verification for self-signed certificates | |
| InsecureSkipVerify: true, | |
| }, | |
| }, | |
| // don't follow redirects so we can assert Location header | |
| CheckRedirect: func(req *http.Request, via []*http.Request) error { | |
| return http.ErrUseLastResponse | |
| }, | |
| } | |
| } | |
| func (tc *GatewayTestCtx) createHTTPClient() *http.Client { | |
| // cookiejar.New never errors with nil options, safe to ignore error | |
| jar, _ := cookiejar.New(nil) | |
| return &http.Client{ | |
| Jar: jar, | |
| Transport: &http.Transport{ | |
| TLSClientConfig: &tls.Config{ | |
| MinVersion: tls.VersionTLS12, | |
| // #nosec G402 -- e2e test environment requires skipping TLS verification for self-signed certificates | |
| InsecureSkipVerify: true, | |
| }, | |
| }, | |
| // don't follow redirects so we can assert Location header | |
| CheckRedirect: func(req *http.Request, via []*http.Request) error { | |
| return http.ErrUseLastResponse | |
| }, | |
| } | |
| } |
🧰 Tools
🪛 ast-grep (0.39.6)
[warning] 659-662: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
// #nosec G402 -- e2e test environment requires skipping TLS verification for self-signed certificates
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🤖 Prompt for AI Agents
In tests/e2e/gateway_test.go around lines 653 to 670, the TLS config currently
only sets InsecureSkipVerify and should also set a minimum TLS version for
defense-in-depth; update the tls.Config to include MinVersion: tls.VersionTLS12
(or tls.VersionTLS13 if you require only the newest protocol) so the client
refuses connections using older TLS versions while keeping InsecureSkipVerify
for the test's self-signed certs.
e9c546f to
73e6e4c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/e2e/gateway_test.go (1)
656-673: Add TLS MinVersion for defense-in-depth.While this is a test environment with self-signed certificates, the TLS configuration should still specify a minimum TLS version. This was previously flagged and remains unaddressed.
Apply this diff:
return &http.Client{ Jar: jar, Transport: &http.Transport{ TLSClientConfig: &tls.Config{ + MinVersion: tls.VersionTLS12, // #nosec G402 -- e2e test environment requires skipping TLS verification for self-signed certificates InsecureSkipVerify: true, }, }, // don't follow redirects so we can assert Location header CheckRedirect: func(req *http.Request, via []*http.Request) error { return http.ErrUseLastResponse }, }
🧹 Nitpick comments (1)
tests/e2e/gateway_test.go (1)
40-54: Consider referencing gateway package constants directly.The local constants (gatewayConfigName, gatewayName, etc.) duplicate values from the imported
gatewaypackage. While this improves test readability, it creates a maintenance burden if source constants change. Consider whether usinggateway.DefaultGatewayNamedirectly throughout the test would be clearer and reduce duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/e2e/controller_test.go(1 hunks)tests/e2e/gateway_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/controller_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/gateway_test.go (9)
api/services/v1alpha1/gateway_types.go (2)
GatewayInstanceName(30-30)GatewayConfig(105-111)internal/controller/services/gateway/gateway_support.go (16)
DefaultGatewayName(22-22)GatewayClassName(20-20)GatewayNamespace(19-19)AuthClientID(25-25)KubeAuthProxyName(29-29)KubeAuthProxyTLSName(31-31)KubeAuthProxySecretsName(30-30)OAuthCallbackRouteName(32-32)AuthProxyOAuth2Path(38-38)AuthProxyHTTPPort(35-35)AuthProxyHTTPSPort(36-36)AuthProxyMetricsPort(37-37)GatewayControllerName(21-21)EnvClientID(50-50)EnvClientSecret(51-51)EnvCookieSecret(52-52)tests/e2e/test_context_test.go (2)
TestContext(38-62)NewTestContext(76-103)tests/e2e/helper_test.go (2)
RunTestCases(90-115)ExtractAndExpectValue(136-147)tests/e2e/resource_options_test.go (4)
WithMinimalObject(155-170)WithCondition(262-266)WithCustomErrorMsg(287-291)WithMutateFunc(241-245)pkg/cluster/gvk/gvk.go (12)
GatewayConfig(340-344)GatewayClass(346-350)Secret(160-164)Namespace(28-32)OAuthClient(370-374)Deployment(112-116)Service(172-176)HTTPRoute(364-368)Group(124-128)EnvoyFilter(328-332)KubernetesGateway(352-356)DataScienceCluster(64-68)pkg/utils/test/matchers/jq/jq_matcher.go (1)
Match(11-15)pkg/utils/test/testf/testf_support.go (1)
Transform(78-106)pkg/cluster/cluster_config.go (1)
GetDomain(97-114)
🪛 ast-grep (0.39.6)
tests/e2e/gateway_test.go
[warning] 662-665: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
// #nosec G402 -- e2e test environment requires skipping TLS verification for self-signed certificates
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: golangci-lint
🔇 Additional comments (21)
tests/e2e/gateway_test.go (21)
56-62: LGTM! Good caching design.Embedding
TestContextand caching the gateway hostname withsync.Onceavoids redundant API calls while maintaining thread safety.
64-86: Verify test coverage completeness.The test suite defines 8 test cases, but
ValidateKubeAuthProxySecretHash(lines 736-752) is not included in the list and will never execute.If this test should run, add it to the
testCasesslice:testCases := []TestCase{ {"Validate GatewayConfig creation", gatewayCtx.ValidateGatewayConfig}, {"Validate Gateway infrastructure", gatewayCtx.ValidateGatewayInfrastructure}, {"Validate OAuth client and secret creation", gatewayCtx.ValidateOAuthClientAndSecret}, {"Validate authentication proxy deployment", gatewayCtx.ValidateAuthProxyDeployment}, {"Validate OAuth callback HTTPRoute", gatewayCtx.ValidateOAuthCallbackRoute}, {"Validate EnvoyFilter creation", gatewayCtx.ValidateEnvoyFilter}, {"Validate Gateway ready status", gatewayCtx.ValidateGatewayReadyStatus}, {"Validate unauthenticated access redirects to login", gatewayCtx.ValidateUnauthenticatedRedirect}, + {"Validate kube-auth-proxy secret hash annotation", gatewayCtx.ValidateKubeAuthProxySecretHash}, }If the test is obsolete, remove the function definition at lines 736-752.
88-94: LGTM!These helper functions provide clear string formatting for OAuth redirect URL and cookie domain assertions.
96-112: LGTM! Comprehensive GatewayConfig validation.The test properly validates the certificate configuration and Ready condition.
114-160: LGTM! Thorough Gateway infrastructure validation.The test validates GatewayClass, TLS secret structure, and Gateway listener configuration including hostname matching.
162-206: LGTM! Comprehensive OAuth validation.The test properly validates the OAuthClient configuration including the redirect URI, and thoroughly checks the credentials secret including base64-decoded CLIENT_ID matching.
208-220: LGTM! Reusable deployment readiness check.The helper provides a clean way to verify deployment health across multiple tests.
222-324: LGTM! Extensive auth proxy validation.The test thoroughly validates the kube-auth-proxy deployment including container configuration, ports, environment variables, TLS mounts, and numerous OAuth2 proxy arguments. The grouped conditions (lines 237-290) provide comprehensive coverage.
326-368: LGTM! Complete OAuth callback route validation.The test validates all critical HTTPRoute aspects including parent references, path matching, backend configuration, and acceptance status.
370-442: LGTM! Comprehensive EnvoyFilter validation.The test validates all three configuration patches including ext_authz setup, Lua token forwarding, and cluster TLS configuration with proper FQDN and service CA references.
444-481: LGTM! Complete Gateway readiness validation.The test validates Gateway operational status including conditions, load balancer addresses, listener health, and HTTPRoute support.
483-495: LGTM! Well-structured end-to-end test orchestration.The function properly sequences dashboard enable/ready/test/disable with defer cleanup.
497-565: LGTM! Clean dashboard lifecycle management.The helper functions properly manage dashboard state transitions and validate deployment and route readiness.
567-573: LGTM!Simple URL construction from gateway hostname.
575-608: LGTM! Proper unauthenticated redirect validation.The test correctly validates OAuth redirect behavior including status codes, Location header, and redirect_uri parameter.
610-639: LGTM! Proper authenticated access validation.The test correctly validates service account token authentication with appropriate skip logic when not running in a pod.
641-654: LGTM! Clean gateway hostname extraction.The function properly fetches and extracts the actual gateway hostname from the cluster.
675-681: LGTM! Simple pod detection logic.The function appropriately checks for the service account token file to detect pod execution.
683-709: LGTM! Robust token retrieval with error handling.The function properly handles all error cases by returning empty string and includes appropriate logging.
711-729: LGTM! Efficient hostname caching with proper error handling.The function uses
sync.Onceto cache the cluster domain lookup and properly fails the test if the hostname cannot be determined.
731-734: LGTM!Simple FQDN construction for Kubernetes services.
GowthamShanmugam
force-pushed
the
gate_e2e_suit
branch
from
73e6e4c to
033cf21
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/e2e/gateway_test.go (1)
657-667: Also set a TLS minimum version in test client.Add MinVersion to defend against legacy protocol downgrades while keeping InsecureSkipVerify for self-signed certs in tests.
TLSClientConfig: &tls.Config{ + MinVersion: tls.VersionTLS12, // #nosec G402 -- e2e test environment requires skipping TLS verification for self-signed certificates InsecureSkipVerify: true, },
🧹 Nitpick comments (7)
tests/e2e/gateway_test.go (7)
642-655: Avoid order dependency on listeners; select the HTTPS listener explicitly.Indexing listeners[0] is brittle. Select by name to resist ordering changes.
- hostname := ExtractAndExpectValue[string](tc.g, gatewayResources[0], `.spec.listeners[0].hostname`, Not(BeNil())) + hostname := ExtractAndExpectValue[string](tc.g, gatewayResources[0], `.spec.listeners[] | select(.name == "https") | .hostname`, Not(BeNil()))
188-204: Use expectedSecretDataKeys to assert exact key count and fix message.Constant expectedSecretDataKeys is unused; assert
.data | lengthequals it and reflect in the error.WithCondition(And( jq.Match(`.type == "%s"`, string(corev1.SecretTypeOpaque)), jq.Match(`.metadata.labels.app == "%s"`, kubeAuthProxyName), + jq.Match(`.data | length == %d`, expectedSecretDataKeys), jq.Match(`.data | has("OAUTH2_PROXY_CLIENT_ID")`), jq.Match(`.data | has("OAUTH2_PROXY_CLIENT_SECRET")`), jq.Match(`.data | has("OAUTH2_PROXY_COOKIE_SECRET")`), jq.Match(`.data.OAUTH2_PROXY_CLIENT_ID | @base64d == "%s"`, oauthClientName), jq.Match(`.data.OAUTH2_PROXY_CLIENT_SECRET | length > 0`), jq.Match(`.data.OAUTH2_PROXY_COOKIE_SECRET | length > 0`), )), - WithCustomErrorMsg("OAuth proxy credentials secret should be Opaque type with app label, exactly 3 non-empty keys, and CLIENT_ID matching OAuthClient name"), + WithCustomErrorMsg("OAuth proxy credentials secret should be Opaque type with app label, exactly %d non-empty keys, and CLIENT_ID matching OAuthClient name", expectedSecretDataKeys),
661-674: Set an HTTP client timeout to avoid hanging tests.Add a sane Timeout on the client to prevent indefinite waits.
return &http.Client{ Jar: jar, Transport: &http.Transport{ TLSClientConfig: &tls.Config{ // #nosec G402 -- e2e test environment requires skipping TLS verification for self-signed certificates InsecureSkipVerify: true, }, }, + Timeout: 30 * time.Second, // don't follow redirects so we can assert Location header CheckRedirect: func(req *http.Request, via []*http.Request) error { return http.ErrUseLastResponse }, }Add import:
import "time"
535-543: Don’t hard-code the dashboard namespace.Use tc.AppsNamespace to align with configurable installs.
- dashboardNamespace := "opendatahub" + dashboardNamespace := tc.AppsNamespace dashboardRouteName := "odh-dashboard"
389-391: Make EnvoyFilter patch-count check resilient.Using equality makes the test brittle if additional safe patches are added; check for at least 3.
- jq.Match(`.spec.configPatches | length == 3`), + jq.Match(`.spec.configPatches | length >= 3`),
74-83: Add the secret-hash validation to the suite.ValidateKubeAuthProxySecretHash is never executed; include it in the testCases list.
{"Validate EnvoyFilter creation", gatewayCtx.ValidateEnvoyFilter}, + {"Validate kube-auth-proxy secret-hash annotation", gatewayCtx.ValidateKubeAuthProxySecretHash}, {"Validate Gateway ready status", gatewayCtx.ValidateGatewayReadyStatus},
611-640: Authenticated-path expectation may be environment-dependent.Using a service account token with provider=openshift and skip-jwt-bearer-tokens=true can still fail if token audience or issuer doesn’t match. Consider accepting a 3xx redirect-to-login as a fallback or guard with a feature flag.
Would you like me to wire a flag like GATEWAY_E2E_AUTH_STRICT=true to control the 200-only assertion?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/e2e/controller_test.go(1 hunks)tests/e2e/gateway_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/controller_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/gateway_test.go (8)
api/services/v1alpha1/gateway_types.go (2)
GatewayInstanceName(30-30)GatewayConfig(105-111)internal/controller/services/gateway/gateway_support.go (16)
DefaultGatewayName(22-22)GatewayClassName(20-20)GatewayNamespace(19-19)AuthClientID(25-25)KubeAuthProxyName(29-29)KubeAuthProxyTLSName(31-31)KubeAuthProxySecretsName(30-30)OAuthCallbackRouteName(32-32)AuthProxyOAuth2Path(38-38)AuthProxyHTTPPort(35-35)AuthProxyHTTPSPort(36-36)AuthProxyMetricsPort(37-37)GatewayControllerName(21-21)EnvClientID(50-50)EnvClientSecret(51-51)EnvCookieSecret(52-52)tests/e2e/test_context_test.go (2)
TestContext(38-62)NewTestContext(76-103)tests/e2e/helper_test.go (2)
RunTestCases(90-115)ExtractAndExpectValue(136-147)tests/e2e/resource_options_test.go (4)
WithMinimalObject(155-170)WithCondition(262-266)WithCustomErrorMsg(287-291)WithMutateFunc(241-245)pkg/cluster/gvk/gvk.go (12)
GatewayConfig(340-344)GatewayClass(346-350)Secret(160-164)Namespace(28-32)OAuthClient(370-374)Deployment(112-116)Service(172-176)HTTPRoute(364-368)Group(124-128)EnvoyFilter(328-332)KubernetesGateway(352-356)DataScienceCluster(64-68)pkg/utils/test/testf/testf_support.go (1)
Transform(78-106)pkg/cluster/cluster_config.go (1)
GetDomain(97-114)
🪛 ast-grep (0.39.6)
tests/e2e/gateway_test.go
[warning] 663-666: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
// #nosec G402 -- e2e test environment requires skipping TLS verification for self-signed certificates
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: golangci-lint
🔇 Additional comments (1)
tests/e2e/gateway_test.go (1)
64-86:gatewayTestSuiteis already properly invoked through the Services dispatcher.The function is registered at
controller_test.go:141in the Services TestGroup (serviceApi.GatewayServiceName: gatewayTestSuite) and is executed whenServices.Runis called from the top-levelTestOdhOperatorfunction at line 260. No additional wrapper test is needed.Likely an incorrect or invalid review comment.
GowthamShanmugam
force-pushed
the
gate_e2e_suit
branch
from
033cf21 to
0ea36ca
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/e2e/gateway_test.go (1)
652-669: Add TLS MinVersion for defense-in-depth.The TLS configuration is missing a minimum TLS version. As noted in previous reviews and static analysis, specifying
MinVersionis a defense-in-depth measure even in test environments with self-signed certificates.Apply this diff to set the minimum TLS version:
return &http.Client{ Jar: jar, Transport: &http.Transport{ TLSClientConfig: &tls.Config{ + MinVersion: tls.VersionTLS12, // #nosec G402 -- e2e test environment requires skipping TLS verification for self-signed certificates InsecureSkipVerify: true, }, },
🧹 Nitpick comments (3)
tests/e2e/gateway_test.go (3)
37-37: Unused constantexpectedSecretDataKeys.This constant is defined but never referenced in the code. If it was intended for validation in
ValidateOAuthClientAndSecret(which checks for 3 specific keys explicitly via jq matchers at lines 195-197), you may want to either use it or remove it.
671-673: Update misleading comment.The comment states the function "attempts to read the service account token from the mounted volume", but the implementation uses
oc whoami -t, which retrieves the token from the current user's authentication context, not from a mounted volume.Update the comment to reflect the actual implementation:
-// getServiceAccountToken attempts to read the service account token from the mounted volume -// Returns empty string if not running in a pod or if token cannot be read. +// getServiceAccountToken retrieves the current user's authentication token using 'oc whoami -t'. +// Fails the test if the token cannot be retrieved. func getServiceAccountToken(t *testing.T) string {
692-710: Consider preserving the original error for better diagnostics.When
cluster.GetDomainfails, the original error is discarded and only a generic "failed to determine cluster domain" message is shown. Preserving the original error would improve debugging.Consider this approach to retain the original error:
+ var cacheErr error tc.once.Do(func() { clusterDomain, err := cluster.GetDomain(tc.Context(), tc.Client()) if err != nil { - // store empty and let caller fail with require if needed - tc.cachedGatewayHostname = "" + cacheErr = fmt.Errorf("failed to get cluster domain: %w", err) return } tc.cachedGatewayHostname = gatewayName + "." + clusterDomain }) - if tc.cachedGatewayHostname == "" { - require.FailNow(t, "failed to determine cluster domain to compute gateway hostname") + if cacheErr != nil { + require.FailNow(t, cacheErr.Error()) }You would also need to add
cacheErr errorto theGatewayTestCtxstruct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/e2e/controller_test.go(1 hunks)tests/e2e/gateway_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/controller_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/gateway_test.go (8)
api/services/v1alpha1/gateway_types.go (2)
GatewayInstanceName(30-30)GatewayConfig(105-111)internal/controller/services/gateway/gateway_support.go (16)
DefaultGatewayName(22-22)GatewayClassName(20-20)GatewayNamespace(19-19)AuthClientID(25-25)KubeAuthProxyName(29-29)KubeAuthProxyTLSName(31-31)KubeAuthProxySecretsName(30-30)OAuthCallbackRouteName(32-32)AuthProxyOAuth2Path(38-38)AuthProxyHTTPPort(35-35)AuthProxyHTTPSPort(36-36)AuthProxyMetricsPort(37-37)GatewayControllerName(21-21)EnvClientID(50-50)EnvClientSecret(51-51)EnvCookieSecret(52-52)tests/e2e/test_context_test.go (2)
TestContext(38-62)NewTestContext(76-103)tests/e2e/helper_test.go (1)
ExtractAndExpectValue(136-147)tests/e2e/resource_options_test.go (4)
WithMinimalObject(155-170)WithCondition(262-266)WithCustomErrorMsg(287-291)WithMutateFunc(241-245)pkg/cluster/gvk/gvk.go (12)
GatewayConfig(340-344)GatewayClass(346-350)Secret(160-164)Namespace(28-32)OAuthClient(370-374)Deployment(112-116)Service(172-176)HTTPRoute(364-368)Group(124-128)EnvoyFilter(328-332)KubernetesGateway(352-356)DataScienceCluster(64-68)pkg/utils/test/testf/testf_support.go (1)
Transform(78-106)pkg/cluster/cluster_config.go (1)
GetDomain(97-114)
🪛 ast-grep (0.39.6)
tests/e2e/gateway_test.go
[warning] 658-661: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
// #nosec G402 -- e2e test environment requires skipping TLS verification for self-signed certificates
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: golangci-lint
- GitHub Check: kube-linter
🔇 Additional comments (3)
tests/e2e/gateway_test.go (3)
96-481: Well-structured validation functions.The validation functions (
ValidateGatewayConfig,ValidateGatewayInfrastructure,ValidateOAuthClientAndSecret,ValidateAuthProxyDeployment,ValidateOAuthCallbackRoute,ValidateEnvoyFilter,ValidateGatewayReadyStatus) are comprehensive and follow good practices:
- Precise jq matchers for configuration verification
- Descriptive error messages with context
- Consistent logging and use of
t.Helper()- Proper use of constants for expected values
497-565: Dashboard management functions are well-implemented.The dashboard enable/disable/wait functions properly handle:
- State transitions with appropriate conditions
- Deployment and HTTPRoute readiness checks
- Cleanup via defer (line 488)
- Clear logging for test observability
575-635: Authentication test functions correctly validate OAuth flow.Both
testUnauthenticatedAccessandtestAuthenticatedAccessproperly verify:
- Unauthenticated requests redirect to OAuth provider (302/307 status with Location header)
- Redirect URIs contain expected OAuth parameters and callback paths
- Authenticated requests with bearer token succeed (200 OK)
- Clear, descriptive assertion messages for failures
GowthamShanmugam
force-pushed
the
gate_e2e_suit
branch
2 times, most recently
from
075b75b to
e9f05c6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/e2e/gateway_test.go (1)
627-644: TLS MinVersion is missing from the configuration.This was already flagged in a previous review. The TLS configuration should specify a minimum TLS version for defense-in-depth, even in test environments with self-signed certificates.
As previously suggested, apply this diff:
return &http.Client{ Jar: jar, Transport: &http.Transport{ TLSClientConfig: &tls.Config{ + MinVersion: tls.VersionTLS12, // #nosec G402 -- e2e test environment requires skipping TLS verification for self-signed certificates InsecureSkipVerify: true, }, },
🧹 Nitpick comments (2)
tests/e2e/gateway_test.go (2)
248-253: Comment accuracy: Not all 4 env vars are from the secret.The comment at line 249 says "env from secret" but the validation checks 4 environment variables, where only 3 come from the secret (
OAUTH2_PROXY_CLIENT_ID,OAUTH2_PROXY_CLIENT_SECRET,OAUTH2_PROXY_COOKIE_SECRET) and the 4th (PROXY_MODE) is a literal value.Consider updating the comment for accuracy:
- // env from secret + // env vars (3 from secret, 1 literal) jq.Match(`.spec.template.spec.containers[0].env | length == 4`),
532-567: Consider extracting dashboard constants to reduce duplication.The dashboard namespace (
"opendatahub") and name ("odh-dashboard") are hard-coded in multiple places (lines 536, 537, 542). While acceptable for E2E tests, extracting these as constants would reduce duplication and improve maintainability.Add constants at the top of the file:
const ( gatewayTLSSecretName = "default-gateway-tls" envoyFilterName = "authn-filter" dashboardNamespace = "opendatahub" dashboardName = "odh-dashboard" )Then use them in the function:
func (tc *GatewayTestCtx) waitForDashboardReady(t *testing.T) { t.Helper() - dashboardNamespace := "opendatahub" - dashboardRouteName := "odh-dashboard" - t.Log("Waiting for dashboard deployment to be ready") tc.EnsureResourceExists( WithMinimalObject(gvk.Deployment, types.NamespacedName{ - Name: "odh-dashboard", + Name: dashboardName, Namespace: dashboardNamespace,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/e2e/controller_test.go(1 hunks)tests/e2e/gateway_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/controller_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/gateway_test.go (7)
api/services/v1alpha1/gateway_types.go (2)
GatewayInstanceName(30-30)GatewayConfig(105-111)internal/controller/services/gateway/gateway_support.go (16)
DefaultGatewayName(22-22)GatewayClassName(20-20)GatewayNamespace(19-19)AuthClientID(25-25)KubeAuthProxyName(29-29)KubeAuthProxyTLSName(31-31)KubeAuthProxySecretsName(30-30)OAuthCallbackRouteName(32-32)AuthProxyOAuth2Path(38-38)AuthProxyHTTPPort(35-35)AuthProxyHTTPSPort(36-36)AuthProxyMetricsPort(37-37)GatewayControllerName(21-21)EnvClientID(50-50)EnvClientSecret(51-51)EnvCookieSecret(52-52)tests/e2e/test_context_test.go (2)
TestContext(38-62)NewTestContext(76-103)tests/e2e/helper_test.go (1)
ExtractAndExpectValue(136-147)pkg/cluster/gvk/gvk.go (12)
GatewayConfig(340-344)GatewayClass(346-350)Secret(160-164)Namespace(28-32)OAuthClient(370-374)Deployment(112-116)Service(172-176)HTTPRoute(364-368)Group(124-128)EnvoyFilter(328-332)KubernetesGateway(352-356)DataScienceCluster(64-68)pkg/utils/test/testf/testf_support.go (1)
Transform(78-106)pkg/cluster/cluster_config.go (1)
GetDomain(97-114)
🪛 ast-grep (0.39.6)
tests/e2e/gateway_test.go
[warning] 633-636: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
// #nosec G402 -- e2e test environment requires skipping TLS verification for self-signed certificates
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
GowthamShanmugam
force-pushed
the
gate_e2e_suit
branch
from
e9f05c6 to
0386c39
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/e2e/gateway_test.go (2)
33-37: Unused constant flagged in previous review.The constant
expectedSecretDataKeysis defined but never referenced in the code. The validation at lines 194-196 checks for three keys explicitly, and line 201's error message hardcodes "exactly 3 non-empty keys."Consider removing the unused constant or using it in the validation logic and error message for maintainability:
const ( gatewayTLSSecretName = "default-gateway-tls" envoyFilterName = "authn-filter" - expectedSecretDataKeys = 3 )
627-644: Add TLS MinVersion as flagged in previous review.The TLS configuration only sets
InsecureSkipVerifywithout specifying a minimum TLS version. While this is a test environment with self-signed certificates, defense-in-depth practice recommends setting a minimum TLS version.Apply the fix suggested in the previous review:
Transport: &http.Transport{ TLSClientConfig: &tls.Config{ + MinVersion: tls.VersionTLS12, // #nosec G402 -- e2e test environment requires skipping TLS verification for self-signed certificates InsecureSkipVerify: true, }, },
🧹 Nitpick comments (2)
tests/e2e/gateway_test.go (2)
231-295: Consider grouping assertions for better readability.The deployment validation at lines 236-293 contains 50+ jq.Match assertions in a single
And()block. While comprehensive and correct, the density makes it harder to review and maintain.Consider grouping related assertions with comments:
tc.EnsureResourceExists( WithMinimalObject(gvk.Deployment, types.NamespacedName{ Name: kubeAuthProxyName, Namespace: gatewayNamespace, }), WithCondition(And( // Pod template and selector jq.Match(`.spec.selector.matchLabels.app == "%s"`, kubeAuthProxyName), jq.Match(`.spec.template.spec.containers | length > 0`), jq.Match(`.spec.template.spec.containers[0].name == "%s"`, kubeAuthProxyName), )), WithCondition(And( // Container ports jq.Match(`.spec.template.spec.containers[0].ports | length == 3`), // ... port checks )), WithCondition(And( // Environment variables // ... env checks )), // ... additional WithCondition blocks for volumes, args, cookies, etc. )This would improve maintainability without changing test coverage.
383-442: Similar readability concern with assertion density.Like the deployment validation, the EnvoyFilter validation contains 50+ assertions in a single block (lines 388-442), making it harder to review and maintain.
Consider the same grouping approach suggested for the deployment validation:
tc.EnsureResourceExists( WithMinimalObject(gvk.EnvoyFilter, types.NamespacedName{ Name: envoyFilterName, Namespace: gatewayNamespace, }), WithCondition(And( // Workload selector and patch count jq.Match(`.spec.workloadSelector.labels."gateway.networking.k8s.io/gateway-name" == "%s"`, gatewayName), jq.Match(`.spec.configPatches | length == 3`), )), WithCondition(And( // Patch 1: ext_authz HTTP filter // ... ext_authz checks )), WithCondition(And( // Patch 2: Lua filter for token forwarding // ... Lua filter checks )), WithCondition(And( // Patch 3: Cluster configuration // ... cluster checks )), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/e2e/controller_test.go(1 hunks)tests/e2e/gateway_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/e2e/controller_test.go (1)
api/services/v1alpha1/gateway_types.go (1)
GatewayServiceName(27-27)
tests/e2e/gateway_test.go (9)
api/services/v1alpha1/gateway_types.go (2)
GatewayInstanceName(30-30)GatewayConfig(105-111)internal/controller/services/gateway/gateway_support.go (16)
DefaultGatewayName(41-41)GatewayClassName(39-39)GatewayNamespace(38-38)AuthClientID(44-44)KubeAuthProxyName(48-48)KubeAuthProxyTLSName(50-50)KubeAuthProxySecretsName(49-49)OAuthCallbackRouteName(51-51)AuthProxyOAuth2Path(57-57)AuthProxyHTTPPort(54-54)AuthProxyHTTPSPort(55-55)AuthProxyMetricsPort(56-56)GatewayControllerName(40-40)EnvClientID(69-69)EnvClientSecret(70-70)EnvCookieSecret(71-71)tests/e2e/test_context_test.go (2)
TestContext(38-62)NewTestContext(76-103)tests/e2e/helper_test.go (1)
ExtractAndExpectValue(136-147)tests/e2e/resource_options_test.go (4)
WithMinimalObject(155-170)WithCondition(262-266)WithCustomErrorMsg(287-291)WithMutateFunc(241-245)pkg/cluster/gvk/gvk.go (12)
GatewayConfig(340-344)GatewayClass(346-350)Secret(160-164)Namespace(28-32)OAuthClient(370-374)Deployment(112-116)Service(172-176)HTTPRoute(364-368)Group(124-128)EnvoyFilter(328-332)KubernetesGateway(352-356)DataScienceCluster(64-68)pkg/utils/test/matchers/jq/jq_matcher.go (1)
Match(11-15)pkg/utils/test/testf/testf_support.go (1)
Transform(78-106)pkg/cluster/cluster_config.go (1)
GetDomain(97-114)
🪛 ast-grep (0.39.6)
tests/e2e/gateway_test.go
[warning] 633-636: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
// #nosec G402 -- e2e test environment requires skipping TLS verification for self-signed certificates
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: golangci-lint
🔇 Additional comments (3)
tests/e2e/gateway_test.go (2)
73-82: Previous review concern appears addressed.A past review flagged that
ValidateKubeAuthProxySecretHashwas defined but not included in the test cases. The function is no longer present in the code, and the secret-hash annotation validation has been integrated intoValidateAuthProxyDeployment(lines 291-292), which properly validates the annotation exists and matches the expected format.
55-669: Well-structured comprehensive test framework.The Gateway test framework is well-organized with:
- Clear separation of concerns across validation methods
- Efficient caching using
sync.Onceto avoid repeated cluster API calls (lines 646-664)- Comprehensive coverage of Gateway infrastructure, OAuth flow, authentication proxy, EnvoyFilter, and operational readiness
- Descriptive error messages that aid debugging
- Logical test flow from infrastructure setup through functional validation
The test suite provides thorough E2E coverage for the Gateway with OpenShift OAuth authentication.
tests/e2e/controller_test.go (1)
153-153: LGTM! Gateway test suite enabled.This change enables the Gateway service E2E tests by uncommenting the test suite entry. The integration with the existing test framework is correct, and the gateway test suite (defined in
gateway_test.go) will now run alongside the Monitoring and Auth service tests.
GowthamShanmugam
force-pushed
the
gate_e2e_suit
branch
from
0386c39 to
3cbe5e9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/e2e/gateway_test.go (1)
627-644: Add TLS MinVersion for defense-in-depth.While this is a test environment with self-signed certificates requiring
InsecureSkipVerify, the TLS configuration should still specify a minimum TLS version as a defense-in-depth measure. Static analysis correctly flagged this as CWE-327.Apply this diff to add a minimum TLS version:
return &http.Client{ Jar: jar, Transport: &http.Transport{ TLSClientConfig: &tls.Config{ + MinVersion: tls.VersionTLS12, // #nosec G402 -- e2e test environment requires skipping TLS verification for self-signed certificates InsecureSkipVerify: true, }, },
🧹 Nitpick comments (1)
tests/e2e/gateway_test.go (1)
33-37: Optional: Remove or use the unused constant.The constant
expectedSecretDataKeysis defined but never referenced in the test code. The validation at lines 194-196 checks for the three expected keys individually without using this constant.Consider either removing it:
const ( gatewayTLSSecretName = "default-gateway-tls" envoyFilterName = "authn-filter" - expectedSecretDataKeys = 3 )Or using it to make the intent clearer at line 201:
- WithCustomErrorMsg("OAuth proxy credentials secret should be Opaque type with app label, exactly 3 non-empty keys, and CLIENT_ID matching OAuthClient name"), + WithCustomErrorMsg("OAuth proxy credentials secret should be Opaque type with app label, exactly %d non-empty keys, and CLIENT_ID matching OAuthClient name", expectedSecretDataKeys),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/e2e/controller_test.go(1 hunks)tests/e2e/gateway_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/e2e/controller_test.go (1)
api/services/v1alpha1/gateway_types.go (1)
GatewayServiceName(27-27)
tests/e2e/gateway_test.go (9)
api/services/v1alpha1/gateway_types.go (2)
GatewayInstanceName(30-30)GatewayConfig(105-111)internal/controller/services/gateway/gateway_support.go (16)
DefaultGatewayName(41-41)GatewayClassName(39-39)GatewayNamespace(38-38)AuthClientID(44-44)KubeAuthProxyName(48-48)KubeAuthProxyTLSName(50-50)KubeAuthProxySecretsName(49-49)OAuthCallbackRouteName(51-51)AuthProxyOAuth2Path(57-57)AuthProxyHTTPPort(54-54)AuthProxyHTTPSPort(55-55)AuthProxyMetricsPort(56-56)GatewayControllerName(40-40)EnvClientID(69-69)EnvClientSecret(70-70)EnvCookieSecret(71-71)tests/e2e/test_context_test.go (2)
TestContext(38-62)NewTestContext(76-103)tests/e2e/helper_test.go (1)
ExtractAndExpectValue(136-147)tests/e2e/resource_options_test.go (4)
WithMinimalObject(155-170)WithCondition(262-266)WithCustomErrorMsg(287-291)WithMutateFunc(241-245)pkg/cluster/gvk/gvk.go (12)
GatewayConfig(340-344)GatewayClass(346-350)Secret(160-164)Namespace(28-32)OAuthClient(370-374)Deployment(112-116)Service(172-176)HTTPRoute(364-368)Group(124-128)EnvoyFilter(328-332)KubernetesGateway(352-356)DataScienceCluster(64-68)pkg/utils/test/matchers/jq/jq_matcher.go (1)
Match(11-15)pkg/utils/test/testf/testf_support.go (1)
Transform(78-106)pkg/cluster/cluster_config.go (1)
GetDomain(97-114)
🪛 ast-grep (0.39.6)
tests/e2e/gateway_test.go
[warning] 633-636: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
// #nosec G402 -- e2e test environment requires skipping TLS verification for self-signed certificates
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: golangci-lint
🔇 Additional comments (4)
tests/e2e/controller_test.go (1)
153-153: LGTM! Gateway test suite properly enabled.The uncommented line correctly activates the gateway test suite, which is well-defined in
gateway_test.go. This change aligns with the PR objective to add comprehensive E2E tests for Gateway with OpenShift OAuth authentication.tests/e2e/gateway_test.go (3)
95-484: Well-structured validation functions.The validation functions are comprehensive and follow consistent patterns. Each validation:
- Uses appropriate jq matchers for assertions
- Includes clear error messages
- Follows the same structure for maintainability
The secret hash annotation validation (lines 290-292) properly addresses the concern from previous reviews by including it within the deployment validation rather than as a separate test case.
486-567: Good integration test structure with proper cleanup.The dashboard integration test properly:
- Enables the dashboard for testing (line 490)
- Uses defer to ensure cleanup (line 491)
- Waits for readiness before testing (line 493)
- Tests the actual authentication flow
The enable/disable/wait pattern is well-implemented for isolated testing.
646-669: Good caching implementation.The hostname caching using
sync.Once(line 650) is well-implemented:
- Avoids repeated API calls to fetch cluster domain
- Handles errors properly with fail-fast on line 660
- The FQDN helper function (lines 666-669) provides a clean utility for service name construction
GowthamShanmugam
force-pushed
the
gate_e2e_suit
branch
from
3cbe5e9 to
791ab43
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/e2e/gateway_test.go (1)
574-577: Add TLS MinVersion for defense-in-depth.While
InsecureSkipVerifyis appropriate for e2e tests with self-signed certificates, the TLS configuration should still specify a minimum TLS version. Static analysis correctly flagged this as CWE-327.Apply this diff to set a minimum TLS version:
Transport: &http.Transport{ TLSClientConfig: &tls.Config{ + MinVersion: tls.VersionTLS12, // #nosec G402 -- e2e test environment requires skipping TLS verification for self-signed certificates InsecureSkipVerify: true, }, },
🧹 Nitpick comments (1)
tests/e2e/gateway_test.go (1)
36-36: Remove unused constant.
expectedSecretDataKeysis defined but never referenced in the code. The validation at lines 194-196 checks for three keys explicitly without using this constant.Apply this diff to remove the unused constant:
const ( gatewayTLSSecretName = "default-gateway-tls" envoyFilterName = "authn-filter" - expectedSecretDataKeys = 3 )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/e2e/components_test.go(1 hunks)tests/e2e/controller_test.go(1 hunks)tests/e2e/gateway_test.go(2 hunks)tests/e2e/test_context_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
tests/e2e/components_test.go (2)
tests/e2e/test_context_test.go (1)
TestContext(39-63)pkg/utils/test/testf/testf.go (1)
TestContext(112-118)
tests/e2e/test_context_test.go (4)
pkg/utils/test/matchers/jq/jq_matcher.go (1)
Match(11-15)tests/e2e/resource_options_test.go (3)
WithMinimalObject(155-170)WithMutateFunc(241-245)WithCondition(262-266)pkg/cluster/gvk/gvk.go (1)
DataScienceCluster(64-68)pkg/utils/test/testf/testf_support.go (1)
Transform(78-106)
tests/e2e/controller_test.go (1)
api/services/v1alpha1/gateway_types.go (1)
GatewayServiceName(27-27)
tests/e2e/gateway_test.go (7)
api/services/v1alpha1/gateway_types.go (2)
GatewayInstanceName(30-30)GatewayConfig(105-111)internal/controller/services/gateway/gateway_support.go (16)
DefaultGatewayName(42-42)GatewayClassName(40-40)GatewayNamespace(39-39)AuthClientID(45-45)KubeAuthProxyName(49-49)KubeAuthProxyTLSName(51-51)KubeAuthProxySecretsName(50-50)OAuthCallbackRouteName(52-52)AuthProxyOAuth2Path(58-58)AuthProxyHTTPPort(55-55)AuthProxyHTTPSPort(56-56)AuthProxyMetricsPort(57-57)GatewayControllerName(41-41)EnvClientID(70-70)EnvClientSecret(71-71)EnvCookieSecret(72-72)tests/e2e/test_context_test.go (2)
TestContext(39-63)NewTestContext(77-104)tests/e2e/helper_test.go (1)
ExtractAndExpectValue(136-147)tests/e2e/resource_options_test.go (3)
WithMinimalObject(155-170)WithCondition(262-266)WithCustomErrorMsg(287-291)pkg/cluster/gvk/gvk.go (11)
GatewayConfig(340-344)GatewayClass(346-350)Secret(160-164)Namespace(28-32)OAuthClient(370-374)Deployment(112-116)Service(172-176)HTTPRoute(364-368)Group(124-128)EnvoyFilter(328-332)KubernetesGateway(352-356)pkg/cluster/cluster_config.go (1)
GetDomain(97-114)
🪛 ast-grep (0.39.7)
tests/e2e/gateway_test.go
[warning] 574-577: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
// #nosec G402 -- e2e test environment requires skipping TLS verification for self-signed certificates
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: golangci-lint
🔇 Additional comments (4)
tests/e2e/test_context_test.go (1)
744-785: LGTM! Well-structured component state update helper.This centralized method properly handles component state updates with several good practices:
- Special mapping for DataSciencePipelines → aipipelines (v2 API compatibility)
- Validates both spec update and status Ready condition
- Uses EventuallyResourcePatched for atomic verification
The delegation from ComponentTestCtx (in components_test.go) to this base method eliminates code duplication.
tests/e2e/controller_test.go (1)
153-153: LGTM! Gateway test suite now enabled.This change activates the gateway e2e tests alongside the monitoring and auth services in the first test scenario.
tests/e2e/components_test.go (1)
253-254: LGTM! Proper delegation to centralized method.This change removes local duplication by delegating to the base TestContext method introduced in test_context_test.go, improving maintainability.
tests/e2e/gateway_test.go (1)
63-610: LGTM! Comprehensive gateway e2e test coverage.This test suite provides thorough validation of:
- Gateway configuration and infrastructure (GatewayClass, Gateway, TLS)
- OAuth integration (client, secret, callback route)
- Authentication proxy deployment and service
- EnvoyFilter configuration for external authorization
- Gateway operational readiness
- Unauthenticated access redirect behavior
Good practices observed:
- Hostname caching with sync.Once to avoid repeated API calls
- Proper defer-based cleanup for component state
- Use of TestContext helpers (EnsureDeploymentReady)
- Comprehensive jq matcher assertions
GowthamShanmugam
force-pushed
the
gate_e2e_suit
branch
from
791ab43 to
8f7f123
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/e2e/gateway_test.go (1)
567-584: Set minimum TLS version for defense-in-depth.While
InsecureSkipVerifyis appropriate for test environments with self-signed certificates, the TLS configuration should still specify a minimum TLS version to prevent connections using outdated protocols. This addresses static analysis warning CWE-327.Apply this diff to set a minimum TLS version:
return &http.Client{ Jar: jar, Transport: &http.Transport{ TLSClientConfig: &tls.Config{ + MinVersion: tls.VersionTLS12, // #nosec G402 -- e2e test environment requires skipping TLS verification for self-signed certificates InsecureSkipVerify: true, }, },
🧹 Nitpick comments (1)
tests/e2e/gateway_test.go (1)
33-37: Remove unused constant or apply it in validation.The
expectedSecretDataKeysconstant is defined but never referenced. The validation at lines 194-198 checks for three specific keys without using this constant.Either remove the unused constant:
const ( gatewayTLSSecretName = "default-gateway-tls" envoyFilterName = "authn-filter" - expectedSecretDataKeys = 3 )Or apply it in the validation for clarity (though this adds minimal value):
WithCustomErrorMsg("OAuth proxy credentials secret should be Opaque type with app label, exactly %d non-empty keys, and CLIENT_ID matching OAuthClient name", expectedSecretDataKeys),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/e2e/components_test.go(1 hunks)tests/e2e/controller_test.go(1 hunks)tests/e2e/gateway_test.go(2 hunks)tests/e2e/test_context_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
tests/e2e/controller_test.go (1)
api/services/v1alpha1/gateway_types.go (1)
GatewayServiceName(27-27)
tests/e2e/components_test.go (2)
tests/e2e/test_context_test.go (1)
TestContext(39-63)pkg/utils/test/testf/testf.go (1)
TestContext(112-118)
tests/e2e/test_context_test.go (4)
pkg/utils/test/matchers/jq/jq_matcher.go (1)
Match(11-15)tests/e2e/resource_options_test.go (3)
WithMinimalObject(155-170)WithMutateFunc(241-245)WithCondition(262-266)pkg/cluster/gvk/gvk.go (1)
DataScienceCluster(64-68)pkg/utils/test/testf/testf_support.go (1)
Transform(78-106)
tests/e2e/gateway_test.go (8)
api/services/v1alpha1/gateway_types.go (2)
GatewayInstanceName(30-30)GatewayConfig(105-111)internal/controller/services/gateway/gateway_support.go (16)
DefaultGatewayName(42-42)GatewayClassName(40-40)GatewayNamespace(39-39)AuthClientID(45-45)KubeAuthProxyName(49-49)KubeAuthProxyTLSName(51-51)KubeAuthProxySecretsName(50-50)OAuthCallbackRouteName(52-52)AuthProxyOAuth2Path(58-58)AuthProxyHTTPPort(55-55)AuthProxyHTTPSPort(56-56)AuthProxyMetricsPort(57-57)GatewayControllerName(41-41)EnvClientID(70-70)EnvClientSecret(71-71)EnvCookieSecret(72-72)tests/e2e/test_context_test.go (2)
TestContext(39-63)NewTestContext(77-104)tests/e2e/helper_test.go (1)
ExtractAndExpectValue(136-147)tests/e2e/resource_options_test.go (3)
WithMinimalObject(155-170)WithCondition(262-266)WithCustomErrorMsg(287-291)pkg/cluster/gvk/gvk.go (11)
GatewayConfig(340-344)GatewayClass(346-350)Secret(160-164)Namespace(28-32)OAuthClient(370-374)Deployment(112-116)Service(172-176)HTTPRoute(364-368)Group(124-128)EnvoyFilter(328-332)KubernetesGateway(352-356)api/components/v1alpha1/dashboard_types.go (1)
DashboardKind(29-29)pkg/cluster/cluster_config.go (1)
GetDomain(97-114)
🪛 ast-grep (0.39.7)
tests/e2e/gateway_test.go
[warning] 573-576: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
// #nosec G402 -- e2e test environment requires skipping TLS verification for self-signed certificates
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: golangci-lint
- GitHub Check: kube-linter
🔇 Additional comments (5)
tests/e2e/controller_test.go (1)
153-153: LGTM! Gateway test suite enabled correctly.The addition of the Gateway service test aligns with the PR objective to add comprehensive E2E tests for Gateway with OpenShift OAuth authentication.
tests/e2e/components_test.go (1)
252-255: LGTM! Clean delegation to centralized logic.The refactoring to delegate component state management to the base
TestContextmethod eliminates code duplication and centralizes the logic. The public API remains unchanged, maintaining backward compatibility.tests/e2e/test_context_test.go (1)
744-785: LGTM! Well-structured component state management method.The new
UpdateComponentStateInDataScienceClusterWithKindmethod provides a centralized way to update component management state in DataScienceCluster. The implementation correctly:
- Maps DataSciencePipelines to aipipelines for v2 API compatibility
- Sets appropriate Ready conditions based on management state
- Validates both spec and status updates using EventuallyResourcePatched
- Follows established test patterns with jq.Match conditions
tests/e2e/gateway_test.go (2)
96-469: Comprehensive and well-structured validation methods.The gateway validation methods provide thorough coverage of:
- GatewayConfig CR with certificate configuration
- Gateway infrastructure (GatewayClass, Gateway, TLS secrets)
- OAuth client and proxy secrets
- kube-auth-proxy deployment with extensive configuration checks
- OAuth callback HTTPRoute
- EnvoyFilter for external authorization
- Gateway ready status with listener health
Each validation uses detailed jq.Match conditions and includes descriptive error messages, making failures easy to diagnose.
471-609: Well-implemented helper methods with proper resource management.The helper methods demonstrate good practices:
- Proper use of
deferfor cleanup (line 476)- Caching with
sync.Onceto avoid repeated API calls (lines 590-604)- Clear separation of concerns (hostname retrieval, HTTP client creation, URL building)
- Comprehensive HTTP redirect testing with appropriate assertions (lines 531-547)
- Helpful logging for test debugging
The comment at lines 485-486 correctly notes that deployment readiness is already validated by the DSC Ready condition.
GowthamShanmugam
force-pushed
the
gate_e2e_suit
branch
from
8f7f123 to
694a8fa
Compare
|
/test opendatahub-operator-e2e |
GowthamShanmugam
force-pushed
the
gate_e2e_suit
branch
from
694a8fa to
ffcffd8
Compare
|
/test opendatahub-operator-e2e |
2 similar comments
|
/test opendatahub-operator-e2e |
|
/test opendatahub-operator-e2e |
|
/override ci/prow/opendatahub-operator-e2e-hypershift |
|
@GowthamShanmugam: Overrode contexts on behalf of GowthamShanmugam: ci/prow/opendatahub-operator-e2e-hypershift In 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. |
|
@ugiordan Need a review and approval for gateway e2e |
|
@davidebianchi Changes done |
…cation Signed-off-by: Gowtham Shanmugasundaram <[email protected]>
GowthamShanmugam
force-pushed
the
gate_e2e_suit
branch
from
ffcffd8 to
b3deadd
Compare
|
@GowthamShanmugam: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
| WithCondition(And(conditions...)), | ||
| ) | ||
| // Delegate to the base TestContext method | ||
| tc.TestContext.UpdateComponentStateInDataScienceClusterWithKind(state, kind) |
There was a problem hiding this comment.
| tc.TestContext.UpdateComponentStateInDataScienceClusterWithKind(state, kind) | |
| tc.UpdateComponentStateInDataScienceClusterWithKind(state, kind) |
| type GatewayTestCtx struct { | ||
| *TestContext | ||
|
|
||
| // cache computed values to avoid repeated API calls |
There was a problem hiding this comment.
| // cache computed values to avoid repeated API calls | |
| // cachedGatewayHostname stores the computed gateway hostname to avoid repeated cluster API calls. | |
| // Computed once via sync.Once on first access to getExpectedGatewayHostname |
|
|
||
| // cache computed values to avoid repeated API calls | ||
| cachedGatewayHostname string | ||
| once sync.Once |
There was a problem hiding this comment.
| once sync.Once | |
| // once ensures thread-safe lazy initialization of cachedGatewayHostname. |
| . "github.com/onsi/gomega" | ||
| ) | ||
|
|
||
| const ( |
There was a problem hiding this comment.
| const ( | |
| // Gateway TLS and EnvoyFilter configuration constants. | |
| const ( |
| expectedSecretDataKeys = 3 | ||
| ) | ||
|
|
||
| const ( |
There was a problem hiding this comment.
| const ( | |
| // Gateway infrastructure and OAuth proxy configuration constants. | |
| // These match the values defined in internal/controller/services/gateway package. | |
| const ( |
| func makeRedirectURL(hostname string) string { | ||
| return fmt.Sprintf("--redirect-url=https://%s%s/callback", hostname, authProxyOAuth2Path) | ||
| } | ||
| func makeCookieDomain(hostname string) string { |
There was a problem hiding this comment.
| func makeCookieDomain(hostname string) string { | |
| // makeCookieDomain constructs the cookie domain argument for the authentication proxy. | |
| // Ensures OAuth cookies work across all routes on the gateway. | |
| func makeCookieDomain(hostname string) string { |
| t.Log("OAuth client and secret validation completed") | ||
| } | ||
|
|
||
| // ValidateAuthProxyDeployment validates the kube-auth-proxy deployment and service. |
There was a problem hiding this comment.
| // ValidateAuthProxyDeployment validates the kube-auth-proxy deployment and service. | |
| // ValidateAuthProxyDeployment validates the kube-auth-proxy deployment and service. | |
| // | |
| // The kube-auth-proxy acts as an OAuth2 proxy that: | |
| // 1. Intercepts unauthenticated requests via EnvoyFilter external authorization | |
| // 2. Redirects users to OpenShift OAuth provider for authentication | |
| // 3. Handles OAuth callback and sets authentication cookies | |
| // 4. Validates authentication on subsequent requests | |
| // | |
| // This test verifies: | |
| // - Deployment exists with correct configuration and secret hash annotation | |
| // - Service exposes HTTP (8080), HTTPS (8443), and metrics (9091) ports | |
| // - Container args include proper redirect URL and cookie domain | |
| // - TLS certificates are properly mounted |
| return tc.cachedGatewayHostname | ||
| } | ||
|
|
||
| // getServiceFQDN returns the fully qualified domain name for a Kubernetes service. |
There was a problem hiding this comment.
| // getServiceFQDN returns the fully qualified domain name for a Kubernetes service. | |
| // getServiceFQDN returns the fully qualified domain name for a Kubernetes service. | |
| // Used to construct service addresses for EnvoyFilter configuration. | |
| // Format: <service-name>.<namespace>.svc.cluster.local |
| t.Log("Gateway ready status validation completed") | ||
| } | ||
|
|
||
| // ValidateUnauthenticatedRedirect tests that unauthenticated requests are redirected to OAuth login. |
There was a problem hiding this comment.
| // ValidateUnauthenticatedRedirect tests that unauthenticated requests are redirected to OAuth login. | |
| // ValidateUnauthenticatedRedirect tests that unauthenticated requests are redirected to OAuth login. | |
| // | |
| // This test validates end-to-end authentication by: | |
| // 1. Temporarily enabling Dashboard component (provides an HTTPRoute to test against) | |
| // 2. Making an unauthenticated HTTP request to the dashboard through the gateway | |
| // 3. Verifying the response is a redirect (302/307) to the OAuth provider | |
| // 4. Checking the redirect URL contains OAuth authorization endpoint and callback parameters | |
| // 5. Cleaning up by removing Dashboard component | |
| // | |
| // Note: Dashboard is used as a test target because it automatically creates an HTTPRoute | |
| // that is attached to the Gateway, providing a real route to test authentication against. |
| InsecureSkipVerify: true, | ||
| }, | ||
| }, | ||
| // don't follow redirects so we can assert Location header |
There was a problem hiding this comment.
| // don't follow redirects so we can assert Location header | |
| // Don't follow redirects automatically so we can inspect the Location header | |
| // and verify the OAuth redirect is working correctly. |
5 participants
Description
RHOAIENG-36997
How Has This Been Tested?
Screenshot or short clip
Merge criteria
E2E test suite update requirement
When bringing new changes to the operator code, such changes are by default required to be accompanied by extending and/or updating the E2E test suite accordingly.
To opt-out of this requirement:
E2E update requirement opt-out justificationsection belowE2E update requirement opt-out justification
Summary by CodeRabbit