[Feature Request]: Add ability to apply labels to individual nodes in a nodepool

[maggie44]

Description

In #894 there is a discussion around node pool limits. While originally thought to be 255 it turns out to be 50 due to Hetzner's limits on subnets.

This is problematic for those who scale their services via nodepools instead of nodes as we now have much lower limits than originally anticipated. I for example add additional node pools to allow me to add labels and then the labels are used to associate a deployment. E.g.:

default = [
    {
      name        = "x1-1",
      server_type = "cax11",
      location    = "fsn1",
      labels      = ["cluster=a1"],
      taints      = [],
      count       = 1
    },
    {
      name        = "x1-2",
      server_type = "cpx11",
      location    = "nbg1",
      labels      = ["cluster=a1"],
      taints      = [],
      count       = 1
    },
    {
      name        = "x1-3",
      server_type = "cpx11",
      location    = "hel1",
      labels      = ["cluster=a1"],
      taints      = [],
      count       = 1
    }
  ]

In the above example I deploy to nodes with the label cluster=a1 which allows me to create a geographically dispersed HA cluster for a particular deployment and be able to target pods at particular sets of nodes. Being able to target pods at particular nodes is important for my services, as is those sets being geographically dispersed.

For a second deployment on different nodes, I add additional nodepools with labels for a second cluster:

default = [
    {
      name        = "x1-1",
      server_type = "cax11",
      location    = "fsn1",
      labels      = ["cluster=a1"],
      taints      = [],
      count       = 1
    },
    {
      name        = "x1-2",
      server_type = "cpx11",
      location    = "nbg1",
      labels      = ["cluster=a1"],
      taints      = [],
      count       = 1
    },
    {
      name        = "x1-3",
      server_type = "cpx11",
      location    = "hel1",
      labels      = ["cluster=a1"],
      taints      = [],
      count       = 1
    },
    {
      name        = "x2-1",
      server_type = "cax11",
      location    = "fsn1",
      labels      = ["cluster=a2"],
      taints      = [],
      count       = 1
    },
    {
      name        = "x2-2",
      server_type = "cpx11",
      location    = "nbg1",
      labels      = ["cluster=a2"],
      taints      = [],
      count       = 1
    },
    {
      name        = "x2-3",
      server_type = "cpx11",
      location    = "hel1",
      labels      = ["cluster=a2"],
      taints      = [],
      count       = 1
    }
  ]

Ideally, we would be able to increase the count on each pool to add additional nodes and that way we do not hit the limit of 50 nodepools. But when scaling nodes via count there is no means of adding labels to the new nodes, and therefore no way to target pods at a particular set of nodes.

I think being able to add labels to nodes within nodepools would be a useful feature to overcome this. It could look something like this:

agent_nodepools = [
    {
      name        = "my_nodes_location1",
      server_type = "cax11",
      location    = "fsn1",
      labels      = [],
      taints      = [],
      count      = [
        {
            name = "node1", 
            labels = ["cluster=1"]
        }, 
        {
            name = "node2",
            labels = ["cluster=2"]
        },
        {
            name = "unique_useful_user_id_for_reference",
            labels = ["cluster=3"]
        }
      ], # <-- starts 3 nodes all in the same region and node pool and with unique cluster labels.
    }
  ]

These could then be deprovisioned like this, assuming we need to set to 0 rather than change the length of the count when deprovisioning (maybe that's only a nodepool thing):

count = [{}, {name = "node2", labels = ["cluster=1"]}]]

It would be nice if this then allows us more control over the node labelling. It currently looks something like:

k3s-mynodename-epo

k3s is customisable via the config file and can be disabled. mynodename is set via name in the agent_nodepool. epo seems to be a random three letter code to distinguish each node.

It would be nice to be able to add identifiers to the nodes within each pool too, so either name in the item within the count array replaces epo and the terraform apply checks for unique values, or we add that name in addition to the random three letters:

 count      = [ 
 {
    name = "nodename1", 
    labels = ["cluster=1"]
 }
]

k3s-mynodename-nodename1
# or
k3s-mynodename-nodename1-epo

It wouldn't mean we would be able to use 255 node pools as originally thought but permits a similar effect and have more control over the nodes.

[mysticaltech]

Oh my, that's a layered request! But doable. However, please don't hesitate to submit a PR, if you you cannot code terraform, GPT-4 can 😂, you just manage it. It would help, otherwise I will get to it sooner or later. Also don't forget about backward compatibility, so all new additions must be optional and well thought of.

[maggie44]

Oh my, that's a layered request!

Hopefully in a good way 😟. I tried to add more detail than less, I never like being on the other end of the feature requests with a one liner and no rationale for the goal.

GPT and I have a very close working relationship already, this one may be a bit of a stretch though 😉.

[mysticaltech]

@maggie44 No worries, will do.

[maggie44]

Giving this a bit more thought, wanted to get some input on potential implementation. The goal being to allow setting specific of labels on individual nodes within each of the nodepools to allow larger pool limits. I think the label idea I mentioned above would work, but isn't as easy to work with as I would have hoped. Any ideas welcome.

    {
      name        = "x1-1",
      server_type = "cax11",
      location    = "fsn1",
      labels      = ["cluster=a1"],
      taints      = [],
      count       = 3 # <-- different labels for each of these 3
    },

[Silvest89]

@maggie44
Your proposal would definitely be a breaking change.
@mysticaltech

