fix(helm): use OCIRegistry consumer identity type for OCI URLs#2055
Merged
fabianburth merged 14 commits intoMar 26, 2026
Merged
Conversation
When helm input method generates consumer identities for OCI repository URLs (oci://), it now returns type "OCIRegistry" instead of "HelmChartRepository". This allows the OCI credential repository plugin to match and resolve credentials correctly. After issue open-component-model#786 fix, the OCI credential repository plugin only handles OCIRegistry type, which caused credential lookup to fail for OCI-hosted Helm charts. This change aligns the consumer identity type with the actual protocol. Non-OCI URLs (http://, https://) continue using HelmChartRepository type for backward compatibility. Refs: open-component-model/ocm-project#967 Signed-off-by: Fabian Burth <fabian.burth@sap.com>
When downloading Helm charts from OCI registries, the credential mapping now supports accessToken in addition to username/password. If password is not present but accessToken is, the token is used as the password for basic authentication. This enables token-based authentication with OCI registries like GitHub Container Registry where credentials may be stored with accessToken rather than password. Refs: open-component-model/ocm-project#967 Signed-off-by: Fabian Burth <fabian.burth@sap.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughGetResourceCredentialConsumerIdentity and related Helm flows now detect OCI-style repository URLs (scheme "oci") and set the credential consumer identity type to the OCI registry type; OCI digest/download paths accept credentials (password falls back to access-token) and use them when creating registry clients. Tests and an indirect go.mod dependency were updated. Changes
Sequence Diagram(s)sequenceDiagram
participant InputMethod as InputMethod
participant AccessBinding as AccessBinding
participant CredentialStore as CredentialStore
participant DigestResolver as DigestResolver
participant RegistryClient as RegistryClient
InputMethod->>AccessBinding: provide helmRepository URL
AccessBinding->>AccessBinding: parse URL -> identity (scheme, hostname)
alt identity.scheme == "oci"
AccessBinding->>CredentialStore: request credentials for OCIRegistry + attrs
else
AccessBinding->>CredentialStore: request credentials for HelmChartRepository + attrs
end
CredentialStore-->>AccessBinding: return credentials (username, password or token)
AccessBinding->>DigestResolver: pass resource + credentials
DigestResolver->>RegistryClient: create client with basic auth (username, password||token) if present
RegistryClient->>RegistryClient: resolve digest / fetch chart
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bindings/go/helm/access/access.go (1)
14-16: Consider extracting shared constants to reduce duplication.
LegacyHelmChartConsumerTypeis defined identically in bothaccess.goandinput/method.go. The conditional type-setting logic (lines 63-67) is also duplicated. A shared helper or constants package could reduce this duplication, though the current approach is acceptable given the separate package responsibilities.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindings/go/helm/access/access.go` around lines 14 - 16, Extract the duplicated constants and conditional type-setting into a shared place and update both modules to import it: move LegacyHelmChartConsumerType and HelmRepositoryType plus the conditional logic that maps legacy types to current types into a common package (e.g., a constants/helper package), then replace the copies in access.go and input/method.go to reference the shared symbols; ensure the helper exposes a function (e.g., NormalizeHelmConsumerType or GetHelmRepositoryType) or exported constants used by the existing code paths so behavior remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bindings/go/helm/access/access.go`:
- Around line 14-16: Extract the duplicated constants and conditional
type-setting into a shared place and update both modules to import it: move
LegacyHelmChartConsumerType and HelmRepositoryType plus the conditional logic
that maps legacy types to current types into a common package (e.g., a
constants/helper package), then replace the copies in access.go and
input/method.go to reference the shared symbols; ensure the helper exposes a
function (e.g., NormalizeHelmConsumerType or GetHelmRepositoryType) or exported
constants used by the existing code paths so behavior remains identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 04183be3-357e-469a-8f92-716366f6ab14
📒 Files selected for processing (6)
bindings/go/helm/access/access.gobindings/go/helm/access/access_test.gobindings/go/helm/go.modbindings/go/helm/input/method.gobindings/go/helm/input/method_test.gobindings/go/helm/internal/download/download.go
- Extracted duplicated identity generation logic into `internal.GetIdentity` to improve maintainability and consistency. - Unified handling of resource access and identity creation for both Helm access and digest processor. - Reduced code duplication across files by introducing a single shared implementation. - Included a TODO referencing consolidation of this logic within the Helm repository package in the future. This refactor simplifies the codebase and creates a foundation for further enhancements without impacting existing functionality. Signed-off-by: Fabian Burth <fabian.burth@sap.com>
Signed-off-by: Fabian Burth <fabian.burth@sap.com>
fabianburth
commented
Mar 26, 2026
- Updated `resolveOCIDigest` method to accept a `credentials` parameter, enabling username/password or token-based authentication. - Enhanced OCI registry client initialization to support basic authentication using provided credentials. - Unified the handling of credentials for both OCI and HTTP digest resolution. - Improves compatibility with various registry authentication schemes, including token-based authentication. This change ensures seamless integration with more OCI registries and aligns with existing credential handling practices. Signed-off-by: Fabian Burth <fabian.burth@sap.com>
we generally lean towards avoiding such defensive programming styles Signed-off-by: Fabian Burth <fabian.burth@sap.com>
Signed-off-by: Fabian Burth <fabian.burth@sap.com>
…dentity' into fix/967-helm-oci-consumer-identity
Signed-off-by: Fabian Burth <fabian.burth@sap.com>
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/helm/digest/digest.go`:
- Around line 216-226: The code currently sets regClientOpts =
[]registry.ClientOption{registry.ClientOptBasicAuth(username, password)}
whenever credentials != nil even if username and password are empty; change the
logic in the block handling credentials (the variables username, password, and
token derived from ocicredentials.CredentialKeyUsername/Password/AccessToken) to
only append registry.ClientOptBasicAuth(username, password) to regClientOpts
when at least one of username or password (after token fallback) is non-empty —
i.e., check for non-empty values before calling registry.ClientOptBasicAuth so
public OCI pulls omit basic auth entirely.
🪄 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: 0a424f8d-34e9-472e-833b-097c2a967558
📒 Files selected for processing (5)
bindings/go/helm/access/access.gobindings/go/helm/digest/digest.gobindings/go/helm/digest/digest_test.gobindings/go/helm/go.modbindings/go/helm/internal/download/download.go
💤 Files with no reviewable changes (1)
- bindings/go/helm/digest/digest_test.go
✅ Files skipped from review due to trivial changes (1)
- bindings/go/helm/go.mod
🚧 Files skipped from review as they are similar to previous changes (2)
- bindings/go/helm/access/access.go
- bindings/go/helm/internal/download/download.go
Signed-off-by: Fabian Burth <fabian.burth@sap.com>
Signed-off-by: Fabian Burth <fabian.burth@sap.com>
jakobmoellerdev
previously approved these changes
Mar 26, 2026
frewilhelm
reviewed
Mar 26, 2026
frewilhelm
left a comment
Contributor
There was a problem hiding this comment.
just two questions. The rest looks good
Signed-off-by: Fabian Burth <fabian.burth@sap.com>
jakobmoellerdev
approved these changes
Mar 26, 2026
frewilhelm
approved these changes
Mar 26, 2026
1 task
5 tasks
This was referenced Jun 1, 2026
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
3 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.
Summary
OCIRegistryinstead ofHelmChartRepositorywhen the URL usesoci://schemeaccessTokencredential mapping for OCI registries (token is mapped to password for basic auth when password is not present)This resolves credential lookup failures for Helm charts hosted on OCI registries after issue #786 fix removed the OCI fallback for
AnyConsumerIdentityType.Changes
Consumer identity type fix (
bindings/go/helm/input/method.go,bindings/go/helm/access/access.go):oci://scheme and returnOCIRegistryconsumer identity typeHelmChartRepositoryfor backward compatibilityCredential mapping (
bindings/go/helm/internal/download/download.go):passwordis not present butaccessTokenis, map the token to password for basic authTest plan
OCIRegistrytype for OCI URLsCloses open-component-model/ocm-project#967