CNTRLPLANE-3271: add External OIDC e2e tests for v2 framework#8674
CNTRLPLANE-3271: add External OIDC e2e tests for v2 framework#8674bryan-cox wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@bryan-cox: This pull request references CNTRLPLANE-3271 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end External OIDC E2E support: broadens four exported test helper signatures to testing.TB, registers two new env vars, adds Keycloak deploy/cleanup utilities (CA extraction and OIDC discovery verification), wires Azure lifecycle to deploy Keycloak and patch HostedCluster auth, and adds a new e2ev2 test suite validating KAS/auth plumbing and Keycloak-based authentication. Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant AzureLifecycle
participant KubernetesAPI
participant Keycloak
participant HostedClusterAPI
TestRunner->>AzureLifecycle: request create external-oidc cluster
AzureLifecycle->>KubernetesAPI: create Keycloak namespace + resources (ConfigMap, Service, StatefulSet, Route)
KubernetesAPI->>Keycloak: start StatefulSet pods
Keycloak->>AzureLifecycle: ready (StatefulSet ready)
AzureLifecycle->>KubernetesAPI: create/update CA ConfigMap and console Secret
AzureLifecycle->>HostedClusterAPI: patch HostedCluster authentication with ExtOIDCConfig
TestRunner->>Keycloak: obtain test token (via issuer URL)
TestRunner->>HostedClusterAPI: SelfSubjectReview using Keycloak token
HostedClusterAPI->>TestRunner: authentication response (username, groups, uid)
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
approved
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8674 +/- ##
=======================================
Coverage 41.43% 41.43%
=======================================
Files 756 756
Lines 93647 93647
=======================================
Hits 38802 38802
Misses 52124 52124
Partials 2721 2721
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/v2/internal/env_vars.go`:
- Around line 222-225: The env var E2E_EXTERNAL_OIDC_TEST_USERS registered via
RegisterEnvVar currently holds raw user:password pairs and can be printed by
PrintEnvVarHelp()/SetupTestEnv() when E2E_SHOW_ENV_HELP is enabled; change this
to avoid exposing secrets by either (A) converting the registration to accept a
file path (e.g., "path to file containing user:password lines") and update any
consumers that read E2E_EXTERNAL_OIDC_TEST_USERS to read that file, or (B) mark
the variable as sensitive in the masking logic used by
PrintEnvVarHelp()/RegisterEnvVar so it is redacted (add
"external_oidc_test_users" or the exact var name to the
secret/password/token/key detection or a dedicated sensitive flag on
RegisterEnvVar) and ensure SetupTestEnv exports it masked. Reference
RegisterEnvVar, E2E_EXTERNAL_OIDC_TEST_USERS, PrintEnvVarHelp, and SetupTestEnv
when making the change.
In `@test/e2e/v2/lifecycle/azure.go`:
- Around line 377-384: The env setup silently ignores os.Setenv errors for the
external OIDC variables; in the SetupTestEnv flow check the return values of
os.Setenv when setting "E2E_EXTERNAL_OIDC_CA_BUNDLE_FILE" and
"E2E_EXTERNAL_OIDC_TEST_USERS" and fail fast (return an error or call t.Fatalf /
log.Fatalf as appropriate) with contextual messages including the filepath and
the original error; update the code around the caPath/file read blocks to handle
and propagate Setenv errors instead of ignoring them.
In `@test/e2e/v2/tests/hosted_cluster_external_oidc_test.go`:
- Around line 60-69: The skipIfNotOIDC helper only checks Authentication.Type
but doesn't guard that Authentication.OIDCProviders exists or has at least one
element, which causes later OIDCProviders[0] accesses to panic; update
skipIfNotOIDC (in test/e2e/v2/tests/hosted_cluster_external_oidc_test.go) to
also check hc.Spec.Configuration.Authentication.OIDCProviders is non-nil and
len(...) > 0 and, if the slice is empty, call Fail (or Failf) with a clear
diagnostic like "External OIDC tests require at least one OIDC provider
configured" instead of Skip so tests fail with a helpful message rather than
panicking. Ensure you reference the Authentication and OIDCProviders fields when
adding the guard.
In `@test/e2e/v2/util/keycloak.go`:
- Line 45: The keycloakImage constant is pinned to
"quay.io/keycloak/keycloak:latest", which makes e2e runs non-reproducible;
update the keycloakImage variable to a specific, tested Keycloak image tag
(e.g., a stable release like 21.x or the exact version you validated), replace
":latest" with that pinned tag in the keycloakImage declaration, and add a short
comment noting why it’s pinned so future changes are explicit (ensure any
related startup script expectations in setup.sh and the readiness probe against
/realms/master are compatible with the chosen version).
- Around line 460-475: The verifyOIDCEndpoint function currently disables TLS
verification by setting tls.Config{InsecureSkipVerify: true}; instead build a
tls.Config that validates the issuer certificate chain using the provided
caBundle: create a x509.NewCertPool(), call AppendCertsFromPEM(caBundle) and set
tls.Config.RootCAs to that pool (remove InsecureSkipVerify), then use that
tls.Config in the http.Transport; keep a fallback only if AppendCertsFromPEM
fails but prefer failing the check and surfacing the error so the OIDC discovery
validates against the router CA.
- Around line 417-440: The getKeycloakIssuerURL function reads the Route once
and can return before spec.host or status.ingress[].host is populated; change it
to poll until a host is available (or a timeout/context deadline is reached). In
getKeycloakIssuerURL use a retry loop (e.g., wait.PollImmediate or simple
time.Ticker) that repeatedly calls client.Get for the Route
(keycloakRouteName/keycloakNamespace), each iteration checking route.Spec.Host
and falling back to route.Status.Ingress[*].Host, and only return the formatted
issuer URL once a non-empty host is found; if the loop times out return a clear
error indicating the host was never assigned. Ensure you reference
getKeycloakIssuerURL, keycloakRouteName, keycloakNamespace and defaultRealm in
the change.
🪄 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: 1439d0d5-16ee-429f-b78d-8f4b49691f60
📒 Files selected for processing (5)
test/e2e/util/external_oidc.gotest/e2e/v2/internal/env_vars.gotest/e2e/v2/lifecycle/azure.gotest/e2e/v2/tests/hosted_cluster_external_oidc_test.gotest/e2e/v2/util/keycloak.go
bryan-cox
force-pushed
the
CNTRLPLANE-3271
branch
from
ecf40c1 to
136b816
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/v2/util/keycloak.go (1)
157-177: 💤 Low valueConsider using
json.Marshalfor JSON construction.The JSON strings are built via string concatenation, which could break if
consoleRedirectURIcontains characters requiring JSON escaping (e.g., quotes, backslashes). While the current caller passes a controlled URL, usingjson.Marshalwould be more robust.Example approach
type clientConfig struct { ClientID string `json:"clientId"` Enabled bool `json:"enabled"` // ... other fields } cfg := clientConfig{ClientID: defaultConsoleClientID, ...} jsonBytes, _ := json.Marshal(cfg)🤖 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/v2/util/keycloak.go` around lines 157 - 177, In buildSetupConfigMap replace the hand-assembled JSON string concatenation for cliClientJSON and consoleClientJSON with strongly-typed structs (e.g., clientConfig with fields ClientID, Enabled, PublicClient, Secret, RedirectUris, DirectAccessGrantsEnabled, DefaultClientScopes, Attributes) and use json.Marshal to produce the JSON strings so values like consoleSecret and consoleRedirectURI are properly escaped; populate the Attributes map for "access.token.lifespan" and set RedirectUris as a []string, marshal to []byte and convert to string to assign to the ConfigMap data entries.
🤖 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.
Nitpick comments:
In `@test/e2e/v2/util/keycloak.go`:
- Around line 157-177: In buildSetupConfigMap replace the hand-assembled JSON
string concatenation for cliClientJSON and consoleClientJSON with strongly-typed
structs (e.g., clientConfig with fields ClientID, Enabled, PublicClient, Secret,
RedirectUris, DirectAccessGrantsEnabled, DefaultClientScopes, Attributes) and
use json.Marshal to produce the JSON strings so values like consoleSecret and
consoleRedirectURI are properly escaped; populate the Attributes map for
"access.token.lifespan" and set RedirectUris as a []string, marshal to []byte
and convert to string to assign to the ConfigMap data entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 64c1f89f-ce0b-4d5e-9a08-23951ba1c475
📒 Files selected for processing (5)
test/e2e/util/external_oidc.gotest/e2e/v2/internal/env_vars.gotest/e2e/v2/lifecycle/azure.gotest/e2e/v2/tests/hosted_cluster_external_oidc_test.gotest/e2e/v2/util/keycloak.go
🚧 Files skipped from review as they are similar to previous changes (4)
- test/e2e/v2/internal/env_vars.go
- test/e2e/util/external_oidc.go
- test/e2e/v2/tests/hosted_cluster_external_oidc_test.go
- test/e2e/v2/lifecycle/azure.go
bryan-cox
force-pushed
the
CNTRLPLANE-3271
branch
from
136b816 to
4eaad18
Compare
|
/test e2e-azure-v2-self-managed |
bryan-cox
force-pushed
the
CNTRLPLANE-3271
branch
from
4eaad18 to
f1e32ba
Compare
|
/test e2e-azure-v2-self-managed |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/v2/lifecycle/azure.go`:
- Around line 237-260: The ConfigMap and Secret names (caCM, consoleSecret) are
hardcoded and must be scoped to the HostedCluster: derive their Name using the
test's `name` (e.g. fmt.Sprintf("%s-oidc-ca", name) and "%s-console-secret") and
set those names on the corev1.ConfigMap (caCM) and corev1.Secret
(consoleSecret), pass those derived names into `ExtOIDCConfig`, and add an
OwnerReference to `hc` on both resources before calling
`createOrUpdateResource(ctx, cl, ...)` so they are unique per HostedCluster and
are garbage-collected with the cluster; apply the same changes to the analogous
resources created around lines 264-275.
🪄 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: d2f76272-3893-40c9-8531-ec2b9495ee63
📒 Files selected for processing (6)
test/e2e/util/external_oidc.gotest/e2e/v2/cmd/create-guests/main.gotest/e2e/v2/internal/env_vars.gotest/e2e/v2/lifecycle/azure.gotest/e2e/v2/tests/hosted_cluster_external_oidc_test.gotest/e2e/v2/util/keycloak.go
🚧 Files skipped from review as they are similar to previous changes (4)
- test/e2e/v2/internal/env_vars.go
- test/e2e/util/external_oidc.go
- test/e2e/v2/util/keycloak.go
- test/e2e/v2/tests/hosted_cluster_external_oidc_test.go
| caCM := &corev1.ConfigMap{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "oidc-ca", | ||
| Namespace: namespace, | ||
| }, | ||
| Data: map[string]string{ | ||
| "ca-bundle.crt": string(kcConfig.CABundle), | ||
| }, | ||
| } | ||
| if err := createOrUpdateResource(ctx, cl, caCM); err != nil { | ||
| return fmt.Errorf("creating OIDC CA configmap: %w", err) | ||
| } | ||
|
|
||
| consoleSecret := &corev1.Secret{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "console-secret", | ||
| Namespace: namespace, | ||
| }, | ||
| Type: corev1.SecretTypeOpaque, | ||
| StringData: map[string]string{ | ||
| "clientSecret": kcConfig.ConsoleClientSecret, | ||
| }, | ||
| } | ||
| if err := createOrUpdateResource(ctx, cl, consoleSecret); err != nil { |
There was a problem hiding this comment.
Scope the OIDC ConfigMap/Secret to the HostedCluster.
These names are hardcoded inside the shared HostedCluster namespace, so another external-oidc run can upsert the same oidc-ca / console-secret objects and break an already-running cluster's auth wiring. Please derive the names from name and feed those names into ExtOIDCConfig; adding an owner reference to hc would also let destroy clean them up automatically.
Suggested change
+ caConfigMapName := fmt.Sprintf("%s-oidc-ca", name)
+ consoleSecretName := fmt.Sprintf("%s-console-secret", name)
+
caCM := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
- Name: "oidc-ca",
+ Name: caConfigMapName,
Namespace: namespace,
},
Data: map[string]string{
"ca-bundle.crt": string(kcConfig.CABundle),
},
}
@@
consoleSecret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
- Name: "console-secret",
+ Name: consoleSecretName,
Namespace: namespace,
},
@@
extOIDCConfig := &e2eutil.ExtOIDCConfig{
@@
- ConsoleClientSecretName: "console-secret",
+ ConsoleClientSecretName: consoleSecretName,
@@
- IssuerCAConfigmapName: "oidc-ca",
+ IssuerCAConfigmapName: caConfigMapName,
TestUsers: kcConfig.TestUsers,
}Also applies to: 264-275
🤖 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/v2/lifecycle/azure.go` around lines 237 - 260, The ConfigMap and
Secret names (caCM, consoleSecret) are hardcoded and must be scoped to the
HostedCluster: derive their Name using the test's `name` (e.g.
fmt.Sprintf("%s-oidc-ca", name) and "%s-console-secret") and set those names on
the corev1.ConfigMap (caCM) and corev1.Secret (consoleSecret), pass those
derived names into `ExtOIDCConfig`, and add an OwnerReference to `hc` on both
resources before calling `createOrUpdateResource(ctx, cl, ...)` so they are
unique per HostedCluster and are garbage-collected with the cluster; apply the
same changes to the analogous resources created around lines 264-275.
f1e32ba to
0c99b2a
Compare
|
/pj-rehearse pull-ci-openshift-hypershift-main-e2e-azure-v2-self-managed |
|
/test e2e-azure-v2-self-managed |
bryan-cox
force-pushed
the
CNTRLPLANE-3271
branch
from
0c99b2a to
b011f85
Compare
|
/test e2e-azure-v2-self-managed |
bryan-cox
force-pushed
the
CNTRLPLANE-3271
branch
2 times, most recently
from
dd7dc57 to
7db41d2
Compare
|
/test e2e-azure-v2-self-managed |
bryan-cox
force-pushed
the
CNTRLPLANE-3271
branch
2 times, most recently
from
aea8371 to
6cb9894
Compare
|
/test e2e-azure-v2-self-managed |
bryan-cox
force-pushed
the
CNTRLPLANE-3271
branch
from
6cb9894 to
c91ab3b
Compare
|
/test e2e-azure-v2-self-managed |
bryan-cox
force-pushed
the
CNTRLPLANE-3271
branch
from
c91ab3b to
434bd87
Compare
|
/test e2e-azure-v2-self-managed |
bryan-cox
force-pushed
the
CNTRLPLANE-3271
branch
from
434bd87 to
cbd1f64
Compare
|
/test e2e-azure-v2-self-managed |
Add platform-agnostic External OIDC testing infrastructure and tests for the v2 e2e framework, using Keycloak as the identity provider. Changes: - Widen OIDC utility function signatures from *testing.T to testing.TB for Ginkgo v2 compatibility (backward compatible) - Add Keycloak deployment utility (test/e2e/v2/util/keycloak.go) that deploys Keycloak on any management cluster with OIDC clients, test users, group mappings, and CA bundle extraction - Add 'external-oidc' cluster variant to the Azure v2 lifecycle that deploys Keycloak in PostCreate and patches the HostedCluster with OIDC authentication config - Add External OIDC validation tests covering: cluster OIDC config, OAuth server not deployed, KAS JWT authenticator configuration, and Keycloak token authentication with claim mapping verification - Register E2E_EXTERNAL_OIDC_CA_BUNDLE_FILE and E2E_EXTERNAL_OIDC_TEST_USERS environment variables Tests are platform-agnostic and skip based on authentication type, not platform. Other platforms can reuse the Keycloak utility and test file by adding an external-oidc variant to their lifecycle config. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bryan-cox
force-pushed
the
CNTRLPLANE-3271
branch
from
cbd1f64 to
6d5b904
Compare
|
/test e2e-azure-v2-self-managed |
|
@bryan-cox: 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. |
|
Confirmed: Only the
Now I have all the evidence needed to write the report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe failure is a network connectivity issue between the hosted cluster worker nodes and the management cluster's Keycloak OIDC provider, introduced by PR #8674's new External OIDC e2e test. Detailed chain of events:
Key evidence that this is network-specific:
Recommendations
Evidence
|
2 participants
What this PR does / why we need it:
Adds automated e2e testing for External OIDC support on self-managed Azure HCP using the v2 test framework. The test deploys Keycloak on the management cluster as a platform-agnostic OIDC provider, creates a new
external-oidccluster variant, and validates the resulting control plane state.Test areas (11 assertions across 4 contexts):
authentication.type: OIDCand is Availableoauth-openshiftoropenshift-oauth-apiserverdeployments in the control plane namespaceauth-configConfigMap has JWT authenticator matching HC spec; no OAuth webhookPlatform-agnostic design: Keycloak deployment utility and test assertions are reusable by any platform. Only the Azure lifecycle integration is platform-specific. Other platforms can add an
external-oidcvariant to their lifecycle config with zero test code changes.Azure AD excluded from automation: Device code flow requires interactive browser sign-in. Manually validated in CNTRLPLANE-3462. Both providers exercise the same KAS JWT authenticator and CPO reconciliation code paths.
Which issue(s) this PR fixes:
Fixes https://issues.redhat.com/browse/CNTRLPLANE-3271
Special notes for your reviewer:
*testing.T→testing.TBsignature widening intest/e2e/util/external_oidc.gois backward compatible — existing v1 callers are unaffectedBeforeEach+Skip()on authentication typePostCreate()is adapted from the CI step registryidp/external-oidc/keycloak/server/Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores