Skip to content

Conversation

@bertinatto
Copy link
Member

@coderabbitai
Copy link

coderabbitai bot commented Nov 19, 2025

Walkthrough

The pull request adds OAuth API server deployment management to the resource controller by registering the openshift-oauth-apiserver/apiserver Deployment in the managed resources, and introduces comprehensive test fixtures with expected output manifests and input configurations for OAuth API server creation scenarios.

Changes

Cohort / File(s) Summary
Source code modification
pkg/cmd/mom/output_resources_command.go
Added ExactDeployment("openshift-oauth-apiserver", "apiserver") to ManagementResources.ExactResources list to include OAuth API server deployment in managed resources.
Test expected outputs - Authentication CR metadata
test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/expected-output/Management/ApplyStatus/cluster-scoped-resources/operator.openshift.io/authentications/*-cluster.yaml
Added three metadata and configuration files defining cluster-scoped Authentication resource with ApplyStatus action, field manager references, and operational status conditions tracking APIServerDeployment availability and progress.
Test expected outputs - Event manifests
test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/expected-output/Management/Create/namespaces/openshift-authentication-operator/core/events/*-authentication-operator.*.yaml
Added Kubernetes Event manifest files documenting deployment creation events with metadata, involved object references, and event timing information.
Test expected outputs - Deployment manifests
test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/expected-output/Management/Create/namespaces/openshift-oauth-apiserver/apps/deployments/*-apiserver.yaml
Added Deployment manifest files specifying OAuth API server Pod configuration with TLS, etcd integration, audit logging, security context, resource limits, health probes, and volume mounts.
Test expected output - Controller results
test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/expected-output/Management/controller-results.yaml
Added controller results summary mapping controller status values with OAuthAPIServerController-WorkloadWorkloadController marked as Succeeded.
Test input data - Cluster-scoped resources
test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/input-dir/cluster-scoped-resources/config.openshift.io/*
Added three cluster configuration files defining Authentication resource, ClusterVersion with update tracking, and Infrastructure platform specification for BareMetal deployments.
Test input data - Operator Authentication CR
test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/input-dir/cluster-scoped-resources/operator.openshift.io/authentications/cluster.yaml
Added operator.openshift.io Authentication custom resource with comprehensive observedConfig, status conditions, and configuration tracking for OAuth server and API server deployments.
Test configuration
test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/test.yaml
Added test definition specifying OAuth API server creation test scenario with controller references, input requirements, and ApplyConfiguration test type.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • The production code change is minimal (single resource addition to managed deployments list)
  • Test data additions are extensive but primarily static manifest files without complex logic
  • Manifest complexity warrants verification of deployment configuration, particularly:
    • Pod template security context and privilege settings in the Deployment manifest
    • TLS configuration and certificate references for OAuth API server
    • Status condition mappings and field manager assignments in Authentication CR
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between d7f0c37 and 8e22d1f.

📒 Files selected for processing (14)
  • pkg/cmd/mom/output_resources_command.go (1 hunks)
  • test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/expected-output/Management/ApplyStatus/cluster-scoped-resources/operator.openshift.io/authentications/c5b3-body-cluster.yaml (1 hunks)
  • test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/expected-output/Management/ApplyStatus/cluster-scoped-resources/operator.openshift.io/authentications/c5b3-metadata-cluster.yaml (1 hunks)
  • test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/expected-output/Management/ApplyStatus/cluster-scoped-resources/operator.openshift.io/authentications/c5b3-options-cluster.yaml (1 hunks)
  • test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/expected-output/Management/Create/namespaces/openshift-authentication-operator/core/events/638b-body-authentication-operator.17fe72c59b829800.a1874ea9.yaml (1 hunks)
  • test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/expected-output/Management/Create/namespaces/openshift-authentication-operator/core/events/638b-metadata-authentication-operator.17fe72c59b829800.a1874ea9.yaml (1 hunks)
  • test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/expected-output/Management/Create/namespaces/openshift-oauth-apiserver/apps/deployments/7350-body-apiserver.yaml (1 hunks)
  • test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/expected-output/Management/Create/namespaces/openshift-oauth-apiserver/apps/deployments/7350-metadata-apiserver.yaml (1 hunks)
  • test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/expected-output/controller-results.yaml (1 hunks)
  • test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/input-dir/cluster-scoped-resources/config.openshift.io/authentications/cluster.yaml (1 hunks)
  • test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/input-dir/cluster-scoped-resources/config.openshift.io/clusterversions.yaml (1 hunks)
  • test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/input-dir/cluster-scoped-resources/config.openshift.io/infrastructures.yaml (1 hunks)
  • test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/input-dir/cluster-scoped-resources/operator.openshift.io/authentications/cluster.yaml (1 hunks)
  • test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/test.yaml (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

  • test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/test.yaml
  • pkg/cmd/mom/output_resources_command.go
  • test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/expected-output/controller-results.yaml
  • test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/expected-output/Management/Create/namespaces/openshift-authentication-operator/core/events/638b-body-authentication-operator.17fe72c59b829800.a1874ea9.yaml
  • test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/input-dir/cluster-scoped-resources/config.openshift.io/infrastructures.yaml
  • test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/input-dir/cluster-scoped-resources/config.openshift.io/authentications/cluster.yaml
  • test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/input-dir/cluster-scoped-resources/operator.openshift.io/authentications/cluster.yaml
  • test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/expected-output/Management/Create/namespaces/openshift-authentication-operator/core/events/638b-metadata-authentication-operator.17fe72c59b829800.a1874ea9.yaml
  • test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/input-dir/cluster-scoped-resources/config.openshift.io/clusterversions.yaml
  • test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/expected-output/Management/ApplyStatus/cluster-scoped-resources/operator.openshift.io/authentications/c5b3-body-cluster.yaml
  • test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/expected-output/Management/ApplyStatus/cluster-scoped-resources/operator.openshift.io/authentications/c5b3-metadata-cluster.yaml
  • test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/expected-output/Management/Create/namespaces/openshift-oauth-apiserver/apps/deployments/7350-metadata-apiserver.yaml
  • test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/expected-output/Management/Create/namespaces/openshift-oauth-apiserver/apps/deployments/7350-body-apiserver.yaml
  • test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/expected-output/Management/ApplyStatus/cluster-scoped-resources/operator.openshift.io/authentications/c5b3-options-cluster.yaml
🪛 Checkov (3.2.334)
test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/expected-output/Management/Create/namespaces/openshift-oauth-apiserver/apps/deployments/7350-body-apiserver.yaml

[medium] 1-215: Containers should not run with allowPrivilegeEscalation

(CKV_K8S_20)


[high] 1-215: Container should not be privileged

(CKV_K8S_16)


[medium] 1-215: Minimize the admission of root containers

(CKV_K8S_23)

🔇 Additional comments (14)
pkg/cmd/mom/output_resources_command.go (1)

33-33: LGTM! OAuth API server deployment properly registered.

The addition correctly registers the oauth-apiserver deployment as a managed resource, following the established pattern for other deployments in the list.

test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/expected-output/Management/Create/namespaces/openshift-authentication-operator/core/events/638b-metadata-authentication-operator.17fe72c59b829800.a1874ea9.yaml (1)

1-9: Test fixture looks good.

The event metadata structure is well-formed and appropriately captures the expected output for the OAuth API server deployment creation event.

test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/input-dir/cluster-scoped-resources/config.openshift.io/infrastructures.yaml (1)

1-99: Test input fixture is properly structured.

The Infrastructure resource provides appropriate test data for a BareMetal cluster configuration with IPv6 networking, suitable for validating OAuth API server deployment in this environment.

test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/input-dir/cluster-scoped-resources/config.openshift.io/authentications/cluster.yaml (1)

1-75: Authentication configuration fixture is well-formed.

The test input properly represents a cluster-scoped Authentication resource with appropriate metadata, spec, and status fields for testing the OAuth API server controller.

test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/input-dir/cluster-scoped-resources/operator.openshift.io/authentications/cluster.yaml (1)

1-1115: Comprehensive test fixture provides good coverage.

This detailed Authentication operator resource includes extensive controller status conditions, observed configuration, and managed fields that provide realistic test coverage for the OAuth API server controller integration.

test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/expected-output/Management/Create/namespaces/openshift-authentication-operator/core/events/638b-body-authentication-operator.17fe72c59b829800.a1874ea9.yaml (1)

1-21: Expected event output is correctly structured.

The Event resource appropriately captures the deployment creation action with a clear message and proper metadata, providing good validation for the controller's event generation.

test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/expected-output/Management/ApplyStatus/cluster-scoped-resources/operator.openshift.io/authentications/c5b3-options-cluster.yaml (1)

1-2: ApplyStatus options are correctly configured.

The fieldManager and force settings appropriately configure server-side apply for the OAuth API server controller's status updates.

test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/expected-output/controller-results.yaml (1)

1-81: Controller results properly validate test scope.

The expected controller results appropriately show only the OAuthAPIServerController-WorkloadWorkloadController as succeeded, with other controllers skipped, which correctly reflects the minimal test scenario for the OAuth API server deployment.

test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/expected-output/Management/ApplyStatus/cluster-scoped-resources/operator.openshift.io/authentications/c5b3-metadata-cluster.yaml (1)

1-9: Skip: This file is test fixture data with simple metadata — no issues to review.

test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/expected-output/Management/Create/namespaces/openshift-oauth-apiserver/apps/deployments/7350-metadata-apiserver.yaml (1)

1-9: Skip: This file is test fixture data with simple metadata — no issues to review.

test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/test.yaml (1)

1-16: Skip: Test configuration looks well-structured with clear controller setup and documented input requirements for the OAuth API server controller test.

test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/input-dir/cluster-scoped-resources/config.openshift.io/clusterversions.yaml (1)

1-166: Skip: This is realistic test input data mirroring actual OpenShift ClusterVersion objects — no correctness issues.

test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/expected-output/Management/ApplyStatus/cluster-scoped-resources/operator.openshift.io/authentications/c5b3-body-cluster.yaml (1)

1-30: Skip: This is test expected output for Authentication resource status — correctly reflects the initial state when OAuth API server deployment is being created (no pods yet, progressing).

test-data/apply-configuration/overall/oauth-apiserver-creation-minimal/expected-output/Management/Create/namespaces/openshift-oauth-apiserver/apps/deployments/7350-body-apiserver.yaml (1)

1-215: Security settings are intentional for OAuth API server requirements.

The oauth-apiserver deployment requires a privileged security context to operate correctly in OpenShift, which explains the flags from static analysis (Checkov CKV_K8S_16/20/23). The annotation at line 5 (openshift.io/required-scc: privileged) confirms this is by design.

The manifest is otherwise well-structured with proper liveness/readiness/startup probes, resource requests, volume mounts for certificates and audit logging, and node affinity constraints for master nodes. This test fixture appropriately mirrors the actual OAuth API server deployment configuration.

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci openshift-ci bot requested review from ibihim and liouk November 19, 2025 21:41
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 20, 2025

@bertinatto: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

libraryoutputresources.ExactConfigMap("openshift-authentication", "audit"),
libraryoutputresources.ExactConfigMap("openshift-authentication", "v4-0-config-system-trusted-ca-bundle"),
libraryoutputresources.ExactDeployment("openshift-authentication", "oauth-openshift"),
libraryoutputresources.ExactDeployment("openshift-oauth-apiserver", "apiserver"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, this should be defined on the Management cluster.

- config.openshift.io/clusterversions: not really used by the controller, but it's required to start the operator CreateOperatorStarter/prepareOauthOperator
- config.openshift.io/authentications/cluster: required by the controller
- config.openshift.io/infrastructures/cluster: required by the controller
- operator.openshift.io/authentications/cluster: required by the controller
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this holds the configuration for the server, right ?

as a next step would could write a test that would create a valid config for the server.

@p0lyn0mial
Copy link
Contributor

/lgtm

@p0lyn0mial
Copy link
Contributor

/assign @liouk

for approval

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 20, 2025
@liouk
Copy link
Member

liouk commented Nov 24, 2025

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 24, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bertinatto, liouk, p0lyn0mial

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

The pull request process is described here

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 24, 2025
@bertinatto
Copy link
Member Author

/retitle NO-JIRA: om: add a test for the oauth-apiserver controller
/verified by ci

@openshift-ci openshift-ci bot changed the title om: add a test for the oauth-apiserver controller NO-JIRA: om: add a test for the oauth-apiserver controller Nov 24, 2025
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 24, 2025
@openshift-ci-robot
Copy link
Contributor

@bertinatto: This pull request explicitly references no jira issue.

In response to this:

/assign @p0lyn0mial @benluddy

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 openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Nov 24, 2025
@openshift-ci-robot
Copy link
Contributor

@bertinatto: This PR has been marked as verified by ci.

In response to this:

/retitle NO-JIRA: om: add a test for the oauth-apiserver controller
/verified by ci

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-merge-bot openshift-merge-bot bot merged commit de25e7d into openshift:master Nov 24, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. verified Signifies that the PR passed pre-merge verification criteria

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants