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

available package handling with flux in multi-tenant mode #5541

Closed
gfichtenholt opened this issue Oct 21, 2022 · 5 comments · Fixed by #5584
Closed

available package handling with flux in multi-tenant mode #5541

gfichtenholt opened this issue Oct 21, 2022 · 5 comments · Fixed by #5584
Assignees
Labels
component/plugin-flux Issue related to kubeapps plugin to manage Helm charts via Flux kind/bug An issue that reports a defect in an existing feature

Comments

@gfichtenholt
Copy link
Contributor

gfichtenholt commented Oct 21, 2022

low priority:
while going through flux multi-tenant use case (see attached notes), I discovered that there is a case where flux plugin GetAvailablePackage...() APIs may be improved

in short, the scenario is:

  • flux helm-controller is deployed with flag -no-cross-namespace-refs=true
  • user A creates a repo in ns A
  • kubeapps-operator cluster-admin logs in and sees available packages from repo A
  • an attempt to install a package will result in error "cross-namespaced references have been blocked" (see screenshot)

Arguably, either our UX or flux plugin should have prevented this scenario. I had one idea I expressed in slack
"One way would be to somehow filter out available packages from namespaces other than current context by the plugin ONLY if the flag --no-cross-namespace-refs is set. It already filters out ones that user does not have access to, but it doesn't help in this case. The list of available packages for namespace "default" should have been empty, as there werent any repos registered in that namespace. Not sure if there is a way to cleanly find out whether the flag is set without breaking abstractions."

multi-tenant-setup.txt
Screen Shot 2022-10-20 at 1 14 25 AM

@gfichtenholt gfichtenholt added the kind/bug An issue that reports a defect in an existing feature label Oct 21, 2022
@dlaloue-vmware
Copy link
Collaborator

dlaloue-vmware commented Oct 21, 2022

Ideally, yes, the plugin should be able to detect the settings from the flux installation. The same way ideally the carvel plugin should detect what the namespace is for global carvel repositories. Doing so though may require additional rbac privileges.

But if you look at the values file, the namespace is provided as a plugin confifuration parameters.
I think it is totally acceptable to have a flag in the flux plugin configuration to let kubeapps know whether flux has been installed with cross-namespace references disabled.

regarding UI vs. backend, because we expose an API, the backend should perform some validation instead of letting it to flux to fail. UI may also have its own checks to avoid going to the API when we know it would be an invalid configuration.

@ppbaena ppbaena added the component/plugin-flux Issue related to kubeapps plugin to manage Helm charts via Flux label Oct 21, 2022
@ppbaena ppbaena added this to the Technical debt milestone Oct 21, 2022
@gfichtenholt gfichtenholt self-assigned this Oct 24, 2022
@ppbaena ppbaena added the next-iteration Issues to be discussed in planning session label Oct 24, 2022
@gfichtenholt
Copy link
Contributor Author

gfichtenholt commented Oct 24, 2022

I think it is totally acceptable to have a flag in the flux plugin configuration to let kubeapps know whether flux has been installed with cross-namespace references disabled.

What if instead of a introducing a new flag in flux plugin, I were to programmatically look at the deployment spec of flux helm-controller to see if -no-cross-namespace-refs=true is set in pod arguments? Would that be crossing a line?

@gfichtenholt
Copy link
Contributor Author

I am going to add a flag to flux plugin for this. The solution I mentioned above is still better, but I need to figure out what to do when flux is installed in, say another namespace, so I'll just put it on the back-burner for now.

BTW, I think that when the flag is set a flux repo can indeed be considered namespace-scoped. At the moment, GetNamespacedScoped() for flux repos always returns false. So I will also make a change that returns true when the flag is set.

gfichtenholt added a commit that referenced this issue Nov 5, 2022
…X, stays around in k8s flux HelmRelease CR #5577  (#5584)

fixed an inconsistency between GetInstalledPackageSummaries() and
GetInstalledPackageDetail() in one corner case.
Main fix is dependent on flux
fluxcd/helm-controller#554

There is only one small change to production code. The rest is
test-related code. Also,

+ added a few integration tests.
+ bump flux version in tests
+ fix for available package handling with flux in multi-tenant mode
#5541
@gfichtenholt
Copy link
Contributor Author

gfichtenholt commented Nov 18, 2022

re-opening to make a change I proposed earlier. @dlaloue-vmware confirmed in slack to me that GetNamespacedScoped() should return true when -no-cross-namespace-refs=true flag is set

@gfichtenholt gfichtenholt reopened this Nov 18, 2022
@gfichtenholt
Copy link
Contributor Author

never mind, above comment. @dlaloue-vmware changed all flux repos to return namespaceScoped: true always in #5664 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/plugin-flux Issue related to kubeapps plugin to manage Helm charts via Flux kind/bug An issue that reports a defect in an existing feature
Projects
Archived in project
3 participants