GetInstalledPackageSummaries in fluxv2 plug-in #3228

gfichtenholt · 2021-07-31T07:00:47Z

Adds GetInstalledPackageSummaries() functionality to fluxv2 plug-in. Unit tests included

absoludity

Hi Greg.

I started going through this, but there's something fundamental that I'm not understanding which probably needs clarifying before I keep going: This is servicing a request for info about the installed packages (HelmRelease CRs). Now with Flux , AIUI, when a user installs a package (ie. creates a HelmRelease), flux creates the related HelmChart CR. So when we're servicing this request, we're returning info about the HelmReleases with possibly some extra info from the related HelmChart. But a lot of the code here seems to be catering for a scenario where the related HelmChart doesn't exist and creating it (temporarily), and I don't yet understand why we'd want to do that?

Thanks for any info.

absoludity · 2021-08-09T04:41:27Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/chart.go

+	// see if we the chart already exists
+	// TODO (gfichtenholt):
+	// see https://github.com/kubeapps/kubeapps/pull/2915
+	// for context. It'd be better if we could filter on server-side. The problem is the set of supported
+	// fields in FieldSelector is very small. things like "spec.chart" or "status.artifact.revision" are
+	// certainly not supported.
+	// see
+	//  - kubernetes/client-go#713 and
+	//  - https://github.com/flant/shell-operator/blob/8fa3c3b8cfeb1ddb37b070b7a871561fdffe788b///HOOKS.md#fieldselector and
+	//  - https://github.com/kubernetes/kubernetes/issues/53459


This is a duplicate of same detailed comment that was already mentioned above? Fine either way, just unsure why it's detailed twice in the same file.

absoludity · 2021-08-09T05:01:23Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/chart.go

+	// TODO (gfichtenholt)
+	// 1. HelmChart object needs to be co-located in the same namespace as the HelmRepository it is referencing.
+	// 2. flux impersonates a "super" user when doing this (see fluxv2 plug-in specific notes at the end of
+	//	design doc). We should probably be doing simething similar to avoid RBAC-related problems


FWIW, I don't think we'll ever include a cluster-admin service account with Kubeapps or a plugin - that's just too dangerous, as you can see from the flux proposal (that hasn't been accepted yet) which you linked to in the doc, about how they're trying to get out of that situation.

Helm itself used to do something similar, having tiller running in the cluster with crazy privileges that could be used to do exactly what they describe in the flux proposal... I'm quite surprised that they have cluster-admin as the default for the helm controller TBH, given the history.
Anyway, fine as is, as long as we're not wanting to deploy a kubeapps component with super privs.
So back to here: The user carrying out the request will need to have RBAC for whatever is done - currently it looks like you're creating the flux HelmChart CR explicitly. If that is necessary (see previous comment above as I'm not sure why it is if the HelmRelease already exists) we can document that as a requirement.

+1. If you'd like me to word the comment a certain way, just say so.

I think the comment is fine as a note - word it however you like :) I just don't think option 2 is something we should do (even flux seems to be backing away from it).

absoludity · 2021-08-09T05:07:41Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/chart.go

+	// Delete the created helm chart regardless of success or failure. At the end of
+	// GetAvailablePackageDetail(), we've already collected the information we need,
+	// so why leave a flux chart chart object hanging around?
+	// Over time, they could accumulate to a very large number...


Yes. More generally it seems a bit icky to be creating them when they should exist if a HelmRelease already exists (and if not, I'm not sure we should be trying to provide the info). It seems analogous to the situation with normal helm, where if the user has deleted the AppRepository (ie. so kubeapps no longer has info about the related charts), we can't provide that info (such as upgrade info or latest version) and that's OK.

GetAvailablePackageDetail() requires me to provide some info not available in the repo index file, like the chart readme/schema/values. For a chart that may not be installed at all. I can only get that if I have the chart tgz file, so that's the only reason I am doing it. I am open to other proposals, please.

Yep, it makes sense for GetAvailablePackageDetail() - and given that's a singular request, the efficiency isn't an issue either. As above, I'd thought this PR was about GetInstalledPackageSummaries only as per the title, so when I saw these changes, assumed it must be for them.

absoludity · 2021-08-09T05:23:37Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/chart.go

+		return url, nil, nil
+	}
+
+	// did not find the chart, need to create


I don't understand why we need to be creating a chart here at all. We're wanting to return information about the installed packages (ie. the HelmReleases). From what I've understood, if a user has created a HelmRelease CR, then the helm-controller would have created the related HelmChart for that release, so it should exist. If it doesn't, because the user has manually deleted it or whatever, then the best we can do is return the info about the installed package that we have (ie. the info from the HelmRelease).

Or what reason do you see here for a HelmRelease without the corresponding HelmChart?

no, this is a different use case - GetAvailablePackageDetail()

Ah - ok. I assumed this PR was just about GetInstalledPackageSummaries . Great, I'll finish going through the PR tomorrow.

absoludity · 2021-08-09T05:33:09Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/chart.go

+		return "", err, nil
+	} else if url != "" {
+		return url, nil, nil
+	}


I'm probably missing something, but I think we should just be doing

return url, nil, nil

here, regardless. More below.

So ignore the above - this was because I thought the fn was part of your work for GetInstalledPackageSummaries - but you pointed out it's just refactoring for the existing GetAvailablePackage... fns.

gfichtenholt · 2021-08-09T06:42:41Z

Hi Greg.

I started going through this, but there's something fundamental that I'm not understanding which probably needs clarifying before I keep going: This is servicing a request for info about the installed packages (HelmRelease CRs). Now with Flux , AIUI, when a user installs a package (ie. creates a HelmRelease), flux creates the related HelmChart CR. So when we're servicing this request, we're returning info about the HelmReleases with possibly some extra info from the related HelmChart. But a lot of the code here seems to be catering for a scenario where the related HelmChart doesn't exist and creating it (temporarily), and I don't yet understand why we'd want to do that?

Thanks for any info.

Hi Michael,
I did move code around a bit. Looks like your comment is about lines 78 or so, in other words, getChartTarball() func. This isn't used in GetInstalledPackageSummaries() at all. Only in GetAvailablePackageDetail(). Otherwise, your understanding is correct.

absoludity

Great, thanks for the info (that lots of the changes in this PR are about GetAvailablePackageDetail, not GetInstalledPackageSummaries). I'll finish it tomorrow then :)

absoludity · 2021-08-09T11:05:16Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/chart.go

+		return url, nil, nil
+	}
+
+	// did not find the chart, need to create


Ah - ok. I assumed this PR was just about GetInstalledPackageSummaries . Great, I'll finish going through the PR tomorrow.

absoludity · 2021-08-09T11:07:21Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/chart.go

+	// TODO (gfichtenholt)
+	// 1. HelmChart object needs to be co-located in the same namespace as the HelmRepository it is referencing.
+	// 2. flux impersonates a "super" user when doing this (see fluxv2 plug-in specific notes at the end of
+	//	design doc). We should probably be doing simething similar to avoid RBAC-related problems


I think the comment is fine as a note - word it however you like :) I just don't think option 2 is something we should do (even flux seems to be backing away from it).

absoludity · 2021-08-09T11:09:10Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/chart.go

+	// Delete the created helm chart regardless of success or failure. At the end of
+	// GetAvailablePackageDetail(), we've already collected the information we need,
+	// so why leave a flux chart chart object hanging around?
+	// Over time, they could accumulate to a very large number...


Yep, it makes sense for GetAvailablePackageDetail() - and given that's a singular request, the efficiency isn't an issue either. As above, I'd thought this PR was about GetInstalledPackageSummaries only as per the title, so when I saw these changes, assumed it must be for them.

absoludity

Great, thanks Greg.

absoludity · 2021-08-09T23:56:12Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/chart.go

+		return "", err, nil
+	} else if url != "" {
+		return url, nil, nil
+	}


So ignore the above - this was because I thought the fn was part of your work for GetInstalledPackageSummaries - but you pointed out it's just refactoring for the existing GetAvailablePackage... fns.

gfichtenholt · 2021-08-10T01:27:10Z

Great, thanks Greg.

thank you

gfichtenholt added 8 commits July 28, 2021 21:54

step 1

b0becc6

Merge branch 'master' into further-fluxv2-plugin-features-3

d6be804

step 2

73cdd04

Merge branch 'master' into further-fluxv2-plugin-features-3

8c14399

step 4

dff3095

Merge branch 'master' into further-fluxv2-plugin-features-3

348643f

step 5

99f4c53

step 6

d39d10d

gfichtenholt self-assigned this Jul 31, 2021

gfichtenholt added this to In progress in Kubeapps via automation Jul 31, 2021

gfichtenholt linked an issue Jul 31, 2021 that may be closed by this pull request

Implement further fluxv2 plugin features #3173

Closed

step 7

f6ff25e

gfichtenholt marked this pull request as draft August 1, 2021 11:06

step 8

284bbbd

gfichtenholt marked this pull request as ready for review August 2, 2021 08:00

gfichtenholt requested a review from absoludity August 2, 2021 08:02

gfichtenholt added 4 commits August 2, 2021 03:35

Merge branch 'master' into further-fluxv2-plugin-features-3

81cc5da

step 9

87feb45

step 9b

862a288

step 10

7194f18

absoludity reviewed Aug 9, 2021

View reviewed changes

gfichtenholt added 2 commits August 9, 2021 16:40

Michael's feedback #1

38dd719

Merge branch 'master' into further-fluxv2-plugin-features-3

7ae5bd7

absoludity approved these changes Aug 10, 2021

View reviewed changes

gfichtenholt merged commit b0fdc48 into vmware-tanzu:master Aug 10, 2021

Kubeapps automation moved this from In progress to Done Aug 10, 2021

gfichtenholt deleted the further-fluxv2-plugin-features-3 branch August 10, 2021 03:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GetInstalledPackageSummaries in fluxv2 plug-in #3228

GetInstalledPackageSummaries in fluxv2 plug-in #3228

gfichtenholt commented Jul 31, 2021

absoludity left a comment

absoludity Aug 9, 2021

absoludity Aug 9, 2021

gfichtenholt Aug 9, 2021

absoludity Aug 9, 2021

absoludity Aug 9, 2021

gfichtenholt Aug 9, 2021 •

edited

Loading

absoludity Aug 9, 2021

absoludity Aug 9, 2021

gfichtenholt Aug 9, 2021

absoludity Aug 9, 2021

absoludity Aug 9, 2021

absoludity Aug 9, 2021

gfichtenholt commented Aug 9, 2021 •

edited

Loading

absoludity left a comment

absoludity Aug 9, 2021

absoludity Aug 9, 2021

absoludity Aug 9, 2021

absoludity left a comment

absoludity Aug 9, 2021

gfichtenholt commented Aug 10, 2021

GetInstalledPackageSummaries in fluxv2 plug-in #3228

GetInstalledPackageSummaries in fluxv2 plug-in #3228

Conversation

gfichtenholt commented Jul 31, 2021

absoludity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfichtenholt Aug 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfichtenholt commented Aug 9, 2021 • edited Loading

absoludity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

absoludity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfichtenholt commented Aug 10, 2021

gfichtenholt Aug 9, 2021 •

edited

Loading

gfichtenholt commented Aug 9, 2021 •

edited

Loading