Skip to content
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

Issue5542 - Unable to edit or delete newly created flux package repository #5664

Merged
merged 5 commits into from
Nov 18, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
323 changes: 174 additions & 149 deletions cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/global_vars_test.go

Large diffs are not rendered by default.

18 changes: 12 additions & 6 deletions cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -232,8 +232,10 @@ func (s *Server) newRepo(ctx context.Context, request *corev1.AddPackageReposito
return nil, status.Errorf(codes.InvalidArgument, "no request Name provided")
}

if request.GetNamespaceScoped() {
return nil, status.Errorf(codes.Unimplemented, "namespaced-scoped repositories are not supported")
// flux repositories are now considered to be namespaced, to support the most common cases.
// see discussion at https://github.com/vmware-tanzu/kubeapps/issues/5542
if !request.GetNamespaceScoped() {
return nil, status.Errorf(codes.Unimplemented, "global-scoped repositories are not supported")
}

typ := request.GetType()
Expand Down Expand Up @@ -377,8 +379,10 @@ func (s *Server) repoDetail(ctx context.Context, repoRef *corev1.PackageReposito
},
Name: repo.Name,
// TODO (gfichtenholt) Flux HelmRepository CR doesn't have a designated field for description
Description: "",
NamespaceScoped: false,
Description: "",
// flux repositories are now considered to be namespaced, to support the most common cases.
// see discussion at https://github.com/vmware-tanzu/kubeapps/issues/5542
NamespaceScoped: true,
Type: typ,
Url: repo.Spec.URL,
Interval: pkgutils.FromDuration(&repo.Spec.Interval),
Expand Down Expand Up @@ -429,8 +433,10 @@ func (s *Server) repoSummaries(ctx context.Context, ns string) ([]*corev1.Packag
},
Name: repo.Name,
// TODO (gfichtenholt) Flux HelmRepository CR doesn't have a designated field for description
Description: "",
NamespaceScoped: false,
Description: "",
// flux repositories are now considered to be namespaced, to support the most common cases.
// see discussion at https://github.com/vmware-tanzu/kubeapps/issues/5542
NamespaceScoped: true,
Type: typ,
Url: repo.Spec.URL,
Status: repoStatus(repo),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"context"
packagingv1alpha1 "github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/packaging/v1alpha1"
"github.com/vmware-tanzu/kubeapps/cmd/kubeapps-apis/plugins/pkg/resources"
"k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime/schema"

corev1 "github.com/vmware-tanzu/kubeapps/cmd/kubeapps-apis/gen/core/packages/v1alpha1"
Expand Down Expand Up @@ -149,17 +148,33 @@ func (s *Server) GetPackageRepositorySummaries(ctx context.Context, request *cor
// trace logging
log.InfoS("+kapp-controller GetPackageRepositories", "cluster", cluster, "namespace", namespace)

// retrieve the list of installed packages
pkgRepositories, err := s.getPkgRepositories(ctx, cluster, namespace)
if err != nil {
if errors.IsForbidden(err) && namespace == "" {
// retrieve the list of repositories
var pkgRepositories []*packagingv1alpha1.PackageRepository
if namespace == "" {
// find globally, either via cluster access or by enumerating through namespaces
if repos, err := s.getPkgRepositories(ctx, cluster, ""); err == nil {
pkgRepositories = append(pkgRepositories, repos...)
} else {
log.Warningf("+kapp-controller unable to list package repositories at the cluster scope in '%s' due to [%v]", cluster, err)
pkgRepositories, err = s.getAccessiblePackageRepositories(ctx, cluster)
if err != nil {
if repos, err = s.getAccessiblePackageRepositories(ctx, cluster); err == nil {
pkgRepositories = append(pkgRepositories, repos...)
} else {
return nil, err
}
}
} else {
// include namespace specific repositories
if repos, err := s.getPkgRepositories(ctx, cluster, namespace); err == nil {
pkgRepositories = append(pkgRepositories, repos...)
} else {
return nil, statuserror.FromK8sError("get", "PackageRepository", "", err)
return nil, err
}

// try to also include global repositories
if namespace != s.pluginConfig.globalPackagingNamespace {
if repos, err := s.getPkgRepositories(ctx, cluster, s.pluginConfig.globalPackagingNamespace); err == nil {
pkgRepositories = append(pkgRepositories, repos...)
}
}
}

Expand Down Expand Up @@ -323,6 +338,7 @@ func (s *Server) DeletePackageRepository(ctx context.Context, request *corev1.De
return response, nil
}

// GetPackageRepositoryPermissions provides permissions available to manage package repository by the 'kapp_controller' plugin
func (s *Server) GetPackageRepositoryPermissions(ctx context.Context, request *corev1.GetPackageRepositoryPermissionsRequest) (*corev1.GetPackageRepositoryPermissionsResponse, error) {
log.Infof("+kapp-controller GetPackageRepositoryPermissions [%v]", request)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -358,15 +358,33 @@ func (s *Server) getAccessiblePackageRepositories(ctx context.Context, cluster s
return nil, err
}
namespaceList = resources.FilterActiveNamespaces(namespaceList)

var accessibleRepos []*packagingv1alpha1.PackageRepository
for _, ns := range namespaceList {
nsRepos, err := s.getPkgRepositories(ctx, cluster, ns.Name)
if err != nil {
if nsRepos, err := s.getPkgRepositories(ctx, cluster, ns.Name); err != nil {
log.Warningf("++kapp-controller could not list PackageRepository in namespace %s", ns.Name)
// Continue. Error in a single namespace should not block the whole list
} else {
accessibleRepos = append(accessibleRepos, nsRepos...)
}
accessibleRepos = append(accessibleRepos, nsRepos...)
}

// in general, the user will not have admin level access to the global namespace, so checking explicitly
var hasglobalns bool
for _, ns := range namespaceList {
if ns.Name == s.pluginConfig.globalPackagingNamespace {
hasglobalns = true
break
}
}
if !hasglobalns {
if nsRepos, err := s.getPkgRepositories(ctx, cluster, s.pluginConfig.globalPackagingNamespace); err != nil {
log.Warningf("++kapp-controller could not list PackageRepository in global namespace")
} else {
accessibleRepos = append(accessibleRepos, nsRepos...)
}
}

return accessibleRepos, nil
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9310,8 +9310,10 @@ func TestGetPackageRepositorySummariesFiltering(t *testing.T) {
request: &corev1.GetPackageRepositorySummariesRequest{
Context: &corev1.Context{Namespace: "default"},
},
existingObjects: repositories,
expectedResponse: []metav1.ObjectMeta{},
existingObjects: repositories,
expectedResponse: []metav1.ObjectMeta{
{Name: "globalrepo", Namespace: demoGlobalPackagingNamespace},
},
},
{
name: "returns repositories from given namespace",
Expand All @@ -9320,6 +9322,7 @@ func TestGetPackageRepositorySummariesFiltering(t *testing.T) {
},
existingObjects: repositories,
expectedResponse: []metav1.ObjectMeta{
{Name: "globalrepo", Namespace: demoGlobalPackagingNamespace},
{Name: "nsrepo", Namespace: "privatens"},
},
},
Expand Down
12 changes: 6 additions & 6 deletions dashboard/src/components/Config/PkgRepoList/PkgRepoForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -532,10 +532,8 @@ export function PkgRepoForm(props: IPkgRepoFormProps) {
setType(RepositoryStorageTypes.PACKAGE_REPOSITORY_STORAGE_HELM);
}

// update the isNampespaced field based on the plugin
setIsNamespaceScoped(
!isGlobalNamespace(namespace, PluginNames.PACKAGES_FLUX, currentNsConfig),
);
// flux is always namespace scoped
setIsNamespaceScoped(true);
break;
}
case PluginNames.PACKAGES_KAPP:
Expand Down Expand Up @@ -849,7 +847,8 @@ export function PkgRepoForm(props: IPkgRepoFormProps) {
onChange={handleRepoScopeChange}
disabled={
!!repo.name ||
isGlobalNamespace(namespace, plugin?.name, currentNsConfig)
isGlobalNamespace(namespace, plugin?.name, currentNsConfig) ||
plugin?.name === PluginNames.PACKAGES_FLUX
}
required={true}
/>
Expand All @@ -870,7 +869,8 @@ export function PkgRepoForm(props: IPkgRepoFormProps) {
onChange={handleRepoScopeChange}
disabled={
!!repo.name ||
isGlobalNamespace(namespace, plugin?.name, currentNsConfig)
isGlobalNamespace(namespace, plugin?.name, currentNsConfig) ||
plugin?.name === PluginNames.PACKAGES_FLUX
}
required={true}
/>
Expand Down
4 changes: 2 additions & 2 deletions dashboard/src/shared/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -328,8 +328,8 @@ it("isGlobalNamespace", () => {
} as IConfig;
expect(isGlobalNamespace("helm-global", PluginNames.PACKAGES_HELM, kubeappsConfig)).toBe(true);
expect(isGlobalNamespace("helm-global", PluginNames.PACKAGES_KAPP, kubeappsConfig)).toBe(false);
expect(isGlobalNamespace("helm-global", PluginNames.PACKAGES_FLUX, kubeappsConfig)).toBe(true);
expect(isGlobalNamespace("helm-global", PluginNames.PACKAGES_FLUX, kubeappsConfig)).toBe(false);
expect(isGlobalNamespace("carvel-global", PluginNames.PACKAGES_HELM, kubeappsConfig)).toBe(false);
expect(isGlobalNamespace("carvel-global", PluginNames.PACKAGES_KAPP, kubeappsConfig)).toBe(true);
expect(isGlobalNamespace("carvel-global", PluginNames.PACKAGES_FLUX, kubeappsConfig)).toBe(true);
expect(isGlobalNamespace("carvel-global", PluginNames.PACKAGES_FLUX, kubeappsConfig)).toBe(false);
});
6 changes: 3 additions & 3 deletions dashboard/src/shared/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -314,9 +314,9 @@ export function isGlobalNamespace(namespace: string, pluginName: string, kubeapp
return namespace === kubeappsConfig.helmGlobalNamespace;
case PluginNames.PACKAGES_KAPP:
return namespace === kubeappsConfig.carvelGlobalNamespace;
// Currently, Flux doesn't namespaced repos, so it will always be global
// Currently, Flux doesn't support global repositories
case PluginNames.PACKAGES_FLUX:
return true;
return false;
default:
return false;
}
Expand All @@ -332,7 +332,7 @@ export function getGlobalNamespaceOrNamespace(
return kubeappsConfig.helmGlobalNamespace;
case PluginNames.PACKAGES_KAPP:
return kubeappsConfig.carvelGlobalNamespace;
// Currently, Flux doesn't namespaced repos, so the given ns will be indeed global
// Currently, Flux doesn't support global repositories, so returning the namespace so we have a value
case PluginNames.PACKAGES_FLUX:
return namespace;
default:
Expand Down