I would do it like this. It doesn't break the way they are. And if users decide do implement this they can :)

    {
      name        = "x1-1",
      server_type = "cax11",
      location    = "fsn1",
      labels      = ["cluster=a1"],
      individual_labels = [{ cluster: a1 }, {}, {}] <-- based on node index. Validation that size of array matches nodepool count
      taints      = [],
      count       = 3 # <-- different labels for each of these 3
    },

Fairly easy to implement. Same goes for taints. I would use these type definitions. I use them myself in #1012

    individual_labels      = optional(map(string), {})
    individual_taints      = optional(list(object({
      key       = string
      value     = string
      effect    = string
    })), [])

To my opinion this is how labels and taints should be ;]. 1:1 match with kubernetes.

[mysticaltech]

@Silvest89 Agree, we should have a 1:1 match, that would be the cleanest way! 🙏

[maggie44]

I think that makes a lot more sense, very useful brainstorm.

I think it might also get hard to identify which node a label is going on to though. If node count was 100, it would be hard to find node 57 to be able to add labels to it. Counting all of the different entries, and hoping it is entry 57?

Maybe they could be numbered and use keys and then also not need to do the validation of matching the node index to the labels?

{
  "1": [
    "cluster=a1",
    "anotherlabel=b1"
  ],
  "2": [
    "cluster=a2",
    "anotherlabel=b2"
  ],
  "7": [
    "cluster=a7",
    "anotherlabel=b7"
  ]
}

[mysticaltech]

@maggie44 Good idea, putting the node index as a label definitely makes sense. @Silvest89 Does that make sense for you too?

[Silvest89]

@mysticaltech
Yes, I think that could work. Saves the hassle of having empty objects in the array

[maggie44]

@Silvest89, you mentioned this would be fairly easy to implement. I have never written any Terraform before but reached the point I really need this feature. Any pointers to set me in the right direction would be appreciated.

[valkenburg-prevue-ch]

Hi all, if I may add my opinion: I think the need for this new feature comes from the challenges that come with the choice to refer to nodes and node pools by their indices in lists. I have the same issue, not regarding labels, but regarding replacing individual faulty nodes. I cannot replace a single node if it is one of the "count" in a nodepool. Kind of the same problem as the labels.

Shouldn't we think a bit harder about this, and try to find a (possibly backwards compatible) solution where nodepools aren't only determined by a count, but have a reference to individual nodes? For example, we could remove the count variable all together from one nodepool, and just demand a map of {node_label : [list of labels]}.

this way, one can label nodes differently in one nodepool, and one can delete a single node from a pool by removing its key:value pair. Under the hood, some rewriting needs to be done, such that the node's key propagates to its terraform key (which is now already a conversion between list positions and map keys, a la null_resource.agents["7-0-agent"]).

[valkenburg-prevue-ch]

SOrry, reading the original proposal and the discussion once more, I think what I proposing is already exactly the original proposal from @maggie44 , where @maggie44 's proposal is actually much better. I would definitely prefer that over the index-as-a-key-in-a-map solution that came out after the solution.

Also, it is not a breaking change! That's just a matter of coding the different input types for 'count' correctly.

[valkenburg-prevue-ch]

Let me know what you think, I went ahead and made one possible implementation. With documentation.

#1185

[valkenburg-prevue-ch]

The node name now is a concatenation of all the ingredients that go in there, at agents.tf https://github.com/kube-hetzner/terraform-hcloud-kube-hetzner/pull/1185/files#diff-7ffeeab0236a27eed0c5a6bac01500df55a12fc10b0bdfd94358eb6f03a2cb06

(hope that link works)

name = "${var.use_cluster_name_in_node_name ? "${var.cluster_name}-" : ""}${each.value.nodepool_name}${try(each.value.node_name_override, "")}"

where node_name_override is set in
https://github.com/kube-hetzner/terraform-hcloud-kube-hetzner/pull/1185/files#diff-9a576c4980b4f226b7ef1042670d564777bbb0ed8ae2d1df33afbd03e2991fcdR167

[maggie44]

The node name now is a concatenation of all the ingredients that go in there, at agents.tf https://github.com/kube-hetzner/terraform-hcloud-kube-hetzner/pull/1185/files#diff-7ffeeab0236a27eed0c5a6bac01500df55a12fc10b0bdfd94358eb6f03a2cb06

(hope that link works)

name = "${var.use_cluster_name_in_node_name ? "${var.cluster_name}-" : ""}${each.value.nodepool_name}${try(each.value.node_name_override, "")}"

where node_name_override is set in https://github.com/kube-hetzner/terraform-hcloud-kube-hetzner/pull/1185/files#diff-9a576c4980b4f226b7ef1042670d564777bbb0ed8ae2d1df33afbd03e2991fcdR167

Nice, that makes a lot of sense. And gave me some useful insight in to the workings of the whole thing, thanks for the links.

I think my vote then would be to use the key as the integer for appending to the IP, and ditching the unique_index_in_node_pool field. I suspect it would be easier to document and understand. Then the name field that already exists is used to identify each individual node just like before. It also then seems to be closer to parity with the node count form.

[maggie44]

P.S.

I think it's worth highlighting that your PR can solve all sorts of problems and improve the user experience in many many ways. I know you know that, but just wanted to make sure it didn't seem like that wasn't recognised. Really great stuff 👏

[valkenburg-prevue-ch]

ha, thanks for the kind works. Don't make me blush.

It's a small pr, standing on the shoulders of gians.

[valkenburg-prevue-ch]

I like the idea to ditch theunique_index_in_node_pool. Keys in maps in tf formally need to be strings, so we would need to do some casting left and right, but that should be ok.

The list of validation rules is going to be long, but that's worth it.