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

feat: set default CoreDNS version #959

Merged
merged 10 commits into from
Nov 6, 2024

Conversation

dkoshkin
Copy link
Contributor

@dkoshkin dkoshkin commented Oct 30, 2024

What problem does this PR solve?:
Automatically upgrades the CoreDNS version. This is done by always setting dns.imageTag in KCP in the CoreDNS handler, based on the mapping to the cluster's Kubernetes version.

This component is different from etcd and kube-proxy that is also installed by kubeadm for a few different reasons.

  • an etcd upgrade is handled by kubeadm
  • kube-proxy, another "addon", is upgraded by CAPI core because its version will match the Kubernetes version

This functiona call is misleading and will only update the version if its set in KCP.

To not cause a rollout of all managed clusters by changing the defaults, this PR introduces a new API to opt in. To enable this functionality a client can set this new API like so for new clusters and during cluster upgrades:

spec:
  topology:
    variables:
      - name: clusterConfig
        value:
          dns:
            coreDNS: {}

Which issue(s) this PR fixes:
Fixes #

How Has This Been Tested?:

Special notes for your reviewer:

@github-actions github-actions bot added the fix label Oct 30, 2024
@dkoshkin dkoshkin force-pushed the dkoshkin/fix-default-coredns-version branch 6 times, most recently from 6053278 to ca8eaec Compare October 30, 2024 22:41
@dkoshkin
Copy link
Contributor Author

dkoshkin commented Oct 31, 2024

🤔 this would actually cause a rollout of new CP machines for all workload clusters after CAREN is updated. Maybe we should do this in a mutating webhook and set the version on the Cluster object?
A webhook won't really work either, we can't tell the difference between a default value and a user set value on an upgrade. Maybe some additional API to enable this functionality?

@jimmidyson
Copy link
Member

@dkoshkin I hope you don't mind but I pushed 07c7ebe which I think is a bit more readable but retains same logic (unchanged and passing tests).

@dkoshkin dkoshkin force-pushed the dkoshkin/fix-default-coredns-version branch 2 times, most recently from f455000 to 5279b62 Compare October 31, 2024 21:38
@github-actions github-actions bot added fix and removed fix labels Oct 31, 2024
@dkoshkin dkoshkin force-pushed the dkoshkin/fix-default-coredns-version branch from 5279b62 to 413e47f Compare October 31, 2024 21:43
@dkoshkin
Copy link
Contributor Author

dkoshkin commented Oct 31, 2024

@jimmidyson I would like your help on the CEL validation, I couldn't find any examples can you point me to some in the repo?

@dkoshkin dkoshkin changed the title fix: set default CoreDNS version feat: set default CoreDNS version Oct 31, 2024
@github-actions github-actions bot added feature and removed fix labels Oct 31, 2024
@dkoshkin
Copy link
Contributor Author

dkoshkin commented Nov 4, 2024

@jimmidyson I was thinking about this some more. Do we even need/want a new API type to define the upgrade strategy? (In that case I would revert the last commit and change the handler to return if dnsVar is empty)
What if having coreDNS: {} enables the auto update logic. This is similar to the addon lifecycle hooks that don't have any "enabled" API, the struct not being nil enables it.

Made the change in 6541992

Copy link
Member

@jimmidyson jimmidyson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

@dkoshkin dkoshkin force-pushed the dkoshkin/fix-default-coredns-version branch from b47e1b7 to 2f7da8d Compare November 5, 2024 22:52
@dkoshkin dkoshkin merged commit f0912e3 into main Nov 6, 2024
20 checks passed
@dkoshkin dkoshkin deleted the dkoshkin/fix-default-coredns-version branch November 6, 2024 00:06
@github-actions github-actions bot mentioned this pull request Nov 5, 2024
dkoshkin pushed a commit that referenced this pull request Nov 6, 2024
🤖 I have created a release *beep* *boop*
---


## 0.22.0 (2024-11-06)

<!-- Release notes generated using configuration in .github/release.yaml
at main -->

## What's Changed
### Exciting New Features 🎉
* feat: set default CoreDNS version by @dkoshkin in
#959
### Fixes 🔧
* fix: Use correct filename for runtime extensions component YAML by
@jimmidyson in
#960
### Other Changes
* docs: Update hugo and docsy by @jimmidyson in
#958
* refactor: Update helm registry initialization by @jimmidyson in
#961


**Full Changelog**:
v0.21.0...v0.22.0

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants