CNTRLPLANE-3375: test(e2e): remove oc dependency in external oidc e2e tests - #9208
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe Keycloak OIDC test utility now uses the fetched ID token directly as the bearer token in a copied anonymous REST configuration. It removes refresh-token extraction, filesystem token-cache setup, exec-provider configuration, related imports, and the exported Sequence Diagram(s)sequenceDiagram
participant OIDC_Test
participant Keycloak
participant REST_Config
OIDC_Test->>Keycloak: fetch ID token
OIDC_Test->>REST_Config: copy anonymous REST configuration
OIDC_Test->>REST_Config: set ID token as bearer token
OIDC_Test->>Keycloak: refresh admin token and create test user
OIDC_Test->>OIDC_Test: validate test-user creation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/testing
|
/test e2e-azure-aks-external-oidc-techpreview |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9208 +/- ##
==========================================
+ Coverage 44.94% 44.96% +0.01%
==========================================
Files 778 778
Lines 97427 97452 +25
==========================================
+ Hits 43790 43820 +30
+ Misses 50616 50607 -9
- Partials 3021 3025 +4 see 5 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Did any of the v2 e2e things need updated here as well? |
Not sure. I'll do some additional digging on this as it looks like I'll have some additional work to troubleshoot based on the new failures I'm seeing. It looks like it might have something to do with Keycloak admin token timeouts causing issues. |
|
/test e2e-azure-aks-external-oidc-techpreview |
![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
🤖 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/external_oidc_test.go`:
- Around line 60-65: The err variable is declared on the first line when calling
kc.GetAdminToken, then on line 65 the code attempts to redeclare err using :=
with the kc.CreateGroup call. Since the underscore (_) does not introduce a new
variable, the := operator fails with "no new variables on left side of :=".
Change the := to = for the kc.CreateGroup call to reassign the existing err
variable instead of attempting to redeclare it.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f7ac328e-4ae6-4c55-88f9-75f44c010db8
📒 Files selected for processing (1)
test/e2e/external_oidc_test.go
|
/test e2e-azure-aks-external-oidc-techpreview |
Following up on this - it doesn't look like it. It looks the external oidc tests in the v2 e2e already directly uses the token instead of relying on |
5ac69d6 to
6dacfdb
Compare
everettraven
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@everettraven: This pull request references CNTRLPLANE-3375 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. |
|
/test e2e-azure-aks-external-oidc-techpreview |
|
/test verify |
bryan-cox
left a comment
bryan-cox
left a comment
There was a problem hiding this comment.
Review: CNTRLPLANE-3375
Nice simplification — removing the oc exec-provider machinery in favor of a direct bearer token is a clean fix for the permafailure. A few notes:
Standards
- Commit message bodies: Both commits have good subjects but are missing "Why" and "How" in the body (CONTRIBUTING.md rule 4.2). The PR description has the context — would be good to squash or amend the body to include it before merge, since commit messages outlive the PR.
Spec
- Token refresh removed: The old path cached both
id_tokenandrefresh_token, lettingoc get-tokentransparently refresh. The newBearerTokenapproach has no refresh mechanism. This is probably fine given short e2e durations and configurable Keycloak token lifetimes — can you confirm the CI token TTL is long enough that no single test subcase would hit a 401?
Overall this looks good. The token lifetime question is the main thing I'd want confirmed.
| } | ||
| userCfg := rest.AnonymousClientConfig(rest.CopyConfig(clientCfg)) | ||
| userCfg.BearerToken = idToken | ||
|
|
There was a problem hiding this comment.
The old code had a refresh_token path via oc's exec credential plugin. With BearerToken set directly, there's no automatic refresh if the id_token expires mid-test. Worth confirming the Keycloak token lifetime configured in CI is longer than the longest test subcase that uses this path.
There was a problem hiding this comment.
Copying from slack for posterity:
AFAIK we fetch a fresh token for each subtest and immediately make a SelfSubjectReview call. The token should be valid long enough for each of those calls and we don't need to make any further calls (we are wanting to make sure identity mapping works correctly - only the SelfSubjectReview call is necessary for that).
If the token expires while that request is in-flight, it should be OK as the initial request would have already gone through the "is token valid?" chunk of the authentication flow. Having run the tests a handful of times with this change, I haven't seen any problems related to token refresh issues here.
…ests to prevent token fetching issues happening when changes to the oc caching logic are made. Because the tests only need a valid ID token, returning a rest.Config that has been anonymized and explicitly setting the bearer token to one we fetched for the test user is sufficient for testing the functionality while removing the dependence on oc client-side token caching behavior. Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
…g test user to prevent stale admin API tokens causing failures to create test user data within Keycloak due to short-lived tokens being issued by Keycloak. Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
6dacfdb to
48fbf42
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, everettraven 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
the
approved
|
/lgtm |
|
Scheduling tests matching the |
Test Resultse2e-aws
e2e-aks
|
|
/retest-required |
2 similar comments
|
/retest-required |
|
/retest-required |
|
/verified by ci |
openshift-ci-robot
added
the
verified
|
@everettraven: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@everettraven: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
6 participants
What this PR does / why we need it:
Removes
ocas a dependency for testing the external OIDC behavior of the hosted cluster. Whileocis typically the tooling that end-users will use, it causes unnecessary fragility in the e2e tests by putting a reliance on no changes to the token caching behavior being made.We encountered this issue when openshift/oc@51c1921 merged and started seeing permafailures for our hypershift tests that use
oc.Which issue(s) this PR fixes:
Fixes permafailures in hypershift OIDC e2e tests due to changes in
octoken caching logic made in openshift/oc@51c1921Special notes for your reviewer:
Checklist:
Summary by CodeRabbit