fix(oracle): return NoSecretErr on a missing secret so deletionPolicy applies

[somaz94]

Problem Statement

The Oracle provider returns the sanitised OCI SDK error when a secret is missing in the vault, not esv1.NoSecretErr. The ExternalSecret controller only applies spec.target.deletionPolicy when the read error satisfies errors.Is(err, esv1.NoSecretErr), so deleting a secret in OCI Vault never propagates to the Kubernetes Secret, even though the support matrix advertises DeletionPolicy Merge/Delete for Oracle.

Related Issue

Fixes #6572

Proposed Changes

GetSecret now returns esv1.NoSecretErr when OCI answers 404, and keeps returning the sanitised error for anything else. The check lives in a new isSecretNotFoundErr helper that asserts common.ServiceError rather than common.ServiceErrorRichInfo, since ServiceError is the interface that declares GetHTTPStatusCode and a plain service error carries no rich info.

getSecretBundleCode now calls that same helper instead of repeating the assertion inline. That part is pure de-duplication: common.IsServiceError is the same plain type assertion the inlined code was already doing (it is not errors.As), so the PushSecret path is behaviourally unchanged.

Dropped the redundant sanitizeOCISDKErr in GetSecretMap as you suggested. It was a no-op in practice: GetSecret already sanitises, and a sanitised error is a plain fmt.Errorf that never satisfies the ServiceErrorRichInfo assertion.

On the trade-off you raised: OCI returns NotAuthorizedOrNotFound for both "missing" and "not permitted", so mapping 404 makes a revoked IAM policy look identical to a deletion, and under deletionPolicy: Delete that removes a live Secret. I mapped it anyway, as you leaned, and documented the risk in two places: the GetSecret doc comment, and a new "Deletion policy" section in docs/provider/oracle-vault.md that points at the Retain default for anyone who would rather keep the Secret when access is lost.

Two things I left out on purpose. Happy to fold either one into this PR if you would rather have them here:

1. sanitizeOCISDKErr still asserts ServiceErrorRichInfo, so a plain non-rich service error passes through unsanitised. That is the separate bug your note points at, and fixing it also changes error text on the PushSecret, DeleteSecret, SecretExists and GetAllSecrets paths.
2. GetSecret still returns errMissingKey when ref.Property is absent from a secret that does exist. A missing field is not a missing secret, and mapping it would delete a Secret over a mistyped property.

Format

fix(oracle): return NoSecretErr on a missing secret so deletionPolicy applies

AI Assistance disclosure

Did you use an script, LLM, or AI assisted development tool for this contribution?

AI assistance used: Yes

If yes provide details:

Tool(s) used: Claude Code (Opus 5)

Purpose of assistance: tracing the path from GetSecret to the controller's deletionPolicy gate, locating the in-repo precedents (the existing 404 check in getSecretBundleCode, the NoSecretError shape used by the beyondtrust provider, and the fake.ServiceError stub already present in the oracle test fake), and drafting the two tests and the docs section.

Parts of the contribution affected: the provider change, the two new test functions, and the docs section.

Human validation performed: ran make check-diff on the committed tree. That runs make reviewable (generate, docs, manifests, helm generate/schema/docs, lint, license check, helm tests, CRD snapshot tests, tf fmt) and then verifies the branch is clean, and it passes: golangci-lint reports 0 issues across every module including providers/v1/oracle, helm tests 62 passed, CRD snapshot tests 20 passed, and no generated file drifted. make test passes too, 90 packages ok, with providers/v1/oracle at 62.2% statement coverage. I also confirmed the new tests actually catch the bug rather than passing vacuously: neutralising isSecretNotFoundErr makes both TestOracleVaultGetSecretNotFound and TestIsSecretNotFoundErr fail, and they pass again once it is restored. Not exercised against a live OCI Vault, so the last box below is left unchecked.

Checklist

• I have read the contribution guidelines
• All commits are signed with git commit --signoff
• My changes have reasonable test coverage
• All tests pass with make test
• I ensured my PR is ready for review with make reviewable
• I confirm that I understand the submitted changes and can explain them without relying on an AI tool.
• I have tested my changes on a live environment to confirm they are working

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 33d234cc-36f6-4595-8c47-db578075a9cc

📥 Commits

Reviewing files that changed from the base of the PR and between 6f6fa86 and 639439c.

📒 Files selected for processing (3)

• docs/provider/oracle-vault.md
• providers/v1/oracle/oracle.go
• providers/v1/oracle/oracle_test.go

---

Walkthrough

Oracle Vault retrieval now converts OCI 404 service errors into esv1.NoSecretErr. GetSecretMap preserves this sentinel error. A shared helper performs 404 detection for secret retrieval and bundle classification. Tests cover 404, 500, unrelated, and nil errors. Documentation describes Delete, Merge, and Retain behavior when OCI cannot distinguish missing from unauthorized secrets.

Possibly related issues

• external-secrets/external-secrets issue 6732: Applies the same esv1.NoSecretErr missing-secret handling pattern to the Keeper provider.
• external-secrets/external-secrets issue 6569: Tracks provider-specific mapping of missing remote secrets to esv1.NoSecretErr for deletion-policy handling.

Possibly related PRs

• external-secrets/external-secrets#6816: Implements similar provider-specific conversion of missing remote secrets to esv1.NoSecretErr.

Merge Risk: ⚪ Minimal · up to 63943

The PR makes missing Oracle secrets honor the configured deletion policy while preserving existing handling for other errors. No actionable merge-blocking risk remains after normal checks and review.

🚥 Pre-merge checks | ✅ 2

✅ Passed checks (2 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The PR detects OCI 404 responses, returns esv1.NoSecretErr, preserves other errors, updates GetSecretMap, and documents deletionPolicy behavior required by issue #6572.
Out of Scope Changes check	✅ Passed	All changes support issue #6572 by implementing Oracle not-found handling, adding focused tests, and documenting the deletion-policy behavior.

---

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

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[evrardj-roche]

1. I have not tested this on Oracle, someone else must approve.
2. The title is misleading: it should be at least fix(oracle)!: to highlight the breaking change.
3. I am not fond (no pun intended) of doing this breaking change for a provider who has decided to not clearly distinguish the two cases (not authorized/not found). If no distinction, why we consider all not authorized as not found?

[evrardj-roche]

losing secrets on policy change is harsh, but fair.

This warning is more than welcomed. I would also mention deletionPolicy: Merge here and insist on the deletion:

Suggested change

	and with `deletionPolicy: Delete` the Kubernetes Secret is removed. Use the default
	and with `deletionPolicy: Delete` or `deletionPolicy: Merge` the Kubernetes Secrets WILL BE DELETED. Use the

[evrardj-roche]

This will get impacted by the change. One missing member will return nil, instead of the secretMap.