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

clusterDNS cannot change when DNS is enabled #379

Merged
merged 1 commit into from
Apr 26, 2024

Conversation

neoaggelos
Copy link
Contributor

@neoaggelos neoaggelos commented Apr 26, 2024

Summary

Found this while working on #375

k8s set dns.service-ip cannot be allowed to change the clusterDNS address when DNS is enabled, as this produces an error. Solves the following error:

// Apr 25 23:50:27 u1 k8s.k8sd[6839]: 2024/04/25 23:50:27 error updating the resource "coredns":
// Apr 25 23:50:27 u1 k8s.k8sd[6839]:          cannot patch "coredns" with kind Service: Service "coredns" is invalid: 
spec.clusterIPs[0]: Invalid value: []string{"10.152.183.168"}: may not change once set
// Apr 25 23:50:27 u1 k8s.k8sd[6839]: 2024/04/25 23:50:27 Looks like there are no changes for Deployment "coredns"
// Apr 25 23:50:27 u1 k8s.k8sd[6839]: 2024/04/25 23:50:27 warning: Upgrade "ck-dns" failed: cannot patch "coredns" with 
kind Service: Service "coredns" is invalid: spec.clusterIPs[0]: Invalid value: []string{"10.152.183.168"}: may not 
change once set
// Apr 25 23:50:27 u1 k8s.k8sd[6839]: 2024/04/25 23:50:27 failed to reconcile DNS configuration, will retry in 5 
seconds: failed to apply configuration: failed to apply DNS configuration: failed to apply coredns: failed to upgrade 
ck-dns: cannot patch "coredns" with kind Service: Service "coredns" is invalid: spec.clusterIPs[0]: Invalid value: []
string{"10.152.183.168"}: may not change once set

We still want to allow changing the clusterDNS service if DNS Is disabled (e.g. an external DNS is used)

Changes

  • Allow ClusterDNS updates only if DNS is disabled, similar to the handling of LocalStorage.LocalPath
  • Add extensive unit tests to verify this behaviour

@neoaggelos neoaggelos requested a review from a team as a code owner April 26, 2024 07:18
Copy link
Contributor

Package Line Rate
github.com/canonical/k8s/api/v1 35%
github.com/canonical/k8s/cmd/k8s-apiserver-proxy 0%
github.com/canonical/k8s/cmd/k8s 33%
github.com/canonical/k8s/cmd/k8sd 0%
github.com/canonical/k8s/cmd 0%
github.com/canonical/k8s/cmd/util 15%
github.com/canonical/k8s/pkg/client/dqlite 48%
github.com/canonical/k8s/pkg/k8s/client 0%
github.com/canonical/k8s/pkg/k8s/client/mock 8%
github.com/canonical/k8s/pkg/k8sd/api 13%
github.com/canonical/k8s/pkg/k8sd/api/impl 0%
github.com/canonical/k8s/pkg/k8sd/app 7%
github.com/canonical/k8s/pkg/k8sd/controllers 62%
github.com/canonical/k8s/pkg/k8sd/database 48%
github.com/canonical/k8s/pkg/k8sd/database/util 0%
github.com/canonical/k8s/pkg/k8sd/features 0%
github.com/canonical/k8s/pkg/k8sd/pki 52%
github.com/canonical/k8s/pkg/k8sd/setup 77%
github.com/canonical/k8s/pkg/k8sd/types 78%
github.com/canonical/k8s/pkg/proxy 6%
github.com/canonical/k8s/pkg/snap/mock 80%
github.com/canonical/k8s/pkg/snap 18%
github.com/canonical/k8s/pkg/snap/util 90%
github.com/canonical/k8s/pkg/utils 68%
github.com/canonical/k8s/pkg/utils/control 100%
github.com/canonical/k8s/pkg/utils/errors 100%
github.com/canonical/k8s/pkg/utils/k8s 69%
github.com/canonical/k8s/pkg/utils/node 0%
github.com/canonical/k8s/pkg/utils/shims 0%
Summary 37% (2429 / 6645)

Copy link
Contributor

@bschimke95 bschimke95 left a comment

Choose a reason for hiding this comment

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

LGTM

@neoaggelos neoaggelos merged commit 221656a into main Apr 26, 2024
14 checks passed
@neoaggelos neoaggelos deleted the fix/cluster-dns-update branch April 26, 2024 08:31
@addyess addyess changed the title clusterDNS cannot change will DNS is enabled clusterDNS cannot change when DNS is enabled Apr 26, 2024
@addyess
Copy link
Contributor

addyess commented Apr 26, 2024

@neoaggelos can we disable and change cluster dns at the same time?

canonical/k8s-operator#35

@neoaggelos
Copy link
Contributor Author

@neoaggelos can we disable and change cluster dns at the same time?

canonical/k8s-operator#35

Yes, that works just fine

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

Successfully merging this pull request may close these issues.

3 participants