-
Notifications
You must be signed in to change notification settings - Fork 704
HELM-703: Add authentication for Helm chart URLs #16360
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
fbb1b8a
503ecc6
198b1d8
b542a5e
f314a52
c304811
ae2bb4e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -126,12 +126,16 @@ | |
| "The OCI URL or HTTP/HTTPS tar file for the Helm chart; for example - oci://registry.example.com/charts/mychart or https://example.com/chart-1.0.0.tgz.": "The OCI URL or HTTP/HTTPS tar file for the Helm chart; for example - oci://registry.example.com/charts/mychart or https://example.com/chart-1.0.0.tgz.", | ||
| "Unique name for Helm release.": "Unique name for Helm release.", | ||
| "The version of chart to install.": "The version of chart to install.", | ||
| "Secret for basic authentication": "Secret for basic authentication", | ||
| "Select a secret": "Select a secret", | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. any helper text, tooltip, link for this "Select a secret" field to tells users how to create one if they don't have one? also, Secret cap S if you mean the Kube kind (global)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a text under the dropdown which says that the Secret should have the keys username and password. |
||
| "A secret with \"username\" and \"password\" keys for OCI/HTTP(S) authentication": "A secret with \"username\" and \"password\" keys for OCI/HTTP(S) authentication", | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should these be listed separately for clarity, for ex., "A Secret with username and password keys for authenticating with OCI or HTTP/HTTPS Helm chart URLs"? And should username and password be formatted as code/monospace since they're literal key names the user must match exactly?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This text is to appear under the dropdown to select Secrets. I thought OCI, HTTP(S) charts was understood, since the chart URL section will mention OCI or HTTP(S).
Yes, username and password are the key names for Secret. Formatted as monospace is possible ? Can you share any examples in console for me reference? |
||
| "Next": "Next", | ||
| "Install Helm chart from Helm registry.": "Install Helm chart from Helm registry.", | ||
| "Helm release": "Helm release", | ||
| "Complete the form to create a Helm release. The Helm chart authors might have provided some default values.": "Complete the form to create a Helm release. The Helm chart authors might have provided some default values.", | ||
| "Configure Helm release": "Configure Helm release", | ||
| "Version": "Version", | ||
| "None": "None", | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. where does "None" display? default drop-down option for Version field? If so, would "Latest" or "No specific version" be clearer to users about what will happen if they don't select it?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. None displays in the Secret field if the user has not selected any Secrets. |
||
| "Install": "Install", | ||
| "Back": "Back", | ||
| "Display Name": "Display Name", | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These changes seem very similar to the ones in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which function are you referring to here? |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| import { useMemo } from 'react'; | ||
| import { useK8sWatchResources } from '@console/internal/components/utils/k8s-watch-hook'; | ||
| import { SecretModel } from '@console/internal/models'; | ||
| import type { K8sResourceKind } from '@console/internal/module/k8s'; | ||
|
|
||
| export const useSecretResources = (namespace: string) => { | ||
| const watchedResources = useK8sWatchResources<{ | ||
| secrets: K8sResourceKind[]; | ||
| }>({ | ||
| secrets: { | ||
| isList: true, | ||
| kind: SecretModel.kind, | ||
| namespace, | ||
| optional: true, | ||
| }, | ||
| }); | ||
|
|
||
| return useMemo( | ||
| () => [ | ||
| { | ||
| data: watchedResources.secrets?.data, | ||
| loaded: watchedResources.secrets?.loaded, | ||
| loadError: watchedResources.secrets?.loadError, | ||
| kind: SecretModel.kind, | ||
| }, | ||
| ], | ||
| [ | ||
| watchedResources.secrets?.data, | ||
| watchedResources.secrets?.loaded, | ||
| watchedResources.secrets?.loadError, | ||
| ], | ||
| ); | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,21 +5,18 @@ import ( | |
| "fmt" | ||
| "net/http" | ||
|
|
||
| "helm.sh/helm/v3/pkg/action" | ||
| "helm.sh/helm/v3/pkg/registry" | ||
| ) | ||
|
|
||
| // newRegistryClient is a package-level variable to allow mocking in tests | ||
| var newRegistryClient = registry.NewClient | ||
|
|
||
| func GetDefaultOCIRegistry(conf *action.Configuration) error { | ||
| return GetOCIRegistry(conf, false, false) | ||
| type UserCredentials struct { | ||
| Username string | ||
| Password string | ||
| } | ||
|
|
||
| func GetOCIRegistry(conf *action.Configuration, skipTLSVerify bool, plainHTTP bool) error { | ||
| if conf == nil { | ||
| return fmt.Errorf("action configuration cannot be nil") | ||
| } | ||
| func GetOCIRegistry(skipTLSVerify bool, plainHTTP bool, userCredentials *UserCredentials) (*registry.Client, error) { | ||
| opts := []registry.ClientOption{ | ||
| registry.ClientOptDebug(false), | ||
| } | ||
|
|
@@ -33,10 +30,17 @@ func GetOCIRegistry(conf *action.Configuration, skipTLSVerify bool, plainHTTP bo | |
| } | ||
| opts = append(opts, registry.ClientOptHTTPClient(&http.Client{Transport: transport})) | ||
| } | ||
| if userCredentials != nil { | ||
| opts = append(opts, registry.ClientOptBasicAuth(userCredentials.Username, userCredentials.Password)) | ||
| } | ||
| registryClient, err := newRegistryClient(opts...) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create registry client: %w", err) | ||
| return nil, fmt.Errorf("failed to create registry client: %w", err) | ||
| } | ||
| conf.RegistryClient = registryClient | ||
| return nil | ||
| return registryClient, nil | ||
|
Comment on lines
+36
to
+40
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this error really providing value? Otherwise, I think you can simplify this to just return newRegistryClient(opts...)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's better to have some error logging for creation of registry client. I will be needed when creating a registry Client object with authentication or any other future modifications fails.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't disagree, but this code isn't performing any logging -- it's just adding text to the error message. Following the call-chain back up, eventually we hit a caller which does do error logging, and it reports "Failed to get default OCI registry"...so (I think...) we already have the context of creating a registry, so the text that we're adding here doesn't actually provide any additional information.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function is also getting called in install_chart.go and get_chart.go. That surfaces the error if this fails. |
||
|
|
||
| } | ||
|
|
||
| func GetDefaultOCIRegistry() (*registry.Client, error) { | ||
| return GetOCIRegistry(false, false, nil) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does "secret" here refer to a Kubernetes Secret resource? if so, should it be capitalized "Secret" to match how OpenShift usually refers to that object type?
also, "basic" in "basic authentication" always gets a cap B.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes secret is for K8 secret so it is capitalized. "Basic" is fixed in #16490