HELM-611: add OCI registry client for chart operations #15925
Conversation
OCI-based Helm charts were failing to install because the action configuration lacked a registry client. This change: - Add GetDefaultOCIRegistry() to create and attach a registry client to the Helm action configuration - Integrate registry client initialization into all Helm handlers: install, upgrade, uninstall, rollback, and chart get operations - Add unit tests for the new registry client function - Update older tests to use mock registry Client function Without a registry client, operations on OCI charts (oci://) would fail with errors about missing registry support. Fixes: HELM-611
|
@sowmya-sl: This pull request references HELM-611 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
@sowmya-sl: This pull request references HELM-611 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdds OCI registry client initialization to Helm action configurations: new GetDefaultOCIRegistry and GetOCIRegistry functions with TLS and plain-HTTP options; config initialization now invokes GetDefaultOCIRegistry and logs debug-level errors without changing control flow. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
openshift-ci
bot
added
component/backend
|
@sowmya-sl: This pull request references HELM-611 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@sowmya-sl: This pull request references HELM-611 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@sowmya-sl: This pull request references HELM-611 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/helm/actions/get_registry.go`:
- Around line 17-26: GetOCIRegistry currently ignores insecure and plainHTTP;
fix by passing registry.ClientOptPlainHTTP() when plainHTTP is true, and handle
insecure by constructing an *http.Client with a custom
tls.Config{InsecureSkipVerify: true} and passing it to newRegistryClient via
registry.ClientOptHTTPClient(httpClient); update GetOCIRegistry to call
newRegistryClient with the appropriate options (plainHTTP and/or HTTP client)
and add imports for crypto/tls and net/http so the custom HTTP client can be
created and supplied to the registry client.
🧹 Nitpick comments (2)
pkg/helm/actions/config.go (1)
53-56: Redundant registry initialization across config and handlers.This call to
GetDefaultOCIRegistryis non-blocking (logs and continues), while handlers.go calls the same function and does fail the request on error. The redundancy means the registry client may be initialized twice per request when the first call succeeds.Consider removing this call from config initialization since handlers.go already performs the authoritative check with proper error handling. If the intent is a "best-effort early init," the pattern is fine but should be documented.
pkg/helm/handlers/handler_test.go (1)
205-207: Consider adding test coverage for registry initialization failures.The fake always returns
nil, which is fine for happy-path testing. However, since handlers now returnBadGatewaywhengetDefaultOCIRegistryfails, consider adding at least one test case that injects a failinggetDefaultOCIRegistryto verify the error response path.func fakeGetDefaultOCIRegistryError(conf *action.Configuration) error { return errors.New("mock registry error") }
|
@sowmya-sl: This pull request references HELM-611 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/retest |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
sowmya-sl
force-pushed
the
add-oci-support
branch
from
8ebc6b6 to
db77391
Compare
|
@sowmya-sl: This pull request references HELM-611 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/helm/actions/get_registry.go`:
- Around line 29-36: When skipTLSVerify is true, don't create a bare
http.Transport (which loses ProxyFromEnvironment and default timeouts/HTTP2
settings); instead clone the default transport, assert
http.DefaultTransport.(*http.Transport), call Clone() to get a copy, set its
TLSClientConfig.InsecureSkipVerify = true, and use that cloned transport in the
http.Client passed to registry.ClientOptHTTPClient so Proxy and other default
behaviors are preserved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
sowmya-sl
force-pushed
the
add-oci-support
branch
from
db77391 to
b560f00
Compare
|
@sowmya-sl: This pull request references HELM-611 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
- Remove getDefaultRegistryClient from helm handler file. - Rename insecure param to skipTLSVerify for clarity - Add TLS skip verification and plainHTTP support - Use mockable newRegistryClient for testability - Fix variable naming (registryClient) - Return nil directly instead of err variable
sowmya-sl
force-pushed
the
add-oci-support
branch
from
b560f00 to
f7030e3
Compare
|
@sowmya-sl: This pull request references HELM-611 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
webbnh
left a comment
webbnh
left a comment
There was a problem hiding this comment.
@sowmya-sl, this looks good to me. However, I did have one late-breaking thought which you might want to consider before letting this merge (or, maybe you'll want to take it up in a follow-on PR, with my other suggestion).
/lgtm
| conf.Init(confFlags, ns, "secrets", klog.Infof) | ||
|
|
||
| err = GetDefaultOCIRegistry(conf) | ||
| if err != nil { | ||
| klog.V(4).Infof("Failed to get default OCI registry: %v", err) | ||
| } |
There was a problem hiding this comment.
My apologies, @sowmya-sl, but I didn't fully appreciate what you had accomplished here!
So, I now have to ask: why implement GetDefaultOCIRegistry() as a separate function, or, more to the point, why call it from here? Why not in-line or call it inside conf.Init()?
Also, as an aside (since it's arguably outside the scope of this PR), my IDE notes that the code is ignoring the error returned by conf.Init(). 😞 We should probably not be doing that -- instead, the code should be returning nil (and maybe an error) and letting the caller address the situation. But, I don't know if you're up for trying to fix that, now (it affects a dozen callers, as you presumably know well).
pkg/helm/handlers/handlers.go
Outdated
| if err != nil { | ||
| serverutils.SendResponse(w, http.StatusBadGateway, serverutils.ApiError{Err: fmt.Sprintf("Failed to get default registry: %v", err)}) | ||
| return |
There was a problem hiding this comment.
As I noted elsewhere, there is a problem inside getActionConfigurations() in that it is ignoring errors from conf.Init() and still returning the configuration. But, my instinct is that addressing that will become a substantial effort which is outside the scope this PR.
But, yes, I would support another PR which checks for errors from conf.Init(), adds an error return from getActionConfigurations(), and modifies each of the dozen callers to handle errors appropriately. And, in the context of that code change, you could add appropriate unit testing for each of those callers which tests all of their respective error cases, which would address some of our technical debt here.
| for _, tt := range tests { | ||
| originalNewRegistryClient := newRegistryClient | ||
| defer func() { | ||
| newRegistryClient = originalNewRegistryClient | ||
| }() |
There was a problem hiding this comment.
We are not running the tests in parallel (via invoking
t.Parallel()). So we should not see this.
In fact, we need to avoid running the tests in parallel, because the mock uses values from the loop iterator, so it is critical that the mock for a given iteration be the one used by the test in that iteration.
we are not using a global mock
But we actually are: the CUT is accessing the mocked function via a global reference; if one iteration of the test changes the value of that reference, then that value will be visible to the other iterations of the test (unless they also change it). So, as long as we run the iterations serially, everything should be fine (because each CUT will see its iteration's mock); however, if we were to run them in parallel, then it is likely that some of them would use the wrong mock and wouldn't be able to pass the test (but, the behavior would be non-deterministic, which makes it worse).
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sowmya-sl, webbnh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@sowmya-sl: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
OCI-based Helm charts were failing to install because the action configuration lacked a registry client. This change:
Without a registry client, operations on OCI charts (oci://) would fail with errors about missing registry support.
Fixes: HELM-611
Original PR on #15830
Steps to test:
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.