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
Bug 1872369: Fix reading TLSConfig for HelmChartRepository #6347
Bug 1872369: Fix reading TLSConfig for HelmChartRepository #6347
Conversation
/retest |
2 similar comments
/retest |
/retest |
/assign @jhadvig |
Can you add more a better description to the PR so it's clear what the purpose is? This will require a Bugzilla bug to merge since we're past feature freeze. |
|
||
func (b helmRepoGetter) unmarshallConfig(repo unstructured.Unstructured) (*helmRepo, error) { | ||
h := &helmRepo{} | ||
urlValue, _, err := unstructured.NestedString(repo.Object, "spec", "connectionConfig", "url") |
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.
If you bump openshift/api, you should have a real struct with these fields, which would be better for type safety.
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.
We would love too, and we tried to add openshift/client-go
as the dependency (it contains all necessary types), but we cannot make it work currently in console, because client-go depends on k8s 1.19, but Helm (we depend on here) requires k8s 1.18. Tried to force it, but Helm is not yet compatible to k8s 1.19.
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.
is there any plan to make it compatible ?
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.
sure, when Helm moves to 1.19.
pkg/helm/chartproxy/repos.go
Outdated
} | ||
|
||
func (b helmRepoGetter) secretValue(name string, dataField string) ([]byte, error) { | ||
secret, err := b.CoreClient.Secrets(configNamespace).Get(context.TODO(), name, v1.GetOptions{}) |
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.
What is the experience for users who don't have authority to get the secret?
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.
They do not have access to that chart repo and consequently to charts managed there. UI will not list them at all.
@pedjak: This pull request references Bugzilla bug 1872369, which is invalid:
Comment In 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 kubernetes/test-infra repository. |
/bugzilla refresh |
@pedjak: This pull request references Bugzilla bug 1872369, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In 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 kubernetes/test-infra repository. |
/retest |
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.
@pedjak thanks for the PR. couple of questions and comments after the first round of review.
pkg/helm/chartproxy/repos.go
Outdated
type helmRepo struct { | ||
Name string | ||
Url *url.URL | ||
TlsClientConfig *tls.Config |
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.
nit:
TlsClientConfig *tls.Config | |
TLSClientConfig *tls.Config |
func (b *helmRepoGetter) List() ([]*helmRepo, error) { | ||
var helmRepos []*helmRepo | ||
repos, err := b.Client.Resource(helmChartRepositoryGVK).List(context.TODO(), v1.ListOptions{}) | ||
if err != nil || len(repos.Items) == 0 { |
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.
Is silencing the error here intentional ?
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, check the comment in the next line.
pkg/helm/handlers/handlers.go
Outdated
|
||
"github.com/openshift/console/pkg/auth" | ||
"github.com/openshift/console/pkg/helm/actions" | ||
"github.com/openshift/console/pkg/serverutils" | ||
) | ||
|
||
var ( | ||
plog = capnslog.NewPackageLogger("github.com/openshift/console", "helm") |
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.
plog = capnslog.NewPackageLogger("github.com/openshift/console", "helm") | |
plog = capnslog.NewPackageLogger("github.com/openshift/console", "helm/handlers") |
pkg/server/server.go
Outdated
handle("/api/helm/template", authHandlerWithUser(helmHandlers.HandleHelmRenderManifests)) | ||
handle("/api/helm/releases", authHandlerWithUser(helmHandlers.HandleHelmList)) | ||
handle("/api/helm/chart", authHandlerWithUser(helmHandlers.HandleChartGet)) | ||
handle("/api/helm/release/history", authHandlerWithUser(helmHandlers.HandleGetReleaseHistory)) | ||
handle("/api/helm/charts/index.yaml", authHandlerWithUser(helmHandlers.HandleGetRepos)) | ||
handle("/api/helm/charts/index.yaml", authHandlerWithUser(helmHandlers.IndexFile)) |
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.
handle("/api/helm/charts/index.yaml", authHandlerWithUser(helmHandlers.IndexFile)) | |
handle("/api/helm/charts/index.yaml", authHandlerWithUser(helmHandlers.HandleIndexFile)) |
pkg/helm/handlers/handler_test.go
Outdated
t.Errorf("Response status code isn't matching expected %d recieved %d", tt.httpStatusCode, response.Code) | ||
} | ||
if response.Code != tt.httpStatusCode { | ||
t.Errorf("Response status code isn't matching expected %d recieved %d", tt.httpStatusCode, response.Code) |
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.
t.Errorf("Response status code isn't matching expected %d recieved %d", tt.httpStatusCode, response.Code) | |
t.Errorf("Response status code isn't matching expected %d received %d", tt.httpStatusCode, response.Code) |
|
||
func (b helmRepoGetter) unmarshallConfig(repo unstructured.Unstructured) (*helmRepo, error) { | ||
h := &helmRepo{} | ||
urlValue, _, err := unstructured.NestedString(repo.Object, "spec", "connectionConfig", "url") |
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.
is there any plan to make it compatible ?
pkg/helm/chartproxy/repos_test.go
Outdated
expectedRepoName: []string{"sample-repo-1", "sample-repo-2"}, | ||
}, | ||
{ | ||
name: "return 1 repos found in cluster", |
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.
nit:
name: "return 1 repos found in cluster", | |
name: "return 1 repo found in cluster", |
pkg/helm/chartproxy/repos_test.go
Outdated
}, | ||
{ | ||
name: "return default repo when none are declared in cluster", | ||
ReposNum: 0, |
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.
Shouldn't this be 1
since the default one is returned ?
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.
it represents the number of HelmChartRepos found in cluster
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.
nope, we are testing the situation when there are no HelmChartRepo CRs available in the cluster - in that case, we should still fall back on the default repo.
"github.com/openshift/library-go/pkg/crypto" | ||
) | ||
|
||
var ( |
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 this needs to be a global variable ?
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.
is it private to this package. we use it to retrieve the instances. I am open to any better approach.
pkg/helm/chartproxy/config.go
Outdated
oscrypto "github.com/openshift/library-go/pkg/crypto" | ||
) | ||
|
||
var ( | ||
log = capnslog.NewPackageLogger("github.com/openshift/console", "pkg/helm") | ||
log = capnslog.NewPackageLogger("github.com/openshift/console", "pkg/helm") |
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.
log = capnslog.NewPackageLogger("github.com/openshift/console", "pkg/helm") | |
log = capnslog.NewPackageLogger("github.com/openshift/console", "helm/chartproxy") |
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.
Some additional comments to fix. Otherwise looks good.
pkg/helm/chartproxy/repos.go
Outdated
return nil, err | ||
} | ||
} | ||
h.TlSClientConfig = &tls.Config{ |
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.
h.TlSClientConfig = &tls.Config{ | |
h.TLSClientConfig = oscrypto.SecureTLSConfig(&tls.Config{ |
pkg/helm/chartproxy/repos.go
Outdated
} | ||
h.TlSClientConfig = &tls.Config{ | ||
RootCAs: rootCAs, | ||
CipherSuites: crypto.DefaultCiphers(), |
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.
This line can be removed, oscrypto.SecureTLSConfig()
will take care of it.
pkg/helm/chartproxy/repos.go
Outdated
func (b helmRepoGetter) secretValue(name string, dataField string) ([]byte, error) { | ||
secret, err := b.CoreClient.Secrets(configNamespace).Get(context.TODO(), name, v1.GetOptions{}) | ||
if err != nil { | ||
return nil, errors.New(fmt.Sprintf("Failed to read secret %s", name)) |
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.
nit:
return nil, errors.New(fmt.Sprintf("Failed to read secret %s", name)) | |
return nil, errors.New(fmt.Sprintf("Failed to GET secret %s", name)) |
pkg/helm/chartproxy/repos.go
Outdated
func (b helmRepoGetter) configMapValue(name string, dataField string) ([]byte, error) { | ||
configMap, err := b.CoreClient.ConfigMaps(configNamespace).Get(context.TODO(), name, v1.GetOptions{}) | ||
if err != nil { | ||
return nil, errors.New(fmt.Sprintf("Failed to read configmap %s", name)) |
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.
nit:
return nil, errors.New(fmt.Sprintf("Failed to read configmap %s", name)) | |
return nil, errors.New(fmt.Sprintf("Failed to GET configmap %s", name)) |
pkg/helm/chartproxy/repos.go
Outdated
|
||
type helmRepo struct { | ||
Name string | ||
Url *url.URL |
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.
Url *url.URL | |
URL *url.URL |
pkg/helm/chartproxy/repos.go
Outdated
if err != nil { | ||
return nil, err | ||
} | ||
indexUrl := hr.Url.String() |
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.
indexUrl := hr.Url.String() | |
indexURL := hr.Url.String() |
pkg/helm/chartproxy/repos.go
Outdated
if !strings.HasSuffix(indexUrl, "/index.yaml") { | ||
indexUrl += "/index.yaml" | ||
} | ||
resp, err := httpClient.Get(indexUrl) |
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.
resp, err := httpClient.Get(indexUrl) | |
resp, err := httpClient.Get(indexURL) |
pkg/helm/chartproxy/config.go
Outdated
srv.HelmChartRepoProxyConfig = &proxy.Config{ | ||
DefaultRepo = helmRepo{ | ||
Name: "redhat-helm-charts", | ||
Url: repoURL, |
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.
Url: repoURL, | |
URL: repoURL, |
pkg/helm/chartproxy/config.go
Outdated
@@ -33,14 +32,13 @@ func RegisterFlags(fs *flag.FlagSet) *config { | |||
return cfg | |||
} | |||
|
|||
func (cfg *config) Configure(srv *server.Server) { | |||
func (cfg *config) Configure() { | |||
repoURL := bridge.ValidateFlagIsURL("helm-chart-repo-repoUrl", cfg.repoUrl) | |||
|
|||
var rootCAs *x509.CertPool | |||
if cfg.repoCaFile != "" { |
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.
if cfg.repoCaFile != "" { | |
if cfg.repoCAFile != "" { |
pkg/helm/chartproxy/config.go
Outdated
@@ -33,14 +32,13 @@ func RegisterFlags(fs *flag.FlagSet) *config { | |||
return cfg | |||
} | |||
|
|||
func (cfg *config) Configure(srv *server.Server) { | |||
func (cfg *config) Configure() { | |||
repoURL := bridge.ValidateFlagIsURL("helm-chart-repo-repoUrl", cfg.repoUrl) |
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.
repoURL := bridge.ValidateFlagIsURL("helm-chart-repo-repoUrl", cfg.repoUrl) | |
repoURL := bridge.ValidateFlagIsURL("helm-chart-repo-url", cfg.repoURL) |
pkg/helm/chartproxy/config.go
Outdated
repoURL := bridge.ValidateFlagIsURL("helm-chart-repo-repoUrl", cfg.repoUrl) | ||
|
||
var rootCAs *x509.CertPool | ||
if cfg.repoCaFile != "" { | ||
rootCAs = x509.NewCertPool() | ||
certPEM, err := ioutil.ReadFile(cfg.repoCaFile) | ||
srv.HelmDefaultRepoCACert = certPEM | ||
if err != nil { | ||
log.Fatalf("failed to read helm chart repo ca file %v : %v", cfg.repoCaFile, err) |
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.
log.Fatalf("failed to read helm chart repo ca file %v : %v", cfg.repoCaFile, err) | |
log.Fatalf("failed to read helm chart repo ca file %v : %v", cfg.repoCAFile, err) |
pkg/helm/chartproxy/config.go
Outdated
@@ -33,14 +32,13 @@ func RegisterFlags(fs *flag.FlagSet) *config { | |||
return cfg | |||
} | |||
|
|||
func (cfg *config) Configure(srv *server.Server) { | |||
func (cfg *config) Configure() { | |||
repoURL := bridge.ValidateFlagIsURL("helm-chart-repo-repoUrl", cfg.repoUrl) | |||
|
|||
var rootCAs *x509.CertPool | |||
if cfg.repoCaFile != "" { | |||
rootCAs = x509.NewCertPool() | |||
certPEM, err := ioutil.ReadFile(cfg.repoCaFile) |
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.
certPEM, err := ioutil.ReadFile(cfg.repoCaFile) | |
certPEM, err := ioutil.ReadFile(cfg.repoCAFile) |
pkg/helm/chartproxy/repos_test.go
Outdated
indexFileContennts = append(indexFileContennts, "") | ||
} | ||
client := fake.K8sDynamicClient(indexFileContennts...) | ||
cfg := config{repoUrl: "https://default-url.com"} |
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.
cfg := config{repoUrl: "https://default-url.com"} | |
cfg := config{repoURL: "https://default-url.com"} |
e5a854a
to
6b8c619
Compare
pkg/helm/chartproxy/repos.go
Outdated
RootCAs: rootCAs, | ||
}) | ||
if tlsReference != "" { | ||
tlsCert, err := b.secretValue(tlsReference, "tls.crt") |
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.
secretValue()
method is making the API call. Should be enough to call it once and check the wanted field.
tlsSecret, err := b.CoreClient.Secrets(configNamespace).Get(context.TODO(), name, v1.GetOptions{})
if err != nil {
return nil, errors.New(fmt.Sprintf("Failed to GET secret %s", name))
}
tlsCert, err := b.secretValue(tlsSecret, "tls.crt")
tlsCert, err := b.secretValue(tlsSecret, "tls.key")
pkg/helm/chartproxy/repos.go
Outdated
|
||
var rootCAs *x509.CertPool | ||
if caReference != "" { | ||
caCert, err := b.configMapValue(caReference, "ca-bundle.crt") |
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.
Would pull the API call for GETting the CM from the configMapValue
method and just check for it ... check the comment below
Additional fixes/improvements: * Refactored Helm chart repo model for better testability * More tests added Co-authored-by: Predrag Knezevic <pknezevi@redhat.com>
277bfe0
to
100377a
Compare
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.
/lgtm
/approve
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhadvig, pedjak, spadgett The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
@pedjak: All pull requests linked via external trackers have merged: Bugzilla bug 1872369 has been moved to the MODIFIED state. In 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 kubernetes/test-infra repository. |
Secret containing TLS config was wrongly expected to be found in
HelmChartRepository.spec.connectionConfig.tlsconfig.name
path.According to https://github.com/openshift/api/blob/master/helm/v1beta1/types_helm.go#L76 TLS client config is to be found in secret whose name is set in
HelmChartRepository.spec.connectionConfig.tlsClientConfig.name
pathAdditional fixes/improvements: