Fix for #3636: Open Charts themselves may need caching in fluxv2 plugin #3899

gfichtenholt · 2021-12-04T06:34:20Z

This PR adds the functionality to cache the contents of helm chart tgz files so that GetAvailablePackageDetail() API completes faster w.r.t. a chart (package) that has already been cached. A few disclaimers:

Sorry, it took a while. I spent several days on existing unit and integration tests to make sure all are passing.
This is another fairly large set of changes, so apologies in advance for having to review that. Take your time, I have a number of TODO items that I will be fixing in the meantime.
The main mechanism the chart cache works is similar to that of the repo cache. There is a work queue that serves as a conduit between a producer and a consumer. The items on this queue are keys like "helmcharts:ns:repo/chart:version". The producer adds items to the work queue. This happens when a new/modified repo is detected or when a repo gets deleted. The way that it happens is that chart cache wraps various onAdd/onModify/onDelete methods from the repo to allow for the producer to add items to the work queue. See https://github.com/kubeapps/kubeapps/pull/3899/files#diff-9bcad052d0cb0a9d47dc70eca649ff8c9fc799ef25c433f47c9a2ca30a736a11. The consumer is the worker goroutine running in the background pulling items off the work queue. For each item the corresponding tar file will be fetched and its bytes added to the cache. Unlike the repo cache, I don't have a "store" in which to check the current state of the object when the consumer gets around to processing an item. So I used an instance of https://pkg.go.dev/k8s.io/client-go/tools/cache#ThreadSafeStore (which is basically a glorified map) to keep some state (such as the chart tgz url) just in the time window between when the producer adds a work item to the queue and when the consumer pulls it out of the queue.
I am happy that with PR I got rid of the old way available chart details were retrieved, which was by creating a flux HelmChart CRD and waiting for it to fully reconcile and deleting it afterwards - a lot of work, therefore slow and the whole approach was kind of iffy for a while. Dimitri pointed out that HelmCharts CRD may not necessarily be meant to used in that way. I still do use HelmCharts for some information on the installed package but only the ones created by flux itself, via HelmRelease CRD.

I will be happy to go over any questions here or over zoom, etc. Thanks

gfichtenholt · 2021-12-05T08:27:27Z

one update: there maybe a cleaner/better way to bootstrap chart cache than this wrap() business. I have just discovered that one can do things like

    			repoCacheConfig = cache.NamespacedResourceWatcherCacheConfig{
				...
				OnAddFunc:    s.onAddRepo,
                                ...

instead of

    			repoCacheConfig = cache.NamespacedResourceWatcherCacheConfig{
				...
				OnAddFunc:    onAddRepo,
                                ...

which looks like a bit of a game changer. Wow. I didn't realize I can pass a receiver instance as a callback argument. I am looking to see if that can simplify things a bit

gfichtenholt · 2021-12-06T08:37:16Z

let me convert it to a draft for now. I am still making some fairly substantial changes...

…xy config

gfichtenholt · 2021-12-09T06:03:22Z

This is ready for review now. Full disclosure: there are a few minor TODOs I am still working on in parallel, but all the major parts are done, so no big structural changes are expected. Just a few shortcuts I took in order to get to this point faster, which I will come back to now and clean up. I am sorry about the volume of code - it just evolved that way. I was solving one issue after another (and I am not done with all of them yet), and the changes just keep on creeping in. Some of the reason for the volume of code has to do with the fact that where previously I could count on flux to do the hard work to download things, I now have to do in my code, which means dealing with various authentication scenarios, HTTP failures, etc etc. Take your time reviewing it. I will be happy to address any questions. Thanks

…t should surface some RBAC limitations

gfichtenholt · 2021-12-09T10:35:24Z

@absoludity ,

just to confirm from our last zoom discussion, something like this

E1209 10:09:30.822480       1 watcher_cache.go:477] Operation XYZ for object {
  "apiVersion": "source.toolkit.fluxcd.io/v1beta1",
  "kind": "HelmRepository",
  ...
}
failed due to: secrets "podinfo-basic-auth-secret" is forbidden: User "system:serviceaccount:kubeapps:kubeapps-internal-kubeappsapis" cannot get resource "secrets" in API group "" in the namespace "default"

is what we WANT to happen in production for private (secure) flux helm repos due to the fact
my code cannot access the secret associated with repository due to default RBAC settings. Please confirm

this may be somewhat academic question, but supposing the answer to (1) is yes, and suppose that during processing/indexing of the helm repo I find out that I can't read the corresponding secret due to RBAC settings. Do I:
a) skip the work of trying to cache latest chart versions because I assume the chart tarball downloads will fail OR
b) try and perform the corresponding chart downloads without any authentication/authorization?
FWIW, option (a) could save a LOT of cycles due to not having possibly 100s of work items each of which may have to go through several retries and still fail
Another way of asking this is: is it theoretically possible for a private repo to have links to charts that are public? If yes, do we need to handle such scenarios?
Finally, what about our local dev environment: should I make it so that this works (meaning allow kubeappsapis account access to secrets everywhere) or not? Whatever the answer is I will need to write a test for it.
Thanks
Greg

absoludity · 2021-12-10T00:30:59Z

@absoludity ,
1. just to confirm from our last zoom discussion, something like this
E1209 10:09:30.822480       1 watcher_cache.go:477] Operation XYZ for object {
  "apiVersion": "source.toolkit.fluxcd.io/v1beta1",
  "kind": "HelmRepository",
  ...
}
failed due to: secrets "podinfo-basic-auth-secret" is forbidden: User "system:serviceaccount:kubeapps:kubeapps-internal-kubeappsapis" cannot get resource "secrets" in API group "" in the namespace "default"
is what we WANT to happen in production for private (secure) flux helm repos due to the fact my code cannot access the secret associated with repository due to default RBAC settings. Please confirm

Yes, when the person installing Kubeapps has not specifically updated the RBAC of the service account, that is correct. By default, I don't think we should ever be running with a service account that can read secrets in arbitrary namespaces. If someone is installing Kubeapps and wants kubeapps to be able to serve a private helm repository, they will need explicitly give the service account access to read secrets in that namespace.

2. this may be somewhat academic question, but supposing the answer to (1) is yes, and suppose that during processing/indexing of the helm repo I find out that I can't read the corresponding secret due to RBAC settings. Do I:
   a) skip the work of trying to cache latest chart versions because I assume the chart tarball downloads will fail OR
   b) try and perform the corresponding chart downloads without any authentication/authorization?
   FWIW, option (a) could save a LOT of cycles due to not having possibly 100s of work items each of which may have to go through several retries and still fail
   Another way of asking this is: is it theoretically possible for a private repo to have links to charts that are public? If yes, do we need to handle such scenarios?

It is possible, but no, I don't think we should handle it. That is, if a repository is configured as private with a secret, and we are unable to read that secret, we should fail that task immediately with (in this case) a log message indicating the error exactly as you've done in the above output :)

3. Finally, what about our local dev environment: should I make it so that this works (meaning allow kubeappsapis account access to secrets everywhere) or not? Whatever the answer is I will need to write a test for it.

IMO we should have the same default and not be running with a super-user service account in dev either. AFAIUI, this should work fine for all public flux helm repos, and only (rightly) fail for a private flux helm repo if I've not given the service account the necessary RBAC.

gfichtenholt · 2021-12-10T01:30:40Z

awesome, it all makes sense. Thanks Michael

Next: transient HTTP failures for chart cache

absoludity · 2021-12-10T05:58:19Z

This is ready for review now. Full disclosure: there are a few minor TODOs I am still working on in parallel, but all the major parts are done, so no big structural changes are expected. Just a few shortcuts I took in order to get to this point faster, which I will come back to now and clean up. I am sorry about the volume of code - it just evolved that way. I was solving one issue after another (and I am not done with all of them yet), and the changes just keep on creeping in. Some of the reason for the volume of code has to do with the fact that where previously I could count on flux to do the hard work to download things, I now have to do in my code, which means dealing with various authentication scenarios, HTTP failures, etc etc. Take your time reviewing it. I will be happy to address any questions. Thanks

Heh - unfortunately time is something I don't have right now (about to go on PTO :P ). I did set some time aside this afternoon to look at this, but realised pretty quickly that I don't have the time that I need to be able to learn and review it properly. But not to worry, Antonio is back next week and will be happy to do so, I'm sure.

