[Enhancement] Refactoring: normalize label flags (k3s node & runtime) (#598, @ejose19)

[ejose19]

What

This PR aims to normalize label flags brought by #378 & #584, both in naming and availability of methods (so both flags can be used in k3d clusters create & k3d node create)

Why

Besides the individual usefulness of having these flags, also being consistent in naming and availability of commands is something users will appreciate

Implications

As labels property (and its cli shorthand -l) is being replaced by --runtime-label to avoid ambiguity (as determined in #584 (comment)), this is a breaking change.

[iwilltry42]

Would it make sense to rather have this under

options:
  k3s:
    nodeLabels: []

?

[ejose19]

Sure, that looks better, wonder if there should be runtime as well under options so only main properties are k3d specific.

[iwilltry42]

Guess that would make sense.. config file is still alpha, so we could do a revamp for the first beta to make things more clean/consistent.

[iwilltry42]

Thanks for your efforts here @ejose19 !
This introduces a breaking change, so we need to double check this before releasing, so it doesn't break existing workflows.
This whole propagation of flags from k3s up to k3d feels a bit odd to me.
I wonder if it wouldn't just be easier to add node filters to the k3s-server-arg and k3s-agent-arg and simplify the syntax a bit so they're more convenient to use.
Otherwise, we may end up kind of obliged to catch up with all the flags k3s provides.
What's your take on this?

[ejose19]

@iwilltry42 Yes, actually I was going to do another PR after this one to add nodeFilter to k3s-server-arg & k3s-agent-arg, that way we cover all k3s flags. However I would still like to proceed with this PR to be api consistent (as k3s-node-label was just added to k3d node create). If you want I can leave the --label flag as it so it's not a breaking change, and we change the naming in next major release.

[iwilltry42]

@ejose19

I was going to do another PR after this one to add nodeFilter to k3s-server-arg & k3s-agent-arg

Thanks for the initiative! I'm already on it though 😬

However I would still like to proceed with this PR to be api consistent (as k3s-node-label was just added to k3d node create).

I'm struggling, if we should actually leave it there, as we cannot even justify it with "node-specific" option, as all of those k3s flags are "node-specific" 🤔

If you want I can leave the --label flag as it so it's not a breaking change, and we change the naming in next major release.

We could add a deprecation notice and internally treat it like the new name.

[ejose19]

@iwilltry42 Kubernetes labels will probably be one of the most used k3s extraServerArgs, so this is just being a convenience API.

However, in case you still don't want to proceed with that (and remove the newly added option from k3d node create so there's no "discrepancy"), what about if the config file looked like this:

// both `servers` and `agents` can take either a number or an array of node specific details
servers: 1
agents:
 - volume: '/my/host/path:/path/in/node'
   port: 8080:80
   k3sExtraArgs:
     - --node-label=foo=bar

reducing the need to specify nodeFilters everywhere when it's targeting a single node, as well keep per-node configuration together.

That of course would require more work, and it's out of the scope of this PR, so let me know how should I proceed here and I will make the adjustments.

[iwilltry42]

Hi @ejose19,
Due to additional larger changes in #605 I decided to go for a new major release soon-ish, that can contain breaking changes (like your flag renaming) without problems.
For this I created the new main-v5 branch and rebased your PR onto that.
Also, the --k3s-node-label flag PR was reverted in main and "re-reverted" in main-v5 so it also only lands in the v5.0.0 release.

[ejose19]

@iwilltry42 that's great! However you didn't let me know how I should proceed in this PR?

[iwilltry42]

@ejose19 , I think this is those are the last missing pieces before merging:

• move to different section in config file: https://github.com/rancher/k3d/pull/598/files#r630251758
• move from config v1alpha2 to v1alpha3

[ejose19]

Yeah adjusting to latest changes is not an issue, but since you said this:

@ejose19

I was going to do another PR after this one to add nodeFilter to k3s-server-arg & k3s-agent-arg

Thanks for the initiative! I'm already on it though grimacing

However I would still like to proceed with this PR to be api consistent (as k3s-node-label was just added to k3d node create).

I'm struggling, if we should actually leave it there, as we cannot even justify it with "node-specific" option, as all of those k3s flags are "node-specific" thinking

If you want I can leave the --label flag as it so it's not a breaking change, and we change the naming in next major release.

We could add a deprecation notice and internally treat it like the new name.

what I want to know is, should I remove --node-label or not?

EDIT: I saw your comments on 7087264#commitcomment-50836483, and now it's clear that you just want to adjust this PR to anything that might have changed in main-v5, I will get it done.

[ejose19]

@iwilltry42 This should be good now, here's the summary of changes:

• Top level labels is being removed (along with its cli flags -l, --labels)
• options.runtime.labels is implemented as replacement for the removed labels (cli shortcut being: --runtime-label)
• options.k3s.nodeLabels is added to allow directly specifying k3s node labels (cli shortcut being: --k3s-node-label)
• label usage is normalized for cluster create & node create
• adjusted migration script to migrate from v1alpha2 labels to v1alpha3 options.runtime.labels
• added test assertions to k3s node label

[iwilltry42]

Looks good!
Just one last thing to clarify 👍

[ejose19]

Ok, should be good now. I also added a validation for user provided --runtime-label (both in config file and in cli flag) in order to let consumers know that some label names are reserved for internal use of k3d:

https://github.com/rancher/k3d/blob/ff61747ff3e995d0a2bc828c3aa168b9ff2195eb/pkg/types/types.go#L108-L138

From that I deducted that anything that begins with k3d. or k3s. shouldn't be allowed, along with app. If you think anything else needs to be added to the validation list let me know, and also future "k3d technical label" should respect that premise in order to not conflict with user labels.

[iwilltry42]

LGTM!
The validation of labels for reserved prefixes (k3d/k3s) makes total sense, thanks for that!

Thanks a lot for your contribution :)