Skip to content

DO-NOT-MERGE: POC: adapt the existing oauth-apiserver component for ExternalOIDCExternalClaimsSourcing - #8920

Closed
liouk wants to merge 4 commits into
openshift:mainfrom
liouk:poc-adapt-existing-component
Closed

liouk wants to merge 4 commits into
openshift:mainfrom
liouk:poc-adapt-existing-component

Conversation

@liouk

@liouk liouk commented Jul 3, 2026

Copy link
Copy Markdown
Member

This PR is for demonstration purposes only and should not be merged as-is.

/hold

Explores keeping a single oauth-apiserver component and forking its deployment adapt logic based on auth mode. When the ExternalOIDCExternalClaimsSourcing gate is enabled and auth type is OIDC, the existing oauth-apiserver component stays alive but uses a separate adapt function (adaptDeploymentOIDC) that rewrites the container to run the external-oidc subcommand with a different config. Shares the predicate, manifests, and component registration — only the deployment shape diverges.

liouk added 4 commits June 2, 2026 11:20
The new feature gate will initially be enabled for TechPreviewNoUpgrade.
This feature extends ExternalOIDC with a webhook that enables sourcing
claims from external sources.
Rewrite ConfigOAuthEnabled as an explicit switch on known authentication
types instead of a negation check against OIDC. Inline the private
oauthEnabled helper into HCPOAuthEnabled and remove unused HCOAuthEnabled.
…ourcing is enabled

When the ExternalOIDCExternalClaimsSourcing feature gate is enabled,
configure KAS to always use the webhook token authenticator regardless
of authentication type; with that feature, even external OIDC will go
via the webhook instead of --authentication-config.
…dapt functions

The oauth-apiserver deployment manifest is slimmed to base-only fields
shared by both modes. Each mode gets its own adapt function and file:
adaptForOAuth (IntegratedOAuth) and adaptForExternalOIDC (external-oidc).
The component predicate is updated to keep the component alive when
ExternalOIDCExternalClaimsSourcing is enabled.
@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

@openshift-ci openshift-ci Bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jul 3, 2026
@openshift-ci

openshift-ci Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci

openshift-ci Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: liouk
Once this PR has been reviewed and has the lgtm label, please assign csrwng for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added area/api Indicates the PR includes changes for the API area/cli Indicates the PR includes changes for CLI area/control-plane-operator Indicates the PR includes changes for the control plane operator - in an OCP release area/hypershift-operator Indicates the PR includes changes for the hypershift operator and API - outside an OCP release and removed do-not-merge/needs-area labels Jul 3, 2026
@codecov

codecov Bot commented Jul 3, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.73%. Comparing base (8c162c4) to head (527aa58).
⚠️ Report is 699 commits behind head on main.

Files with missing lines Patch % Lines
support/util/oauth.go 0.00% 13 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8920      +/-   ##
==========================================
+ Coverage   40.69%   41.73%   +1.04%     
==========================================
  Files         755      518     -237     
  Lines       93373    80905   -12468     
==========================================
- Hits        37994    33763    -4231     
+ Misses      52646    44796    -7850     
+ Partials     2733     2346     -387     
Files with missing lines Coverage Δ
support/util/oauth.go 0.00% <0.00%> (ø)

... and 355 files with indirect coverage changes

Flag Coverage Δ
cmd-support 35.88% <0.00%> (+1.17%) ⬆️
cpo-hostedcontrolplane ?
cpo-other 45.10% <ø> (+3.70%) ⬆️
hypershift-operator 50.70% <ø> (-0.14%) ⬇️
other 31.69% <ø> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@hypershift-jira-solve-ci

Copy link
Copy Markdown
Contributor

I now have all evidence from all four jobs. Let me compile the final report.

Test Failure Analysis Complete

Job Information

  • PR: #8920 — DO-NOT-MERGE: POC: adapt the existing oauth-apiserver component for ExternalOIDCExternalClaimsSourcing
  • Repository: openshift/hypershift
  • Author: liouk
  • Failed Jobs: 4 (Unit Tests, Lint, Verify, codecov/patch)

Test Failure Analysis

Error

1) Unit Tests (cpo-hostedcontrolplane): TestControlPlaneComponents FAIL — 5 fixture YAML files
   have stale volume mount ordering for openshift-oauth-apiserver deployment

2) Lint: 3 files have incorrectly ordered Go imports (gci violations)
   - config_test.go:5:1, deployment_oauth.go:8:1, deployment_oauth_test.go:9:1

3) Verify: config_test.go not formatted — `go fmt` produces a diff

4) codecov/patch: support/util/oauth.go has 0% coverage on 13 new lines (target: 40.69%)

Summary

All four failures stem from a single root cause: the PR refactors the openshift-oauth-apiserver deployment to support a new ExternalOIDCExternalClaimsSourcing feature gate, splitting deployment.go into deployment_oauth.go (OAuth path) and deployment_oidc.go (OIDC path), but the author did not run the standard pre-commit formatting and fixture-regeneration commands (go fmt, gci write, UPDATE=true go test ./...). The deployment YAML asset was stripped down (removing args, volume mounts, probes, sidecar containers) so that each path rebuilds it programmatically, which changed volume mount ordering in the rendered output vs. the golden fixture files. The import blocks in three new/modified files violate the project's gci import grouping rules, and one file also has go fmt formatting issues. Additionally, the new helper functions in support/util/oauth.go (HCPExternalOIDCEnabled, refactored ConfigOAuthEnabled) lack unit test coverage.

