Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Completeness of node configurations #863

Open
jonsch318 opened this issue Feb 19, 2025 · 5 comments
Open

Completeness of node configurations #863

jonsch318 opened this issue Feb 19, 2025 · 5 comments

Comments

@jonsch318
Copy link
Contributor

I was wondering about the completeness of the talconfig with respect to talos. There are multiple configuration keys that are not available in the talconfig format but they are available in the talos config.

This is obvously not a problem due to configuration patches that enable all those configs, but it is weird to have nearly all configs in talconfig.yaml and the few remaining rest in patches.

Here is the list of missing machine configs that I have encountered/use (all can be found [in the talos reference](https://www.talos.dev/v1.9/reference/configuration/v1alpha1/config/:

.machine.featues/* (for example hostDNS and stuff like this)
.machine.sysctls/*
.machine.time

.machine.kubelet/* (i use them for extraArgs and extraMounts)
.machine.network.kubespan

A question would be if there is a reason why there are not configurable in talconfig.yaml

Some of these should be the same on all nodes. Here patches work reasonably fine i think.
But for example .machine.sysctls is very similar to kernelModules in that it can easily be different on different nodes. I think it would only make sense to include these in talconfig and have a more complete mapping of talos config

@budimanjojo
Copy link
Owner

You're correct. The reasons vary, but here's the brief breakdown on why they're not in the configurations:

  • machine.features is handled by the upstream generate.WithVersionContract and it depends on the talosVersion you defined. For example, if you put talosVersion: v1.1.0 then the machine.features.stableHostname field will be filled, but not on v1.0.1 etc.
  • machine.sysctls, machine.time, and a lot more are not here because I didn't add them. It's a mess I know, I added too many things at the beginning and find that it's too much to keep adding it to the config field. My current view is it's better to use patches more and keep nodes with only required fields. Because it will end up with you want to add something just added upstream for alpha version, but it's not yet available in talhelper and you need to wait for me to add it scenario, which is not good experience.
  • machine.kubelet also depends on kubernetesVersion which will be handled by the upstream code so I prefer keeping it handled by upstream in case Talos wants to have extraArgs for specific kubernetes version and I don't want Talhelper to override it causing problem.

Hope this answers your question.

@jonsch318
Copy link
Contributor Author

sure does and I agree. Of course now it is a bit weird that e.g. nameserver config is in the config but time config is not etc.

For the rest i would wait until Talos implements the multi doc machine config and then do a breaking change organizing a bit.

We could also write about the other/missing configurations in the docs, so that one does not have to wonder if one missed something.


Though one extra thing.

I find the placement of additionalMachineCertSans as a general configuration and not as a node configuration problematic. I for one have add them to the nodes like this

patches:
  - |-
    - op: add
      path: /machine/certSANs
      value:
        - k8s.$TLD
        - infra2
        - infra2.p.$TLD
        - infra2.i.$TLD

where only the first is shared with all nodes and i think that is widely used? Wouldn't it make sense to move it to be a node config?
I could be wrong on this or I use talos wrongly :D

@budimanjojo
Copy link
Owner

I can add field for machine.time if you really need that. And sure I'll add to the docs about this later, probably just suggesting users to use patches for missing fields.

About placement of additionalMachineCertSans, it's a cluster input from upstream (inline with CP endpoint and cluster name) so I just followed them. https://github.com/siderolabs/talos/blob/79ee304e11df7cfb2ccc6eeeb39ab6112975db45/pkg/machinery/config/generate/generate.go#L30-L46

But now that you mentioned, yeah I should also have a certSANs field for node.

@jonsch318
Copy link
Contributor Author

jonsch318 commented Feb 20, 2025

For the time servers i dont know if it make sense. It is an easy patch

I can do it as well i have already added the time server config in my fork just need to test it and add tests.

I can try and add the certsSANs & docs as well and do a PR today or next days if you wish

@budimanjojo
Copy link
Owner

That would be awesome, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants