OSAC-2982: Add client_secret_secret to IdentityProvider with dual-path reconciler

[danielerez]

Summary

• Add SecretLocalReference client_secret_secret to OidcConfig (public + private), mutually exclusive with inline client_secret
• Add server-side validation: reject empty {}, mutual exclusion (including update-mask-aware conflict vs the stored IDP), existence check via secrets DAO, and write-back of resolved {id, name}
• IdentityProvider reconciler prefers the Secret ref (Secrets.Get with controller auth, extract data["value"]) and falls back to inline client_secret
• Add unit tests for Create/Update validation and secret resolution / fallback / missing value

Context

Next vertical slice of the secret references migration (OSAC-2953), following OSAC-2978. Uses SecretLocalReference (id + name) rather than a string field.

No DB migration needed. Resources use GenericDAO with a JSON data column, so proto field additions are picked up automatically.

Jira

https://issues.redhat.com/browse/OSAC-2982

Test plan

• Create IDP with client_secret_secret by id — resolves and stores id + name
• Create IDP with client_secret_secret by name — resolves and stores id + name
• Create with both client_secret and client_secret_secret — returns INVALID_ARGUMENT
• Create with nonexistent secret reference — returns INVALID_ARGUMENT
• Create with empty {} ref — returns INVALID_ARGUMENT
• Create with inline client_secret only — still works unchanged
• Update with a valid secret ref — succeeds
• Update that adds a secret ref while DB still has inline client_secret (mask-aware) — returns INVALID_ARGUMENT
• Update with both fields set — returns INVALID_ARGUMENT
• Reconciler resolves secret via Secrets API data["value"]
• Reconciler falls back to inline client_secret when no ref is set
• Reconciler errors when secret is missing data["value"]
• IdentityProvider unit suite passes

[openshift-ci-robot]

@danielerez: This pull request references OSAC-2982 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 task to target the "5.1.0" version, but no target version was set.

Details

In response to this:

Summary

• Add SecretLocalReference client_secret_secret to OidcConfig (public + private), mutually exclusive with inline client_secret
• Add server-side validation: reject empty {}, mutual exclusion (including update-mask-aware conflict vs the stored IDP), existence check via secrets DAO, and write-back of resolved {id, name}
• IdentityProvider reconciler prefers the Secret ref (Secrets.Get with controller auth, extract data["value"]) and falls back to inline client_secret
• Add unit tests for Create/Update validation and secret resolution / fallback / missing value

Context

Next vertical slice of the secret references migration (OSAC-2953), following OSAC-2978. Uses SecretLocalReference (id + name) rather than a string field.

No DB migration needed. Resources use GenericDAO with a JSON data column, so proto field additions are picked up automatically.

Jira

https://issues.redhat.com/browse/OSAC-2982

Test plan

• Create IDP with client_secret_secret by id — resolves and stores id + name
• Create IDP with client_secret_secret by name — resolves and stores id + name
• Create with both client_secret and client_secret_secret — returns INVALID_ARGUMENT
• Create with nonexistent secret reference — returns INVALID_ARGUMENT
• Create with empty {} ref — returns INVALID_ARGUMENT
• Create with inline client_secret only — still works unchanged
• Update with a valid secret ref — succeeds
• Update that adds a secret ref while DB still has inline client_secret (mask-aware) — returns INVALID_ARGUMENT
• Update with both fields set — returns INVALID_ARGUMENT
• Reconciler resolves secret via Secrets API data["value"]
• Reconciler falls back to inline client_secret when no ref is set
• Reconciler errors when secret is missing data["value"]
• IdentityProvider unit suite passes

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]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danielerez

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:

• fulfillment-service/OWNERS [danielerez]

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

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:53 PM UTC · Completed 3:11 PM UTC

Commit: 60a8d16 · View workflow run →

[fullsend-ai-review]

Review

Findings

Medium

• [error handling / observability] fulfillment-service/internal/controllers/identityprovider/identity_provider_reconciler_function.go:183 — When resolveClientSecret returns an error (secret not found, fetch failure, missing data["value"]), syncToIDP returns at line 184 without updating the IDP status. The IDP stays in UNKNOWN phase with no status message while the controller retries indefinitely. In contrast, IDP creation failure (lines 197-199) writes an ERROR phase and a descriptive message. A misconfigured client_secret_secret reference causes silent retries with no user feedback.
  Remediation: Set the status phase to ERROR with a descriptive message on resolveClientSecret failure, consistent with the IDP creation error path.

Low

• [data-exposure] fulfillment-service/internal/servers/private_identity_providers_server.go:144 — redact() clears client_secret but not the new client_secret_secret reference. The reference contains only id/name (no secret material), but consider clearing it for consistency or documenting the intentional non-redaction.

• [test adequacy] fulfillment-service/internal/controllers/identityprovider/identity_provider_reconciler_function_test.go:648 — Secret resolution test sets identityProvidersClient to nil, skipping the production full-object fetch path. Consider setting it so the complete fetch-resolve-sync flow is covered in a single test.

• [test adequacy] fulfillment-service/internal/controllers/identityprovider/identity_provider_reconciler_function_test.go:628 — No test for the Secrets.Get transient error path. A test would document the retry-without-status-update behavior and catch regressions.

• [test-organization] fulfillment-service/internal/servers/private_identity_providers_server_test.go:627 — Secret seeded with Tenant: testTenant while createIdp uses Tenant: "my-tenant". Guest tenancy allows this to pass without exercising tenant isolation for secret references.

• [test-coverage-gap] fulfillment-service/internal/servers/private_identity_providers_server.go:269 — Reverse update scenario (setting inline client_secret when DB has client_secret_secret) has validation code at lines 269-272 but no dedicated test. The code is symmetric and tested in one direction.

• [duplicate-validation] fulfillment-service/internal/servers/private_identity_providers_server.go:276 — validateClientSecretSecret duplicates the reference interceptor's existence check. The mutual-exclusion and update-mask checks are server-specific and necessary.

• [validation] fulfillment-service/proto/private/osac/private/v1/identity_provider_type.proto:121 — No buf.validate annotations on client_secret_secret. Consistent with existing SecretLocalReference usages; consider adding proto-level mutual exclusion validation as a follow-up.

• [error-handling-idiom] fulfillment-service/internal/controllers/identityprovider/identity_provider_reconciler_function.go:225 — fmt.Errorf without %w wrapping for nil-client and empty-id errors, while the fetch error at line 236 uses %w. The non-wrapping may be intentional for sentinel-style errors.

• [authorization-tenant-isolation] fulfillment-service/internal/servers/private_identity_providers_server.go:289 — Empty tenant/project strings passed to NewDAOLookupFunc. Functionally correct (DAO enforces tenant scoping from gRPC context) but aesthetically unclean.

Previous run

Review

Findings

Medium

• [error-handling-gap] fulfillment-service/internal/servers/private_identity_providers_server.go — getExistingIdentityProvider does not handle the DAO ErrNotFound case. When the identity provider does not exist in the database (e.g., deleted between the update request and the validation fetch), the DAO error is propagated as a raw error. The caller (validateClientSecretMutualExclusionForUpdate) returns this directly, surfacing as an unstructured internal error rather than a proper gRPC NOT_FOUND status. While the subsequent generic.Update would also fail, it would produce the correct gRPC status — this path produces an opaque internal error instead.
  Remediation: Check for the IsNotFound interface in getExistingIdentityProvider and return (nil, false, nil) instead, since a not-found existing object means there is nothing to conflict with.

Low

• [edge-case] fulfillment-service/internal/servers/private_identity_providers_server.go — validateClientSecretSecret is called in the Update path without checking whether client_secret_secret is included in the update mask. If a client sets the field on the object but does not include it in the mask, validation still runs — potentially performing an unnecessary DAO lookup or rejecting a valid update. This follows the established pattern (validatePullSecretSecret on the clusters server also lacks this guard).

• [data-exposure] fulfillment-service/internal/servers/private_identity_providers_server.go — The redact function clears client_secret from event payloads but does not address the new client_secret_secret field. Since SecretLocalReference only contains id and name (not the secret value), this is not a material secret leak, but it is inconsistent with the function's documented purpose of redacting secrets from events.

• [test-coverage-gap] fulfillment-service/internal/controllers/identityprovider/identity_provider_reconciler_function_test.go — The reconciler test suite does not cover the case where the Secrets API Get call returns an error (e.g., network failure, permission denied). The resolveClientSecret error wrapping path is untested.

• [missing-test-scenario] fulfillment-service/internal/servers/private_identity_providers_server_test.go — Missing test for the reverse conflict scenario on Update: existing IDP has client_secret_secret, update adds inline client_secret via mask. Only the forward direction (existing has inline, update adds ref) is tested.

• [error-handling-idiom] fulfillment-service/internal/controllers/identityprovider/identity_provider_reconciler_function.go — When secretsClient.Get fails, resolveClientSecret returns the error directly, causing the reconciler to retry. When idpClient.CreateIdentityProvider fails later in the same method, the reconciler sets PHASE_ERROR and returns nil (terminal). The inconsistency is defensible — secret resolution failures are likely transient — but worth noting.

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

Auto-dismissed: only Prow labels gate merging

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:07 PM UTC · Completed 7:28 PM UTC

Commit: 1a1e462 · View workflow run →