[DO NOT MERGE] Generate go structs from helm chart values

[HomayoonAlimohammadi]

[DO NOT MERGE] (well obviously)

Overview

This PR adds a yaml to Go struct generator that extracts and reads the values.yaml files from the tgz. feature charts, and generates a Go struct with regards to the content of that values.yaml file.

This is an initial PR to spark conversations around the path forward.
Please move to the How to review section to prevent headaches.

IMPORTANT

• POC files should be deleted before merge (src/k8s/pkg/k8sd/features/loadbalancer/test/main.go)

Rationale

The generated structs can be used to pass and overwrite default helm values when applying a feature chart. E.g.:

lbValues := values.CkLoadbalancerValues{
	Driver: "metallb",
	L2: values.CkLoadbalancerValues_L2{
		Enabled:    loadbalancer.GetL2Mode(),
		Interfaces: features.ToAnyList(loadbalancer.GetL2Interfaces()),
	},
	IpPool: values.CkLoadbalancerValues_IpPool{
		Cidrs: features.ToAnyList(cidrs),
	},
	Bgp: values.CkLoadbalancerValues_Bgp{
		Enabled:  loadbalancer.GetBGPMode(),
		LocalAsn: int64(loadbalancer.GetBGPLocalASN()),
		Neighbors: features.ToAnyList([]map[string]any{
			{
				"peerAddress": loadbalancer.GetBGPPeerAddress(),
				"peerASN":     loadbalancer.GetBGPPeerASN(),
				"peerPort":    loadbalancer.GetBGPPeerPort(),
			},
		}),
	},
	UNSAFE_MISC_FIELDS: map[string]any{
		"random-key": "random-val",
		"another": map[string]any{
			"key": "val",
		},
		"driver": "overwritten",
	},
}

lbValuesMap, err := lbValues.ToMap()
if err != nil {
	log.Fatalf("failed to convert LoadBalancer values to map: %v", err)
}

if _, err := m.Apply(ctx, ChartMetalLBLoadBalancer, helm.StatePresent, lbValuesMap); err != nil {
	log.Fatalf("failed to apply MetalLB LoadBalancer configuration: %v", err)
}

This will prevent us to go through the daunting task of ensuring the hardcoded fields and values are compatible with what each values.yaml file expects.

This is how we're handling it right now:

values := map[string]any{
// "driver": "metallb",
"l2": map[string]any{
	"enabled":    loadbalancer.GetL2Mode(),
	"interfaces": loadbalancer.GetL2Interfaces(),
},
"ipPool": map[string]any{
	"cidrs": cidrs,
},
"bgp": map[string]any{
	"enabled":  loadbalancer.GetBGPMode(),
	"localASN": loadbalancer.GetBGPLocalASN(),
	"neighbors": []map[string]any{
		{
			"peerAddress": loadbalancer.GetBGPPeerAddress(),
			"peerASN":     loadbalancer.GetBGPPeerASN(),
			"peerPort":    loadbalancer.GetBGPPeerPort(),
		},
	},
},
}

if _, err := m.Apply(ctx, ChartMetalLBLoadBalancer, helm.StatePresent, values); err != nil {
	log.Fatalf("failed to apply MetalLB LoadBalancer configuration: %v", err)
}

We previously had an issue with the values getting passed to the ingress controller where they did not match the expected ones in the values.yaml file. That issue was surfaced COMPLETELY by chance otherwise it would have been a very frustrating debugging challenge.

How to review

• The files under values/ are autogenerated and will not need a review.
• The values.yaml files like coredns-1.2.3.tgz_values.yaml are also only extracted from the corresponding .tgz files and should not be reviewed.
• The core of this PR is the generator.go and the Makefile.

Usage

cd k8s/manifests/charts
make clean # cleans extracted and generated files
make gen # extracts values.yaml files and generates Go files containing the structs

To further control the generation process use:

go run generator.go -sources=path/to/sources/file -pkg=myvalues -dir=path/to/target/dir -advanced-types=true -unsafe-field=true

[HomayoonAlimohammadi]

Next steps:

• First ensure that we even want to have this
• Do any sort of cleanup
• Add docs and README describing the process
• Integrate it with the main build process of k8s-snap
• Ensure that these structs generate the exact same map[string]any as the current hardcoded values.
• Replace the hardcoded values with these structs.

[bschimke95]

I really appreciate that work @HomayoonAlimohammadi and this will greatly stabilize our code-base. I'm +1 for moving forward with that change.
As you already said, this is way to much to review and only serves as a PoC.

My suggestion would be to start with a PR that implements the generator and adds unittests that take the existing maps as validation input.
And then we add the generated files and integrations into subsequent PRs.

[eaudetcobello]

So there's clearly been a lot of thought out into this, so I don't want to diminish your work @HomayoonAlimohammadi. Thanks for submitting this.

One of the main issues is that it seems to implement the entire values.yaml struct for a chart, whereas our current implementation is simple in that we only define and maintain values we care about.

I guess I don't see why we need this. We have a specific list of helm charts, and in our current code it's very clear which values we care about, because they're the only ones thay we define in the code, and the rest are defaulted by Helm automatically.

This will prevent us to go through the daunting task of ensuring the hardcoded fields and values are compatible with what each values.yaml file expects.

I think this adds a lot of complexity, specifically 15,000 lines worth, for something that only took maybe 100-200 LoC originally. Personally I went through the upgrade process of bumping a chart version and reviewing the maps we create, it was not complicated and could be done by anyone new to the codebase.

EDIT: The benefit of having compile-time type safety is definitely appealing. We can even automate the process of pulling new Chart versions from Upstream and know at compile-time if our code needs to change. After meeting with @HomayoonAlimohammadi, I think this is a cool idea and I'm +1.

[bschimke95]

I don't see this as a problem. It is still very clear which values are used and which are not.
You'd typically don't look at the generated code but rather at places where this struct is used, e.g.

lbValues := values.CkLoadbalancerValues{
	Driver: "metallb",
	L2: values.CkLoadbalancerValues_L2{
		Enabled:    loadbalancer.GetL2Mode(),
		Interfaces: features.ToAnyList(loadbalancer.GetL2Interfaces()),
	},
	IpPool: values.CkLoadbalancerValues_IpPool{
		Cidrs: features.ToAnyList(cidrs),
	},
	Bgp: values.CkLoadbalancerValues_Bgp{
		Enabled:  loadbalancer.GetBGPMode(),
		LocalAsn: int64(loadbalancer.GetBGPLocalASN()),
		Neighbors: features.ToAnyList([]map[string]any{
			{
				"peerAddress": loadbalancer.GetBGPPeerAddress(),
				"peerASN":     loadbalancer.GetBGPPeerASN(),
				"peerPort":    loadbalancer.GetBGPPeerPort(),
			},
		}),
	},
	UNSAFE_MISC_FIELDS: map[string]any{
		"random-key": "random-val",
		"another": map[string]any{
			"key": "val",
		},
		"driver": "overwritten",
	},
}

Looks very similar to what we have today, with the extra benefits of type safety.

[HomayoonAlimohammadi]

Let's close this one. Maybe we can investigate this further later.