From 6336e0fbf4e844501c43bb828de5dfa43e67ac9f Mon Sep 17 00:00:00 2001 From: gfichtenholt Date: Wed, 2 Nov 2022 21:33:36 -0700 Subject: [PATCH] available package handling with flux in multi-tenant mode #5541 --- .../fluxv2/packages/v1alpha1/common/utils.go | 4 ++ .../packages/v1alpha1/common/utils_test.go | 60 +++++++++++++++++++ .../plugins/fluxv2/packages/v1alpha1/repo.go | 25 ++++---- .../fluxv2/packages/v1alpha1/repo_test.go | 50 +++++++++++++--- .../fluxv2/packages/v1alpha1/server.go | 20 ++++--- 5 files changed, 134 insertions(+), 25 deletions(-) diff --git a/cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/common/utils.go b/cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/common/utils.go index cb8bb370a58..a70084d5e9a 100644 --- a/cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/common/utils.go +++ b/cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/common/utils.go @@ -453,6 +453,8 @@ type FluxPluginConfig struct { // see comments in design spec under AddPackageRepository. // false (i.e. kubeapps manages secrets) by default UserManagedSecrets bool + // ref https://github.com/vmware-tanzu/kubeapps/issues/5541 + NoCrossNamespaceRefs bool } // ParsePluginConfig parses the input plugin configuration json file and return the @@ -476,6 +478,7 @@ func ParsePluginConfig(pluginConfigPath string) (*FluxPluginConfig, error) { V1alpha1 struct { DefaultUpgradePolicy string `json:"defaultUpgradePolicy"` UserManagedSecrets bool `json:"userManagedSecrets"` + NoCrossNamespaceRefs bool `json:"noCrossNamespaceRefs"` } `json:"v1alpha1"` } `json:"packages"` } `json:"flux"` @@ -502,6 +505,7 @@ func ParsePluginConfig(pluginConfigPath string) (*FluxPluginConfig, error) { TimeoutSeconds: config.Core.Packages.V1alpha1.TimeoutSeconds, DefaultUpgradePolicy: defaultUpgradePolicy, UserManagedSecrets: config.Flux.Packages.V1alpha1.UserManagedSecrets, + NoCrossNamespaceRefs: config.Flux.Packages.V1alpha1.NoCrossNamespaceRefs, }, nil } } diff --git a/cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/common/utils_test.go b/cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/common/utils_test.go index fac8329f15a..4b2c8306dab 100644 --- a/cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/common/utils_test.go +++ b/cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/common/utils_test.go @@ -275,3 +275,63 @@ flux: }) } } + +func TestParsePluginConfigNoCrossNamespaceRefs(t *testing.T) { + testCases := []struct { + name string + pluginYAMLConf []byte + exp_flag bool + exp_error_str string + }{ + { + name: "no policy specified in plugin config", + pluginYAMLConf: nil, + exp_flag: false, + exp_error_str: "", + }, + { + name: "specific policy in plugin config", + pluginYAMLConf: []byte(` +flux: + packages: + v1alpha1: + noCrossNamespaceRefs: true + `), + exp_flag: true, + exp_error_str: "", + }, + } + opts := cmpopts.IgnoreUnexported(pkgutils.VersionsInSummary{}) + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + filename := "" + if tc.pluginYAMLConf != nil { + pluginJSONConf, err := yaml.YAMLToJSON(tc.pluginYAMLConf) + if err != nil { + log.Fatalf("%s", err) + } + f, err := os.CreateTemp(".", "plugin_json_conf") + if err != nil { + log.Fatalf("%s", err) + } + defer os.Remove(f.Name()) // clean up + if _, err := f.Write(pluginJSONConf); err != nil { + log.Fatalf("%s", err) + } + if err := f.Close(); err != nil { + log.Fatalf("%s", err) + } + filename = f.Name() + } + config, err := ParsePluginConfig(filename) + if err != nil && !strings.Contains(err.Error(), tc.exp_error_str) { + t.Errorf("err got %q, want to find %q", err.Error(), tc.exp_error_str) + } + if err == nil { + if got, want := config.NoCrossNamespaceRefs, tc.exp_flag; !cmp.Equal(want, got, opts) { + t.Errorf("mismatch (-want +got):\n%s", cmp.Diff(want, got, opts)) + } + } + }) + } +} diff --git a/cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/repo.go b/cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/repo.go index 5bd98dd0649..93d8cf2edd6 100644 --- a/cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/repo.go +++ b/cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/repo.go @@ -50,9 +50,10 @@ var ( defaultPollInterval = metav1.Duration{Duration: 10 * time.Minute} ) -// returns a list of HelmRepositories from all namespaces (cluster-wide), excluding +// returns a list of HelmRepositories from specified namespace. +// ns can be "", in which case all namespaces (cluster-wide), excluding // the ones that the caller has no read access to -func (s *Server) listReposInAllNamespaces(ctx context.Context) ([]sourcev1.HelmRepository, error) { +func (s *Server) listReposInNamespace(ctx context.Context, ns string) ([]sourcev1.HelmRepository, error) { // the actual List(...) call will be executed in the context of // kubeapps-internal-kubeappsapis service account // ref https://github.com/vmware-tanzu/kubeapps/issues/4390 for explanation @@ -63,7 +64,10 @@ func (s *Server) listReposInAllNamespaces(ctx context.Context) ([]sourcev1.HelmR } var repoList sourcev1.HelmRepositoryList - if err := client.List(backgroundCtx, &repoList); err != nil { + listOptions := ctrlclient.ListOptions{ + Namespace: ns, + } + if err := client.List(backgroundCtx, &repoList, &listOptions); err != nil { return nil, statuserror.FromK8sError("list", "HelmRepository", "", err) } else { // filter out those repos the caller has no access to @@ -146,11 +150,10 @@ func (s *Server) filterReadyReposByName(repoList []sourcev1.HelmRepository, matc } // Notes: -// 1. with flux, an available package may be from a repo in any namespace accessible to the caller -// 2. can't rely on cache as a real source of truth for key names +// 1. can't rely on cache as a real source of truth for key names // because redis may evict cache entries due to memory pressure to make room for new ones -func (s *Server) getChartsForRepos(ctx context.Context, match []string) (map[string][]models.Chart, error) { - repoList, err := s.listReposInAllNamespaces(ctx) +func (s *Server) getChartsForRepos(ctx context.Context, ns string, match []string) (map[string][]models.Chart, error) { + repoList, err := s.listReposInNamespace(ctx, ns) if err != nil { return nil, err } @@ -386,12 +389,12 @@ func (s *Server) repoDetail(ctx context.Context, repoRef *corev1.PackageReposito }, nil } -func (s *Server) repoSummaries(ctx context.Context, namespace string) ([]*corev1.PackageRepositorySummary, error) { +func (s *Server) repoSummaries(ctx context.Context, ns string) ([]*corev1.PackageRepositorySummary, error) { summaries := []*corev1.PackageRepositorySummary{} var repos []sourcev1.HelmRepository var err error - if namespace == apiv1.NamespaceAll { - if repos, err = s.listReposInAllNamespaces(ctx); err != nil { + if ns == apiv1.NamespaceAll { + if repos, err = s.listReposInNamespace(ctx, ns); err != nil { return nil, err } } else { @@ -401,7 +404,7 @@ func (s *Server) repoSummaries(ctx context.Context, namespace string) ([]*corev1 // error should be raised, as opposed to returning an empty list with no error var repoList sourcev1.HelmRepositoryList var client ctrlclient.Client - if client, err = s.getClient(ctx, namespace); err != nil { + if client, err = s.getClient(ctx, ns); err != nil { return nil, err } else if err = client.List(ctx, &repoList); err != nil { return nil, statuserror.FromK8sError("list", "HelmRepository", "", err) diff --git a/cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/repo_test.go b/cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/repo_test.go index 20b72a88460..43ece77a749 100644 --- a/cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/repo_test.go +++ b/cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/repo_test.go @@ -40,11 +40,12 @@ type testSpecGetAvailablePackageSummaries struct { func TestGetAvailablePackageSummariesWithoutPagination(t *testing.T) { testCases := []struct { - name string - request *corev1.GetAvailablePackageSummariesRequest - repos []testSpecGetAvailablePackageSummaries - expectedResponse *corev1.GetAvailablePackageSummariesResponse - expectedErrorCode codes.Code + name string + request *corev1.GetAvailablePackageSummariesRequest + repos []testSpecGetAvailablePackageSummaries + expectedResponse *corev1.GetAvailablePackageSummariesResponse + expectedErrorCode codes.Code + noCrossNamespaceRefs bool }{ { name: "it returns a couple of fluxv2 packages from the cluster (no request ns specified)", @@ -386,6 +387,30 @@ func TestGetAvailablePackageSummariesWithoutPagination(t *testing.T) { }}, expectedErrorCode: codes.Unimplemented, }, + { + name: "it returns expected fluxv2 packages when noCrossNamespaceRefs flag is set", + repos: []testSpecGetAvailablePackageSummaries{ + { + name: "bitnami-1", + namespace: "default", + url: "https://example.repo.com/charts", + index: testYaml("valid-index.yaml"), + }, + { + name: "jetstack-1", + namespace: "ns1", + url: "https://charts.jetstack.io", + index: testYaml("jetstack-index.yaml"), + }, + }, + request: &corev1.GetAvailablePackageSummariesRequest{Context: &corev1.Context{Namespace: "ns1"}}, + expectedResponse: &corev1.GetAvailablePackageSummariesResponse{ + AvailablePackageSummaries: []*corev1.AvailablePackageSummary{ + cert_manager_summary, + }, + }, + noCrossNamespaceRefs: true, + }, } for _, tc := range testCases { @@ -408,8 +433,19 @@ func TestGetAvailablePackageSummariesWithoutPagination(t *testing.T) { t.Fatalf("error instantiating the server: %v", err) } - if err = s.redisMockExpectGetFromRepoCache(mock, tc.request.FilterOptions, repos...); err != nil { - t.Fatalf("%v", err) + if tc.noCrossNamespaceRefs { + s.pluginConfig.NoCrossNamespaceRefs = true + for _, r := range repos { + if r.Namespace == tc.request.Context.Namespace { + if err = s.redisMockExpectGetFromRepoCache(mock, nil, r); err != nil { + t.Fatalf("%v", err) + } + } + } + } else { + if err = s.redisMockExpectGetFromRepoCache(mock, tc.request.FilterOptions, repos...); err != nil { + t.Fatalf("%v", err) + } } response, err := s.GetAvailablePackageSummaries(context.Background(), tc.request) diff --git a/cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/server.go b/cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/server.go index 7479ae9b877..80c0bc6c40a 100644 --- a/cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/server.go +++ b/cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/server.go @@ -160,12 +160,13 @@ func NewServer(configGetter core.KubernetesConfigGetter, kubeappsCluster string, // GetAvailablePackageSummaries returns the available packages based on the request. // Note that currently packages are returned only from repos that are in a 'Ready' -// state. For the fluxv2 plugin, the request context namespace (the target -// namespace) is not relevant since charts from a repository in any namespace -// accessible to the user are available to be installed in the target namespace. -// TODO (gfichtenholt) if flux helm-controller flag "-no-cross-namespace-refs=true" is -// enabled we might need to filter out all namespaces except for request target namespace -// ref https://github.com/vmware-tanzu/kubeapps/issues/5541 +// state. For the fluxv2 plugin: +// - if flux helm-controller flag "-no-cross-namespace-refs=true" is +// enabled only the request target namespace is relevant +// ref https://github.com/vmware-tanzu/kubeapps/issues/5541 +// - otherwise the request context namespace (the target +// namespace) is not relevant since charts from a repository in any namespace +// accessible to the user are available to be installed in the target namespace. func (s *Server) GetAvailablePackageSummaries(ctx context.Context, request *corev1.GetAvailablePackageSummariesRequest) (*corev1.GetAvailablePackageSummariesResponse, error) { log.Infof("+fluxv2 GetAvailablePackageSummaries(request: [%v])", request) defer log.Info("-fluxv2 GetAvailablePackageSummaries") @@ -185,7 +186,12 @@ func (s *Server) GetAvailablePackageSummaries(ctx context.Context, request *core return nil, err } - charts, err := s.getChartsForRepos(ctx, request.GetFilterOptions().GetRepositories()) + ns := metav1.NamespaceAll + if s.pluginConfig.NoCrossNamespaceRefs { + ns = request.Context.Namespace + } + + charts, err := s.getChartsForRepos(ctx, ns, request.GetFilterOptions().GetRepositories()) if err != nil { return nil, err }