Repository navigation
Conversation
NitinKumar004
left a comment
There was a problem hiding this comment.
Review notes
Real data-plane engine (per #427): N/A — OCI Vault is a secrets/KMS service.
CI: the Build failure is a flaky runner I/O error (fork/exec .../link: input/output error), not a code defect — a re-run should clear it. Findings below are the real items.
Findings
Medium · real-engine — Secret name uniqueness enforced globally across all vaults instead of per-vault
providers/oci/vault/secret.go:117
If a user creates a secret named db-password in vault A, then calls CreateSecret (CreateOCISecret) for db-password in vault B -> the handler returns 409 AlreadyExists, but real OCI creates it, because validateSecretSpecLocked (secret.go:117) calls secretByNameLocked (secret.go:398) which scans every secret in every vault and never compares spec.VaultID (OCI scopes secret-name uniqueness to the vault). CancelOCISecretDeletion (secret.go:339) reuses the same global check and can likewise wrongly refuse a restore when an unrelated vault reused the name.
Medium · docs — docs/services.md not updated for OCI Vault (Definition-of-done miss)
docs/services.md:1389
If a user or contributor consults docs/services.md (the human service reference) for OCI secrets -> the ~30-op Vault/KMS surface is invisible: the Secrets header (line 1389) still lists only AWS/Azure/GCP and there is no OCI Vault section, unlike every sibling OCI service (VCN line 407, Monitoring 623, Identity 749). oci-conventions.md Definition-of-done explicitly requires 'Operations added to docs/services.md'; only the generated coverage pages (7 portable ops) were updated, and those cannot describe the OCI-only Extras surface.
Low · docs — Garbled doc comment on VaultManagement interface
server/oci/vault/extras.go:7
If a maintainer reads the VaultManagement doc comment (extras.go:7-11) -> they get an out-of-order, incoherent sentence (a dangling 'container above a secret at all...' fragment and a //nolint:revive directive that runs on into prose), because the comment lines were reordered incorrectly in a tidy edit. The nolint still functions; cosmetic only.
Low · coverage — server/oci/vault wire handler coverage 79.7% (below 90% pillar)
server/oci/vault/handler.go:246
If a client sends an unsupported verb (e.g. DELETE /vaults/{id}) or walks a multi-page list -> those paths ship untested (notFound 0%, listKeys 37.5%, listSecretVersions 50%, getSecretVersion 54.5%, paginate next-page 70%), so a regression in the negative-verb or pagination branch would pass the suite silently.
Low · coverage — No oci-go-sdk SDK-compat test
server/oci/vault/handler_test.go:31
If the wire envelope subtly diverges from what the real oci-go-sdk KMS/Vaults/Secrets client expects (field casing, work-request waiter behavior) -> the hand-rolled map[string]any assertions may still pass while the actual SDK client would fail, because no test round-trips through the real SDK marshaller/waiter (convention calls this the strongest evidence, framed as 'ideally').
NitinKumar004
left a comment
There was a problem hiding this comment.
Review — OCI Vault (KMS + Secrets)
Verdict: Request changes — one persistence blocker, one real state-name fidelity bug, and a rebase. State guards and scope are otherwise correct.
On the red CI
The failing "Build & Vet" job is an infrastructure flake, not a code defect — the runner hit a disk I/O error corrupting its build cache:
go: error obtaining buildID for go tool link: fork/exec .../tool/linux_amd64/link: input/output error
which cascaded into spurious no such file or directory import errors across unrelated packages (azureai, bedrock, cache, …). go build ./... and go vet ./... pass clean locally on the pushed SHA. Re-run the job. (Separately, the PR reports mergeable: CONFLICTING — a real rebase requirement, below.)
Blocking
Mock is not Snapshottable — providers/oci/vault/ (no snapshot.go).
vault.Mock holds four memstore.Store fields (vaults, keys, keyVersions, secrets) plus default IDs, but ships no snapshot.go and implements no snapshot.Snapshottable — the only providers/oci/* package missing it. On the merged tree:
oci: field Vault (*vault.Mock) holds a memstore.Store but is not Snapshottable — add persistence (see #582)
Consequence: CI Test goes red on rebase, and Vault state is silently dropped by every snapshot/restore. Fix: add providers/oci/vault/snapshot.go (+ test) implementing Snapshottable for all four stores (deep-copying key versions, secret versions, and tag maps), mirroring providers/oci/vcn/snapshot.go.
Should fix
KMS keys and key versions report lifecycleState: "ACTIVE" — real OCI reports ENABLED — providers/oci/vault/vault.go:31 (StateActive), key.go:142, key.go:445.
Per the OCI SDK KeyLifecycleStateEnum (CREATING, ENABLING, ENABLED, DISABLING, DISABLED, DELETING, DELETED, PENDING_DELETION, …), KMS keys/key-versions never report ACTIVE — that value belongs to Vaults and Secrets (both correctly ACTIVE here). Live-confirmed: POST /20180608/keys returned "lifecycleState":"ACTIVE". Cascade: terraform-provider-oci's oci_kms_key waits for ENABLED after create/rotate, so an ACTIVE key hangs or errors the apply; SDK consumers comparing against KeyLifecycleStateEnabled misclassify every key. Fix: use ENABLED/DISABLED for the key + key-version surface (keep ACTIVE for vault + secret).
Non-blocking
- No
enableKey/disableKeyand noDISABLEDstate —providers/oci/vault/key.go. The key lifecycle isACTIVE ⇄ PENDING_DELETIONonly; OCI's enable/disable management ops andDISABLING/DISABLED/ENABLINGstates are absent. Fits Track-1 scope (metadata only, no crypto) but is a genuine feature gap — recommend a follow-up ticket, provided theENABLEDnaming above is fixed now. scheduleSecretDeletionreturns204; sibling key/vault deletions return200+ body —server/oci/vault/secret.go:184. Worth confirming the status code + body against the OCI spec for consistency with the key/vault paths.- Branch is behind
development/ CONFLICTING — rebase; only generateddocs/coverage/README.md+oci/README.mdconflict, resolved by re-runninggo generate.
Verified good
Full lifecycle over the wire (create vault → key ACTIVE → rotate a PENDING_DELETION key → 409 IncorrectState → schedule→PENDING_DELETION with timeOfDeletion → cancel→ACTIVE → create secret → get bundle round-trips content → schedule secret deletion): state guards present and correct (CreateKeyVersion/UpdateOCISecret/ScheduleSecretVersionDeletion all guard on live state; current secret version can't be scheduled). Scope correct — no real crypto: encrypt/decrypt/sign/verify/generateDataEncryptionKey/exportKey return 501 naming the gap; secrets store base64 content in-memory (matches the AWS KMS metadata-only precedent). Wiring complete; opc-request-id/opc-work-request-id headers + OCID formats correct; name check-then-set atomic under one mutex (-race clean); reads deep-copy.
0777bf2 to
73c6719
Compare
|
Rebased onto On the red CI — your diagnosis was rightOnly the two generated coverage files conflicted, exactly as you predicted; took the upstream side and re-ran Blocking —
|
NitinKumar004
left a comment
There was a problem hiding this comment.
The earlier points are mostly fixed. On the merged tree a real OCI SDK client still can't read secret versions, read a bundle by name or create a key. Details below.
Earlier review points
- Mock not Snapshottable: fixed.
providers/oci/vault/snapshot.gohas the compile assertion.go test ./persist/...passes on the tree merged with current development, and/_cloudemu/snapshoton serve includes the vault secrets. - Keys reporting
ACTIVE: fixed. Keys and key versions now reportENABLED, while vaults and secrets keepACTIVE. - Secret names unique across all vaults: fixed. Uniqueness is now per vault, and the portable lookup rejects an ambiguous name.
docs/services.mdmissing OCI Vault: fixed.- Garbled
VaultManagementcomment: fixed. - Wire coverage below 90%: fixed. It is now 94.8% for the wire package and 98.0% for the provider.
- No oci-go-sdk compat test: not addressed. The High items below are what such a test would have caught (see Low 3).
scheduleSecretDeletionreturning 204: fine. The SDK'sScheduleSecretDeletionResponsehas no body. It carriesetagandopc-request-idbut noopc-work-request-id, so stamping one is harmless but not real.- enableKey/disableKey: still absent, as agreed for a follow-up.
- Rebase: needed again (see Medium 1).
High
1. Secret version paths don't match the SDK. GetSecretVersion and version deletion return 404.
server/oci/vault/secret.go:48 routes /secrets/{id}/versions/{n} and /secrets/{id}/versions/{n}/actions/{scheduleDeletion,cancelDeletion}. The real Vaults API (oci-go-sdk vault/vault_vaults_client.go) uses the singular form for these three calls:
GET /secrets/{secretId}/version/{secretVersionNumber}POST /secrets/{secretId}/version/{secretVersionNumber}/actions/scheduleDeletionPOST /secrets/{secretId}/version/{secretVersionNumber}/actions/cancelDeletion
Only ListSecretVersions uses the plural /versions.
Repro on serve:
GET /20180608/secrets/<id>/version/1 -> 404 "no such vault resource"
GET /20180608/secrets/<id>/versions/1 -> 200
Fix: route version/{n} and version/{n}/actions/*, keep versions for the list only, and update the handler tests and docs/services.md.
2. GetSecretBundleByName must be POST. The handler answers 405.
server/oci/vault/bundle.go:21 rejects every non-GET under /20190301/secretbundles. The SDK sends POST /secretbundles/actions/getByName with secretName and vaultId as query params.
Repro:
POST /20190301/secretbundles/actions/getByName?secretName=db&vaultId=<v> -> 405 MethodNotAllowed
GET same URL -> 200
Fix: accept POST for actions/getByName and keep GET for /{secretId} and /{secretId}/versions.
3. A real KMS client can't create a key. The vault comes from a made-up query param, and the advertised endpoints point at Oracle.
server/oci/vault/key.go:120reads the vault from?vaultId=.providers/oci/vault/key.go:110rejects a create without it.- In real OCI the vault is implied by the per-vault host.
KmsManagementClientis built withclient.Host = vault.managementEndpoint, andCreateKey/ListKeyssend no vaultId. providers/oci/vault/vault.go:422-423mintscryptoEndpoint/managementEndpointashttps://<full vault OCID>-management.kms.<region>.oraclecloud.com. A user who follows the vault's endpoint, as the SDK pattern andoci_kms_key.management_endpointdo, sends traffic to Oracle instead of the emulator.
Repro:
POST /20180608/vaults -> managementEndpoint "https://ocid1.vault.oc1.iad....-management.kms.us-ashburn-1.oraclecloud.com"
POST /20180608/keys {compartmentId, displayName, keyShape:{AES,32}} (SDK shape, no vaultId)
-> 400 "vaultId is required"
Fix: mint both endpoints against the emulator's own address, derived from the request host. Carry the vault in something the client sends back. One option is a host label the handler parses, such as a *.localhost name. Keep ?vaultId= as a fallback. Resolve the vault from the host in CreateKey and ListKeys. The prefix also shouldn't be the full dotted OCID, since real endpoints use a short host label.
Medium
1. Branch conflicts with current development.
A trial merge onto origin/development (770204c) conflicts in docs/coverage/README.md and docs/coverage/oci/README.md. Code merges cleanly. Taking development's side and re-running go generate . gives the right rows (secrets gains [Vault](./oci/vault.md), and the OCI index gains Vault), and a second run is idempotent.
2. Vault, key and secret creates skip the compartment existence check the OCI server now does for VCN.
server/oci/oci.go (vault registration) wires vault.New(d.Vault, d.WorkRequests) with no compartment checker, while VCN gets one from Identity (#1295).
Repro on the same serve:
POST /20180608/vaults {"compartmentId":"ocid1.compartment.oc1..doesnotexist", ...} -> 200 ACTIVE
POST /20160918/vcns same compartment -> 404 NotAuthorizedOrNotFound
Fix: add the same SetCompartmentChecker hook to the vault handler. Call it on CreateVault, CreateKey, CreateSecret and the three changeCompartment actions, and wire it in server/oci/oci.go like VCN.
Low
/20180608/secrets/actions/getByNameis not a real OCI operation (server/oci/vault/secret.go:36). The Vaults API has no GetSecretByName. Name lookup isListSecrets?name=or the bundle-side getByName. Drop it, or label it clearly as an emulator extension indocs/services.md.VaultCompartment(providers/oci/vault/vault.go:336) andKeyCompartment(providers/oci/vault/key.go:298) are only called from tests. They are declared inExtras(server/oci/vault/extras.go:19,34), but the handler never calls them. OnlySecretCompartmentis used. Remove them from the interface and the mock.- There is still no oci-go-sdk round-trip test. A
httptesttest drivingvault.VaultsClient,secrets.SecretsClientandkeymanagement.KmsManagementClientwould have caught all three High items. Please add one covering create, get, list versions, get version and bundle by name. - Known gaps, fine as a follow-up but worth a ticket:
keys/{id}/actions/enable|disablecurrently answer 404 "unknown action". Key version schedule/cancel deletion (/keys/{keyId}/keyVersions/{id}/actions/*) is also not routed.
Checked
- Build and vet pass on the PR merged into current development.
go testpasses on providers/oci, server/oci, persist and internal/coveragegen, and-racepasses on the vault packages and server/oci. golangci-lint reports 0 issues on the touched packages. go generate .is idempotent after the conflict resolution.- Live serve run: vault, key and secret create. GetSecret and GetSecretBundle return identical bytes on repeated reads. Schedule secret deletion returns 204 and the secret moves to PENDING_DELETION. Paging returns
opc-next-pageand the next page.opc-request-idis echoed. The crypto endpoints return 501. - Terraform: not run. The oracle/oci provider has no plain-HTTP per-service endpoint override, and
oci_kms_keywould follow the vault'smanagement_endpointanyway (High 3). Raw requests in the exact oci-go-sdk shapes were used instead. --enforce-auth: OCI has no auth gate in this repo (AWS and Azure only), so there is nothing for this handler to declare.- Wiring: provider slot,
from_provider.go, server registration, Snapshottable, and the docs/services.md section are all in place.
|
Please merge the latest |
|
Thanks. Everything is addressed in the latest commit, rebased on current High
Medium
Low
Wire tests: Local: |
73c6719 to
3125e4f
Compare
NitinKumar004
left a comment
There was a problem hiding this comment.
Thanks for the update. The secret version paths and the bundle getByName are fixed, and so is the compartment check. The SDK round trip test is a good addition. I re-ran everything on 3125e4f merged with current development (58fdd8c). The merge is clean and go generate is idempotent.
One blocker remains. The KMS management endpoint now points back at the emulator, but *.kms.localhost does not resolve on macOS. A real SDK or Terraform client that follows the vault's managementEndpoint gets no such host. The new SDK test passes only because it swaps in a dialer that ignores the hostname. I also found a few lifecycle gaps against the OCI docs, listed below.
Earlier findings
| # | Finding | Status |
|---|---|---|
| H1 | Secret version paths (/version/{n} singular) |
Fixed. Verified live with the real SDK: GetSecretVersion, ScheduleSecretVersionDeletion and CancelSecretVersionDeletion all succeed |
| H2 | GetSecretBundleByName must be POST | Fixed. SDK GetSecretBundleByName returns the content |
| H3 | KMS client can't create a key; endpoints point at Oracle | Partly fixed. The endpoints now use the emulator's port and a short label, and CreateKey/ListKeys read the vault from the host. But the host doesn't resolve on macOS (High 1 below) |
| M1 | Merge development | Fixed. The branch is 1 commit behind, merges cleanly, and regenerated docs show no diff |
| M2 | Compartment existence check | Fixed. CreateVault into an unknown tenancy and changeCompartment to an unknown compartment both return 404 NotAuthorizedOrNotFound |
| L1 | /secrets/actions/getByName is not a real op |
Fixed (404) |
| L2 | VaultCompartment / KeyCompartment only used by tests |
Fixed |
| L3 | No oci-go-sdk round trip | Fixed, with one caveat: the custom dialer hides High 1 |
| L4 | enable/disable, key version deletion | Not fixed. Tracked in #1550, which is fine. Live: EnableKey returns 404 "unknown action enable" |
| (r2) | Snapshottable, ENABLED for keys, per-vault names, docs, comment, coverage | Still fixed |
High
1. *.kms.localhost doesn't resolve on macOS, so a client that follows managementEndpoint can't reach the emulator.
server/oci/vault/vault.go:199 (endpointDomain = "kms.localhost") and the claim in docs/services.md:1468.
On macOS (Go 1.27.1, darwin/arm64, cgo resolver):
CreateVault -> managementEndpoint http://0000000000008-management.kms.localhost:47425
keymanagement.NewKmsManagementClientWithConfigurationProvider(cp, managementEndpoint).CreateKey(...)
-> dial tcp: lookup 0000000000008-management.kms.localhost: no such host
ping 0000000000008-management.kms.localhost -> cannot resolve: Unknown host
curl http://0000000000008-management.kms.localhost:47425/... -> 200 (curl special-cases .localhost itself)
The docs say *.localhost "resolves through the system resolver on macOS". It doesn't. Only curl and browsers handle it internally. Linux without systemd-resolved or nss-myhostname fails the same way, and that covers most slim or Alpine client containers. This affects:
- the oci-go-sdk
KmsManagementClientandKmsCryptoClientbuilt on the vault's endpoints, as the SDK docs show - Terraform's
oci_kms_key, whosemanagement_endpointis normallyoci_kms_vault.x.management_endpoint - the OCI CLI's
--endpoint
TestVaultSDKRoundTrip passes only because loopbackClient dials the test server whatever the host is.
I checked whether a path-based label could work instead. The SDK drops any path on the client host: an endpoint of http://127.0.0.1:P/kms/abc sends /20180608/keys. So the label has to stay in the hostname, and that name has to resolve. Suggested fix:
- Make the advertised KMS endpoint host configurable on serve, the same way
--k8s-hostis. Default it to something that resolves without extra setup. - When no label is present, keep the
?vaultId=and single-vault fallbacks so a client on the plain address keeps working. - Correct the docs. Explain how to add
/etc/hostsentries on macOS, or how to point the KMS client at the plain address while only one vault exists. - Make the SDK test cover the default path: a client that only knows the plain address must work, and the docs must not promise resolution that the OS doesn't provide.
Medium
2. Scheduling a vault's deletion doesn't move its keys to PENDING_DELETION.
providers/oci/vault/vault.go:289. The SDK doc for ScheduleVaultDeletion says: "This sets the lifecycle state of the vault and all keys in it that are not already scheduled for deletion to PENDING_DELETION". It also says the call is rejected with 409 if any key's deletion is already scheduled later than the vault's. The vault deletion docs say the same: "the vault and all its associated keys go into a pending deletion state".
Live: after ScheduleVaultDeletion on v2, GetKey on its key still returns ENABLED. A user's teardown or cleanup test that checks key state gets the wrong answer.
Fix: cascade inside the same lock, setting each key that isn't already pending to PENDING_DELETION with the vault's timeOfDeletion. Return 409 when an already-pending key is due later than the vault. CancelVaultDeletion should restore the keys it moved.
3. A secret pending deletion frees its name, which real OCI doesn't do.
providers/oci/vault/secret.go:387 (&& s.LifecycleState == StateActive).
Live: ScheduleSecretDeletion on db, then CreateSecret db in the same vault returns 200 with a second OCID. Real OCI rejects this until the deletion window ends with 400 InvalidParameter ... name already exists (terraform-provider-oci#1765). It's a well-known pain point for anyone who destroys and re-applies oci_vault_secret. The AWS Secrets Manager mock in this repo also refuses to recreate a name that's scheduled for deletion (providers/aws/secretsmanager/secretsmanager.go:301), so the PR body's comparison to the AWS mock doesn't hold.
Fix: keep the name reserved while the secret is PENDING_DELETION. Then portable DeleteSecret followed by CreateSecret fails exactly as it does on AWS. That's the honest behavior, and it removes the need for the "recreated while pending" branch in CancelOCISecretDeletion.
4. Bundles are still served for a secret pending deletion.
providers/oci/vault/version.go:169-193, and secret.go:422-426, which deliberately falls back to a pending secret in getByName.
The secret deletion docs say: "When a secret is pending deletion, resources or services that rely on that secret immediately become inaccessible."
Live: after ScheduleSecretDeletion, both GetSecretBundle and GetSecretBundleByName still return the content. An app whose secret has been revoked keeps working against cloudemu.
Fix: refuse bundle reads (by OCID and by name) while the secret is PENDING_DELETION, the same way the portable reads already do.
5. KMS crypto returns 501 although every other KMS in the repo does real crypto.
server/oci/vault/handler.go:233. AWS KMS (providers/aws/kms/crypto.go), GCP KMS (providers/gcp/kms/crypto.go, with per-version material and the version id carried in the ciphertext) and Azure Key Vault keys (providers/azure/keyvault/keys_azure.go) all encrypt, decrypt, sign and verify for real. Here a key is a record with no material. Every encrypt, decrypt, generateDataEncryptionKey, sign and verify call is 501, confirmed live via KmsCryptoClient.
I accepted this in an earlier round. Seeing the other three clouds side by side, a key that can't encrypt misses the main reason to create one. Envelope encryption with OCI keys, which is how apps use OCI Vault, can't be tested at all.
Fix: generate material per key version at create and rotate time. Keep it out of the response and persist it via the snapshot. Serve encrypt/decrypt/generateDataEncryptionKey for AES and sign/verify for RSA and ECDSA on the crypto endpoint. Embed the key version in the ciphertext so decryption after a rotation still works, as the GCP mock does. Refuse crypto on keys that aren't ENABLED. If you'd rather land this PR without crypto, open a ticket for it and link it here. In that case the 501 can stay, since it names the gap honestly.
Low
- A key that's PENDING_DELETION can still be used for a new secret or a re-key (
providers/oci/vault/secret.go:108-115,secret.go:255-262). Live: CreateSecret with a pending key returns 200. Real OCI requires an ENABLED key. Checkk.LifecycleState == StateEnabledin both places. - The provider-level
VaultInfo.CryptoEndpointandManagementEndpointstill minthttps://<label>-*.kms.<region>.oraclecloud.com(providers/oci/vault/vault.go:466-467), while the wire layer re-renders them against the emulator. The typed Go API and the wire disagree, and the provider fields aren't used by the handler. Either drop them and expose onlyEndpointLabel, or compute them in one place. - Wire shape gaps against the SDK models:
Vault.wrappingkeyIdismandatory:"true"inkeymanagement/vault.gobut never sent (server/oci/vault/types.go:29-41).SecretVersion.contentTypeis also missing; live, every version showscontentType=. - No
etagheader on Get or Update for vaults, keys and secrets, andif-matchis ignored. This is already tracked in #1550. - Once the portable default vault is scheduled for deletion, every portable
CreateSecretfails with FailedPrecondition from then on.defaultVaultLocked(providers/oci/vault/portable.go:208) checks only that the vault exists, not that it's ACTIVE. Mint a new default when the stored one is no longer ACTIVE.
Terraform and serve
Terraform was not run. The oracle/oci provider has no plain-HTTP per-service endpoint override, and routing it through a local intercepting proxy wasn't an option here. Also, oci_kms_key would fail on High 1 regardless: its management_endpoint is the unresolvable *.kms.localhost host. As the fallback, I drove the real oci-go-sdk v65.106.1 clients (KmsVaultClient, KmsManagementClient, KmsCryptoClient, VaultsClient, SecretsClient) against cloudemu serve --providers oci --oci-port 47425 --persist:
- Vault: create (ACTIVE, opc-work-request-id set) → GET ×2 byte-identical (also identical via
localhostvs127.0.0.1) → UpdateVault displayName → ScheduleVaultDeletion (PENDING_DELETION). OCI has no hard delete, so the follow-up GET returns the vault as PENDING_DELETION, as real OCI does. GET of an unknown OCID returns 404 NotAuthorizedOrNotFound. - Key: create through the management endpoint failed on DNS (High 1). With a loopback dialer it succeeded: ENABLED, vault taken from the host label, then GET ×2 byte-identical → UpdateKey → CreateKeyVersion (2 versions) → ScheduleKeyDeletion → CancelKeyDeletion back to ENABLED.
- Secret: create → GET ×2 byte-identical → UpdateSecret with a PENDING version → stages
[CURRENT]/[PENDING LATEST]→ promote v2 → bundles CURRENT=v2 "p2", PREVIOUS=v1 "p1", LATEST=v2 → version schedule/cancel deletion → bad base64 returns 400 InvalidParameter → ScheduleSecretDeletion (PENDING_DELETION, 30-day default) → bundle still readable (Medium 4) → same-name recreate allowed (Medium 3). - Pagination:
limit=1returnsopc-next-page.opc-request-idis present on every response. - Persistence: I stopped serve with SIGINT and restarted it on the same state file. The vault, key and secret came back unchanged. A new vault minted after the restart got a fresh OCID (
...0033, no collision). CreateKey through the old endpoint label still resolved to the original vault, and the bundle content survived. Secret content is in the 0600 state file as base64, which persistence needs. Nothing secret appears in the serve log. - Docker: the Dockerfile and serve wiring are unchanged (OCI stays opt-in on 4571), so I didn't rebuild the image. serve and the in-process server behave the same.
What I verified
- Clean build of the PR head merged with development.
go veton providers/oci, server/oci and persist is clean. go test -racepasses for providers/oci/..., server/oci/... and persist. Coverage: provider vault 96.9%, wire vault 94.9%.go mod tidyand gofmt are clean, andgo generate .leaves no diff. golangci-lint reports 0 issues on the vault packages and on providers/oci and server/oci.- CI is all green.
- A concurrency probe ran 8 goroutines of concurrent secret create/update/bundle read/version list, vault and key tag updates, key rotation, schedule/cancel and Snapshot through the assembled server. It was clean under
-racewith-count=3. - Architecture: the portable
services/secretsdriver is implemented, with the OCI-only surface in a consumer-sideExtras, following thevcnpattern.services/kms/driveris AWS-shaped, and GCP KMS and Azure Key Vault don't implement it either, so not using it is consistent. Wiring inproviders/oci/oci.goandserver/oci/oci.goplus the compartment checker is in place.Snapshottableis compile-asserted. OCIDs come from idgen.RequireCompartmentIDis on every list. Errors useocirest. No shared portable driver changed, so the other clouds are unaffected.
Verdict
Request changes: High 1 and Mediums 2 to 4. Medium 5 can become a linked follow-up ticket if you'd rather keep this PR to the control plane.
| const ( | ||
| endpointManagement = "management" | ||
| endpointCrypto = "crypto" | ||
| endpointDomain = "kms.localhost" |
There was a problem hiding this comment.
*.kms.localhost doesn't resolve on macOS. With the real oci-go-sdk KmsManagementClient built on this endpoint, CreateKey fails with lookup 0000000000008-management.kms.localhost: no such host, and ping can't resolve it either. Only curl works, because curl handles .localhost itself. Slim Linux client images without systemd-resolved or nss-myhostname fail the same way. Terraform's oci_kms_key follows this endpoint too. Please make the advertised host configurable on serve (like --k8s-host), default it to something that resolves without extra setup, and keep the ?vaultId= and single-vault fallbacks for clients on the plain address. The SDK drops any path on the client host, so a path-based label can't replace this.
| endpoint stays on the emulator, and `/keys` reads the vault from the request's | ||
| host label. A client on the plain emulator address can pass `?vaultId=` | ||
| instead, and with a single active vault in the compartment it may omit both. | ||
| `*.localhost` resolves through the system resolver on macOS and most Linux |
There was a problem hiding this comment.
This isn't accurate on macOS. The system resolver doesn't resolve *.localhost there (ping fails, and so do Go and Python clients). Please correct it and say what a user has to do instead, e.g. add /etc/hosts entries or use the plain address while only one vault exists.
| return nil, err | ||
| } | ||
|
|
||
| v.LifecycleState = StatePendingDeletion |
There was a problem hiding this comment.
Per the SDK doc for ScheduleVaultDeletion, this "sets the lifecycle state of the vault and all keys in it that are not already scheduled for deletion to PENDING_DELETION", and returns 409 if a key is already scheduled to be deleted after the vault. Right now the vault's keys stay ENABLED (verified live). Please cascade under the same lock, and have CancelVaultDeletion restore the keys it moved.
| // until its deletion is canceled. | ||
| func (m *Mock) secretByNameLocked(vaultID, name string) (*secretData, bool) { | ||
| for _, s := range m.secrets.SortedValues() { | ||
| if s.VaultID == vaultID && s.Name == name && s.LifecycleState == StateActive { |
There was a problem hiding this comment.
Real OCI keeps the name reserved while a secret is PENDING_DELETION. Recreating it returns 400 InvalidParameter ... name already exists (oracle/terraform-provider-oci#1765). The AWS Secrets Manager mock here also refuses (providers/aws/secretsmanager/secretsmanager.go:301). Live, this returns 200 with a second OCID. Please count pending secrets when checking the name.
| return nil, err | ||
| } | ||
|
|
||
| return selectBundle(s, sel) |
There was a problem hiding this comment.
OCI docs: "When a secret is pending deletion, resources or services that rely on that secret immediately become inaccessible." GetSecretBundle and GetSecretBundleByName (which deliberately falls back to a pending secret at secret.go:422) still return the content after ScheduleSecretDeletion. Please refuse bundle reads while the secret is PENDING_DELETION.
| // unemulatedCrypto reports a crypto endpoint operation. CloudEmu stores no key | ||
| // material — a master encryption key here is a record, not a cipher — so an | ||
| // encrypt or sign call would have to invent a ciphertext. | ||
| func unemulatedCrypto(w http.ResponseWriter, r *http.Request, operation string) { |
There was a problem hiding this comment.
AWS KMS, GCP KMS (per-version material, with the version id in the ciphertext) and Azure Key Vault keys all do real crypto in this repo. Here every encrypt/decrypt/generateDataEncryptionKey/sign/verify call is 501, so envelope encryption with OCI keys can't be tested. Please add per-key-version material, persisted via the snapshot and never returned, and serve the crypto endpoint for ENABLED keys. If you'd rather do that separately, open a ticket and link it here.
| return err | ||
| } | ||
|
|
||
| if k.VaultID != spec.VaultID { |
There was a problem hiding this comment.
This doesn't check the key's state. A key in PENDING_DELETION is accepted for a new secret (verified live), and rekeyLocked has the same gap. Real OCI requires an ENABLED key.
| CompartmentID: v.Scope.Compartment, | ||
| DisplayName: v.DisplayName, | ||
| VaultType: v.VaultType, | ||
| CryptoEndpoint: "https://" + label + "-crypto.kms." + region + ".oraclecloud.com", |
There was a problem hiding this comment.
The wire layer re-renders both endpoints against the emulator, so these oraclecloud.com values are only visible through the typed Go API, where they disagree with what the wire returns. Please drop them and keep EndpointLabel, or build the endpoints in one place.
| // defaultVaultLocked returns the vault and key the portable driver stores its | ||
| // secrets in, creating them on first use. | ||
| func (m *Mock) defaultVaultLocked() (vaultID, keyID string) { | ||
| if m.vaults.Has(m.defaultVaultID) && m.keys.Has(m.defaultKeyID) { |
There was a problem hiding this comment.
If the default vault is scheduled for deletion, it still exists, so this returns it and every later portable CreateSecret fails with FailedPrecondition. Please mint a new default when the stored vault isn't ACTIVE.
|
Thanks. Fixed in 1226c97, rebased on current High
Medium Low Local: |
3125e4f to
1226c97
Compare
1226c97 to
58ad50d
Compare
|
Rebased onto |
NitinKumar004
left a comment
There was a problem hiding this comment.
One point before approving: with the plain shared management endpoint as the default, a stock KmsManagementClient request no longer says which vault it is for. In my run, CreateKey sent to the endpoint of a vault pending deletion created the key in a different vault. With two ACTIVE vaults in the same compartment, which one gets the key? In OCI each vault has its own management endpoint, so a key always lands in the vault you addressed. Please make sure CreateKey/ListKeys/GetKey through the default endpoint can't silently target the wrong vault (for example, give each vault a distinct path or port on the default address, or reject the request when it's ambiguous), and add an SDK test with two active vaults. Everything else from the last round is fixed and looks good.
Thanks, this round closes everything from my last review. I re-ran it all on 58ad50d, which sits on current development (4cc5c57, with OCI Logging and Notifications). I drove the real oci-go-sdk v65.106.1 against cloudemu serve on this Mac with stock clients and no custom dialer. The squashed commit matches what you described: the vault code changes since 3125e4f are the fixes below, and the rest of the delta is the rebase over #421 and #423.
Earlier findings
| # | Finding | Status |
|---|---|---|
| H1 | *.kms.localhost endpoint doesn't resolve on macOS |
Fixed. By default managementEndpoint and cryptoEndpoint are the plain address (http://127.0.0.1:<port>). A stock KmsManagementClient built on that endpoint creates a key in the right vault and lists keys, with no dialer. Label hosts are opt-in via --oci-kms-endpoint-domain. With kms.localhost set, the stock client fails with no such host as the docs now say, and a request carrying the label host routes to the right vault |
| H1 docs | docs/services.md macOS claim |
Fixed. The text now describes the default, the ?vaultId= and single-vault rules, and the /etc/hosts requirement for label mode. The flag is in docs/standalone-server.md |
| M2 | Vault deletion doesn't cascade to keys | Fixed. Live: scheduling v2 for day 20 moved its ENABLED key to PENDING_DELETION at day 20 and left a key already due at day 10 alone. A key due at day 25 makes the call 409 IncorrectState, and nothing changes. CancelVaultDeletion restores only the keys it moved. That also holds after a --persist restart |
| M3 | Pending secret frees its name | Fixed. Live: recreating db while it's pending returns 400 InvalidParameter. Cancel brings it back |
| M4 | Bundles served while pending | Fixed. Live: GetSecretBundle and GetSecretBundleByName both return 404 NotAuthorizedOrNotFound while the secret is pending, and work again after cancel |
| M5 | KMS crypto 501 | Deferred to #1552, as offered. The docs and the handler comment link it |
| L6 | Pending key accepted for a secret | Fixed. CreateSecret and the re-key path both return 409 IncorrectState for a pending key |
| L7 | oraclecloud.com endpoints in the provider |
Fixed. VaultInfo keeps only EndpointLabel, and only the wire layer builds endpoints |
| L8 | wrappingkeyId, contentType missing |
Fixed. Live: each vault reports a stable wrappingkeyId, which survives a restart, and secret versions report contentType=BASE64 |
| L9 | etag / if-match | Tracked in #1550 |
| L10 | Pending default vault breaks portable CreateSecret | Fixed (TestPortableCreateReplacesAPendingDefaultVault) |
New findings
No High or Medium.
Low
-
On the plain address, ListKeys returns the keys of every vault in the compartment.
providers/oci/vault/key.go:202, reached fromserver/oci/vault/key.go:150with an empty vault id.
CreateKey on the plain address resolves to the only active vault and returns 400 when the choice is ambiguous. ListKeys with no vault skips that step and filters by compartment only. Live, with four vaults in the tenancy, ListKeys through a stock client on a vault'smanagementEndpointreturned 8 keys from 4 vaults, including PENDING_DELETION keys from vaults already scheduled for deletion. Vaults are never reaped, so destroying and re-applying a stack is enough to trigger this: listing keys for the new vault also returns the old vault's pending keys. Real OCI's ListKeys only returns keys from the vault whose endpoint you call, so anoci_kms_keysdata source or an SDK listing gets extra keys here.
The same fallback also sends a key to the wrong vault. With v2 pending and v1 active, a stock CreateKey on v2's endpoint created the key in v1. Real OCI would refuse it because v2 is pending. All vaults share one endpoint by default, so the emulator can't know which vault the client meant.
Fix: when no vault is named, have ListKeys go throughsoleVaultLockedjust like CreateKey, so it lists the single active vault or returns the same 400. Also add a line to the docs: on the plain address,/keysalways means the compartment's single active vault, whichever vault's endpoint the client took it from. -
UpdateSecret accepts
keyId, but OCI has no such field.
server/oci/vault/secret.go:218passesreq.KeyIDtorekeyLocked(providers/oci/vault/secret.go:202). OCI'sUpdateSecretDetails(API reference, andvault/update_secret_details.goin the SDK) has nokeyId. A secret's key is fixed at creation. No SDK, CLI or Terraform client can send this field, so the behavior is reachable only through raw requests, and it shows a capability that real OCI lacks. Fix: rejectkeyIdon update with 400 InvalidParameter, as the handler already does for other inputs it doesn't model, and droprekeyLocked. -
server.Newnow trips gocyclo (12 > 10).
server/oci/oci.go:85. It's new against the merge base. CI lint is advisory, so the check stays green. With vault added, the identical compartment-checker closure now appears four times (logging, VCN, notifications and vault, at lines 108, 122, 136 and 151). Move it into one helper, e.g.compartmentChecker(d.Identity) func(string) bool, which returns nil when Identity has no compartments. That fixes the complexity and the duplication together.
OCI docs
- ScheduleVaultDeletion / CancelVaultDeletion (SDK doc and Deleting a Vault): the cascade, the 409 for a key due later than the vault, and restoring only the keys that were moved now match.
- Deleting a Secret: a pending secret is now inaccessible.
Vault.wrappingkeyId(mandatory in the SDK model) andSecretVersion.contentTypeare now sent.- UpdateSecretDetails has no
keyId(Low 2).
Terraform and serve
I didn't run Terraform. The oracle/oci provider has no plain-HTTP per-service endpoint override, and routing it through a local intercepting proxy wasn't available here. Now that managementEndpoint is the plain address by default, oci_kms_key should be able to follow it while there's one active vault in the compartment. As the fallback I used the real oci-go-sdk clients against cloudemu serve --providers oci --oci-port 47525 --persist, all stock and with no dialer override:
- Vault → stock
KmsManagementClienton itsmanagementEndpoint→ CreateKey (ENABLED, right vault) → ListKeys. A second vault in the compartment makes CreateKey return 400 with a clear message, as documented. - The cascade, refusal and cancel flows, the secret name reservation, the pending-bundle refusal and the pending-key refusal, with results as in the table above.
- Persistence: I scheduled a vault with an ENABLED key, stopped serve with SIGINT and restarted it on the same state file. The key came back PENDING_DELETION, and CancelVaultDeletion after the restart returned it to ENABLED, so the moved-key tracking survives. A new vault minted after the restart got fresh OCIDs for both the vault and its
wrappingkeyId, with no collisions. - Label mode:
--oci-kms-endpoint-domain kms.localhostadvertises<label>-management.kms.localhost:<port>. A stock client getsno such host, as the docs now warn. A request with that Host header lands in the labeled vault, and an unknown label with two vaults returns 400. - Docker: the Dockerfile is unchanged. The new flag goes through
serveflags→serverkit→ociserver.Driverslike the other serve options, so the image picks it up through the same binary.
What I verified
go build ./...andgo veton providers/oci, server/oci, serveflags, serverkit and persist are clean.go test -racepasses for providers/oci/vault, server/oci/..., server/serveflags and persist. Coverage: provider vault 97.1%, wire vault 94.9%.- gofmt,
go mod tidyandgo generate .leave no diff. - golangci-lint on the touched packages: the only new issue against the merge base is Low 3. The rest are pre-existing.
- CI: all 23 checks are green.
- A concurrency probe ran 8 goroutines of vault schedule/cancel, key create/schedule/cancel, OCI and portable secret creates, ListKeys and Snapshot against one mock. It was clean under
-racewith-count=3. The cascade runs under the vault mutex, so it can't interleave with a key or secret write. - Architecture: unchanged from last round. The new flag follows the k8s host flags, and the provider no longer formats endpoints. No shared portable driver changed.
Verdict
Approve. The three Lows can go in this PR or a follow-up. Low 1 is the one I'd fix first, since it returns wrong data silently.
| out := make([]KeyInfo, 0, m.keys.Len()) | ||
|
|
||
| for _, k := range m.keys.SortedValues() { | ||
| if !k.Scope.Matches(filter) || (vaultID != "" && k.VaultID != vaultID) { |
There was a problem hiding this comment.
With no vault named, this lists every key in the compartment across all vaults. Live, ListKeys through a stock client on one vault's managementEndpoint returned 8 keys from 4 vaults, including pending keys from vaults already scheduled for deletion. Vaults are never reaped, so destroying and re-applying a stack is enough to hit this. CreateKey resolves the same request through soleVaultLocked, and ListKeys should too, so it lists the single active vault or returns the same 400.
|
|
||
| upd := &vaultprovider.SecretUpdate{ | ||
| Description: req.Description, | ||
| KeyID: req.KeyID, |
There was a problem hiding this comment.
OCI's UpdateSecretDetails has no keyId (see the API reference and vault/update_secret_details.go in the SDK). A secret's key is fixed at creation, and no SDK, CLI or Terraform client can send this. I'd reject keyId on update with 400 InvalidParameter and drop rekeyLocked.
| vaultHandler := vault.New(d.Vault, d.WorkRequests) | ||
| vaultHandler.SetEndpointDomain(d.KMSEndpointDomain) | ||
|
|
||
| if comps, ok := d.Identity.(identity.Compartments); ok { |
There was a problem hiding this comment.
This is the fourth copy of the same compartment-checker closure, and with it New reaches gocyclo 12. A small compartmentChecker(d.Identity) func(string) bool helper, returning nil when Identity has no compartments, would remove the duplication and the lint hit together.
KMS vaults and keys with versions and scheduled deletion, secrets with staged versions and the bundle data plane, compartment checks, persistence, an oci-go-sdk round trip, and opt-in per-vault KMS endpoint hosts.
|
Thanks for the approval. All three Lows are in, rebased on current
The rebase conflicts were the shared wiring imports and the serve-flag name list ( Local: |
58ad50d to
bb011fa
Compare
NitinKumar004
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround. All three Lows are fixed. The vault routing point from my last note is only partly closed: two ACTIVE vaults in the same compartment are now refused instead of guessed, but two other cases still put a key in a vault the client never addressed. I re-ran everything on bb011fa with the real oci-go-sdk v65.106.1 against cloudemu serve --providers oci on this Mac. The clients were stock, built on each vault's own managementEndpoint, with no custom dialer.
Earlier findings
| # | Finding | Status |
|---|---|---|
| Note | Default shared endpoint can't tell vaults apart | Partly fixed. Two ACTIVE vaults in one compartment now return 400 for CreateKey and ListKeys. A key sent to a pending vault's endpoint, or to the endpoint of a vault in another compartment, still lands silently in some other vault (Medium 1). No SDK test with two active vaults was added |
| L1 | ListKeys without a vault lists every vault | Fixed. Live: after scheduling one vault's deletion, ListKeys on the plain address returned only the active vault's keys, and the old vault's pending keys no longer appear |
| L2 | UpdateSecret accepts keyId |
Fixed. Live: PUT /secrets/{id} with keyId returns 400 InvalidParameter, and a description update still returns 200. rekeyLocked and SecretUpdate.KeyID are gone |
| L3 | gocyclo on server.New, closure copied four times |
Fixed on this head (compartmentChecker, no gocyclo hit). It comes back on rebase, see Low 1 |
| M5 / L9 | KMS crypto, etag / if-match | Still tracked in #1552 and #1550 |
New findings
Medium
- On the default address, a key can still land in a vault the client didn't address.
providers/oci/vault/vault.go:409(soleVaultLocked), called fromproviders/oci/vault/key.go:113(CreateKey) and:200(ListKeys).
Every vault advertises the same managementEndpoint (http://127.0.0.1:<port>). The oci-go-sdk keeps only the scheme and host of a client's endpoint and drops any path (common/client.go prepareRequest). So the only thing a stock request carries is the key's compartmentId, and the emulator picks "the only ACTIVE vault in that compartment". I checked three cases live:
- Two ACTIVE vaults va and vb in compartment c2: CreateKey and ListKeys on either endpoint return 400. That's correct now, though the message says to pass
?vaultId=, and a stock SDK, CLI or Terraform client can't send that. With two vaults in a compartment, those clients can't create a key on the default address at all. - va scheduled for deletion, vb ACTIVE, both in c2: CreateKey on va's endpoint returned 200 with
vaultId= vb. ListKeys on va's endpoint returned vb's keys. This is the case from my last note, and it hasn't changed. Real OCI refuses the call because va is pending. - vc ACTIVE in compartment c3, CreateKey on vc's endpoint with
compartmentIdc2: the key was created in vb, a different vault. ListKeys on vc's endpoint for c2 listed vb's keys. OCI lets a key sit in a different compartment from its vault (CreateKeyDetails.compartmentId is the key's compartment, and the vault comes from the endpoint). So this is two ACTIVE vaults with a silent wrong target, which is what the note asked to rule out.
Two related effects. When a compartment's only vault is pending, ListKeys on its endpoint returns 400 vaultId is required instead of listing its keys, and CreateKey returns the same 400 instead of 409 IncorrectState. GetKey also ignores the endpoint: va's key comes back through vb's endpoint, where real OCI would return 404.
What breaks for a user: a stack that destroys and re-creates a vault, or that keeps keys in an application compartment separate from a shared security compartment, gets keys in the wrong vault. Later secrets or key operations then refer to a vault the user never created the key in, and nothing errors.
Fix: the endpoint has to identify the vault, and with the SDK dropping the path, only the host or port can carry it. The clean option is a per-vault port on the default address: advertise http://127.0.0.1:<vault-port> and route requests by the listener they arrived on, with the same port kept after a --persist restart. If that's too much for this PR, at least make the fallback refuse every case it can't be sure of. Resolve across all vaults in the tenancy whatever their compartment, count vaults in any lifecycle state, and return 409 IncorrectState when the sole candidate isn't ACTIVE. Fix the 400 message too, so it doesn't point stock clients at a parameter they can't send. Either way, please add the SDK test from the note: two ACTIVE vaults, plus one pending vault and one vault in another compartment, all through stock clients on the default address, asserting that each key lands in and is listed from the addressed vault or the call is refused.
Low
- The gocyclo hit comes back when you rebase over #426.
server/oci/oci.go:131after merging current development (f488586). #426 (Compute) added its own copy of the compartment-checker closure. On the merged treeNewis at gocyclo 11. The merge itself is clean, andgo build ./...passes on it. When you rebase, switch Compute tocomputeHandler.SetCompartmentChecker(checker)like the others.
OCI docs
- CreateKeyDetails:
compartmentIdis the key's compartment, and the vault is the one whose management endpoint is called (Medium 1). - UpdateSecretDetails: no
keyId, and the handler now refuses it.
Terraform and serve
I didn't run Terraform. The oracle/oci provider has no plain-HTTP per-service endpoint override, and a local TLS intercepting proxy wasn't available here. As the fallback I used the real oci-go-sdk against cloudemu serve --providers oci --oci-port 47625, with stock KmsVaultClient, KmsManagementClient and IdentityClient. Each KMS client was built on the vault's own managementEndpoint with no dialer override. That run produced the results in Medium 1 and the table above. The Dockerfile is unchanged, and the new and changed flags go through serveflags → serverkit → ociserver.Drivers as before.
What I verified
go build ./...is clean on this head and on this head merged with current development.go veton server/oci and providers/oci is clean.go test -racepasses for providers/oci/vault, server/oci/..., server/serveflags and persist. Coverage: provider vault 97.3%, wire vault 95.0%.- gofmt,
go mod tidyandgo generate .leave no diff. - golangci-lint: no new issues on this head (the serveflags goconst hits are pre-existing). Merged with development, there's one gocyclo hit (Low 1).
- CI: all 23 checks are green.
- Delta since 58ad50d: the ListKeys fallback, the
keyIdrefusal, thecompartmentCheckerhelper, and the docs lines for both. The rest is the rebase over #422 (Object Storage wiring and its--oci-tenancyflag).SetCompartmentChecker(nil)is safe in every handler, since each one treats a nil checker as "skip". - No persisted state changed in this round. Removing
rekeyLockedonly drops a code path.
Verdict
Changes requested on Medium 1. The two-active-vault case is handled, but a stock client can still put a key in a vault it didn't address, and that's the case my note was about. Everything else looks good.
| // soleVaultLocked picks the vault a key request that names none addresses: | ||
| // the only active vault in the compartment. This covers a KMS client on the | ||
| // emulator's plain address. Callers hold mu. | ||
| func (m *Mock) soleVaultLocked(compartmentID string) (string, error) { |
There was a problem hiding this comment.
This picks the only ACTIVE vault in the key's compartment, but every vault advertises the same endpoint and the SDK drops any path, so a stock request never says which vault it's for. Live with stock clients: CreateKey on a pending vault's endpoint created the key in the other, active vault. CreateKey on vault vc's endpoint (compartment c3) with a key compartment of c2 created the key in vb, the active vault in c2. OCI allows a key in a different compartment from its vault. Either give each vault its own port on the default address, or refuse every case that isn't certain: resolve across all vaults in any state and compartment, and return 409 IncorrectState when the sole candidate isn't ACTIVE.
| // Real OCI lists the keys of the vault whose endpoint was called. With no | ||
| // vault named, resolve it as CreateKey does rather than list every vault. | ||
| if vaultID == "" { | ||
| id, err := m.soleVaultLocked(compartmentID) |
There was a problem hiding this comment.
Same resolution for ListKeys. On a pending vault's endpoint this lists the other active vault's keys. When a compartment's only vault is pending, it returns 400 vaultId is required, though real OCI would list that vault's keys. The 400 message also points stock SDK, CLI and Terraform clients at ?vaultId=, which they can't send.
| always resolves. A key request on that address names its vault with | ||
| `?vaultId=`, and with a single active vault in the compartment it may omit it, | ||
| so a stock SDK client built on the endpoint works while there is one vault. | ||
| Every vault shares that address, so on it `/keys` (create and list alike) |
There was a problem hiding this comment.
Worth stating the limits here once the routing is settled. With several vaults, stock clients can't use the default address at all, and a key's compartment has to match the vault's for the fallback to find it.
Summary
secretsdriver.Extrasinterface. Nothing added toservices/secrets/driver, per Move OCI-only capabilities out of shared driver packages #393.Closes #413. Part of #376.
Changes
providers/oci/vault/—Mockovermemstoreimplementingdriver.Secrets, onesync.RWMutexguarding all four stores: vaults, master encryption keys and key versions, OCI-shaped secrets, secret versions and stages.server/oci/vault/—/20180608management and/20190301retrieval, claimed separately inMatches.providers/oci/oci.goandserver/oci/oci.go.How
DeleteSecretmaps onto scheduled deletionThe interesting design question in this service.
DeleteSecret(name)takes the same path asScheduleSecretDeletion, withtimeOfDeletionat OCI's soonest permitted value — one day out — rather than the 30-day default an unspecified wire call receives.The secret moves to
PENDING_DELETION, keeping its OCID and every version. All seven portable operations then treat it as gone (NotFoundfrom Get/Put/GetValue/ListVersions/Delete; absent fromListSecrets), while the OCI surface still lists it asPENDING_DELETIONandCancelSecretDeletionrestores it — the same soft delete the AWS Secrets Manager mock exposes. Nothing reaps; a scheduled resource stays pending until cancelled.One deliberate consequence: a pending secret releases its name, so a portable delete→recreate works.
CancelSecretDeletionthen fails withAlreadyExistsif the name was retaken, rather than producing two live secrets with the same name.Deletion windows are enforced — 7–30 days for vaults and keys, 1–30 for secrets; out-of-window or unparseable
timeOfDeletionis a 400.Judgement calls
CreateSecretmintscloudemu-default-vaultand an AES-256 key. Tested that a second create reuses them, and that 16 concurrent creates mint exactly one vault.encrypt/decrypt/sign/verify/generateDataEncryptionKey/exportKeyanswer 501 naming the gap, because CloudEmu stores no key material. A fake ciphertext would be worse than an honest refusal.RequireCompartmentIDon/vaults,/keys,/secrets; deliberately not onkeyVersions,secrets/{id}/versions,getByNameor any/20190301path — the real API takes none there and the parent resource already names the compartment.ScheduleVaultDeletionResponsecarries aVault), 204 headers-only for secret and version deletions (those responses carry no body), 202 forchangeCompartment.definedTags,restoreFromFile,restoreFromObjectStore,externalKeyManagerMetadata,autoKeyRotationDetails,externalKeyReference,desiredState,secretRules,rotationConfig,secretGenerationContext, a non-BASE64contentType, and reshaping a key after create.Provider Coverage
Checklist
go test ./...) — exit 0, 272 packagesgolangci-lint run --timeout=9m) — 0 issuescloudemu_test.go— driver + handler tests insteadTest Plan
Coverage leak check clean: no OCI operation in
docs/coverage/{aws,azure,gcp}/*.md;git diff development -- services/empty.docs/coverage/oci/vault.mdlists exactly the 7 portable operations, as expected with capabilities consumer-side.Lock sweep over all 63
*Mockmethods: every one of the 42 exported methods takesm.muexactly once, no locking method calls another locking method, no unexported helper touchesm.mu, and there is nostore.Updateanywhere — so no closure can re-enter its store.End-to-end on a running server (port 4614):
OCIDs came out as
ocid1.{vault,key,keyversion,vaultsecret}.oc1.iad.….A note on parallel worktrees
The session first reported
cmd/cloudemu TestServeOutOfProcessas a pre-existing failure. That was wrong and I have re-checked it: with nothing else running, the test passes on cleandevelopmentin under 6s. The failure is contention on the shared~/.cloudemudaemon lock between the six Wave 2 worktrees running suites in parallel. This branch's full suite is exit 0.Left out
KMS crypto operations (disclosed as 501), OCI secret rules and rotation targets, tag namespaces, and vault backup/restore — each rejected by name rather than silently dropped.