[Regression] Allow package details to fetch from config.kubeappsCluster

[aanthonyrizzo]

Description:

The kubeapps dashboard no longer supports kubeappsCluster or kubeappsNamespace.

As of version 2.4.2 the kubeapps dashbord no longer respects the kubeappsCluster or kubeappsNamespace` configurations.
as of v2.4.2, dashboard components no longer read the config for the kubeappCluster:

kubeapps/dashboard/src/components/DeploymentForm/DeploymentForm.tsx

Lines 44 to 47 in a1f95a5

	const {
	packages: { isFetching: packagesIsFetching, selected: selectedPackage },
	apps,
	} = useSelector((state: IStoreState) => state);

v2.4.1 does:

kubeapps/dashboard/src/components/DeploymentForm/DeploymentForm.tsx

Lines 44 to 48 in b8702bf

	const {
	apps,
	config,
	packages: { isFetching: packagesIsFetching, selected },
	} = useSelector((state: IStoreState) => state);

Previously these values were taken into account when fetching the package details from the UI. This is no longer the case. GetAvailablePackageDetail will not use the kubeappsCluster value when fetching the details.

Related to #4563

Corresponding slack thread: https://kubernetes.slack.com/archives/C9D3TSUG4/p1649178273135249

Repro Instructions:

1. Ensure that kubeappsCluster is not included in clusters in the configmap, this will make sure that the subsequet GetAvailablePackageDetail request will fail.
2. Load kubeapps as normal and try to deply a helm chart
3. Be greeted with:
4. Verify it's fetching GetAvailablePackageDetail with a value in the clusters[] and not the kubeappsCluster. Ex: +core GetAvailablePackageDetail (cluster="default", namespace="kubeapps")

[absoludity]

Sorry @aanthonyrizzo , reading your issue description, and re-reading your comments in slack, I think I initially misunderstood the issue that you're seeing. Can you add some steps to reproduce to this issue? That'd help to clarify for me.

[aanthonyrizzo]

Sorry @aanthonyrizzo , reading your issue description, and re-reading your comments in slack, I think I initially misunderstood the issue that you're seeing. Can you add some steps to reproduce to this issue? That'd help to clarify for me.

Updated and edited to better articulate the issue. LMK if there is a something wrong.

[absoludity]

OK, so we were talking about the same problem (great), but I think the solution may be different since I don't think it involves the config. See what you think, but I believe the issue was introduced (by me) while getting flux deployments working at this point:

https://github.com/vmware-tanzu/kubeapps/blob/v2.4.4/dashboard/src/shared/url.ts#L20-L23

which was part of #3640 , where I've explicitly updated the dashboard to include the package cluster in the route (fine) and if it's not set, to default to the current cluster (default in your case). That change was done with the incorrect assumption that the only time an AvailablePackageReference will be received from the kubeapps apis backend with an empty cluster is if the plugin doesn't support multi-cluster. But that is false since it will also be empty if the cluster on which Kubeapps is installed is not one of the configured clusters for users to target.

The flux plugin has already been updated so that it always includes the kubeapps cluster in the response, so it should be as simple as removing the workaround in the dashboard above (I hope).

[aanthonyrizzo]

@absoludity yeah that looks like the offending commit/PR. So in my case with the current routing the path would be:

/c/default/ns/namespace-name/packages/helm.packages/v1alpha1//kubeapps/coreweave%2Fargo-cd

and the // will break the routing sine '' is tje kubeapps cluster. Do we want to add the isGlobal logic back and revert the URL path? or we want to just handle '' as gloabl in the route as the packageCluster and keep the current routing? The route would look like:

/c/default/ns/namespace-name/packages/helm.packages/v1alpha1/global/kubeapps/coreweave%2Fargo-cd

[absoludity]

@absoludity yeah that looks like the offending commit/PR. So in my case with the current routing the path would be:

/c/default/ns/namespace-name/packages/helm.packages/v1alpha1//kubeapps/coreweave%2Fargo-cd

and the // will break the routing sine '' is tje kubeapps cluster.

A couple of thoughts here:

1. All plugins now always return the kubeapps cluster for an available package, so we can (and should) now assume that if "" is returned, it's specifically the case here that we're dealing with (ie. the plugin is unable to specify a cluster name since the cluster in context is specifically not named in the config)
2. This situation only affects the Helm plugin, since flux doesn't (yet) try to support multi-cluster) and carvel has actual cluster resources for available packages (that need to be on the cluster where the package will be installed, in our usage).
3. We could therefore make this more explicit by having the helm plugin return "-" as the cluster name in this case, so the URL would look like /c/default/ns/namespace-name/packages/helm.packages/v1alpha1/-/kubeapps/coreweave%2Fargo-cd
4. This would (I think) ensure that the UI doesn't need to care at all - it's just a reference for the available package returned by the API.

Does that make sense?

Do we want to add the isGlobal logic back and revert the URL path?

Nope, not keen to re-introduce that.

or we want to just handle '' as gloabl in the route as the packageCluster and keep the current routing? The route would look like:

/c/default/ns/namespace-name/packages/helm.packages/v1alpha1/global/kubeapps/coreweave%2Fargo-cd

This would conflict if people (for reasons) configure a cluster called "global".

So I think we may be able to just update the helm plugin to return - for the cluster name when it needs to refer to the cluster on which Kubeapps is installed but that cluster is not named in the config. Sorry, rather than explaining exactly what I meant there, I've done a diff for discussion at #4591 . Let me know what you think.