[NotTheEvilOne]

What this PR does / why we need it:
This PR implements changes required to suit ADR034 and 035.

Main changes

• Implementation of ADR 034 and 035. This requires renaming and introducing inherited classes for Flavor, VersionedFlavor and ArtifactBaseName in gardenlinux.features`.
• Fixing sorting of feature lists for flavors.yaml parser in gardenlinux.flavors.Parser
• Changes to various classes utilizing gardenlinux.features
• Implement changed naming to gl-features-parse and remove shortcut command line tool gl-cname.
• The gardenlinux.oci.Container class was (and is still) derived from oras.container.Container but the name is misleading from an OCI naming perspective. Therefore it has been renamed to gardenlinux.oci.Registry and some methods and gl-oci command line parameters have been changed.

Which issue(s) this PR fixes:
Fixes #376
Fixes #377
Requires #390
Related gardenlinux/gardenlinux#5533
Related gardenlinux/gardenlinux#5534

[codecov]

Codecov Report

❌ Patch coverage is 90.37711% with 74 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.67%. Comparing base (acc50b5) to head (23fe45e).
⚠️ Report is 18 commits behind head on main.

Files with missing lines	Patch %	Lines
src/gardenlinux/features/artifact_base_name.py	86.39%	20 Missing ⚠️
src/gardenlinux/oci/__main__.py	90.27%	18 Missing ⚠️
src/gardenlinux/features/__main__.py	82.55%	15 Missing ⚠️
src/gardenlinux/features/c_name.py	94.18%	5 Missing ⚠️
src/gardenlinux/oci/repository.py	89.36%	5 Missing ⚠️
src/gardenlinux/oci/podman.py	77.77%	4 Missing ⚠️
src/gardenlinux/s3/__main__.py	50.00%	4 Missing ⚠️
src/gardenlinux/features/parser.py	88.88%	2 Missing ⚠️
src/gardenlinux/oci/image_manifest.py	96.42%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #393      +/-   ##
==========================================
+ Coverage   91.28%   91.67%   +0.39%     
==========================================
  Files          49       51       +2     
  Lines        2971     3075     +104     
==========================================
+ Hits         2712     2819     +107     
+ Misses        259      256       -3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[gardenlinux-docs-bot]

📚 Documentation Preview PR: gardenlinux/docs#184
😎 Deploy Preview: https://deploy-preview-184--gardenlinux-docs.netlify.app

A PR has been created/updated to preview the documentation changes from this PR.

[NotTheEvilOne]

Added some comments in the code.

Also please justify in the PR description or fix:

1. `GL_ALLOW_FRANKENSTEIN` and `GL_ALLOW_MULTIPLE_PLATFORMS` env vars are still evaluated and `GARDENLINUX_PLATFORM` from `/etc/os-release` (created by builder) is not taken into account. This is different as outlined in #376.

2. `CName.platform` still exists (related to 1.) but should be reworked as outlined in #377.

Thanks for taking your time reviewing. Regarding platform this field is intended in addition to platform_variant to be visible first class citizens of the CName class. Beside the availability of the property feature_set_platform it reflects the os_release key GARDENLINUX_PLATFORM instead of GARDENLINUX_FEATURES_PLATFORMS. As we accept input from os_release without parsing the features tree if cached both properties exist.

As the alternative to the enviromment variables an command line argument --allow-frakenstein would be needed that needs to be reevaluated in different code paths (like within gardenlinux.features.CName and __main__.py) this implementation specific behaviour has not been changed. python-gardenlinux-lib can be extended and reused in other Python projects while the builder is a standalone application. This causes implementation details to be adjusted based on intended usage.

[yeoldegrove]

Added some comments in the code.
Also please justify in the PR description or fix:

1. `GL_ALLOW_FRANKENSTEIN` and `GL_ALLOW_MULTIPLE_PLATFORMS` env vars are still evaluated and `GARDENLINUX_PLATFORM` from `/etc/os-release` (created by builder) is not taken into account. This is different as outlined in #376.

2. `CName.platform` still exists (related to 1.) but should be reworked as outlined in #377.

Thanks for taking your time reviewing. Regarding platform this field is intended in addition to platform_variant to be visible first class citizens of the CName class. Beside the availability of the property feature_set_platform it reflects the os_release key GARDENLINUX_PLATFORM instead of GARDENLINUX_FEATURES_PLATFORMS. As we accept input from os_release without parsing the features tree if cached both properties exist.

Sounds fine for me to stay this way, then.

As the alternative to the enviromment variables an command line argument --allow-frakenstein would be needed that needs to be reevaluated in different code paths (like within gardenlinux.features.CName and __main__.py) this implementation specific behaviour has not been changed. python-gardenlinux-lib can be extended and reused in other Python projects while the builder is a standalone application. This causes implementation details to be adjusted based on intended usage.

Ok, so to not pull in the builder as a dependency, these stay for now. Fine with me.

[NotTheEvilOne]

Reviewers note: The gardenlinux.oci.Container class was (and is still) derived from oras.container.Container but the name is misleading from an OCI naming perspective. Therefore it has now been renamed to gardenlinux.oci.Registry and some methods and gl-oci command line parameters have been changed.

[nkraetzschmar]

We probably can NOT change the content of this annotation. This annotation is load bearing for the gardenlinux-update tool¹.

So while this change would correctly bring the annotation in line with cname as defined in ADR035, it would break compatibility with existing versions of the update tooling. While we can update the tooling and in the future read this annotation correctly or use a different annotation, we can not do that retroactively for existing released images.

The existing images will need to continue to be able to fetch updates of new versions from the OCI, so the check:

strings.HasPrefix(entry.Annotations.Cname, cname)

will need to continue to return true => we MUST maintain the incorrect value in the cname field of the OCI manifest or we brake the ability of existing versions to update to future versions.

Footnotes

1. https://github.com/gardenlinux/package-gardenlinux-update/blob/bf86a4e56204bae69df7c6edc9bce9dcbff2906b/src/main.go#L167 ↩

[NotTheEvilOne]

This has been addressed in commit e3eeb2f. I used the prefix io.gardenlinux for the time being as wen used the same for our mime types as well. Changing the prefix would most likely need an ADR in https://github.com/gardenlinux/gardenlinux.

[nkraetzschmar]

Are these changes in the right place? as in is this the annotation that goes into the images index, or is this an annotation inside a single artifacts image manifest?

The update tooling matches against the index annotation, so I'd guess we actually need the new io.gardenlinux.cname and the legacy cname = {artifact_base} annotations somewhere here:

python-gardenlinux-lib/src/gardenlinux/oci/image_manifest.py

Lines 332 to 336 in e3eeb2f

	metadata_annotations = {
	"cname": self.cname,
	"architecture": self.arch,
	"feature_set": self.feature_set,
	}

[NotTheEvilOne]

In fact the relevant lines are for extended_dict in

python-gardenlinux-lib/src/gardenlinux/oci/image_manifest.py

Lines 240 to 247 in e3eeb2f

	if (
	ImageManifest.ANNOTATION_CNAME_KEY in manifest_annotations
	and ImageManifest.ANNOTATION_VERSION_KEY in manifest_annotations
	and ImageManifest.ANNOTATION_COMMIT_KEY in manifest_annotations
	):
	manifest_annotations[ImageManifest.ANNOTATION_CNAME_DEPRECATED_KEY] = (
	f"{self.flavor}-{self.version}-{self.commit}"
	)

But in fact you pointed out correctly that we should write out the artifact_base_name to our metadata files as well.

[nkraetzschmar]

my understanding is this here is what creates the combined OCI index:

python-gardenlinux-lib/src/gardenlinux/oci/repository.py

Line 297 in e3eeb2f

index.append_manifest(manifest)

and that directly uses the values written by

python-gardenlinux-lib/src/gardenlinux/oci/image_manifest.py

Lines 332 to 336 in e3eeb2f

	metadata_annotations = {
	"cname": self.cname,
	"architecture": self.arch,
	"feature_set": self.feature_set,
	}

so this IS the place the needs the new io.gardenlinux.cname = {cname} and the legacy support cname = {cname}-{arch}-{version}-{commit} in order to not break in-place updates since the update tool checks annotations on the index, NOT the per flavour manifest.

[NotTheEvilOne]

It is not helpful to create a new hard-coded set of annotations at all here again. Fixing that right away.

[nkraetzschmar]

such a function can never work correctly. there simply is no way to construct a real cname from just a set of features without access to the underlying graph. else one can't be sure that it is the minimal set, that things are sorted in the correct canonical order, etc.

[nkraetzschmar]

In the caller here:

python-gardenlinux-lib/src/gardenlinux/features/__main__.py

Lines 284 to 293 in e3eeb2f

	graph = parser.filter(cname, additional_filter_func=additional_filter_func)
	sorted_features = Parser.sort_graph_nodes(graph)
	minimal_feature_set = Parser.get_minimal_feature_set(graph)
	sorted_minimal_features = Parser.subset(
	minimal_feature_set, sorted_features
	)
	cname = Parser.get_cname_from_feature_set(sorted_minimal_features)

both a cname and the graph is available, thus there either the input cname should just be kept or assuming the aim is to sanitize the input cname in case it's not actually the right order etc (akin to how the builders parse_features corrects cname order) then this sorting should never the less be done aware of the graph. so there something that uses sort_graph_nodes could be used instead.

the only other caller I could find is:

python-gardenlinux-lib/src/gardenlinux/flavors/parser.py

Lines 116 to 117 in e3eeb2f

	# Process features
	cname = FeaturesParser.get_cname_from_feature_set([name] + features)

which genuinely does not have a cname or graph available and only the flavors.yml. but given that it's not actually possible to construct a true cname.

maybe we should split this into 2 functions, one that uses sort_graph_nodes and actually constructs a true cname with guaranteed correct sorting based on the graph, and a create_pseudo_cname for getting a cname like object out of the flavors yml, that gets the platform as a dedicated input and all other features as a list an creates the cname-like strings that the builder will happily consume yet may not always be correct sorted cnames.

[NotTheEvilOne]

[...]

the only other caller I could find is:

python-gardenlinux-lib/src/gardenlinux/flavors/parser.py

Lines 116 to 117 in e3eeb2f

	# Process features
	cname = FeaturesParser.get_cname_from_feature_set([name] + features)

which genuinely does not have a cname or graph available and only the flavors.yml. but given that it's not actually possible to construct a true cname.

[...]

In fact this is a very specific use case described above before:

The lexicographical topological sorting needs a feature graph to work. As CName generation from a feature set based on an cached *.release should work without a features directory only part of the sorting can be implemented in the named methods. They are sort by platform (first element of the feature set), features and flags, each sorted alphabetically.

This "pseudo" canonical name is a prerequisite of the above to work. Usually canonical name generation is always based on a minimal feature set and contains therefore the correct information required to generate the name.

[nkraetzschmar]

when working from a .release file there is NO reason to re-construct the cname. the cname is in the file. with gardenlinux/builder#171 and gardenlinux/gardenlinux#5536 this has been adjusted to be the correct form.

[nkraetzschmar]

where is the assumption that the platform would be the first (and only the first) element in the list?

[NotTheEvilOne]

gardenlinux.features.Parser.sort_graph_nodes() adds, as a base for our lexicographical topological sorting, platform as the first item. Based on ADR 020 only one platform (including "frankenstein") should exist.

[nkraetzschmar]

frankenstein would not even be part of the features though. frankenstein is simply the placeholder value that should be used in place of a feature as the platform the actual features list of a frankenstein image simply contain multiple platforms.

Also arguing here that this works because the input will come from sort_graph_nodes conflicts with this function not being able to call sort_graph_nodes to also work for callers without graph access.

So I still think we should split this up into one function that does it correctly, via the graph when available (and therefore does not potentially resort features incorrectly), and one that is explicitly intended to create a cname like string given a platform and some features which is exactly what we have when parsing the flavors.yml but that makes it clear that it is not guaranteed to produce a correct cname and that it only constructs a cname like builder target, not a true cname.

[nkraetzschmar]

python-gardenlinux-lib/src/gardenlinux/oci/index.py

Lines 98 to 114 in e314683

	cname = manifest["annotations"]["cname"]
	existing_manifest_index = 0
	for existing_manifest in self["manifests"]:
	if "cname" not in existing_manifest.get("annotations", {}):
	existing_manifest_index += 1
	continue
	if cname == existing_manifest["annotations"]["cname"]:
	break
	existing_manifest_index += 1
	if len(self["manifests"]) > existing_manifest_index:
	self["manifests"].pop(existing_manifest_index)
	self["manifests"].append(manifest)

This logic has not been adjusted by this PR, yet making the cname annotation only the cname without the arch means that two different arch flavours of the same cname would collide and overwrite each other.

Changing the annotation to the one we need for in-place update compat will circumvent this issue, but we probably should already switch the check to use io.gardenlinux.cname plus arch as a de-dup mechanisme

[NotTheEvilOne]

python-gardenlinux-lib/src/gardenlinux/oci/index.py

Lines 98 to 114 in e314683

	cname = manifest["annotations"]["cname"]
	existing_manifest_index = 0
	for existing_manifest in self["manifests"]:
	if "cname" not in existing_manifest.get("annotations", {}):
	existing_manifest_index += 1
	continue
	if cname == existing_manifest["annotations"]["cname"]:
	break
	existing_manifest_index += 1
	if len(self["manifests"]) > existing_manifest_index:
	self["manifests"].pop(existing_manifest_index)
	self["manifests"].append(manifest)

This logic has not been adjusted by this PR, yet making the cname annotation only the cname without the arch means that two different arch flavours of the same cname would collide and overwrite each other.

Changing the annotation to the one we need for in-place update compat will circumvent this issue, but we probably should already switch the check to use io.gardenlinux.cname plus arch as a de-dup mechanisme

As we added cname as a deprecated annotation including the artifact base name the logic continue to work as expected. Meanwhile it should be adjusted to match the new io.gardenlinux.* ones.