Fix Helm index validation for Artifactory

[bb-Ricardo]

Implements mitigation suggested in #1515

just a copy of the updated validation logic in Helm 3.14.0+

[darkowlzz]

Thanks for the detailed explanation in #1515 and this fix.
Looking at the upstream fix, it won't be fixed with a dependency update as we have our own index loading code and I don't see any upstream work on fixing the validation that causes this issue.
This change looks good to me.

Left a comment to add a reference for more information about where the code came from.

[bb-Ricardo]

The validation of the index is also not ideal. Have tests lying around which I were not able to commit and hope to push next week. But the same issue is present in the same bit of code in Helm. As I said, hope to finish this next week.

[bb-Ricardo]

Sorry it took longer than expected.

Added some tests and also comments of where the code came from.

[bb-Ricardo]

Added additional tests to verify index filtering is working as expected.

Also opened issue in for Helm helm/helm#13176

[stefanprodan]

@bb-Ricardo can you please rebase with upstream main and force push

[bb-Ricardo]

@stefanprodan, rebased on fluxed/source-controller main branch. please have a look

[bb-Ricardo]

I can see failing test, lets see if I can reproduce and fix them

[bb-Ricardo]

Hi @stefanprodan,

After looking at it for a while I now assume that the tests are kind of broken.

Due to the fixed validation the Helm charts are removed from the index as they failed validation and now are missing in the test.

The issue is subtle. The last chart in the index was always kept in the index, no matter if invalid or not. Now they are probably removed and the test fail as they try to test against an invalid chart.

Two options I guess:

• remove the test (not desired)
• fix the test Helm chart, which probably causes other tests to fail.

Can you have a look? What do you think?

[darkowlzz]

I had a look at the failing test and it seems the underlying error that results in the chart version to be removed comes from the index value used in that particular test. In

source-controller/internal/helm/chart/builder_remote_test.go

Lines 98 to 101 in 58b4e6d

	- urls:
	- https://example.com/grafana.tgz
	description: string
	version: 6.17.4

, no name is set. In all the other test indices used in the other tests, we set a name field which makes the index entries valid. Without it, the Validate() method returns validation: chart.metadata.name is required which is not a skippable error.
I added the following and it fixed the test

diff --git a/internal/helm/chart/builder_remote_test.go b/internal/helm/chart/builder_remote_test.go
index d43966d..ebe31ae 100644
--- a/internal/helm/chart/builder_remote_test.go
+++ b/internal/helm/chart/builder_remote_test.go
@@ -99,6 +99,7 @@ entries:
         - https://example.com/grafana.tgz
       description: string
       version: 6.17.4
+      name: grafana
 `)
 
        mockGetter := &mockIndexChartGetter{

This was not an issue before as you found out, the original entries were not mutated, returning the index content as read from the input.

[bb-Ricardo]

@darkowlzz,

thank you. so simple and actually obvious, feeling a bit stupid now.

however, I added the name to the chart in the index and these test then passed successfully. I also rebased the branch again on top of upstream/main.

please have a look

[stefanprodan]

LGTM

Thanks @bb-Ricardo 🏅

PS. Please squash your commits and force push.

[bb-Ricardo]

Hi,

I added suggested changes, squashed the changes into one commit and pushed it again.

[darkowlzz]

LGTM!
Thanks for the fix and all the tests.

I suggested a minor test rename which can be accepted, if it sounds good, before merging.

[fluxcdbot]

Successfully created backport PR for release/v1.3.x:

• [release/v1.3.x] Fix Helm index validation for Artifactory #1554