Thanks for the great explanation in the PR description above! One general question that I have (I think we chatted briefly about this on a call previously, sorry if you answered it already): how does this handle the case where we scale up the kubeapps-apis deployment to 2 or 3 or 4 pods? As in, it looks like each pod will then have its own controller and work queues here with its own shared informer... ah, but perhaps that's fine as long as the queue itself is stored in redis and accessed from there? I wasn't able to tell from the brief look here, nor dig deeper right now, but keen to understand what happens in that scenario (knowing you, it's probably already got an integration test proving that it works without issue :) )

RE the volume of code evolving that way in this PR: In other teams/companies in the past I've struggled a lot with the same thing, code evolving to large PRs, until someone convinced me of the benefit of small, targeted PRs and gave me strategies to avoid evolving large ones (such as tracer-bullet branches and prequel branches which I already mentioned). If you get a chance, please take a look at the two examples I've done recently to do a larger chunk of work in smaller, easily reviewable PRs. They're not the best examples, but:

Update dashboard to use new k8s resources endpoint of api server #3779 has 13 PRs associated with it, the last of which I finished earlier this week, closing the issue. Each PR is landable on its own and easy to review as the change is very targeted.
Replace remaining Dashboard calls to K8s API server #3896 is the one I'm currently working on (and will continue when back) and has currently 7 similarly targeted PRs but has at least 4-6 more to come.

It's a bit more work on the author to get the work in small chunks but useful for me as the author also, and of course also hugely helpful for the reviewer. Happy to chat more about it when back. See you then! o/

gfichtenholt · 2021-12-10T08:00:02Z

Thanks for the great explanation in the PR description above! One general question that I have (I think we chatted briefly about this on a call previously, sorry if you answered it already): how does this handle the case where we scale up the kubeapps-apis deployment to 2 or 3 or 4 pods? As in, it looks like each pod will then have its own controller and work queues here with its own shared informer... ah, but perhaps that's fine as long as the queue itself is stored in redis and accessed from there? I wasn't able to tell from the brief look here, nor dig deeper right now, but keen to understand what happens in that scenario (knowing you, it's probably already got an integration test proving that it works without issue :) )

Great question. One nit-picky correction, what you probably meant to say is "scale out" not "scale up". I think when we talk about increasing number of pods that is "horizontal" scaling or "scaling out". "Scaling up" refers to increasing resources on each pod, which isn't what you meant. Anyway, I think the question applies to both the repo cache as well as chart cache, so not specific to this PR. As of now, the work items in the queue (i.e. keys) are not stored in redis, but in local memory on each pod. When the items are processed by the background worker(s), the end result goes to redis, which will then be visible to add pods. So it is possible for 2 pods to simultaneously work on the same queue key, e.g. "helmrepositories:default:bitnami-1" or "helmcharts:default:bitnami-1/redis:14.4.0"? I don't see why not at the moment. Everything is triggered via k8s watchers. So your question might be re-phrased as "how do k8s watchers scale out" or "is it possible for multiple pods to be processing the same event"? Both pods should get a notification in their respective k8s watcher that the resource "bitnami-1/redis" has been added or changed, and as a result, both will end up processing the same set of charts. I can see this isn't ideal, so I am going to look further into this. I can also say this is the pattern used by lots of code out there, this thing you pointed out probably applies to them as well. Not a great answer, I know, but I will follow-up and post here once I have it

Update: I think I have the answer
https://github.com/kubeapps/kubeapps/blob/master/chart/kubeapps/values.yaml#L782
wink wink

I think this is not a great answer, but good enough answer for short term. I have ideas how to make things better, but it's safe to say that will be a separate PR. This PR has got enough in it already that I shouldn't add any more features. Let me know if you disagree

gfichtenholt · 2021-12-10T08:31:07Z

It's a bit more work on the author to get the work in small chunks but useful for me as the author also, and of course also hugely helpful for the reviewer. Happy to chat more about it when back. See you then! o/

Not entirely certain if you're asking me at this point to break up this specific PR into smaller ones otherwise you won't approve? I would like to offer an alternative - we set aside a hour for a zoom call and I will go through all the changes line by line, file by file. Should be enough.

… need it

gfichtenholt · 2022-01-10T05:03:22Z

Document is at
https://docs.google.com/presentation/d/1o2FXSUUgrfxHTD95EOVldXbigDG9loR9/edit?usp=sharing&ouid=112444106102121063639&rtpof=true&sd=true
Guys I am working on PowerPoint slide deck that should help us go through the review process. It occurred to me that In addition to attaching it here I could convert it to PDF and check it into our docs folder and/or add it to our Google drive design docs folder. Let me know

absoludity · 2022-01-10T05:45:43Z

+1 to pop it in the Google drive design docs folder. I usually just ensure such docs allow public/anon read and suggestions. Thanks Greg.

…t can be used by multiple plug-ins

antgamdia

As I said offline, thank you so much for this thorough work. It's a clear example of a 90/10 rule: the last 10% is the most agonizing part of the project and will take 90% of the time.

Let me share with you some general comments and doubts (besides the inline ones). Note I hadn't had gone through this code yet, so please excuse me if some comments are with regard to an existent piece of code. Feel totally free to take/discard them unless otherwise stated.

You're updating the apprepository-controller git submodule, aren't you? If so, please ensure it is using the same tag as the k8s release we currently support (Kubernetes 1.21) [you'll find this info in the CircleCI config, under the GKE_REGULAR_VERSION variable]
Think about an already cached repo, if the repo authors eventually sunset a given version and it gets removed from the index (case 1), what would happen then? What if a whole chart got removed (case 2)?
Let's suppose:
- (case 1): my-chart: v1, v2 --> then my-chart:v2
- (case 2): my-chart: v1, foo-chart: v1 --> then foo-chart: v1

Sorry for the delay in reviewing the PR, crazy months we had :S

LGTM, personally +1-ing, let's see what others also think

antgamdia · 2022-01-14T15:44:33Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/cache/rate_limiting_queue.go

 	"k8s.io/client-go/util/workqueue"
+	log "k8s.io/klog/v2"
 )

 // RateLimitingInterface is an interface that rate limits items being added to the queue.
 type RateLimitingInterface interface {


Should we rename this interface so that we clearly know it's extending the original one from the k8s code?
Also, perhaps it worth adding some comments explaining what is the difference between yours and the original one?

I thought I did, lines 13-15. I can add more comments if you like. Renaming the file doesn't seem important to me. That's what packages are for. One is

github.com/kubernetes/client-go/blob/master/util/workqueue/rate_limiting_queue.go vs

github.com/kubeapps/kubeapps/cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/cache/rate_limiting_queue.go

Isn't this (my comments at the top of the file and different package) enough to clearly separate my code from k8s utils?

BTW, There are plenty of examples where the name of the module is same, yet implemented by totally different code. One example:

sigs.k8s.io/yaml

github.com/ghodss/yaml

k8s.io/apimachinery/pkg/util/yaml

antgamdia · 2022-01-14T15:53:21Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/cache/rate_limiting_queue.go

+	name string
+
+	// just for debugging purposes
+	verbose bool


We might want to be able to configure it at the server level. I barely recall some Satya's work on unifying the log formats. However, it's ok as it is now.

I am not against doing that. My primary objective was to be able to be able to EASILY turn verbose output for a particular workqueue on and off. So I chose an approach to set an environment variable to control verbose output for a particular queue as I was debugging it. I am now past this point, and I will consider any reasonable proposal

antgamdia · 2022-01-14T15:56:48Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/cache/chart_cache.go

+	// max number of retries due to transient errors
+	maxChartCacheRetries = 5
+	// number of background workers to process work queue items
+	maxChartCacheWorkers = 2


Does increasing this number yield better performance/response time rates? If so, perhaps it is worths splitting it apart to a user-faced plugin configuration (as we already have for the helm plugin).
Not in this PR, of course, just pointing it out for the future.

I assume you're talking about maxChartCacheWorkers. Yes, it kind of does, lets say you have 50 charts to cache, having 2 workers to do it in parallel is a lot faster than having 1 worker do it sequentially. But, it also increases contention when getting things from a queue and writing to redis. Moving on, I am all for adding it to user-faced plugin configuration. Just file an issue or even just tell me what I need to do and I will put it on my TODO list. The other thing to keep in mind is, eventually, I will get to addressing "scale out" scenario mentioned by Michael above, each worker will probably be represented by a single pod, so configuring maxChartCacheWorkers will just be a matter of how many such pods you want, in other words, an entry in values.yaml

antgamdia · 2022-01-14T15:58:24Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/cache/chart_cache.go

+
+var (
+	// pretty much a constant, init pattern similar to that of asset-syncer
+	verboseChartCacheQueue = os.Getenv("DEBUG_CHART_CACHE_QUEUE") == "true"


Just wondering if this value should eventually be added to the chart (even if it is set to false; just out of completeness)

see my comment above. I am open to (almost) any solution including that one. I do have one "nice-to-have" constraint though. I would very much like to be able to turn verbose flag on/off ONLINE, i.e. while the corresponding pod is running without having to restart it to take effect. But if its not possible, not the end of the world, I can live without it.

antgamdia · 2022-01-14T16:00:07Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/cache/chart_cache.go

+	verboseChartCacheQueue = os.Getenv("DEBUG_CHART_CACHE_QUEUE") == "true"
+)
+
+type ChartCache struct {


Maybe rename it to ChartRedisCache in the case of. in the (distant) future, we might want to move to another k=v storage provider?

Eeeh, then I would have to rename NamespacedResourceWatcherCache to NamespacedResourceWatcherRedisCache as well, for consistency. I vote for "let's cross that bridge when we come to it". It is a very simple change, it won't be a big deal any time we choose to do it

antgamdia · 2022-01-14T16:48:19Z

cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server.go

 	appRepov1 "github.com/kubeapps/kubeapps/cmd/apprepository-controller/pkg/apis/apprepository/v1alpha1"
 	"github.com/kubeapps/kubeapps/cmd/assetsvc/pkg/utils"
 	"github.com/kubeapps/kubeapps/cmd/kubeapps-apis/core"
 	corev1 "github.com/kubeapps/kubeapps/cmd/kubeapps-apis/gen/core/packages/v1alpha1"
-	"github.com/kubeapps/kubeapps/cmd/kubeapps-apis/gen/plugins/helm/packages/v1alpha1"
 	helmv1 "github.com/kubeapps/kubeapps/cmd/kubeapps-apis/gen/plugins/helm/packages/v1alpha1"


I'd rather rename this dep, as it seems to be referring to the helm client itself; also, I think it's not v1 but v1alpha1 (I wish we had a stable version of everything haha)

sorry which line are you referring to?

I guess it was:
helmv1 "github.com/kubeapps/kubeapps/cmd/kubeapps-apis/gen/plugins/helm/packages/v1alpha1"

helmv1 seems to be referring to helm, rather than our plugin

eeh, probably best to take that one to @absoludity. I didn't change any of that

antgamdia · 2022-01-14T16:57:57Z

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

-		if pkgDetail, err = availablePackageDetailFromTarball(chartID, tarUrl); err != nil {
-			return nil, err
+	if repoName != "" && helmChartRef != "" && chartName != "" {
+		parts := strings.Split(helmChartRef, "/")


I guess so, but we should double-check (not now) helmChartRef does not contain any additional / as part of its name.

got it. Will add a check for this.

antgamdia · 2022-01-14T16:59:57Z

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

+//    per https://github.com/kubeapps/kubeapps/pull/3640#issuecomment-949315105
+// 3. spec.targetNamespace, where flux will install any artifacts from the release
+func (s *Server) newFluxHelmRelease(chart *models.Chart, targetName types.NamespacedName, versionRef *corev1.VersionReference, reconcile *corev1.ReconciliationOptions, values map[string]interface{}) (*unstructured.Unstructured, error) {
+	unstructuredRel := unstructured.Unstructured{


Not now, but perhaps we can use the typed struct instead of dealing with the unstructured data directly (as I'm doing in the kapp plugin).

yes, this has been on my TODO list ever since you mentioned it first a couple months ago). Just need to confirm with Pepe I have bandwidth to do this.

antgamdia · 2022-01-14T17:02:19Z

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

+	// which is what is used when the repo is being processed/indexed.
+	// I don't think it's necessarily a bad thing if the incoming user's RBAC
+	// settings are more permissive than that of the default RBAC for
+	// kubeapps-internal-kubeappsapis account. If we don't like that behavior,


But perhaps the @castelblanque 's work here could be relevant, pinging him just in case.

antgamdia · 2022-01-14T17:05:14Z

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

@@ -250,6 +327,141 @@ func indexOneRepo(unstructuredRepo map[string]interface{}) (charts []models.Char
 	return charts, nil
 }

+// onModifyRepo essentially tells the cache whether to and what to store for a given key
+func (s *repoEventSink) onModifyRepo(key string, unstructuredRepo map[string]interface{}, oldValue interface{}) (interface{}, bool, error) {


We had a way to manually force a resync in the AppRepo (via asset-syncer), I'd say it was by adding a resync: <new value> in the k8s CR.
I guess it still will be detected as a change by this eventSink, isn't it?

I am not removing any existing functionality from kubeapps AppRepo

let me point out that in general, flux is not kubeapps AppRepo, so it's not likely have a set of the same features. Taking a cursory look at their API I don't see a way to force a manual resync.

if you are referring to ResyncRequests uint json:"resyncRequests"
field in type AppRepositorySpec, we (me, Michael and Dimitri) actually had a chat about this field when discussing new RepositoriesService API. You were on vacation, I think. All present agreed it was a hack and did not merit a place in the new API. https://docs.google.com/document/d/1z03msZRGsRvcRaom-yrvEwIWcEfNy6fSA5Zg28gjYvA/edit#heading=h.yheooaol1kzd, page 28, AddPackageRepository section

having said that, if there is a genuine need to have a manual resync feature, we need to design it such that it works for all plugins. So far I haven't heard the need, but will be on the look out

Thanks for the clarification. Yep, I was referring to ResyncRequests field; I wasn't aware of the outcome of the meeting . I do agree it's was a hacky way to force a resync.
ATM, I don't have any strong opinion, though I feel the "sync" button, from a user's point of view, is important. Perhaps we could trigger a kind of "kubectl apply" command to still trigger an update event somehow.

Nevertheless, this discussion does not belong in this PR, but the AppRepo API refactor issue. Thanks for the context again!

gfichtenholt · 2022-01-14T19:28:39Z

You're updating the apprepository-controller git submodule, aren't you? If so, please ensure it is using the same tag as the k8s release we currently support (Kubernetes 1.21) [you'll find this info in the CircleCI config, under the GKE_REGULAR_VERSION variable]

No, I am not touching apprepository-controller. The scope of this PR is only to add a cache for charts in flux plug-in

gfichtenholt · 2022-01-14T20:18:30Z

2. Think about an already cached repo, if the repo authors eventually sunset a given version and it gets removed from the index (case 1), what would happen then? What if a whole chart got removed (case 2)?
   Let's suppose:
   
   * (case 1): `my-chart: v1, v2` --> then `my-chart:v2`
   * (case 2): `my-chart: v1, foo-chart: v1` --> then `foo-chart: v1`

in both cases there will be an event 'repo XYZ' was modified. In handling this event, I will call SyncCharts(repo XYZ). Currently I just go through the most recent versions for every chart and add that. I don't delete any charts prior to that. So in case 1: you'll end up with my-chart:v2, same as before the update. In case 2, you'll end up with my-chart: v1, foo-chart: v1. I recognize that I can do better, so that you'd end up with just foo-chart: v1 in case 2. IMHO its a low(er) priority issue, because I don't but I will have it handled in the next PR. I wrote it in my TODO list

…d by multiple plug-ins 2 unit tests are commented out, I have yet to figure out how to fix them also, some feedback from Antonio

antgamdia · 2022-01-17T12:18:29Z

No, I am not touching apprepository-controller. The scope of this PR is only to add a cache for charts in flux plug-in

That's what I thought, but it seems GitHub PR viewer does not agree:

gfichtenholt · 2022-01-17T17:17:49Z

No, I am not touching apprepository-controller. The scope of this PR is only to add a cache for charts in flux plug-in

That's what I thought, but it seems GitHub PR viewer does not agree:

Have no clue what that is, I didn't do it (at least on purpose). Its very strange. These files (307 of them apparently), didn't even count in the total count. I Probably should have manually deleted this from PR but forgot to

gfichtenholt added 2 commits December 3, 2021 21:27

step 1

006c3e4

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

0039063

gfichtenholt self-assigned this Dec 4, 2021

gfichtenholt added this to In progress in Kubeapps via automation Dec 4, 2021

gfichtenholt linked an issue Dec 4, 2021 that may be closed by this pull request

Charts themselves may need caching in fluxv2 plugin #3636

Closed

gfichtenholt requested a review from absoludity December 4, 2021 07:00

step 2

6b4b8a2

gfichtenholt marked this pull request as draft December 6, 2021 08:36

gfichtenholt added 7 commits December 6, 2021 23:11

step 3: add basic auth support to chart cache. Next: TLS and http pro…

ff02690

…xy config

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

fd78fc4

step 3a: clean up bootstap of cache

1a7272f

step 3b. Use helm HttpGetter for downloading charts like flux does

7a03d5e

step 3c. get rid of HashSet in favor of sets.String

15d50a1

step 4: added TLS support in chart cache

6feaf9e

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

7431610

gfichtenholt marked this pull request as ready for review December 9, 2021 05:58

step 4: http proxy support in chart cache. next: integration test tha…

f33a081

…t should surface some RBAC limitations

gfichtenholt added 2 commits December 9, 2021 19:53

step 5. added integration test for private repo with basic auth.

2823eda

Next: transient HTTP failures for chart cache

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

b93fe00

gfichtenholt added 2 commits December 11, 2021 00:27

step 6: handle transient HTTP errors

ce1354f

step 7: unit tests don't bootstrap chart chacht unless they actuallyy…

8ecf201

… need it

gfichtenholt added 3 commits January 7, 2022 15:40

small fixes and cleanup

43163c3

more cleanup

54d69b3

minor fixes

fcfdb13

gfichtenholt added 5 commits January 11, 2022 18:41

small cleanup

ef4fa91

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

184478c

make use of nifty statuserror.FromK8sError() func

c41a880

moved PackageAppVersionsSummary func and unit test into pkg so that i…

acba7e1

…t can be used by multiple plug-ins

add support for plugin config YAML

51c7cfb

antgamdia approved these changes Jan 14, 2022

View reviewed changes

gfichtenholt added 4 commits January 16, 2022 02:06

moved ResourceRefsFromManifest func into pkg so that it can be re-use…

35388f1

…d by multiple plug-ins 2 unit tests are commented out, I have yet to figure out how to fix them also, some feedback from Antonio

- couple unintended changes

dc2701b

update comment

35103f8

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

a0dde32

gfichtenholt merged commit dd27635 into vmware-tanzu:master Jan 16, 2022

Kubeapps automation moved this from In progress to Done Jan 16, 2022

gfichtenholt deleted the further-fluxv2-plugin-features-19 branch January 16, 2022 11:35

gfichtenholt mentioned this pull request Jun 17, 2022

Flux oci support 2 #4932

Merged

Fix for #3636: Open Charts themselves may need caching in fluxv2 plugin #3899

Fix for #3636: Open Charts themselves may need caching in fluxv2 plugin #3899

Conversation

gfichtenholt commented Dec 4, 2021 • edited

gfichtenholt commented Dec 5, 2021

gfichtenholt commented Dec 6, 2021

gfichtenholt commented Dec 9, 2021

gfichtenholt commented Dec 9, 2021 • edited

absoludity commented Dec 10, 2021

gfichtenholt commented Dec 10, 2021

absoludity commented Dec 10, 2021

gfichtenholt commented Dec 10, 2021 • edited

gfichtenholt commented Dec 10, 2021

gfichtenholt commented Jan 10, 2022 • edited

absoludity commented Jan 10, 2022

antgamdia 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 Jan 15, 2022 • edited

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

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

Choose a reason for hiding this comment

gfichtenholt Jan 15, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfichtenholt commented Jan 14, 2022

gfichtenholt commented Jan 14, 2022

antgamdia commented Jan 17, 2022

gfichtenholt commented Jan 17, 2022 • edited

gfichtenholt commented Dec 4, 2021 •

edited

gfichtenholt commented Dec 9, 2021 •

edited

gfichtenholt commented Dec 10, 2021 •

edited

gfichtenholt commented Jan 10, 2022 •

edited

gfichtenholt Jan 15, 2022 •

edited

gfichtenholt Jan 15, 2022 •

edited

gfichtenholt commented Jan 17, 2022 •

edited