Root Cause

The PR introduces the ExternalOIDCExternalClaimsSourcing feature gate and restructures the oauth-apiserver component to support two deployment paths:

  1. OAuth path (adaptForOAuth in deployment_oauth.go): The existing behavior where the oauth-apiserver runs with full args, etcd connections, audit logging, liveness/readiness probes, and an audit-log sidecar container. This path is used when OAuth is enabled (the default).

  2. OIDC path (adaptForExternalOIDC in deployment_oidc.go): A minimal deployment where the oauth-apiserver runs in external-oidc mode with only an auth-config, serving cert, and TLS configuration. No etcd, no audit logging, no probes, no sidecar.

The deployment.yaml asset was stripped to a bare minimum (only the serving-cert volume mount remains in the template), and each path programmatically adds its required args, volume mounts, volumes, probes, and sidecar containers. This architectural change causes the serving-cert volume mount to appear at a different position in the final rendered YAML (it's now first, from the template, instead of last), which triggers fixture mismatches.

Specific failures:

  • Unit Tests: TestControlPlaneComponents compares rendered deployment YAML against 5 golden fixture files (default, TechPreviewNoUpgrade, IBMCloud, GCP, AROSwift). The serving-cert volume mount now appears before aggregator-ca instead of after it, and the audit-log sidecar script uses | instead of |- block scalar style. The fixtures were not regenerated with UPDATE=true go test ./....

  • Lint: The three new/modified Go files have import blocks that don't follow the project's gci (Go Comment Imports) ordering convention. Specifically, configv1 "github.com/openshift/api/config/v1" is grouped incorrectly with the hypershift imports instead of being in a separate block.

  • Verify: config_test.go was not run through go fmt before committing, so the verify step's go fmt ./... produces a diff and the dirty-tree check fails.

  • codecov/patch: The refactored ConfigOAuthEnabled() function and new HCPExternalOIDCEnabled() function in support/util/oauth.go have 0% test coverage (13 lines), failing the 40.69% patch coverage threshold.

Recommendations
  1. Regenerate test fixtures — Run UPDATE=true go test ./control-plane-operator/controllers/hostedcontrolplane/... and commit the updated fixture YAML files. This resolves the Unit Tests failure.

  2. Fix Go formatting — Run go fmt ./... to fix config_test.go. This resolves the Verify failure.

  3. Fix import ordering — Run the project's gci formatter (or make lint-fix) on the three flagged files:

    • control-plane-operator/controllers/hostedcontrolplane/v2/kas/config_test.go
    • control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment_oauth.go
    • control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment_oauth_test.go

    This resolves the Lint failure.

  4. Add unit tests for support/util/oauth.go — Cover HCPExternalOIDCEnabled() and the refactored ConfigOAuthEnabled() switch logic with tests. This resolves the codecov/patch failure.

  5. Shortcut: Running make verify locally before pushing would have caught all four issues.

Evidence
Evidence Detail
Unit Test failure TestControlPlaneComponents — 5 fixture files have stale volume mount ordering: serving-cert now appears before aggregator-ca due to deployment template refactoring
Affected fixtures testdata/openshift-oauth-apiserver/{default,TechPreviewNoUpgrade,IBMCloud,GCP,AROSwift}/zz_fixture_*_deployment.yaml
Test hint "If this is expected, re-run the test with UPDATE=true go test ./... to update the fixtures."
Lint error 1 config_test.go:5:1: File is not properly formatted (gci)
Lint error 2 deployment_oauth.go:8:1: File is not properly formatted (gci)
Lint error 3 deployment_oauth_test.go:9:1: File is not properly formatted (gci)
Lint note 2/5 issues with text "File is not properly formatted" were hidden, use --max-same-issues (5 total gci issues, 3 shown)
Verify failure config_test.go: needs update — go fmt ./... modified the file, git diff --exit-code HEAD failed
codecov/patch support/util/oauth.go — 13 new lines at 0.00% coverage vs. 40.69% target
Key code change deployment.go deleted → split into deployment_oauth.go (265 lines, OAuth path) + deployment_oidc.go (63 lines, OIDC path)
Feature gate ExternalOIDCExternalClaimsSourcing added to Default and TechPreviewNoUpgrade feature sets
Deployment asset openshift-oauth-apiserver/deployment.yaml stripped from 142 lines to 26 lines (bare template)

@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 30, 2026
@openshift-ci

openshift-ci Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

PR needs rebase.

Details

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.

@liouk

liouk commented Sep 11, 2026

Copy link
Copy Markdown
Member Author

Closing this POC -- we'll use #8921 instead.

/close

@openshift-ci openshift-ci Bot closed this Sep 11, 2026
@openshift-ci

openshift-ci Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

@liouk: Closed this PR.

Details

In response to this:

Closing this POC -- we'll use #8921 instead.

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/api Indicates the PR includes changes for the API area/cli Indicates the PR includes changes for CLI area/control-plane-operator Indicates the PR includes changes for the control plane operator - in an OCP release area/hypershift-operator Indicates the PR includes changes for the hypershift operator and API - outside an OCP release do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant