feat: typed credential bindings#2343
Merged
matthiasbruns merged 24 commits intoApr 28, 2026
Merged
Conversation
✅ Deploy Preview for ocm-website ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe credential graph now treats credentials and identities as typed objects and exposes ResolveTyped(runtime.Typed) alongside a deprecated Resolve. Ingestion, caching, and resolution paths were updated to operate on runtime.Typed; identity↔credential compatibility is validated via an IdentityTypeRegistry and credential-type scheme provider. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Client
participant Graph
participant Registry as IdentityTypeRegistry
participant DAG as Credential DAG
participant Scheme as Credential Type Scheme
Caller->>Graph: ResolveTyped(identity)
Graph->>Registry: AcceptedCredentialTypes(identity.type)?
Graph->>DAG: lookup nodeID(identity)
alt cached
DAG-->>Graph: cached runtime.Typed
Graph-->>Caller: runtime.Typed
else not cached
Graph->>Graph: extract raw credential from config
Graph->>Scheme: Scheme().Convert -> runtime.Typed
Scheme-->>Graph: typed credential / conversion error
Graph->>Registry: validate identity↔credential types
Graph->>DAG: setCredentials(nodeID, runtime.Typed)
Graph-->>Caller: runtime.Typed
end
sequenceDiagram
participant Legacy as Legacy Client
participant Graph
participant Typed as ResolveTyped
participant Convert as typedToMap
Legacy->>Graph: Resolve(identity)
Graph->>Typed: ResolveTyped(identity)
Typed-->>Graph: runtime.Typed
Graph->>Convert: typedToMap(runtime.Typed)
Convert-->>Graph: map[string]string
Graph-->>Legacy: map[string]string
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
13 tasks
fba6d06 to
d2b9e5a
Compare
3 tasks
Skarlso
reviewed
Apr 22, 2026
Skarlso
left a comment
Contributor
There was a problem hiding this comment.
Some initial findings! :) Nothing too major.
frewilhelm
reviewed
Apr 22, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
bindings/go/credentials/synced_dag.go (1)
50-66:⚠️ Potential issue | 🟠 MajorKeep credential attribute reads under the DAG lock.
Line 55 reads
v.AttributesaftergetVertexhas already releaseddagMu, while Line 66 writes the same map under the write lock. ConcurrentResolveTypedcalls can race or panic with concurrent map access.🔒 Proposed fix
func (g *syncedDag) getCredentials(id string) (runtime.Typed, bool) { - v, ok := g.getVertex(id) + g.dagMu.RLock() + defer g.dagMu.RUnlock() + + v, ok := g.dag.Vertices[id] if !ok { return nil, false } credentials, ok := v.Attributes[attributeCredentials].(runtime.Typed) return credentials, ok }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindings/go/credentials/synced_dag.go` around lines 50 - 66, getCredentials currently calls getVertex which releases dagMu before accessing v.Attributes, causing a race with setCredentials; change getCredentials to take the DAG read lock (g.dagMu.RLock()) and access g.dag.Vertices[id] and then v.Attributes[attributeCredentials] while the read lock is held, releasing it with RUnlock() before returning. Ensure you reference attributeCredentials and runtime.Typed the same way as setCredentials, and do not rely on getVertex to avoid releasing the lock before reading the Attributes map.bindings/go/credentials/doc.go (1)
185-188:⚠️ Potential issue | 🟡 MinorUpdate the stale DirectCredentials-only wording.
The graph now stores and resolves registered
runtime.Typedcredentials directly, so this section contradicts the new typed-resolution documentation above.Proposed wording update
-// The Graph itself only supports resolution of [ocm.software/open-component-model/bindings/go/credentials/spec/config/v1.DirectCredentials]. -// Any Type that is not a DirectCredentials needs a supporting CredentialPlugin implementation for successful resolution. +// The Graph itself supports direct resolution of [runtime.Typed] credentials known to the configured credential scheme. +// Credential types that are not directly resolvable need a supporting CredentialPlugin implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindings/go/credentials/doc.go` around lines 185 - 188, Update the doc section that currently says "The Graph itself only supports resolution of DirectCredentials" to reflect that the Graph now stores and resolves registered runtime.Typed credentials directly: replace the restrictive wording referencing ocm.software/open-component-model/bindings/go/credentials/spec/config/v1.DirectCredentials and change the explanation of resolution to note that any credential type implementing runtime.Typed and registered with the graph will be resolved without requiring a CredentialPlugin; keep a note that legacy or unregistered types still need a CredentialPlugin implementation for resolution (referencing runtime.Typed, DirectCredentials, CredentialPlugin, and the graph).bindings/go/credentials/graph.go (1)
124-124:⚠️ Potential issue | 🟠 MajorReturn a deep copy from
ResolveTyped.
credscan be the object cached inside the graph; returning it directly lets callers mutate cached credentials and creates data races with concurrent resolvers. UseDeepCopyTyped()before crossing the public API boundary.Proposed fix
- return creds, nil + if creds == nil { + return nil, nil + } + return creds.DeepCopyTyped(), nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindings/go/credentials/graph.go` at line 124, ResolveTyped currently returns the cached creds object directly, allowing callers to mutate shared state and causing races; update the final return in ResolveTyped to return a deep copy by calling creds.DeepCopyTyped() (or equivalent DeepCopyTyped method on the credentials type) and return that copy, ensuring the cached object isn't exposed across the public API boundary; keep error handling unchanged and only replace the returned value with the deep-copied instance.
♻️ Duplicate comments (1)
bindings/go/credentials/interface.go (1)
20-25:⚠️ Potential issue | 🟠 MajorKeep
Resolversource-compatible if Phase 1 must be non-breaking.Adding Line 25 to the exported
Resolverinterface forces every external implementation to addResolveTyped. If compatibility is required, keepResolverunchanged and add a separate typed interface instead.♻️ Possible non-breaking shape
type Resolver interface { // TODO(matthiasbruns): Remove once all consumers use ResolveTyped https://github.com/open-component-model/ocm-project/issues/980 // // Deprecated: Migrate to ResolveTyped instead for typed credential support Resolve(ctx context.Context, identity runtime.Identity) (map[string]string, error) +} + +type TypedResolver interface { + Resolver ResolveTyped(ctx context.Context, identity runtime.Identity) (runtime.Typed, error) }To verify in-repo implementers and call sites before deciding whether this break is acceptable:
#!/bin/bash # Description: Inspect Resolver interface and concrete Resolve implementations in Go files. # Expected: all in-repo implementations are visible; external downstreams still require API-policy confirmation. rg -nP --type=go -C3 'type\s+Resolver\s+interface|func\s*\([^)]+\)\s*Resolve\s*\(\s*[^)]*context\.Context[^)]*runtime\.Identity[^)]*\)\s*\(map\[string\]string,\s*error\)' rg -nP --type=go -C3 '\bResolveTyped\s*\('🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindings/go/credentials/interface.go` around lines 20 - 25, The change adds ResolveTyped to the exported Resolver interface which breaks external implementations; revert the public Resolver shape to its original form (keep Resolve(ctx, identity) (map[string]string, error)) and introduce a new separate interface (e.g., TypedResolver) that defines ResolveTyped(ctx, identity) (runtime.Typed, error); update internal callers to type-assert to TypedResolver where typed credentials are required and leave Resolver-based call sites unchanged so existing implementers remain source-compatible.
🧹 Nitpick comments (1)
bindings/go/credentials/resolve_direct_test.go (1)
51-52: Assert thattypeis absent, not merely empty.This better matches the helper’s contract and avoids passing if
"type": ""is accidentally included.Proposed test tightening
// type field should be excluded - assert.Empty(t, result["type"]) + assert.NotContains(t, result, "type")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindings/go/credentials/resolve_direct_test.go` around lines 51 - 52, The test currently uses assert.Empty(t, result["type"]) which only checks the value is empty; change it to assert the key is absent instead (e.g., check the map for the "type" key and assert the boolean is false or use assert.NotContains on result) so the test verifies the helper contract that the "type" field must not be present; update the assertion replacing the assert.Empty(t, result["type"]) line in resolve_direct_test.go to assert absence of the "type" key from the result map.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindings/go/credentials/ingest.go`:
- Around line 63-72: The current logic in the ingest handling (e.g., where
typedPerIdentity[node] is compared and merged with *v1.DirectCredentials) makes
DirectCredentials order-dependent; fix extractResolvable/ingest merge logic so
DirectCredentials are treated strictly as a fallback: during iteration
accumulate and merge any DirectCredentials into a separate temp (e.g.,
mergedDirect) rather than writing them into typedPerIdentity immediately,
continue selecting/overwriting concrete typed credentials as you do now, and
after the loop assign the mergedDirect to typedPerIdentity[node] only if no
concrete typed credential was selected for that identity; apply the same change
to the other occurrence around the 198-207 block so typed credentials always win
and DirectCredentials only fill missing entries.
---
Outside diff comments:
In `@bindings/go/credentials/doc.go`:
- Around line 185-188: Update the doc section that currently says "The Graph
itself only supports resolution of DirectCredentials" to reflect that the Graph
now stores and resolves registered runtime.Typed credentials directly: replace
the restrictive wording referencing
ocm.software/open-component-model/bindings/go/credentials/spec/config/v1.DirectCredentials
and change the explanation of resolution to note that any credential type
implementing runtime.Typed and registered with the graph will be resolved
without requiring a CredentialPlugin; keep a note that legacy or unregistered
types still need a CredentialPlugin implementation for resolution (referencing
runtime.Typed, DirectCredentials, CredentialPlugin, and the graph).
In `@bindings/go/credentials/graph.go`:
- Line 124: ResolveTyped currently returns the cached creds object directly,
allowing callers to mutate shared state and causing races; update the final
return in ResolveTyped to return a deep copy by calling creds.DeepCopyTyped()
(or equivalent DeepCopyTyped method on the credentials type) and return that
copy, ensuring the cached object isn't exposed across the public API boundary;
keep error handling unchanged and only replace the returned value with the
deep-copied instance.
In `@bindings/go/credentials/synced_dag.go`:
- Around line 50-66: getCredentials currently calls getVertex which releases
dagMu before accessing v.Attributes, causing a race with setCredentials; change
getCredentials to take the DAG read lock (g.dagMu.RLock()) and access
g.dag.Vertices[id] and then v.Attributes[attributeCredentials] while the read
lock is held, releasing it with RUnlock() before returning. Ensure you reference
attributeCredentials and runtime.Typed the same way as setCredentials, and do
not rely on getVertex to avoid releasing the lock before reading the Attributes
map.
---
Duplicate comments:
In `@bindings/go/credentials/interface.go`:
- Around line 20-25: The change adds ResolveTyped to the exported Resolver
interface which breaks external implementations; revert the public Resolver
shape to its original form (keep Resolve(ctx, identity) (map[string]string,
error)) and introduce a new separate interface (e.g., TypedResolver) that
defines ResolveTyped(ctx, identity) (runtime.Typed, error); update internal
callers to type-assert to TypedResolver where typed credentials are required and
leave Resolver-based call sites unchanged so existing implementers remain
source-compatible.
---
Nitpick comments:
In `@bindings/go/credentials/resolve_direct_test.go`:
- Around line 51-52: The test currently uses assert.Empty(t, result["type"])
which only checks the value is empty; change it to assert the key is absent
instead (e.g., check the map for the "type" key and assert the boolean is false
or use assert.NotContains on result) so the test verifies the helper contract
that the "type" field must not be present; update the assertion replacing the
assert.Empty(t, result["type"]) line in resolve_direct_test.go to assert absence
of the "type" key from the result map.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ff0cacc3-1c71-4444-943d-c1390be22886
📒 Files selected for processing (15)
bindings/go/credentials/doc.gobindings/go/credentials/graph.gobindings/go/credentials/graph_test.gobindings/go/credentials/ingest.gobindings/go/credentials/ingest_test.gobindings/go/credentials/interface.gobindings/go/credentials/plugins.gobindings/go/credentials/resolve_direct.gobindings/go/credentials/resolve_direct_test.gobindings/go/credentials/resolve_indirect.gobindings/go/credentials/resolve_static.gobindings/go/credentials/spec/config/v1/config.gobindings/go/credentials/synced_dag.gobindings/go/credentials/type_registry.godocs/adr/0018_typed_credentials.md
Skarlso
previously approved these changes
Apr 24, 2026
Skarlso
left a comment
Contributor
There was a problem hiding this comment.
Largely okay with this now. :) 👍
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindings/go/credentials/ingest.go`:
- Around line 213-220: The accumulator for DirectCredentials can leave
mergedDirect.Properties nil and then panic when maps.Copy is called; in the loop
that converts via scheme.Convert into local variable direct (type
v1.DirectCredentials), ensure when setting mergedDirect for the first time you
initialize mergedDirect.Properties to a non-nil map (clone or make a map) before
assigning/merging, mirroring the approach used in processDirectCredentials so
subsequent maps.Copy(mergedDirect.Properties, direct.Properties) is safe even if
direct.Properties or the previous map were nil.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: effa01cc-06af-4b0d-86bf-923be34ec3f7
📒 Files selected for processing (1)
bindings/go/credentials/ingest.go
piotrjanik
reviewed
Apr 24, 2026
fabianburth
reviewed
Apr 24, 2026
Contributor
Author
|
Open topics after the WR:
moving the issue back into draft until I am done |
5 tasks
Contributor
Author
|
matthiasbruns
added a commit
that referenced
this pull request
Apr 27, 2026
…2399) #### What this PR does / why we need it Adds `Scheme.ResolveCanonicalType(typ Type) Type` to `runtime.Scheme`. This method resolves alias types to their canonical (default) type, or returns the type unchanged if it is already a default or not registered. This is needed by the typed credentials work (#2343) to validate identity → credential type compatibility through alias resolution. Previously, alias comparison required instantiating objects via `NewObject` and comparing `reflect.TypeOf` — this is simpler and avoids reflection. **Example:** If `HelmHTTPCredentials/v1` is registered as the default with `HelmHTTPCredentials` as an unversioned alias, then: - `ResolveCanonicalType("HelmHTTPCredentials")` → `HelmHTTPCredentials/v1` - `ResolveCanonicalType("HelmHTTPCredentials/v1")` → `HelmHTTPCredentials/v1` (already canonical) - `ResolveCanonicalType("Unknown/v1")` → `Unknown/v1` (not registered, returned as-is) #### Which issue(s) this PR fixes Related to open-component-model/ocm-project#704 Part of #2343 #### Testing ##### How to test the changes Run the runtime tests: ```bash cd bindings/go/runtime && go test ./... -run TestScheme_ResolveCanonicalType -v ``` ##### Verification - [x] I have added/updated tests for my changes (see [Test Requirements](../CONTRIBUTING.md#test-requirements)) - [x] Tests pass locally (`task test` and `task test/integration` if applicable) - [x] If touching multiple modules, `go work` is enabled (see `go.work`) - [x] My changes do not decrease test coverage - [ ] I have tested the changes locally by running `ocm` --------- Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
c61c48b to
57201a7
Compare
Contributor
Author
On-behalf-of: SAP <matthias.bruns@sap.com> Signed-off-by: Matthias Bruns <git@matthiasbruns.com> # Conflicts: # bindings/go/credentials/go.sum
9 tasks
On-behalf-of: SAP <matthias.bruns@sap.com> Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
fabianburth
reviewed
Apr 28, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
bindings/go/credentials/graph.go (1)
92-99:⚠️ Potential issue | 🟠 MajorReject non-
runtime.Identityinputs up front.
ResolveTypedaccepts anyruntime.Typed, but the rest of the pipeline still assumesruntime.Identity: wildcard matching can panic intypedMatch, and the repository path later falls back totoIdentity(...). Returning a clear error here keeps the new API from crashing on exactly the typed-identity inputs it now advertises.💡 One possible fix
func (g *Graph) ResolveTyped(ctx context.Context, identity runtime.Typed) (runtime.Typed, error) { if identity == nil { return nil, fmt.Errorf("to be resolved from the credential graph, a valid identity is required: %w", ErrUnknown) } + + if _, ok := identity.(runtime.Identity); !ok { + return nil, fmt.Errorf( + "to be resolved from the credential graph, identity %T must currently implement runtime.Identity: %w", + identity, + ErrUnknown, + ) + } if !hasType(identity) { return nil, fmt.Errorf("to be resolved from the credential graph, a consumer identity type is required: %w", ErrUnknown) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindings/go/credentials/graph.go` around lines 92 - 99, ResolveTyped currently accepts any runtime.Typed but later code assumes runtime.Identity and can panic; add an upfront type assertion in Graph.ResolveTyped to reject non-runtime.Identity inputs (e.g. id, ok := identity.(runtime.Identity)) and return a clear error (wrapping ErrUnknown) if the assertion fails. Keep the existing hasType check but perform the identity type check first so typedMatch and toIdentity are never called with an invalid type; update any error message to mention that a runtime.Identity is required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindings/go/credentials/ingest.go`:
- Around line 190-207: When deserializing with
g.credentialTypeScheme().NewObject/Convert, reject objects that are actually
unregistered round-tripped as *runtime.Raw so they don’t count as “resolved” and
suppress plugin edge creation; after Convert succeeds, detect if the resulting
typed value is a *runtime.Raw (or otherwise indicates an unknown type) and if so
push cred into remaining (instead of assigning to resolved), otherwise treat it
as a bona fide resolved typed credential. Use the symbols
g.credentialTypeScheme(), NewObject, Convert, resolved, remaining and
runtime.Raw to locate and implement this check.
---
Duplicate comments:
In `@bindings/go/credentials/graph.go`:
- Around line 92-99: ResolveTyped currently accepts any runtime.Typed but later
code assumes runtime.Identity and can panic; add an upfront type assertion in
Graph.ResolveTyped to reject non-runtime.Identity inputs (e.g. id, ok :=
identity.(runtime.Identity)) and return a clear error (wrapping ErrUnknown) if
the assertion fails. Keep the existing hasType check but perform the identity
type check first so typedMatch and toIdentity are never called with an invalid
type; update any error message to mention that a runtime.Identity is required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 25efc9c4-cd2e-42bf-8600-1308998c3f16
📒 Files selected for processing (10)
bindings/go/credentials/doc.gobindings/go/credentials/graph.gobindings/go/credentials/graph_test.gobindings/go/credentials/ingest.gobindings/go/credentials/ingest_test.gobindings/go/credentials/resolve_indirect.gobindings/go/credentials/synced_dag.gobindings/go/credentials/synced_dag_test.gobindings/go/credentials/type_registry.gobindings/go/credentials/type_registry_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- bindings/go/credentials/type_registry_test.go
- bindings/go/credentials/type_registry.go
- bindings/go/credentials/synced_dag_test.go
- bindings/go/credentials/doc.go
On-behalf-of: SAP <matthias.bruns@sap.com> Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
On-behalf-of: SAP <matthias.bruns@sap.com> Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
…iasbruns/open-component-model into feat/704_typed_credential_bindings
On-behalf-of: SAP <matthias.bruns@sap.com> Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
…iasbruns/open-component-model into feat/704_typed_credential_bindings t p:# especially if it merges an updated upstream into a topic branch.
fabianburth
approved these changes
Apr 28, 2026
This was referenced Apr 29, 2026
This was referenced May 8, 2026
This was referenced May 20, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it
Phase 1 of the typed credentials ADR
This PR only touches the credential bindings and should not break anything.
Which issue(s) this PR fixes
Contributes: open-component-model/ocm-project#704
You can see the whole picture in #2148
Testing
How to test the changes
Unit & integration tests only.
I ran
task init/go.workwith the updatedcredentialsbinding to see if the udate breaks ocm - it does not :)Verification
task testandtask test/integrationif applicable)