Cannot view newly created flux 2 package from UX

[absoludity]

Description:

Kubeapps UX fails to display a newly installed flux2 package

Steps to reproduce the issue:

1. Configure Kubeapps with the flux2 plugin
2. Add a HelmRepository manually via kubectl (I used bitnami's repo)
3. Create a service account with admin role in your chosen namespace
4. Deploy apache (or other chart) from the catalog

Expected result:

Created package can be viewed

Actual result:

The UI displays an error returned from the plugin:

Application not found: Unable to get the resource refs for the package "test-apache" using the plugin "fluxv2.packages": rpc error: code = NotFound desc = Unable to find Helm release "test-apache" in namespace "kubeapps-user-namespace": release: not found

Additional information you deem important (e.g. issue happens only occasionally):

The issue appears to be that the plugin code currently assumes that the flux HelmRelease and the actual helm release object have the same name, but they do not:

$ k -n kubeapps-user-namespace get helmrelease
NAME          READY   STATUS                             AGE
test-apache   True    Release reconciliation succeeded   104s

$ helm -n kubeapps-user-namespace ls  
NAME                                    NAMESPACE               REVISION        UPDATED                                 STATUS          CHART           APP VERSION
kubeapps-user-namespace-test-apache     kubeapps-user-namespace 1               2022-01-27 03:11:42.567993337 +0000 UTC deployed        apache-9.0.1    2.4.52     

The reason for the different names is due to the defaults that flux uses depending whether the targetNamespace is set on the release as outlined on #4172 .

[gfichtenholt]

reproduced the problem in the UX

.
However, with the command line grpcurl there is no problem:
$ grpcurl -plaintext -d '{"installed_package_ref": {"context": {"cluster": "default", "namespace": "default"}, "plugin": {"name": "fluxv2.packages", "version": "v1alpha1"}, "identifier": "my-podinfo"}}' -H "Authorization: $token" localhost:8080 kubeappsapis.core.packages.v1alpha1.PackagesService.GetInstalledPackageDetail { "installedPackageDetail": { "installedPackageRef": { "context": { "cluster": "default", "namespace": "default" }, "identifier": "my-podinfo", "plugin": { "name": "fluxv2.packages", "version": "v1alpha1" } }, "pkgVersionReference": { "version": "6.0.3" }, "name": "my-podinfo", "currentVersion": { "pkgVersion": "6.0.3", "appVersion": "6.0.3"
So, need to see what UX is doing behind the scenes...

[gfichtenholt]

ah, its the GetInstalledPackageResourceRefs API that is failing:

$  grpcurl -plaintext -d '{"installed_package_ref": {"context": {"cluster": "default", "namespace": "default"}, "plugin": {"name": "fluxv2.packages", "version": "v1alpha1"}, "identifier": "my-podinfo"}}' -H "Authorization: $token" localhost:8080 kubeappsapis.core.packages.v1alpha1.PackagesService.GetInstalledPackageResourceRefs
ERROR:
  Code: NotFound
  Message: Unable to get the resource refs for the package "my-podinfo" using the plugin "fluxv2.packages": rpc error: code = NotFound desc = Unable to find Helm release "my-podinfo" in namespace "default": release: not found
helm.sh/helm/v3/pkg/storage/driver.init
	/go/pkg/mod/helm.sh/helm/v3@v3.7.2/pkg/storage/driver/driver.go:29
runtime.doInit
	/opt/bitnami/go/src/runtime/proc.go:6498
runtime.doInit
...

[gfichtenholt]

Ah, I think I got to the bottom of it. A bit of history first: Prior to GetInstalledPackageResourceRefs(), which was added fairly recently, I had another API call GetInstalledPackageDetail() where I also needed to fetch some data for an installed release from helm API (like post installation nodes, etc.). To do this, I wrote the following logic:
https://github.com/kubeapps/kubeapps/blob/ba47105db908d1fd3974e94343d294f7b6e58ed7/cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/release.go#L338

(I am linking to the file in my most recent PR, where I switched to using native Flux types, but you can find same code in main too)

helmReleaseName := rel.Spec.ReleaseName
	// according to docs ReleaseName is optional and defaults to a composition of
	// '[TargetNamespace-]Name'.
	if helmReleaseName == "" {
		targetNamespace := rel.Spec.TargetNamespace
		// according to docs targetNamespace is optional and defaults to the namespace of the HelmRelease
		if targetNamespace == "" {
			targetNamespace = key.Namespace
		}
		helmReleaseName = fmt.Sprintf("%s-%s", targetNamespace, key.Name)
	}

This logic was coded by looking at flux public docs
https://fluxcd.io/docs/components/helm/helmreleases/ see description for ReleaseName field
So this is the key. This is what results in a string like "default-my-podinfo", when used in the context of GetInstalledPackageDetail(), which works fine. GetInstalledPackageResourceRefs() does not use this logic, so it ends up using a string "my-podinfo" as an argument to helm API and that fails. My proposal would be not to change the code to not add TargetNamespace or add ReleaseName of the Flux CR. I don't feel that's required to solve this or that it would even always work (see last sentence below).

Rather, how about we somehow make the GetInstalledPackageResourceRefs() use the same logic as GetInstalledPackageDetail() to come up with the name of the helm release? I feel like this would be a good solution because it would compute the helm release name consistently regardless of the code path.
Now, having said that, there is a little hiccup, in that logic in GetInstalledPackageResourceRefs() is in pkg (i.e. shared code my multiple plugins) whereas logic for GetInstalledPackageDetail() is entirely in flux plugin. But if we agree that is the right direction, I can come up with some way to make GetInstalledPackageResourceRefs() pull the right name (either passing it as an argument or via some callback to plugin to get it, etc - implementation details). @absoludity what do you think?
If you don't like this, then probably forcing an explicit ReleaseName on the CR would be my 2nd (distant) vote. Again, keep in mind we can only control those CRs created by us. Nothing stops the user from doing 'kubectl apply' or 'flux create helmrelese' that completely disregards our convention and has a potential to break our code.

[hongkunyoo]

I have temporarily solved this problem by just deleting targetNamespace in here (https://github.com/kubeapps/kubeapps/blob/1d5f92aa7386282500c36bd4c9ded07079a77936/cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/release.go#L523)

I understand the concern that @gfichtenholt mentioned:

My proposal would be not to change the code to not add TargetNamespace or add ReleaseName of the Flux CR.

However, we are using HelmRelease natively and currently planning to use kubeapps as well. And we do not use targetNamespace in our native case. So maybe in our case, just deleting targetNamespace might be the best.
I might be wrong since I do not know the details of kubeapps-apis, however please consider our use cases when deciding changes.

We really want to use both(using HelmRelease natively, by kubectl apply -f and using Web UI, kubapps as well).

By the way, kubeapps seems a really awesome project that we exactly needed!

[gfichtenholt]

Thank you for your feedback.

I have temporarily solved this problem by just deleting targetNamespace in here (

https://github.com/kubeapps/kubeapps/blob/1d5f92aa7386282500c36bd4c9ded07079a77936/cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/release.go#L523
)

Would love to understand why you had to do that. As far as I can tell, setting targetNamespace or not setting it are both perfectly valid use cases for flux. One user might prefer the field set (i.e. what kubeapps is doing today). Another user (you?) might prefer the field NOT set. Difficult to accommodate both :-). Regardless, whether or not to set the targetNamespace when kubeapps is creating a CR may be argued is a separate issue from the one that causes the error in this case. You may decide to file an issue asking not to set the targetNamespace on CRs that kubeapps creates, state your reasoning, etc. but that would be a separate issue we can discuss

I understand the concern that @gfichtenholt mentioned:

My proposal would be not to change the code to not add TargetNamespace or add ReleaseName of the Flux CR.

However, we are using HelmRelease natively and currently planning to use kubeapps as well. And we do not use targetNamespace in our native case.

If you examine the code snippet for GetInstalledPackageDetail I pasted earlier, you will see that it handles both cases (when targetNamespace is set and when it isn't). So it should work in your usage when you are not setting targetNamespace on the CRs you create outside kubeapps. All I am advocating for this particular issue to use the same pattern for GetInstalledPackageResourceRefs() to be consistent. It will solve the issue and I believe will handle your use case.

So maybe in our case, just deleting targetNamespace might be the best.

I am not convinced it is yet.

I might be wrong since I do not know the details of kubeapps-apis, however please consider our use cases when deciding changes.

We really want to use both(using HelmRelease natively, by kubectl apply -f and using Web IU, kubapps as well).

By the way, kubeapps seems a really awesome project that we exactly needed!

Would love to consider your use cases, if you can point me to them. Thanks

[absoludity]

reproduced the problem in the UX

cough, yes, the first thing I plan to fix is that error - updating to use Rafa's component which cleans the error to present something useful to the UX (something we've wanted to do since updating to use the apis server).

My proposal would be not to change the code to not add TargetNamespace or add ReleaseName of the Flux CR. I don't feel that's required to solve this or that it would even always work (see last sentence below).

To help me understand, can you explain why you're keen to explicitly set the TargetNamespace (I think you said previously because you prefer to be explicit and avoid defaults) while you're also keen not to set the ReleaseName, to me the same argument would apply... but either way, I'd rather we focus on the user perspective... (more below).

Rather, how about we somehow make the GetInstalledPackageResourceRefs() use the same logic as GetInstalledPackageDetail() to come up with the name of the helm release? I feel like this would be a good solution because it would compute the helm release name consistently regardless of the code path.

Yes, we could explicitly compute the default value to match what flux does... and as you say, since Kubeapps is just a frontend and people could create the HelmRelease via kubectl, we still want to be able to do this (if the ReleaseName is blank), so +1 to ensuring we can do that when ReleaseName is blank.

Now, having said that, there is a little hiccup, in that logic in GetInstalledPackageResourceRefs() is in pkg (i.e. shared code my multiple plugins) whereas logic for GetInstalledPackageDetail() is entirely in flux plugin. But if we agree that is the right direction, I can come up with some way to make GetInstalledPackageResourceRefs() pull the right name (either passing it as an argument or via some callback to plugin to get it, etc - implementation details). @absoludity what do you think?

Yep, I'm sure you'll find a sane way there.

If you don't like this, then probably forcing an explicit ReleaseName on the CR would be my 2nd (distant) vote.

This is my second preference too, if you're not keen to remove the TargetNamespace explicit default (even though we're not using target namespaces). Let me explain why...

Again, keep in mind we can only control those CRs created by us.

We want to do the best we can to interact with any HelmReleases, but your existing code that you pointed to first checks if ReleaseName is set before computing the default, which should cover that for us - great.

Nothing stops the user from doing 'kubectl apply' or 'flux create helmrelese' that completely disregards our convention and has a potential to break our code.

Right, I agree - we don't want to encode any conventions that make assumptions we can avoid (like just pretending TargetNamespace doesn't exist because Kubeapps doesn't use it). I am all for the flux plugin handling the calculation of the helm release name if the target namespace is set and the ReleaseName is not. I would also be +1 for setting the TargetNamespace when we (later) allow the user to specify it in the UX. I'm just not sure why we want to explicitly set it now when the user is not explicitly setting it (and doing so has an unexpected result, IMHO). Setting the ReleaseName would at least avoid the unexpected result.

That is, the problem from my POV is that, because we are explicitly setting the TargetNamespace, this changes flux's behaviour for computing the default release name - as if setting the target namespace is taken by Flux to mean you are using target namespaces different to the helm release (which Kubeapps is not). When it is not set, the release name is the same as the HelmRelease.metadata.name as you would expect, but when the TargetNamespace is set the helm release name is (unexpectedly for users who aren't even using target namespace). See my previous #4172 for examples.

So, for example, I think @hongkunyoo's main use-case that has been mentioned is that they are wanting to interact with HelmReleases via both kubeapps and kubectl. Pushing that even further, if I as a user, want to also use the helm cli, when I create a helm release in Kubeapps called my-release in a namespace called mynamespace, I am personally expecting to find a HelmRelease CR with that name (good) as well as a helm release (ie. helm ls) with the same name... as is the default behaviour for flux if you don't set TargetNamespace. Whether that's by not explicitly setting the TargetNamespace unless it is (eventually) explicitly requested in the request, or by explicitly setting the ReleaseName is less important to me (though setting the ReleaseName does have implications too, which is why I prefer the former).

So in summary, I'm keen to do what Flux does by default... if a user creates a HelmRelease without explicitly specifying TargetNamespace, then the helm release name is what would be expected (the same as HelmRelease.metadata.name). If a user creates a HelmRelease with an explicit TargetNamespace and doesn't set the ReleaseName, then we need to calculate the release name the same way flux does as you suggest.

Anyway, see what you think Greg. Up to you to decide, I just want to be sure we think what a user would expect when creating an app in Kubeapps. I'll approve either way, unless others have other thoughts?

[gfichtenholt]

So in summary, I'm keen to do what Flux does by default...

by that you mean Flux CLI?

[gfichtenholt]

Here is what I think:
0) I actually don't feel that strongly about removing the targetNamespace, in principle. I feel strongly that removing targetNamespace as a fix for this issue is not sufficient

1. regardless of whether or not we decide to set the targetNamespace when creating a CR, we MUST add the logic I described earlier GetInstalledPackageResourceRefs(). If we don't, there are legit use cases (where the user is creating a CR however they like it) that will break us.
2. Once we add this logic to GetInstalledPackageResourceRefs(), this issue goes away
3. At that point we may start asking, 'hey, why do we need to set the target namespace in the first place'? And I quite possibly, I may agree, we don't really need to. See (0). But we can only get to (3) after (1-2)

[gfichtenholt]

To answer 'can you explain why you're keen to explicitly set the TargetNamespace (I think you said previously because you prefer to be explicit and avoid defaults) while you're also keen not to set the ReleaseName'.

At the time I was coding this part of the plug-in, there were 3 different and potentially distinct namespaces that I had to deal with (discussion in #3640 (comment)). The issue was "in my face" so to speak, I had to make a choice to in order to move on. We had a discussion, I interpreted the outcome of this discussion in one way (turns out possibly mis-interpreted), coded it up and moved on. With ReleaseName I didn't have to, so it just didn't get on my radar at the time

[hongkunyoo]

I think I made it little complicated by suggesting some incorrect solution.
I do not insist to remove targetNamespace. I just wanted to eliminate the error and thought it would work.
I believe that the author of the code will know the best solution.

On the other hand, as @absoludity mentioned, I would like to have same helm release name as HelmRelease CR name by default unless I explicitly configure it. and as you said this is another issue to discuss. Just telling you my personal preference.

[gfichtenholt]

thank you, understood. I suggest we fix this issue the way I suggested above. In addition, I will file a new issue 'do not set targetNamespace, but set the releaseName field when creating Flux HelmRelease CR per customer request or default Flux CLI behavior' or something similar and assign to myself.

[absoludity]

Here is what I think: 0) I actually don't feel that strongly about removing the targetNamespace, in principle. I feel strongly that removing targetNamespace as a fix for this issue is not sufficient

1. regardless of whether or not we decide to set the targetNamespace when creating a CR, we MUST add the logic I described earlier `GetInstalledPackageResourceRefs()`. If we don't, there are legit use cases (where the user is creating a CR however they like it) that will break us.

2. Once we add this logic to `GetInstalledPackageResourceRefs()`, this issue goes away

3. At that point we may start asking, 'hey, why do we need to set the target namespace in the first place'? And I quite possibly, I may agree, we don't really need to. See (0). But we can only get to (3) after (1-2)

Yes - agree 100% with everything here.

[gfichtenholt]

Ah, whew! (wipes cold sweat off his forehead). Thank you! Will proceed as stated.

JUst FYI: checked what Flux CLI does by default:

$ flux create source helm podinfo \
>     --url https://stefanprodan.github.io/podinfo \
>     --namespace default
✚ generating HelmRepository source
► applying HelmRepository source
✔ source created
◎ waiting for HelmRepository source reconciliation
✔ HelmRepository source reconciliation completed
✔ fetched revision: 83a3c595163a6ff0333e0154c790383b5be441b9db632cb36da11db1c4ece111
$ flux create hr my-podinfo \
>     --namespace=default \
>     --source=HelmRepository/podinfo.default \
>     --chart=podinfo
✚ generating HelmRelease
► applying HelmRelease
✔ HelmRelease created
◎ waiting for HelmRelease reconciliation
✔ HelmRelease my-podinfo is ready
✔ applied revision 6.0.3
$ kubectl get helmrelease/my-podinfo -o yaml
apiVersion: helm.toolkit.fluxcd.io/v2beta1
kind: HelmRelease
metadata:
creationTimestamp: "2022-01-31T07:50:56Z"
finalizers:
- finalizers.fluxcd.io
generation: 1
name: my-podinfo
namespace: default
resourceVersion: "2363"
uid: 24731219-6d27-422b-8f9a-2f71da306882
spec:
chart:
  spec:
    chart: podinfo
    reconcileStrategy: ChartVersion
    sourceRef:
      kind: HelmRepository
      name: podinfo
      namespace: default
    version: '*'
interval: 1m0s
status:
conditions:
- lastTransitionTime: "2022-01-31T07:50:57Z"
  message: Release reconciliation succeeded
  reason: ReconciliationSucceeded
  status: "True"
  type: Ready
- lastTransitionTime: "2022-01-31T07:50:57Z"
  message: Helm install succeeded
  reason: InstallSucceeded
  status: "True"
  type: Released
helmChart: default/default-my-podinfo
lastAppliedRevision: 6.0.3
lastAttemptedRevision: 6.0.3
lastAttemptedValuesChecksum: da39a3ee5e6b4b0d3255bfef95601890afd80709
lastReleaseRevision: 1
observedGeneration: 1

So:

1. TargetNamespace field is NOT set.
2. neither is ReleaseName field

This doesn't mean we have to do exactly the same thing. But that discussion will go into a new issue

[absoludity]

Ah, whew! (wipes cold sweat off his forehead). Thank you! Will proceed as stated.

Thanks Greg. Let us know if this is a small change that you'll be able to do soon. We'll plan to release with this fixed (though the UX isn't finished for flux, it'll unblock people trying it out).

[gfichtenholt]

ETA 1 day. Working out all unit test issues

[absoludity]

Great, thanks Greg. No stress if it takes a bit longer, just wanted to be sure you knew.

[gfichtenholt]

Great, thanks Greg. No stress if it takes a bit longer, just wanted to be sure you knew.

yes, I knew. Working on unit/integration tests as usual... production code = 25% of the work, tests = 75%