Skip to content

OSAC-5618: Enable system tenant owned secrets - #1215

Merged
osac-ci-bot merged 1 commit into
osac-project:mainfrom
DakCrowder:osac-5618/allow-system-secrets
Sep 25, 2026
Merged

osac-ci-bot merged 1 commit into
osac-project:mainfrom
DakCrowder:osac-5618/allow-system-secrets

Conversation

@DakCrowder

@DakCrowder DakCrowder commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

In order to support future usecases from Cloud Provider Admins creating system level secrets, we should enable the system tenant to allow for creation of secrets by authorized users.

This defines no consumers, but seeks to unblock/enable them in the future.

Summary

  • Authorization and secret handling: Private secret operations now permit platform administrators to manage Secrets owned by either the shared tenant or the system tenant. Tenant-scoped identities remain restricted from system Secret creation and retrieval. System Secret metadata remains hidden by tenant visibility filtering.
  • Vault and controller: The tenant reconciler now provisions a Vault namespace for the system tenant when needed. System and shared Secrets must use Vault, and creation fails if the Vault backend is not configured.
  • API surface: No proto or public API contract changes are reported. The existing private Secrets API now accepts system-tenant Secrets under the updated authorization rules.
  • Tests: Tests cover platform-admin access and tenant-user restrictions for system Secrets. The controller test that expected system tenants not to provision Vault namespaces was removed. Test execution results were not provided.
  • Other areas: No database, deployment, CI, or documentation changes are reported.

Backward compatibility: System-tenant Secret operations that were previously denied can now succeed for authorized platform administrators. Secret creation for shared or system tenants now returns an error if the backend is not Vault or Vault is not configured.

Risk classification

Applied label: Unavailable. The supplied evidence does not identify a risk label applied to this change or provide criteria for risk:ship, risk:show, or risk:ask. No supported comparison with another classification is available.

@openshift-ci-robot

openshift-ci-robot commented Sep 24, 2026 •

Copy link
Copy Markdown

@DakCrowder: This pull request references OSAC-5618 which is a valid jira issue.

Details

In 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.

@coderabbitai

coderabbitai Bot commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 8931c089-3047-428e-98ab-b1c35fc80b16

📥 Commits

Reviewing files that changed from the base of the PR and between 07154b2 and 744fd60.

📒 Files selected for processing (4)
  • fulfillment-service/internal/controllers/tenant/tenant_reconciler_function.go
  • fulfillment-service/internal/controllers/tenant/tenant_reconciler_function_test.go
  • fulfillment-service/internal/servers/private_secrets_server.go
  • fulfillment-service/internal/servers/private_secrets_server_test.go
💤 Files with no reviewable changes (2)
  • fulfillment-service/internal/controllers/tenant/tenant_reconciler_function_test.go
  • fulfillment-service/internal/controllers/tenant/tenant_reconciler_function.go

Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review.


Walkthrough

The tenant reconciler now provisions Vault namespaces for the system tenant. The private Secret server applies platform authorization to system-tenant Secrets and requires Vault for their creation.

Changes

System Tenant Platform Secrets

Layer / File(s) Summary
System tenant Vault namespace provisioning
fulfillment-service/internal/controllers/tenant/tenant_reconciler_function.go, fulfillment-service/internal/controllers/tenant/tenant_reconciler_function_test.go
The reconciler no longer skips Vault namespace setup for the system tenant. The test that expected system tenants to remain unready without namespace provisioning was removed.
Platform Secret authorization and operations
fulfillment-service/internal/servers/private_secrets_server.go, fulfillment-service/internal/servers/private_secrets_server_test.go
The server extends platform Secret authorization to shared and system tenants. Creation requires Vault and returns tenant-specific errors for an invalid backend or unconfigured Vault. Tests cover platform administrator access and tenant-scoped identity restrictions for system-tenant Secrets.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Feature

Suggested labels: risk:ask

Suggested reviewers: eliorerz

Merge Risk: ⚪ Minimal · up to 744fd

No concrete merge-blocking issue is established by the supplied evidence; normal validation can proceed.

🚥 Pre-merge checks | ✅ 11
✅ Passed checks (11 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
No-Hardcoded-Secrets ✅ Passed The pull request introduces no hardcoded credentials. The production changes add only authorization logic and error messages. The new test literals are resource names (system-admin-secret, `system-p…
No-Weak-Crypto ✅ Passed The PR changes tenant namespace provisioning and secret authorization. The authoritative diff adds no MD5, SHA-1, DES, 3DES, RC4, Blowfish, ECB, custom cryptography, or non-constant-time secret compar…
No-Injection-Vectors ✅ Passed The pull request does not introduce any listed injection vector. The production diff only expands tenant authorization, Vault namespace handling, and error formatting. Added lines contain no SQL const…
Container-Privileges ✅ Passed PASS. The pull request changes only Go source files and tests. It adds no container or Kubernetes manifest changes. No added diff lines contain privileged, hostPID, hostNetwork, hostIPC, SYS_ADMIN, al…
No-Sensitive-Data-In-Logs ✅ Passed The PR adds no logging calls and does not log secret data, passwords, tokens, API keys, or PII. The reconciler only removes a system-tenant guard, which makes existing Vault namespace logs reachable f…
Ai-Attribution ✅ Passed PASS. The reviewed range contains one commit, and its message has no AI-tool mention, Assisted-by, Generated-by, or AI-related Co-Authored-By trailer. The PR description also contains no AI-tool…
Title check ✅ Passed The title clearly and concisely describes the main change: enabling system-tenant-owned secrets.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Create a new PR

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

@osac-ai

osac-ai Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

✅ E2E CaaS Full Install -- Passing

Previously failing; now passing as of this run.

✅ E2E VMaaS Full Install -- Passing

Previously failing; now passing as of this run.

⏳ E2E BMaaS Full Install -- Running

Follow along.

Total AI diagnostic cost for this PR: $0.6787 (199134 input + 23372 output tokens across 6 diagnoses)

@github-actions

github-actions Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

🧭 Jobs Selection (informational only)

E2E Suites

Suite Decision Source Reason
VMAAS regression gemini-escalation touches core fulfillment-service secret and tenant logic
CAAS regression gemini-escalation touches core fulfillment-service secret and tenant logic
BMAAS sanity gemini-inconclusive AI judgment was inconclusive for this suite

AI judgment confidence: 90%.
Estimated cost: $0.0132 (3978 input + 438 output tokens, gemini-3.1-pro-preview)

Unit Tests

Job Decision Reason
fulfillment-service run This workflow has no per-component scoping -- runs for any non-doc change
osac-metering run This workflow has no per-component scoping -- runs for any non-doc change
osac-metering/adapters run This workflow has no per-component scoping -- runs for any non-doc change
osac-metering/schema run This workflow has no per-component scoping -- runs for any non-doc change

Integration Tests

Job Decision Reason
fulfillment-service run This workflow has no per-component scoping -- runs for any non-doc change
osac-operator run This workflow has no per-component scoping -- runs for any non-doc change
bare-metal-fulfillment-operator run This workflow has no per-component scoping -- runs for any non-doc change
osac-aap run This workflow has no per-component scoping -- runs for any non-doc change
osac-installer run This workflow has no per-component scoping -- runs for any non-doc change

Helm Lint

Job Decision Reason
osac-operator skip No changed files matched this job's path filter
bare-metal-fulfillment-operator skip No changed files matched this job's path filter
fulfillment-service skip No changed files matched this job's path filter
osac-aap skip No changed files matched this job's path filter
osac-csi-driver skip No changed files matched this job's path filter
osac-metering skip No changed files matched this job's path filter
osac-installer skip No dependent component chart changed

Checks & Builds

Job Decision Reason
Check generated code (proto) skip No changed files matched this job's path filter
fulfillment-service checks run Matches this job's path filter
Build container image (osac-operator) skip No changed files matched this job's path filter
Build container image (bare-metal-fulfillment-operator) skip No changed files matched this job's path filter
ansible-lint (osac-aap) skip No changed files matched this job's path filter
Darwin keychain tests skip No changed files matched this job's path filter

Every table above is informational only -- nothing here gates whether a job actually runs. The E2E Suites table can use AI judgment for ambiguous files; every other table is deterministic-only (no AI).

@github-actions

Copy link
Copy Markdown

E2E on CodeRabbit approval

CodeRabbit APPROVED — starting expensive e2e (PR run replay).

  • Started: 3/3
  • Did not POST e2e-*-gate Checks API checks (native jobs report; required gates stay pending until then).

@CrystalChun CrystalChun left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci

openshift-ci Bot commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CrystalChun, DakCrowder

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

The pull request process is described 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

@github-actions

Copy link
Copy Markdown

E2E on lgtm

All merge-required e2e gates already success on HEAD — skipping replay.

Accepted: e2e-vmaas-gate, e2e-bmaas-gate, e2e-caas-gate on 80c3ab8.

@osac-ci-bot
osac-ci-bot added this pull request to the merge queue Sep 25, 2026
Merged via the queue into osac-project:main with commit d2781f6 Sep 25, 2026
105 of 106 checks passed

This branch was successfully deployed

1 active deployment
e2e-test — 80c3ab89 Deployed Sep 25, 2026 by DakCrowder via e2e-bmaas-full-install / e2e #7517
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants