Skip to content
This repository has been archived by the owner on Oct 24, 2023. It is now read-only.

refactor: make kube-dns addon user-configurable #2393

Merged
merged 2 commits into from
Dec 6, 2019

Conversation

jackfrancis
Copy link
Member

@jackfrancis jackfrancis commented Dec 5, 2019

Reason for Change:

This PR makes the kube-dns component a user-configurable addon, exposed via the existing kubernetesConfig.addons interface.

The existing "disable if 1.12" functionality has been carried over via addons upgrade defaults enforcement. This is a temporary arrangement (though functionally equivalent to what we do at present) until coredns is similarly converted to a user-configurable addon. At which point, we should consider relaxing the strict "you may only run kube-dns < 1.12 and you must run coredns if > 1.11" requirement. I.e., we should give users a choice to use either kube-dns or coredns (but default to coredns, as 1.12 support is going away soon anyways).

Issue Fixed:

Requirements:

Notes:

@jackfrancis
Copy link
Member Author

/hold

@Azure Azure deleted a comment from codecov bot Dec 5, 2019
@codecov
Copy link

codecov bot commented Dec 6, 2019

Codecov Report

Merging #2393 into master will increase coverage by 0.03%.
The diff coverage is 64.28%.

@@            Coverage Diff             @@
##           master    #2393      +/-   ##
==========================================
+ Coverage   72.08%   72.11%   +0.03%     
==========================================
  Files         130      130              
  Lines       23341    23350       +9     
==========================================
+ Hits        16825    16840      +15     
+ Misses       5491     5488       -3     
+ Partials     1025     1022       -3

@jackfrancis
Copy link
Member Author

/hold release

@jackfrancis
Copy link
Member Author

/hold cancel

// First, Configure addons
cs.setAddonsConfig(isUpgrade)
// Defaults enforcement flows below inherit from addons configuration,
// so it's critical to enforce default addons configuration first
Copy link
Member

Choose a reason for hiding this comment

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

We're sure this comment enforce default addons configuration first is no longer true?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for noticing this, forgot to add commentary.

Yes, specifically, kube-dns inherits some of its configuration from the kubelet config (I think it's correct to say that the kubelet config is the source of truth). Before this change, the sources of truth were held by various actors during runtime template generation, and then eventually the potato was mashed via sed into the yaml on disk.

I did some investigation based on the comments about addons needing to be first and they were overblown. Basically, all of these serially enforced default flows have to arranged rationally, such that changes to one area that may affect another area are reconciled. (See the // Pool-specific defaults that depend upon OrchestratorProfile defaults comment above for an example.)

Unfortunately we don't have any useful graph interfaces for expressing this, and so we are doing it by manually ordering imperative commands, and then (hopefully) writing comprehensive tests to validate the various inter-dependent default enforcements behave the way we want them to.

:(

@mboersma
Copy link
Member

mboersma commented Dec 6, 2019

/lgtm

@acs-bot
Copy link

acs-bot commented Dec 6, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis, mboersma

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [jackfrancis,mboersma]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jackfrancis jackfrancis merged commit 691e3d3 into Azure:master Dec 6, 2019
@jackfrancis jackfrancis deleted the addons-kube-dns branch December 6, 2019 22:59
devigned pushed a commit to devigned/aks-engine that referenced this pull request Dec 9, 2019
@jackfrancis jackfrancis added this to the v0.45.0 milestone Dec 16, 2019
@jackfrancis
Copy link
Member Author

Related to #2251

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

Successfully merging this pull request may close these issues.

None yet

3 participants