[WIP] CNTRLPLANE-3851: Oauth proxy config e2e#31398
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@ehearne-redhat: This pull request explicitly references no jira issue. 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. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ehearne-redhat The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThe change exports Keycloak and OIDC authentication test helpers, updates their call sites, adds proxy and trusted-CA OAuth coverage, and upgrades the Go toolchain and selected dependencies. ChangesOIDC authentication test infrastructure
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant OIDCProxyTest
participant Keycloak
participant AuthenticationOperator
participant OAuthServer
participant KubernetesAPI
OIDCProxyTest->>Keycloak: Create test user and group
OIDCProxyTest->>AuthenticationOperator: Update proxy and trusted CA configuration
AuthenticationOperator->>OAuthServer: Reconcile authentication configuration
OIDCProxyTest->>OAuthServer: Request OAuth token
OIDCProxyTest->>KubernetesAPI: Submit SelfSubjectReview
KubernetesAPI-->>OIDCProxyTest: Return username and groups
Suggested reviewers: 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/extended/oauth/oidc_proxy.go`:
- Around line 89-94: Replace the placeholder values in the
AuthenticationProxyConfig assigned to modified.Spec.Proxy with a provisioned or
discovered reachable test proxy endpoint for both HTTPProxy and HTTPSProxy.
Update the OIDC authentication test to assert that requests use the proxy, while
preserving the NoProxy behavior for dontGoHere.
- Around line 52-53: Update the test setup surrounding AdmittedURLForRoute to
provision the Keycloak fixture before resolving its route, rather than assuming
a prior package deployed it. Register the fixture’s cleanup in the spec’s
cleanups collection so resources are removed after the test, and preserve the
existing route lookup and test flow once provisioning succeeds.
🪄 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: 2dc7acdd-97e1-48ce-9b79-8e32b97736d7
⛔ Files ignored due to path filters (70)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apps/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/authorization/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/build/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/cloudnetwork/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_network.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/image/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/types_internalreleaseimage.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/network/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/networkoperator/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/oauth/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_csi_cluster_driver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_ingresscontroller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/types_clusterapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/osin/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/osin/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/project/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/route/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/samples/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/security/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/template/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/user/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/encoding/protodelim/protodelim.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/encoding/protojson/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/encoding/protojson/well_known_types.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/encoding/prototext/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/descfmt/stringer.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/editionssupport/editions.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/encoding/tag/tag.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/encoding/text/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/filedesc/desc.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/filedesc/desc_init.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/filedesc/desc_lazy.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/filedesc/editions.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/genid/descriptor_gen.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/impl/codec_map.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/impl/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/impl/validate.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/version/version.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/proto/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/reflect/protodesc/desc.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/reflect/protodesc/desc_init.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/reflect/protodesc/editions.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/reflect/protodesc/proto.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/types/descriptorpb/descriptor.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/types/known/timestamppb/timestamp.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (5)
go.modtest/extended/authentication/keycloak_client.gotest/extended/authentication/keycloak_helpers.gotest/extended/authentication/oidc.gotest/extended/oauth/oidc_proxy.go
| // We assume keycloak has been deployed and tests done previously that it works fine. | ||
| kcUrl, err := exauth.AdmittedURLForRoute(ctx, oc, exauth.KeycloakResourceName, ns) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Provision the Keycloak fixture in this test.
This spec never deploys Keycloak, so AdmittedURLForRoute polls for a nonexistent route in oidc-oauth-login-flow; cleanups is consequently always empty. E2e selection must not depend on another package having run first. Deploy the fixture and register its cleanup within this spec.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/extended/oauth/oidc_proxy.go` around lines 52 - 53, Update the test
setup surrounding AdmittedURLForRoute to provision the Keycloak fixture before
resolving its route, rather than assuming a prior package deployed it. Register
the fixture’s cleanup in the spec’s cleanups collection so resources are removed
after the test, and preserve the existing route lookup and test flow once
provisioning succeeds.
| // actual proxy values will be set at a later stage | ||
| modified.Spec.Proxy = operatorv1.AuthenticationProxyConfig{ | ||
| HTTPProxy: "httpProxyURL", | ||
| HTTPSProxy: "httpsProxyURL", | ||
| NoProxy: []string{"dontGoHere"}, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Replace placeholder proxy endpoints with a reachable test proxy.
httpProxyURL and httpsProxyURL are not usable proxy targets. The authentication flow cannot demonstrate proxied OIDC login with these values. Provision or discover a reachable proxy endpoint and assert the expected proxy behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/extended/oauth/oidc_proxy.go` around lines 89 - 94, Replace the
placeholder values in the AuthenticationProxyConfig assigned to
modified.Spec.Proxy with a provisioned or discovered reachable test proxy
endpoint for both HTTPProxy and HTTPSProxy. Update the OIDC authentication test
to assert that requests use the proxy, while preserving the NoProxy behavior for
dontGoHere.
|
Scheduling required tests: |
ehearne-redhat
changed the title
|
@ehearne-redhat: This pull request references CNTRLPLANE-3851 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 sub-task 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. |
|
/jira-refresh |
|
/jira refresh |
|
@ehearne-redhat: This pull request references CNTRLPLANE-3851 which is a valid jira issue. 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. |
|
Scheduling required tests: |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 102559c
New tests seen in this PR at sha: 102559c
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/extended/oauth/oidc_proxy.go (1)
182-183: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the expected mapped username.
Checking only for a non-empty username allows an incorrectly mapped identity to pass. Match the expected
<username>@payload.openshift.io`` value, as the authentication OIDC coverage does.Proposed fix
- gomega.Expect(ssr.Status.UserInfo.Username).NotTo(o.BeEmpty()) + gomega.Expect(ssr.Status.UserInfo.Username).To(o.Equal(fmt.Sprintf("%s@payload.openshift.io", username)))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/oauth/oidc_proxy.go` around lines 182 - 183, Update the username assertion in the OIDC proxy test to compare ssr.Status.UserInfo.Username against the expected mapped value "<username>`@payload.openshift.io`" instead of only checking that it is non-empty; keep the existing groups assertion unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/extended/oauth/oidc_proxy.go`:
- Around line 273-313: Update the rotation flow in the test around
assertOIDCLogin and the openshift-config ConfigMap update so the proxy or IdP
TLS endpoint serves a certificate signed by newCA. Wait until the synchronized
ConfigMap in openshift-authentication contains newCAPEM, then assert login
succeeds and verify the oauth-server pod list remains unchanged, proving hot
reload without redeployment.
- Around line 239-241: Update the temporary-directory cleanup deferred in the
OIDC proxy test to handle the error returned by os.RemoveAll instead of
discarding it. Preserve cleanup execution for tempDir and report any cleanup
failure through the test’s existing assertion or error-handling mechanism.
- Around line 344-350: Update updateAuthenticationProxyConfig to fetch the
current Authentication resource from the operator client before modifying it,
rather than deep-copying the stale authConfig from BeforeAll. Apply the proxy
change to the freshly retrieved object and then perform the existing Update
call.
---
Outside diff comments:
In `@test/extended/oauth/oidc_proxy.go`:
- Around line 182-183: Update the username assertion in the OIDC proxy test to
compare ssr.Status.UserInfo.Username against the expected mapped value
"<username>`@payload.openshift.io`" instead of only checking that it is non-empty;
keep the existing groups assertion unchanged.
🪄 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: ebf00ea9-0bee-4fbc-b1d8-9975e1348b9b
📒 Files selected for processing (1)
test/extended/oauth/oidc_proxy.go
| tempDir, err := os.MkdirTemp("", "testca") | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| defer os.RemoveAll(tempDir) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Check the temporary-directory cleanup error.
defer os.RemoveAll(tempDir) discards its error and can silently leave CA key material behind.
As per path instructions, “Never ignore error returns.”
Proposed fix
- defer os.RemoveAll(tempDir)
+ g.DeferCleanup(func() {
+ o.Expect(os.RemoveAll(tempDir)).To(o.Succeed())
+ })📝 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.
| tempDir, err := os.MkdirTemp("", "testca") | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| defer os.RemoveAll(tempDir) | |
| tempDir, err := os.MkdirTemp("", "testca") | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| g.DeferCleanup(func() { | |
| o.Expect(os.RemoveAll(tempDir)).To(o.Succeed()) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/extended/oauth/oidc_proxy.go` around lines 239 - 241, Update the
temporary-directory cleanup deferred in the OIDC proxy test to handle the error
returned by os.RemoveAll instead of discarding it. Preserve cleanup execution
for tempDir and report any cleanup failure through the test’s existing assertion
or error-handling mechanism.
Source: Path instructions
| updateAuthenticationProxyConfig(ctx, oc, authConfig, operatorv1.AuthenticationProxyConfig{ | ||
| HTTPProxy: "httpProxyURL", | ||
| HTTPSProxy: "httpsProxyURL", | ||
| NoProxy: []string{"dontGoHere"}, | ||
| TrustedCA: operatorv1.AuthenticationConfigMapReference{ | ||
| Name: configMapName, | ||
| }, | ||
| }) | ||
|
|
||
| assertOIDCLogin("should be able to log in after setting spec.proxy with trustedCA") | ||
|
|
||
| // Explicitly verify trustedCA ConfigMap is synced to openshift-authentication namespace. | ||
| cm, err := oc.AdminKubeClient().CoreV1().ConfigMaps("openshift-authentication").Get(ctx, configMapName, metav1.GetOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "trustedCA ConfigMap %s should exist in openshift-authentication", configMapName) | ||
| o.Expect(cm).NotTo(o.BeEmpty(), "trustedCA ConfigMap %s should have data", configMapName) | ||
|
|
||
| // Grab oauth-server pod list to confirm no re-deployment later | ||
| oauthServerPodList, err := oc.AdminKubeClient().CoreV1().Pods("openshift-authentication").List(ctx, metav1.ListOptions{LabelSelector: "app=oauth-openshift"}) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "should be able to query for oauth-server pods") | ||
| o.Expect(oauthServerPodList.Items).NotTo(o.BeEmpty(), "pod items list should contain at least one") | ||
|
|
||
| // Update CA file contents | ||
| newCA, err := crypto.MakeSelfSignedCA( | ||
| path.Join(tempDir, "ca-new.crt"), | ||
| path.Join(tempDir, "ca-new.key"), | ||
| path.Join(tempDir, "serial-new"), | ||
| "proxy-e2e-ca-rotated", | ||
| 100*24*time.Hour, | ||
| ) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| newCAPEM, _, err := newCA.Config.GetPEMBytes() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| cm, err = oc.AdminKubeClient().CoreV1().ConfigMaps("openshift-config").Get(ctx, configMapName, metav1.GetOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| cm.Data["ca-bundle.crt"] = string(newCAPEM) | ||
| _, err = oc.AdminKubeClient().CoreV1().ConfigMaps("openshift-config").Update(ctx, cm, metav1.UpdateOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| assertOIDCLogin("should be able to log in after setting spec.proxy with trustedCA") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make login depend on the rotated CA.
The generated CA is only stored in a ConfigMap; no proxy or IdP endpoint presents a certificate signed by it. After rotation, assertOIDCLogin can also succeed before the updated bundle is synchronized. Consequently, this test does not prove hot reload.
Rotate the certificate used by the TLS endpoint, wait until the synchronized ConfigMap contains newCAPEM, and then verify login succeeds without pod replacement.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/extended/oauth/oidc_proxy.go` around lines 273 - 313, Update the
rotation flow in the test around assertOIDCLogin and the openshift-config
ConfigMap update so the proxy or IdP TLS endpoint serves a certificate signed by
newCA. Wait until the synchronized ConfigMap in openshift-authentication
contains newCAPEM, then assert login succeeds and verify the oauth-server pod
list remains unchanged, proving hot reload without redeployment.
| func updateAuthenticationProxyConfig(ctx context.Context, oc *exutil.CLI, authConfig *operatorv1.Authentication, proxy operatorv1.AuthenticationProxyConfig) { | ||
| g.GinkgoHelper() | ||
|
|
||
| modified := authConfig.DeepCopy() | ||
| modified.Spec.Proxy = proxy | ||
|
|
||
| _, err := oc.AdminOperatorClient().OperatorV1().Authentications().Update(ctx, modified, metav1.UpdateOptions{}) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='test/extended/oauth/oidc_proxy.go'
echo '--- outline ---'
ast-grep outline "$file" --view expanded || true
echo '--- relevant lines 1 ---'
sed -n '1,220p' "$file" | cat -n
echo '--- relevant lines 220-420 ---'
sed -n '220,420p' "$file" | cat -n
echo '--- call sites ---'
rg -n "updateAuthenticationProxyConfig|authConfig" "$file"Repository: openshift/origin
Length of output: 20110
Fetch the current Authentication before updating it.
authConfig comes from BeforeAll, so its resourceVersion becomes stale after the first AfterEach update. The next ordered spec reuses that object and can hit an Update conflict.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/extended/oauth/oidc_proxy.go` around lines 344 - 350, Update
updateAuthenticationProxyConfig to fetch the current Authentication resource
from the operator client before modifying it, rather than deep-copying the stale
authConfig from BeforeAll. Apply the proxy change to the freshly retrieved
object and then perform the existing Update call.
|
@ehearne-redhat: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Summary by CodeRabbit
TrustedCAconfigured, verifying hot-reload behavior.