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

Add support for additional cmdline options to K3s container #319

Merged

Conversation

Donnerbart
Copy link
Contributor

@Donnerbart Donnerbart commented Sep 17, 2024

See #163

I wanted to add similar support for KindContainer, but struggled a bit to find the according place to do so. I ended up with kubeadm init --patch=/kubeadm-patch but that cannot be combined with --config. But that's where the warnings and deprecations commit for KindContainer originates from (first clean up, then add new features). I could also drop it from this PR to separate it from the new K3s feature.

@dajudge dajudge self-assigned this Sep 18, 2024
@dajudge dajudge added the enhancement New feature or request label Sep 18, 2024
@Donnerbart Donnerbart force-pushed the feature/add-container-customization branch from ada2391 to ab32621 Compare September 18, 2024 10:23
@Donnerbart Donnerbart changed the title Add support for additional commands to K3s container Add support for additional cmdline options to K3s container Sep 18, 2024
@dajudge
Copy link
Owner

dajudge commented Sep 19, 2024

Thank you for incoporating the changes so quickly! 💪

I've pondered this approach a bit and I'm worried that in the current form we might be making the API too constrained for future extension.

I'm thinking towards a more generic way to modify the entire k3s command line right before container start - something along these lines.

I'd be super happy to hear your feedback on that approach!

@Donnerbart
Copy link
Contributor Author

Donnerbart commented Sep 20, 2024

I'm thinking towards a more generic way to modify the entire k3s command line right before container start - something along these lines.

I'd be super happy to hear your feedback on that approach!

The modifier approach comes with the benefit of full flexibility, but the cost of redundant configuration options and the risk to easily break the container start.

An example for the redundancy would be the node port range configuration. That has a dedicated configuration API, but you could also tinker with the string in the command modifier.

An example to break something would be to remove the server part in the command modifier. I'm also not sure if there is a real-life use-case to modify on the traefik or tls-san options without breaking anything.

So with the current default command I don't see the need for this modifier. And if you'd ever add something to the default command it's probably either needed or should get its own dedicated configuration API (because it's actually no fun to modify an existing command with regex or string replacements).

@dajudge
Copy link
Owner

dajudge commented Sep 23, 2024

Thank you for your feedback @Donnerbart, it provides some good points to be considered in this context! 🙏

Over the weekend I've pondered the issue a bit together w/ @bobrossthepainter and I believe we should move forward with the modifier approach.

Here's the reasoning:

The modifier approach comes with the benefit of full flexibility, but the cost of redundant configuration options and the risk to easily break the container start.

I don't think redundancy is a big problem here: the convenience functions can cover the most common use cases while the modifier approach gives developers a power tool to cover all other use cases that we haven't foreseen or haven't gotten around to.

An example to break something would be to remove the server part in the command modifier.

As for the risk of shooting your own foot: if one dives into modifying the k3s cmdline one probably should know what they're doing anyway.

I'm also not sure if there is a real-life use-case to modify on the traefik or tls-san options without breaking anything.

I guess that's the point here: we're simply not sure of the use-cases people might have (e.g. I would never have come up with all the ideas that popped up in #163). So while kindcontainer likes having an opinionated API for the most common use cases, at least having an API that can serve as a fallback in such cases (which to my knowledge is not present at the moment) is a net win in my book.

Would you be willing to integrate the changes into this PR @Donnerbart ?

@Donnerbart
Copy link
Contributor Author

No worries, I was mostly playing devil's advocate here, because I thought you might have enough pro arguments for the modifier approach.

I will update the PR in a minute.

@Donnerbart Donnerbart force-pushed the feature/add-container-customization branch from ab32621 to e2b56a1 Compare September 25, 2024 12:14
@Donnerbart Donnerbart force-pushed the feature/add-container-customization branch from e2b56a1 to 1286e73 Compare September 25, 2024 12:15
@dajudge dajudge merged commit 70b640c into dajudge:master Sep 25, 2024
44 checks passed
@Donnerbart Donnerbart deleted the feature/add-container-customization branch September 25, 2024 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants