Lokomotive: Refactor platforms configuration to only include platform-specific parameters

[invidian]

Currently our cluster block mixes all kinds of configuration and lack any structure. We have there AWS credentials, Packet credentials, which CNI to use, what CIDRs to use etc. This means, that if we want to add a new feature e.g. to the kube-apiserver (#122), we need to do that separately for all platforms.

Most of the configuration I mentioned is platform-agnostic, so I think it would make sense to move them to separate configuration block, so platform code will only deal with platform-specific operations eventually.

[invidian]

Thought from #126: if we separate the configuration, we need to have a way of validating the dependencies between the setups: e.g. that Packet only supports calico networking.

[iaguis]

The refactoring should include unit tests.

[invidian]

Perhaps as part of this task, we could also tackle streamlining configration decoding and it's validation, so it's done in one place instead of spread across all the code.

[invidian]

Kind of, as a part of this refactoring, we should also define the way, how cluster information can be shared and used by the components. Example use: #207.

[johananl]

While I agree it could be useful to have one place in the code to define all platform-agnostic config params we expose to users, I think it would be a mistake to break down the cluster block at the configuration level.

As a user, I think in terms of clusters: "I want one cluster on Packet based on Flatcar edge with 3 worker nodes and no HA. I want two clusters on AWS based on Flatcar stable and an HA control plane." Therefore, I think the configuration structure should reflect this line of thinking, namely have all cluster-related knobs within the cluster block.

Breaking down the cluster block to smaller, top-level blocks such as platform, network etc. sounds counter-intuitive to me. As a user, for example, I don't care too much if the facility knob is platform-agnostic or not. All I care about is having a cluster in ams1. Organizing the knobs in such "categories" inside the config sounds to me like mixing maintainer concerns with UX concerns.

So what I think we should do?

First, I'd like to verify we are addressing a real problem. Are we currently doing lots of copy-pasting to justify the effort of such a big refactoring task?
Assuming we indeed have a problem which needs solving, I propose we look for a way to do the necessary refactoring without changing the UX, because it seems to me this issue doesn't aim to fix a UX-level problem but rather a code maintenance problem.
Lastly, if we cannot find an implementation which doesn't negatively affects the UX, I'd suggest we simply decide to tolerate some duplication between the platforms, or use well-known strategies for eliminating duplication such as code generation.

[ipochi]

@johananl

I am currently organizing a new PR in such a way that there is maximum refactoring without any UX change. I will create a new PR with the necessary changes cherry picking a lot of commits from the existing PR.

Thank you for the reviews and feedback.

[iaguis]

There was an OOB discussion about this, I'll try to summarize:

1. There's a lot of discussion around UI changes in the configuration file, so this needs careful consideration and we're not doing those changes right away.
2. There's a need to refactor the code to make adding new flags to different platforms and unit tests easy.

It seems this issue deals with both points, I suggest keeping it for 2 and open a new one to discuss 1.

The most pressing need right now is adding a way to configure the OIDC flags used by Dex+Gangway, so we should tackle this first.

[ipochi]

Over the long weekend I was trying out a strategy to remove code duplication.

I stumbled upon the idea how packer is using in its codebase to provide support to HCL2 syntax for their configurations.

One of the similar problems that we are having is the duplication of common fields across platform implementations. Packer internally uses code-generation and mapstructure library instead of HCL tags.

for code-generation packer used https://github.com/hashicorp/packer/blob/master/cmd/mapstructure-to-hcl2/mapstructure-to-hcl2.go

In HCL we don't have the ability to use squash as a property of the tag which allows embedded structs to be squashed as a flat structure. Instead hcl provides remain property, however the downside is that we can only have one field having remain property.

I tried to use the weekend to come with with something similar for lokomotive in http://github.com/ipochi/lokoloader (a lot of inspiration and code snippets are used from packer codebase).

Note: I did this in an external repo as I wasnt sure how it would turn out.

There are still issues with it such as decoding worker pools. however over the weekend I realized its too much of a change/effort for little benefit using this approach.

The idea suggested by @schu is fairly easily to incorporate, however I want to add that we can only have one embedded struct with the property remain, hence if we include baremetal as well to get a union of common fields. then we we only get two fields AssetDir and ClusterName.

As of this point I would love to have your opinions and feedback on other options or a consensus to use Michael's approach.