CNTRLPLANE-2523: deploy oauth-apiserver in new OIDC mode when authentication type is OIDC - #858
Conversation
|
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:
WalkthroughAdds a feature‑gated external‑OIDC deployment path for the oauth-apiserver, threads a feature‑gate accessor into auth‑config checks and sync flows, conditionally mirrors Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 7 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
approved
45ab4b5 to
562dc8c
Compare
everettraven
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@everettraven: This pull request references CNTRLPLANE-2523 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 "4.22.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. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
![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
🧹 Nitpick comments (3)
pkg/operator/workload/sync_openshift_oauth_apiserver_test.go (1)
199-237: Incorrect scenario numbering in comments.The comments say "scenario 2" and "scenario 3" (lines 199, 237), but these are actually scenarios 5 and 6 in the overall list. The first set of three scenarios (1-3) with feature gate disabled is followed by three more (4-6) with feature gate enabled but OIDC not configured.
Consider updating the comments to match the actual scenario numbers or removing the inline numbering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/workload/sync_openshift_oauth_apiserver_test.go` around lines 199 - 237, The inline comment labels "scenario 2" and "scenario 3" are incorrect for the test cases that follow; update those comments (near the table-driven test entries that include goldenFile "./testdata/sync_ds_scenario_2.yaml", featureGates usage, authConfigChecker, and customAPIServerArgsJSON) to the correct scenario numbers (5 and 6) or remove the numeric labels altogether so they don't mislead readers; locate the comments immediately above the test case structs that reference featureGates.NewHardcodedFeatureGateAccess and adjust the text accordingly.pkg/operator/workload/sync_openshift_oauth_apiserver.go (1)
326-415: Consider extracting shared logic between deployment sync functions.
syncExternalOIDCDeploymentandsyncStandardDeploymentshare significant templating and annotation logic (~60% overlap). While the current duplication is acceptable for initial implementation clarity, consider extracting common code into a helper function in the future to reduce maintenance burden.Key intentional differences noted:
- External OIDC uses different template and excludes InitContainers processing
- External OIDC only hashes
trusted-ca-bundle(no etcd dependencies)- External OIDC skips KMS plugin volume injection
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/workload/sync_openshift_oauth_apiserver.go` around lines 326 - 415, syncExternalOIDCDeployment duplicates much of the templating/annotation/argument logic from syncStandardDeployment; extract a shared helper (e.g., renderAndPrepareDeployment(templatePath string, operatorSpec *operatorv1.OperatorSpec, operatorStatus *operatorv1.OperatorStatus, imagePullSpec string, operatorImagePullSpec string, excludedReferences []string, hashedConfigMaps []string, injectKMS bool) (*appsv1.Deployment, error)) that performs: loading the YAML template, parsing and inserting args (including setting "v"), performing string replacements for IMAGE/REVISION/FLAGS, validating unreplaced references, encoding FLAGS, applying annotations/labels (including operator-pull-spec and revision), computing resourcehash.MultipleObjectHashStringMapForObjectReferences for a provided list of configmaps/secrets and applying those annotations to both deployment and pod template, and returning the prepared Deployment object; keep syncExternalOIDCDeployment and syncStandardDeployment to call this helper and perform only the unique steps (initContainer handling, KMS volume injection, different hashed resources, and replica counting/ensureAtMostOnePodPerNode) so behavior remains identical but shared behavior is centralized.test/e2e-oidc/external_oidc_test.go (1)
904-909: Track the TODO for future cleanup.The comment indicates this is a temporary solution that reduces validation thoroughness. Consider tracking this for follow-up once the re-architecture is complete.
Would you like me to open an issue to track improving the operand version validation once the new architecture is stable?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e-oidc/external_oidc_test.go` around lines 904 - 909, Replace the vague TODO above the conditional that uses newExternalOIDCArchitectureEnabled and operands.Delete("oauth-apiserver") with a tracked follow-up: open a repo issue describing restoring/strengthening operand version validation after the re-architecture, then update the inline comment to include the created issue number or URL and a short owner/priority tag (e.g., "TODO(`#1234`) - restore thorough validation"), so future reviewers can find and resolve the temporary operand deletion behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindata/oauth-apiserver/externaloidc-deploy.yaml`:
- Around line 36-53: The oauth-apiserver container attempts to copy the trusted
CA bundle into /etc/pki which requires elevated privileges; add a
securityContext block to the container spec for the container named
"oauth-apiserver" with privileged: true and runAsUser: 0 so the cp operation
succeeds (i.e., add securityContext: privileged: true and runAsUser: 0 to the
oauth-apiserver container spec).
- Around line 79-84: The pod references a conditionally-synced ConfigMap
"auth-config" but doesn't mark it optional or guard its use, causing pod startup
failures when OIDC is disabled; fix by either adding "optional: true" under the
auth-config volume mount (the same way trusted-ca-bundle is marked) or modify
the container startup script/command to check for
/var/run/configmaps/auth-config/auth-config.json before appending "--config=..."
(follow the trusted-ca-bundle conditional pattern and the
SyncConfigMapConditionally behavior in starter.go to decide which approach to
apply).
---
Nitpick comments:
In `@pkg/operator/workload/sync_openshift_oauth_apiserver_test.go`:
- Around line 199-237: The inline comment labels "scenario 2" and "scenario 3"
are incorrect for the test cases that follow; update those comments (near the
table-driven test entries that include goldenFile
"./testdata/sync_ds_scenario_2.yaml", featureGates usage, authConfigChecker, and
customAPIServerArgsJSON) to the correct scenario numbers (5 and 6) or remove the
numeric labels altogether so they don't mislead readers; locate the comments
immediately above the test case structs that reference
featureGates.NewHardcodedFeatureGateAccess and adjust the text accordingly.
In `@pkg/operator/workload/sync_openshift_oauth_apiserver.go`:
- Around line 326-415: syncExternalOIDCDeployment duplicates much of the
templating/annotation/argument logic from syncStandardDeployment; extract a
shared helper (e.g., renderAndPrepareDeployment(templatePath string,
operatorSpec *operatorv1.OperatorSpec, operatorStatus
*operatorv1.OperatorStatus, imagePullSpec string, operatorImagePullSpec string,
excludedReferences []string, hashedConfigMaps []string, injectKMS bool)
(*appsv1.Deployment, error)) that performs: loading the YAML template, parsing
and inserting args (including setting "v"), performing string replacements for
IMAGE/REVISION/FLAGS, validating unreplaced references, encoding FLAGS, applying
annotations/labels (including operator-pull-spec and revision), computing
resourcehash.MultipleObjectHashStringMapForObjectReferences for a provided list
of configmaps/secrets and applying those annotations to both deployment and pod
template, and returning the prepared Deployment object; keep
syncExternalOIDCDeployment and syncStandardDeployment to call this helper and
perform only the unique steps (initContainer handling, KMS volume injection,
different hashed resources, and replica counting/ensureAtMostOnePodPerNode) so
behavior remains identical but shared behavior is centralized.
In `@test/e2e-oidc/external_oidc_test.go`:
- Around line 904-909: Replace the vague TODO above the conditional that uses
newExternalOIDCArchitectureEnabled and operands.Delete("oauth-apiserver") with a
tracked follow-up: open a repo issue describing restoring/strengthening operand
version validation after the re-architecture, then update the inline comment to
include the created issue number or URL and a short owner/priority tag (e.g.,
"TODO(`#1234`) - restore thorough validation"), so future reviewers can find and
resolve the temporary operand deletion behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 04ea6cf1-c730-4ea1-9f4f-cd5a52e3856c
📒 Files selected for processing (12)
bindata/oauth-apiserver/externaloidc-deploy.yamlpkg/controllers/common/external_oidc.gopkg/controllers/common/external_oidc_test.gopkg/controllers/routercerts/controller_test.gopkg/controllers/webhookauthenticator/webhookauthenticator_controller.gopkg/operator/starter.gopkg/operator/workload/sync_openshift_oauth_apiserver.gopkg/operator/workload/sync_openshift_oauth_apiserver_test.gopkg/operator/workload/testdata/sync_ds_scenario_4.yamlpkg/operator/workload/testdata/sync_ds_scenario_5.yamlpkg/operator/workload/testdata/sync_ds_scenario_6.yamltest/e2e-oidc/external_oidc_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e-oidc/external_oidc_test.go (1)
904-912: Temporary workaround is reasonable but should be tracked.The TODO comment at line 907 notes this is a temporary solution. Consider opening an issue to track restoring full
oauth-apiserveroperand version validation once the re-architecture is complete.Would you like me to open an issue to track restoring the full operand version validation?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e-oidc/external_oidc_test.go` around lines 904 - 912, The TODO notes removing "oauth-apiserver" from operands in validateOperandVersions as a temporary workaround; create a tracked follow-up by opening an issue that references validateOperandVersions, the operands set (including "oauth-apiserver" and "oauth-openshift"), and the flag newExternalOIDCArchitectureEnabled, describing that full validation for oauth-apiserver should be restored once the re-architecture finishes; add the issue ID to the TODO comment so future reviewers can find the tracking ticket.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/operator/workload/sync_openshift_oauth_apiserver.go`:
- Around line 382-390: The dependency hash currently only includes the
"trusted-ca-bundle" ConfigMap; update the call to
resourcehash.MultipleObjectHashStringMapForObjectReferences in the external OIDC
deployment sync so it also includes the "auth-config" ConfigMap reference. Add
another
resourcehash.NewObjectRef().ForConfigMap().InNamespace(c.targetNamespace).Named("auth-config")
to the argument list (alongside the existing trusted-ca-bundle ref) so updates
to auth-config will change the computed hash and trigger pod redeploys.
---
Nitpick comments:
In `@test/e2e-oidc/external_oidc_test.go`:
- Around line 904-912: The TODO notes removing "oauth-apiserver" from operands
in validateOperandVersions as a temporary workaround; create a tracked follow-up
by opening an issue that references validateOperandVersions, the operands set
(including "oauth-apiserver" and "oauth-openshift"), and the flag
newExternalOIDCArchitectureEnabled, describing that full validation for
oauth-apiserver should be restored once the re-architecture finishes; add the
issue ID to the TODO comment so future reviewers can find the tracking ticket.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c35d9686-19f1-49fe-a886-47226120b00b
📒 Files selected for processing (9)
bindata/oauth-apiserver/externaloidc-deploy.yamlpkg/controllers/common/external_oidc.gopkg/controllers/common/external_oidc_test.gopkg/operator/workload/sync_openshift_oauth_apiserver.gopkg/operator/workload/sync_openshift_oauth_apiserver_test.gopkg/operator/workload/testdata/sync_ds_scenario_4.yamlpkg/operator/workload/testdata/sync_ds_scenario_5.yamlpkg/operator/workload/testdata/sync_ds_scenario_6.yamltest/e2e-oidc/external_oidc_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/controllers/common/external_oidc_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/controllers/common/external_oidc.go
e12da8f to
7252295
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e-oidc/external_oidc_test.go (1)
68-69: Use an “external mode active” flag here, not just gate state.Per the PR objective, the new path is selected only when auth type is
OIDCand the feature gate is enabled. This boolean is later used to skip resource/condition/version assertions, so on gate-enabled clusters the IntegratedOAuth phases stop validating parts of the standard oauth-apiserver path.Please derive and pass a flag that reflects the active mode, not just
FeatureGateExternalOIDCExternalClaimsSourcing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e-oidc/external_oidc_test.go` around lines 68 - 69, The current boolean newExternalOIDCArchitectureEnabled only reflects the feature gate; change it to represent "external mode active" by computing a flag that is true only when the cluster auth type is OIDC and the feature gate is enabled (e.g., externalModeActive := (authType == "OIDC") && featureGateEnabled(testCtx, testClient.configClient, features.FeatureGateExternalOIDCExternalClaimsSourcing)). Replace uses of newExternalOIDCArchitectureEnabled with this externalModeActive when passing into the IntegratedOAuth phase checks and when deciding to skip resource/condition/version assertions so the new path is selected only when both auth type and gate are satisfied. Ensure you read auth type from the same config source used elsewhere in the test and keep the original featureGateEnabled call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controllers/common/external_oidc.go`:
- Around line 96-113: The ExternalOIDC feature-gated branch reads from
c.oaasConfigMapLister without ensuring the informer cache is synced; add a
pre-check using c.oaasNamespaceConfigMapsInformer.HasSynced() before calling
ConfigMaps(...).Get(...) and return false with an error like "AuthConfigChecker
oaas configmaps informer has not synced yet" if it hasn't synced, so the code in
the featureGates.Enabled(features.FeatureGateExternalOIDCExternalClaimsSourcing)
block follows the same informer sync pattern as the rest of AuthConfigChecker.
---
Nitpick comments:
In `@test/e2e-oidc/external_oidc_test.go`:
- Around line 68-69: The current boolean newExternalOIDCArchitectureEnabled only
reflects the feature gate; change it to represent "external mode active" by
computing a flag that is true only when the cluster auth type is OIDC and the
feature gate is enabled (e.g., externalModeActive := (authType == "OIDC") &&
featureGateEnabled(testCtx, testClient.configClient,
features.FeatureGateExternalOIDCExternalClaimsSourcing)). Replace uses of
newExternalOIDCArchitectureEnabled with this externalModeActive when passing
into the IntegratedOAuth phase checks and when deciding to skip
resource/condition/version assertions so the new path is selected only when both
auth type and gate are satisfied. Ensure you read auth type from the same config
source used elsewhere in the test and keep the original featureGateEnabled call.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b05dd5f7-fe19-48df-bc7c-354a3fa6ec3a
📒 Files selected for processing (12)
bindata/oauth-apiserver/externaloidc-deploy.yamlpkg/controllers/common/external_oidc.gopkg/controllers/common/external_oidc_test.gopkg/controllers/routercerts/controller_test.gopkg/controllers/webhookauthenticator/webhookauthenticator_controller.gopkg/operator/starter.gopkg/operator/workload/sync_openshift_oauth_apiserver.gopkg/operator/workload/sync_openshift_oauth_apiserver_test.gopkg/operator/workload/testdata/sync_ds_scenario_4.yamlpkg/operator/workload/testdata/sync_ds_scenario_5.yamlpkg/operator/workload/testdata/sync_ds_scenario_6.yamltest/e2e-oidc/external_oidc_test.go
✅ Files skipped from review due to trivial changes (2)
- pkg/controllers/webhookauthenticator/webhookauthenticator_controller.go
- pkg/controllers/common/external_oidc_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/operator/starter.go
ee49819 to
2fa3418
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/operator/workload/sync_openshift_oauth_apiserver_test.go (1)
6-6: Consider replacing deprecatedioutilimport.
io/ioutilis deprecated since Go 1.16. Consider usingio.ReadAllinstead.♻️ Suggested fix
- "io/ioutil" + "io"And at line 548:
- data, err := ioutil.ReadAll(file) + data, err := io.ReadAll(file)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/workload/sync_openshift_oauth_apiserver_test.go` at line 6, Replace the deprecated "io/ioutil" import with "io" and update all usages of ioutil.ReadAll to io.ReadAll (e.g., replace ioutil.ReadAll calls found in the tests around the existing ReadAll usage) ensuring error handling and types remain the same; update the import list to remove "io/ioutil" and add "io".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/operator/workload/sync_openshift_oauth_apiserver_test.go`:
- Line 6: Replace the deprecated "io/ioutil" import with "io" and update all
usages of ioutil.ReadAll to io.ReadAll (e.g., replace ioutil.ReadAll calls found
in the tests around the existing ReadAll usage) ensuring error handling and
types remain the same; update the import list to remove "io/ioutil" and add
"io".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 84e4aa71-dff6-4654-8540-9db87a6f228e
📒 Files selected for processing (12)
bindata/oauth-apiserver/externaloidc-deploy.yamlpkg/controllers/common/external_oidc.gopkg/controllers/common/external_oidc_test.gopkg/controllers/routercerts/controller_test.gopkg/controllers/webhookauthenticator/webhookauthenticator_controller.gopkg/operator/starter.gopkg/operator/workload/sync_openshift_oauth_apiserver.gopkg/operator/workload/sync_openshift_oauth_apiserver_test.gopkg/operator/workload/testdata/sync_ds_scenario_4.yamlpkg/operator/workload/testdata/sync_ds_scenario_5.yamlpkg/operator/workload/testdata/sync_ds_scenario_6.yamltest/e2e-oidc/external_oidc_test.go
✅ Files skipped from review due to trivial changes (1)
- test/e2e-oidc/external_oidc_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/controllers/webhookauthenticator/webhookauthenticator_controller.go
- pkg/controllers/common/external_oidc_test.go
2fa3418 to
943edc6
Compare
|
@everettraven: This pull request references CNTRLPLANE-2523 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 "4.22.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. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/operator/workload/sync_openshift_oauth_apiserver_test.go (1)
164-348: Add oneOIDCAvailable()error-path scenario.The new cases cover
trueandfalse, but not the third outcome fromsyncDeployment()whenauthConfigChecker.OIDCAvailable()returns an error. Since the mock already supportserr, a single failing scenario here would lock down the new feature-gated branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/workload/sync_openshift_oauth_apiserver_test.go` around lines 164 - 348, Add a test case in the table that exercises the error path from authConfigChecker.OIDCAvailable() (used by syncDeployment) by using the existing mockAuthConfigChecker with its err field set (e.g. err: fmt.Errorf("...")). Model the new case on one of the existing entries (use a new golden file like ./testdata/sync_ds_scenario_7.yaml) and set authConfigChecker: &mockAuthConfigChecker{err: someError} and expectedActions to match the behavior of syncDeployment when OIDCAvailable() returns an error (mirror the assertions used for the non-available branch to lock down that feature-gated branch).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controllers/common/external_oidc.go`:
- Around line 110-117: The OIDCAvailable function currently treats the presence
of the openshift-oauth-apiserver/auth-config ConfigMap as sufficient; change the
logic (in OIDCAvailable where you call
c.oaasConfigMapLister.ConfigMaps("openshift-oauth-apiserver").Get("auth-config"))
to also verify the ConfigMap contains the required "auth-config.json" key (and
that its value is non-empty/valid payload) before returning true; if the key is
missing or empty return false, nil, and if needed return a formatted error on
retrieval failures as before.
In `@pkg/operator/replacement_starter.go`:
- Line 146: The MOM starter hardcodes featureGateAccessor via
staticFeatureGateAccessor disabling
features.FeatureGateExternalOIDCExternalClaimsSourcing, causing
CreateOperatorInputFromMOM (invoked from RunApplyConfiguration) to ignore live
cluster gates; change featureGateAccessor to consult live gates instead of
statically disabling that feature (e.g., remove the forced-disabled entry and
use the same live feature gate accessor used by the normal starter or pass
through the real featureGateAccessor), ensuring CreateOperatorInputFromMOM sees
the actual state of features.FeatureGateExternalOIDCExternalClaimsSourcing.
---
Nitpick comments:
In `@pkg/operator/workload/sync_openshift_oauth_apiserver_test.go`:
- Around line 164-348: Add a test case in the table that exercises the error
path from authConfigChecker.OIDCAvailable() (used by syncDeployment) by using
the existing mockAuthConfigChecker with its err field set (e.g. err:
fmt.Errorf("...")). Model the new case on one of the existing entries (use a new
golden file like ./testdata/sync_ds_scenario_7.yaml) and set authConfigChecker:
&mockAuthConfigChecker{err: someError} and expectedActions to match the behavior
of syncDeployment when OIDCAvailable() returns an error (mirror the assertions
used for the non-available branch to lock down that feature-gated branch).
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c165be09-80e3-4cbb-a250-c52b275610a9
📒 Files selected for processing (13)
bindata/oauth-apiserver/externaloidc-deploy.yamlpkg/controllers/common/external_oidc.gopkg/controllers/common/external_oidc_test.gopkg/controllers/routercerts/controller_test.gopkg/controllers/webhookauthenticator/webhookauthenticator_controller.gopkg/operator/replacement_starter.gopkg/operator/starter.gopkg/operator/workload/sync_openshift_oauth_apiserver.gopkg/operator/workload/sync_openshift_oauth_apiserver_test.gopkg/operator/workload/testdata/sync_ds_scenario_4.yamlpkg/operator/workload/testdata/sync_ds_scenario_5.yamlpkg/operator/workload/testdata/sync_ds_scenario_6.yamltest/e2e-oidc/external_oidc_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/controllers/webhookauthenticator/webhookauthenticator_controller.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/controllers/routercerts/controller_test.go
- pkg/operator/workload/sync_openshift_oauth_apiserver.go
- test/e2e-oidc/external_oidc_test.go
- pkg/controllers/common/external_oidc_test.go
- pkg/operator/starter.go
943edc6 to
147d959
Compare
|
Manually verified that the changes work as expected and got a clean run of the oidc e2e tests on a devpreview enabled clusterbot cluster. /verified by @everettraven |
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. |
openshift-ci-robot
removed
the
verified
8d1117c to
9d561be
Compare
|
/retest-required |
| name string | ||
| authInformerSynced bool | ||
| kasInformerSynced bool | ||
| cmInformerSynced bool |
There was a problem hiding this comment.
Just checking if there is a reason why we don't have oaasInformerSynced bool ?
There was a problem hiding this comment.
cmInformerSynced is the thing that is used for both the oauth-apiserver and kube-apiserver namespaced configmap informers.
This means the new line in external_oidc.go that checks if the oauth-apiserver namespaced configmap informer has synced technically would never have it's error return path validated as part of our tests. I didn't feel like covering that path was all that necessary as it is pretty straightforward behavior.
kasInformerSynced is a separate informer entirely to watch for changes to the KubeAPIServer type that is used to communicate the state of the kube-apiserver configuration. This was necessary for the older architecture so that we could ensure that all instances of the kube-apiserver were successfully configured and running with the new OIDC authentication mode before removing the oauth-apiserver.
There isn't a need for us to do this same check here because the oauth-apiserver will always be deployed and all kube-apiserver instances will be configured to communicate with it.
ehearne-redhat
left a comment
ehearne-redhat
left a comment
There was a problem hiding this comment.
I have one concern about not checking oaasInformerSynced not being included in testing, but other than that changes look fine.
|
@everettraven What I would also like to know is if you know why some tests are failing? I can see they are also failing for others too. |
All the tests that contain The required tests that are failing are:
|
|
/retest-required |
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
9d561be to
44a8076
Compare
|
/lgtm |
|
/verified by @everettraven |
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. |
|
/retest-required |
|
@everettraven: 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. |
|
thanks for the explanation @everettraven ! /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ehearne-redhat, everettraven, liouk 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 |
|
The /override ci/prow/e2e-agnostic |
|
@liouk: Overrode contexts on behalf of liouk: ci/prow/e2e-agnostic DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
1a2579c
into
openshift:master
4 participants
Description
This PR updates the cluster-authentication-operator to deploy the oauth-apiserver in the new external OIDC mode when the authentication type is set to
OIDCand the newExternalOIDCExternalClaimsSourcingfeature gate is enabled.Currently, this feature gate is only enabled in DevPreviewNoUpgrade.
For more information as to the motivation of this change, see openshift/enhancements#1907
Summary by CodeRabbit
Release Notes
New Features
Refactor