Support for semantic versioned charts

[verenion]

Frigate doesn't seem to work if a chart requirement is not specified directly, and instead uses a range:

We have this in our Chart.yaml:

  - name: common
    version: ^1
    repository: oci://registry.xxx.com/charts

Helm update runs fine:

Saving 1 charts
Downloading common from repo oci://registry.xxx.com/charts
Pulled: registry.xxx.com/charts/common:1.7.4

Frigate fails with:

    shutil.unpack_archive(dependency_path, tmpdirname)
  File "/usr/lib/python3.10/shutil.py", line 1271, in unpack_archive
    func(filename, extract_dir, **kwargs)
  File "/usr/lib/python3.10/shutil.py", line 1199, in _unpack_tarfile
    tarobj = tarfile.open(filename)
  File "/usr/lib/python3.10/tarfile.py", line 1622, in open
    return func(name, "r", fileobj, **kwargs)
  File "/usr/lib/python3.10/tarfile.py", line 1688, in gzopen
    fileobj = GzipFile(name, mode + "b", compresslevel, fileobj)
  File "/usr/lib/python3.10/gzip.py", line 174, in __init__
    fileobj = self.myfileobj = builtins.open(filename, mode or 'rb')
FileNotFoundError: [Errno 2] No such file or directory: './charts/common-^1.tgz'

There is a ./charts/common-1.7.4.tgz.

[jacobtomlinson]

Thanks for raising this! I agree this is something we should fix.

One pain point will be that Helm charts strictly use semver with extensions like ~ and ^.

In Python we typically use PEP440 which is less restrictive and covers semver so we could use packaging.versions from the standard library but it doesn't (and won't ever) support extended grammar.

There seems to also be python-semver which we could absolutely add as a dependency, but while that handles semver it also doesn't seem to support the extended grammar (xref python-semver/python-semver#241).

So in the short term, I expect we could add some support for semver today with either of these libraries. Given that python-semver may add extended grammar in the future that might be a good bet for now.

So I don't think we can get to a place where your example with ^1 works in the short term, but >=1 <2 would work fine.

[aogier]

Hello, today I've just stumbled upon this and I'd like to drop my 2 cents. While I understand a proper implementation of semver parse could offer some benefits, especially in the long run, it seems we could complete a significant iteration in a much simpler way.
What I see by reading code is at the moment charts subdir population is delegated to helm dep update: just like any other subprocess we popen, helm is a blackbox from frigate standpoint, but something can be expected:

• for every dependencies[].name, there is a charts/file*.tgz
• there is no undefined/outdated dep archives in charts ie. no Chart.yaml entry -> no file

so it is indeed very possible that using a glob instead of dependency version could solve the vast majority of use cases: semver notations is already properly resolved by helm itself and this should Just Work in frigate.gen.load_chart_with_dependencies:

dependency_path = glob.glob(os.path.join(
                chartdir, "charts", f"{dependency_name}-*.tgz",
            ))

What would be left out/not properly work in this case? If we consider such a dependencies snippet where the same chart is alias included more than once with different versions:

dependencies:
- alias: redisfoo
  condition: redisfoo.enabled
  name: redis
  repository: https://charts.bitnami.com/bitnami
  version: ~16.4.0
- alias: redisbar
  condition: redisbar.enabled
  name: redis
  repository: https://charts.bitnami.com/bitnami
  version: ~16.3.0

or indeed two inclusions of two charts with the same name but different repos (unsure if helm would be cool on that tho):

dependencies:
- name: george
  repository: https://charts.george.site/funky
  version: ~16.4.0
- name: george
  repository: https://charts.bonzi.buddy/ehi
  version: 101.9

then the proposed implementation would very probably miss the right thing with unpredictable effects. Nonetheless I'd dare to say those are very uncommon practice, for sure way less common that using helm semver implementation at its full potential, for which we could consider, in a very far future :P, to eg. gopy wrap the very semver library so we know we don't lag/mismatch stuff.

What do you think? Thank you, regards

[consideRatio]

This code is what is relevant for this error, the error is that frigate assumes the .tgz file added to the charts/ folder after helm dep up is run, will be named based on the version specified in Chart.yaml for the chart dependency.

frigate/frigate/gen.py

Lines 58 to 74 in 4153bf5

	root = [] if root is None else root
	chart, values = load_chart(chartdir, root=root)
	if "dependencies" in chart:
	for dependency in chart["dependencies"]:
	dependency_name = dependency["name"]
	dependency_path = os.path.join(
	chartdir, "charts", f"{dependency_name}-{dependency['version']}.tgz",
	)
	with tempfile.TemporaryDirectory() as tmpdirname:
	shutil.unpack_archive(dependency_path, tmpdirname)
	dependency_dir = os.path.join(tmpdirname, dependency_name)
	update_chart_dependencies(tmpdirname, dependency_name)
	_, dependency_values = load_chart_with_dependencies(
	dependency_dir, root + [dependency_name]
	)
	values = squash_duplicate_values(values + dependency_values)

I think helm chart works like this, that anything in the charts folder is considered something to be installed, so if there is a folder called charts/my-subchart and a file called /charts/my-downloaded-external-chart.tgz I think both will be installed as part of intalling the main chart.

If that is the case, the change of logic in frigate should be to not just look for certain files, but look for all charts. Practically, it is now ignoring subcharts I think, and only accepting external chart dependencies. So, fixing this, would be to fix two separate things. An incremental improvement would be to look for all .tgz files.

[aogier]

@consideRatio looks like interesting material for starting another issue as it seems a bit off-topic here, nor the proposed lookup suggestion add much value, given that you will have to decide anyway which file to analyze in every loop cycle. What do you think?

[consideRatio]

I created #49 to represent the issue about frigate not considering subcharts. I found it related to this issue as it is also a broken expectation on the frigate logic with regards to what charts to document.

I think this discussion is highly relevant to this bug, and I also think #44 is relevant to merge as it fixes another bug that also touches this logic if not only to make this code more readable.

My idea on how to fix this issue, not addressing #49, would be to not loop over declared dependencies in a charts Chart.yaml, but loop over .tgz files in the charts/ folder. @aogier I did not understand the part about given that you will have to decide anyway which file to analyze in every loop cycle though, so maybe I'm off about this strategy.

[jacobtomlinson]

@aogier your approach sounds reasonable. The other case I can think of is if you modify the version and run helm dep up again does it clean up the now unused version?

Either way do you have interest in raising a PR to implement this/

[aogier]

@jacobtomlinson I can confirm helm dep up do the expected housekeeping against tgz files under charts/, ie. it removes any archive which does not directly satisfy declared dependencies in chart's manifest.
I'm already testing on my env a little modification and it seems to do, I'll take my time to write some tests and I'll open a PR then!

[verenion]

Are there any updates to this issue? It would be nice to get this working. I'm not a python guy, but if noone is working on this I can take a look when I get time.

[aogier]

@verenion you can start from there - it currently works, I only lacked enough motivation to push another PR because others' conduct but that code could still be helpful ;)