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

[WIP] kubeadm: v1beta3 addons #84991

Closed
wants to merge 4 commits into from

Conversation

@rosti
Copy link
Member

rosti commented Nov 8, 2019

What type of PR is this?
/kind api-change

What this PR does / why we need it:

This change introduces a list of addons to the ClusterConfiguration.

It is required for a couple of reasons:

  • Users want to opt out of any of the kubeadm default addons. So far this has
    proven difficult and unreliable even with the help of phases.

  • kubeadm has to advance its config closer to what is expected to be required
    by the cluster addons project.

If omitted the list is initialized by default with kube-proxy and CoreDNS with
automatically selected tags and repositories.
Possible addons to install are kube-proxy, kube-dns and CoreDNS. The latter two
can also have their image repository and image tag overwritten.
If no addons are to be installed, a user can simply initialize the addon list
as an empty array.
The existing DNS fields in the ClusterConfiguration of older configs are
superseded by the more general addons field and not present in v1beta3.

Which issue(s) this PR fixes:

Refs kubernetes/kubeadm#1796 kubernetes/enhancements#970

Special notes for your reviewer:

The PR is a WIP as it needs some tests reenabled and a couple of new ones written. It is also a quick solution, given the fact, that kubeadm does not have an internal addon manager. Hence changes were necessary throughout kubeadm's code base.

Please, review the last commit only.
This PR depends on:

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
/area kubeadm
/priority important-soon
/assign @timothysc @fabriziopandini @neolit123

Does this PR introduce a user-facing change?:

kubeadm now supports specifying an addons list to be used in a cluster in its v1beta3 ClusterConfiguration

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 8, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rosti
To complete the pull request process, please assign ixdy
You can assign the PR to them by writing /assign @ixdy in a comment when ready.

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

Needs approval from an approver in each of these files:

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

// AddOn defines settings to be used when deploying a specific addon kind
type AddOn struct {
// Kind defines the addon type
Kind string

This comment has been minimized.

Copy link
@neolit123

neolit123 Nov 8, 2019

Member

Kind defines the addon type

should this be

Type AddonType

a courtesy of:

 	// Type defines the DNS add-on to be used
	Type DNSAddOnType

on Addon vs AddOn i'm in favor of using the former.
the correct spelling in english is "add-on", but kubernetes has usage of "addon" in a lot of places and it's not invalid spelling.

This comment has been minimized.

Copy link
@yastij

yastij Nov 8, 2019

Member

+1 for addon

This comment has been minimized.

Copy link
@fabriziopandini

fabriziopandini Nov 10, 2019

Member

Might be I'm missing something, but If this should be an enum, then how the user is expected to pass any addon different from what is the list?

Generalizing, How this is expected to translate into

apiVersion: addons.config.k8s.io/v1alpha1
kind: AddonInstallerConfiguration
addons:
- name: kube-proxy
  ref: github.com/kubernetes/kubernetes//cluster/addons/kustomize/kube-proxy/?ref=v1.17.0

This comment has been minimized.

Copy link
@neolit123

neolit123 Nov 10, 2019

Member

Might be I'm missing something, but If this should be an enum, then how the user is expected to pass any addon different from what is the list?

my impression is that this feature was targeting only core kubeadm addons.
for non-core addons users can always kubeactl apply as a post step.

This comment has been minimized.

Copy link
@rosti

rosti Nov 11, 2019

Author Member

At this point this is a rough translation of what we have into what we need.
My idea here is to allow for a few kubeadm concerned settings to be tweaked (currently image tag and repo). The addon manager and addon deployment code will do the rest.

This comment has been minimized.

Copy link
@rosti

rosti Nov 11, 2019

Author Member

P.S. This is deliberately not going to be an enum. We don't know what addons are going to be supported by the addons installer, hence we don't want to restrict the user here.

This comment has been minimized.

Copy link
@neolit123

neolit123 Nov 11, 2019

Member

commented on another location in the PR, IMO/AFAIK we would want the addon installer to not embed it's config in the kubeadm config.

Kind string

// ImageMeta allows to customize the image used for the addon
ImageMeta

This comment has been minimized.

Copy link
@neolit123

neolit123 Nov 8, 2019

Member

just wanting to get some discussion on how we are going to handle upgrades for these.
until now:

  • kube-proxy versions is the k8s version
  • coredns version is pinned in kubeadm

if the user feeds custom values, on upgrade via ImageMeta do we:

  • override the user tags with the sane defaults from the latest kubeadm?
    or
  • if they are modified tags/repo leave them as is and leave it to the user to patch the deployments and the kubeadm CM post upgrade? we just merged a coredns PR for the migration to skip if the tag or CM is unknown.

a side question, kube-proxy support X-1 skew? e.g. k8s at 1.17, but kube-proxy at 1.16.

This comment has been minimized.

Copy link
@yastij

yastij Nov 8, 2019

Member

kube-proxy support the skew IIRC. Although, we might want to keep it in sync with kubelet since they both share the KUBE-MARK-DROP and KUBE-MARK-MASQ iptable chains

This comment has been minimized.

Copy link
@rosti

rosti Nov 11, 2019

Author Member

Right now ImageMeta in the kube-proxy case is completely ignored. No behavior change is introduced in this kubeadm version. ImageMeta is acknowledged only for CoreDNS and kube-dns.

This comment has been minimized.

Copy link
@neolit123

neolit123 Nov 11, 2019

Member

ImageMeta is acknowledged only for CoreDNS and kube-dns.

ok, this goes to the question from my comment above:

if the user feeds custom values, on upgrade via ImageMeta do we:

we need to figure out what to do here. i think my vote is for the second option.

@@ -198,6 +180,15 @@ type ImageMeta struct {
//TODO: evaluate if we need also a ImageName based on user feedbacks

This comment has been minimized.

Copy link
@neolit123

neolit123 Nov 8, 2019

Member

i don't think we got any strong requests for this one.

This comment has been minimized.

Copy link
@ereslibre

ereslibre Nov 11, 2019

Member

If we want folks to use their own hyperkube image, a possibility would be to remove the boolean (already discussed elsewhere), and introduce ImageName, so they can force hyperkube on the Kubernetes components, hence having a similar feature.

However, if we don't want so support this feature at all because we don't have strong evidence about users wanting this, I'm fine to drop all comments on our types (this comment comes from other older types as well).

This comment has been minimized.

Copy link
@neolit123

neolit123 Nov 11, 2019

Member

imagename was mainly not supported because we don't know how to handle upgrades with it, there are also custom tags.

@@ -21,7 +21,7 @@ import (
"reflect"
"testing"

kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
"k8s.io/kubernetes/cmd/kubeadm/app/constants"

This comment has been minimized.

Copy link
@neolit123

neolit123 Nov 8, 2019

Member

i think i like this change.

_, kubeDNS := cfg.AddOns[kubeadmconstants.KubeDNS]

if coreDNS && kubeDNS {
return errors.Errorf("multiple DNS addons specified in config")

This comment has been minimized.

Copy link
@neolit123

neolit123 Nov 8, 2019

Member

errors.New

@@ -47,6 +47,11 @@ const (

// EnsureProxyAddon creates the kube-proxy addons
func EnsureProxyAddon(cfg *kubeadmapi.ClusterConfiguration, localEndpoint *kubeadmapi.APIEndpoint, client clientset.Interface) error {
// Do we have to skip kube-proxy?
if _, ok := cfg.AddOns[constants.KubeProxy]; !ok {
return nil

This comment has been minimized.

Copy link
@neolit123

neolit123 Nov 8, 2019

Member

wondering if we should print V>0 messages for such skips.

// DNSType describes the type of DNS add-on used in the cluster.
DNSType kubeadmapi.DNSAddOnType
// DNSKind describes the type of DNS add-on used in the cluster.
DNSKind string

This comment has been minimized.

Copy link
@neolit123

neolit123 Nov 8, 2019

Member

type was fine IMO, added comments above about kind vs type.

This comment has been minimized.

Copy link
@rosti

rosti Nov 11, 2019

Author Member

I am having some long term concerns here. What if at some point we move to cluster addons here and we are passing a GVK with a group fixed for the cluster addons project? We'll end up with different kinds, that will sound something similar to the component name they are deploying.
Hence, that caused the change from type to kind.

This comment has been minimized.

Copy link
@neolit123

neolit123 Nov 11, 2019

Member

IMO if the cluster-addons project integrates with kubeadm it's config will not be embeded in the kubeadm-config. this was what i gathered from initial discussions.

  • using json / yaml multi-doc.
@@ -111,17 +111,28 @@ func PerformPostUpgradeTasks(client clientset.Interface, cfg *kubeadmapi.InitCon

func removeOldDNSDeploymentIfAnotherDNSIsUsed(cfg *kubeadmapi.ClusterConfiguration, client clientset.Interface, dryRun bool) error {

This comment has been minimized.

Copy link
@neolit123

neolit123 Nov 8, 2019

Member

hoping that one day we can deprecate kube-dns.
we hardly support it in terms of being in contact with the devs and i haven't seen users since coredns went GA.

func RunCoreDNSMigrationCheck(client clientset.Interface, ignorePreflightErrors sets.String, dnsType kubeadmapi.DNSAddOnType) error {
if dnsType != kubeadmapi.CoreDNS {
return nil
}

This comment has been minimized.

Copy link
@neolit123

neolit123 Nov 8, 2019

Member

i don't think we need these migration checks for kube-dns.

This comment has been minimized.

Copy link
@rosti

rosti Nov 11, 2019

Author Member

Probably yes, but it's too late in the cycle to do anything about them.

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Nov 8, 2019

thanks @rosti this seems fine. added some comments.

// AddOn defines settings to be used when deploying a specific addon kind
type AddOn struct {
// Kind defines the addon type
Kind string

This comment has been minimized.

Copy link
@yastij

yastij Nov 8, 2019

Member

+1 for addon

Kind string

// ImageMeta allows to customize the image used for the addon
ImageMeta

This comment has been minimized.

Copy link
@yastij

yastij Nov 8, 2019

Member

kube-proxy support the skew IIRC. Although, we might want to keep it in sync with kubelet since they both share the KUBE-MARK-DROP and KUBE-MARK-MASQ iptable chains

cmd/kubeadm/app/cmd/phases/util.go Show resolved Hide resolved
cmd/kubeadm/app/cmd/token.go Show resolved Hide resolved
cmd/kubeadm/app/cmd/token_test.go Show resolved Hide resolved
}

This comment has been minimized.

Copy link
@yastij

yastij Nov 8, 2019

Member

nit: remove this

Copy link
Member

fabriziopandini left a comment

@rosti Thanks for trying to push this effort forward. Really appreciated.

My major concert is not around the current PR, but the overall story about the Addon project integration.

There are still so many open questions around the UX on the KEP Install Addons via kubeadm, that is almost impossible for me to understand if this PR is going in the right direction or not. Therefore I'm personally inclined to a "hold" until there is more clarity around this topic. But of course, I'm open to discussing this and to align with the majority.

Kind string

// ImageMeta allows to customize the image used for the addon
ImageMeta

This comment has been minimized.

Copy link
@fabriziopandini

fabriziopandini Nov 10, 2019

Member

I'm not sure assuming all the addons have only one image is correct (e.g. kube-dns has three images)
Additionally, despite asking this several times, it is not yet clear to how we are going to pass this to the addon project, because the assumption to "use kustomize" implies kubeadm should know the internal (e.g. the name of the Deployments/the name of the container) of every possible addon which is not practically possible

This comment has been minimized.

Copy link
@rosti

rosti Nov 11, 2019

Author Member

That's the reason we introduced ImageMeta as it is in the first place. It allows users to change the image repo and tags, but leave the exact name to kubeadm to manage. In the usual case, all of the images to be used will be on the same repo and even tagged in the same manner. If not, that's easy to fix.
But I see your concern here. We might want to think about this a bit more.

// AddOn defines settings to be used when deploying a specific addon kind
type AddOn struct {
// Kind defines the addon type
Kind string

This comment has been minimized.

Copy link
@fabriziopandini

fabriziopandini Nov 10, 2019

Member

Might be I'm missing something, but If this should be an enum, then how the user is expected to pass any addon different from what is the list?

Generalizing, How this is expected to translate into

apiVersion: addons.config.k8s.io/v1alpha1
kind: AddonInstallerConfiguration
addons:
- name: kube-proxy
  ref: github.com/kubernetes/kubernetes//cluster/addons/kustomize/kube-proxy/?ref=v1.17.0
// DNS defines the options for the DNS add-on installed in the cluster.
DNS DNS
// AddOns defines a map of addons deployed in this cluster. The key is the addon kind.
AddOns map[string]AddOn

This comment has been minimized.

Copy link
@fabriziopandini

fabriziopandini Nov 10, 2019

Member

While it is clear to me that this enable user to customize the list of addons, I'm still missing if/how this will be reflected on how the user are expected to pass addons configurations.
Today this is handled inconsistently in kubeadm:

  • kube-proxy via component config
  • CoreDNS & kube-DNS not supported
    I would like to see this somehow harmonized and made consistent, but TBH I don't understand if this is under the kubeadm responsibility or under the addon installer responsibility

This comment has been minimized.

Copy link
@neolit123

neolit123 Nov 10, 2019

Member

@rosti should confirm, but i'm seeing this feature as a stepping stone before kubeadm transition into using the addon installer types eventually a this getting deprecation.

but if that's the case this is weird because with each iteration of our API which should be approaching stability. Removing the list of addons later on and delegating it to an addon installer API will probably break users.

allowing addon manifest customization goes back to my proposal from 2018, which was rejected due to upgrade complications.

This comment has been minimized.

Copy link
@rosti

rosti Nov 11, 2019

Author Member

My current idea about this when the cluster addon installer is ready is the following:

apiVersion: kubeadm.k8s.io/v1beta4
kind: ClusterConfiguration
addons:
- kind: KubeProxy  # This is a complete GVK, forwarded as is to the cluster addons installer
  apiVersion: addons.config.k8s.io
  ...
---
# The component config (as is done currently by kubeadm), forwarded to the cluster addons installer as a component config
apiVersion: kubeproxy.config.k8s.io/v1alpha1
kind: KubeProxyConfiguration
...

It might not be ideal though. We need more discussion around this.

rosti added 4 commits Oct 11, 2019
Signed-off-by: Rostislav M. Georgiev <rostislavg@vmware.com>
Some other components (like Cluster API) embed kubeadm types in their configs.
This causes some problems as the API server would perform strict type checking
and would complain about missing mandatory fields, that are in fact optional.
Previously, optional fields in kubeadm types were identified by the `omitempty`
JSON tag. With this patch, all such fields are getting the `+optional`
annotation.

Signed-off-by: Rostislav M. Georgiev <rostislavg@vmware.com>
Up until now, kubeadm's config types lacked ObjectMeta. This began to cause
some compatibility issues mostly with Kustomize and projects using it (such as
kind).
However, using a full blown `metav1.ObjectMeta` proves problematic too.
It contains way too many fields, that are not relevant to kubeadm, it clobbers
the structs in which it's embedded with unnecessary fields and it generates a
lot of unnecessary output when marshalling types to YAML (such as always nil
`DeletionTimestamp`). Some tests (such as the fuzzer) are also broken and need
unwieldy patches to get them fixed.
As the only needed field out of `metav1.ObjectMeta` is `Name`, we introduce and
leverage on a reduced kubeadm v1beta3 local stripped down version of
ObjectMeta, that has only a `Name` field.
The new type is embedded in InitConfiguration, JoinConfiguration and
ClusterConfiguration. Most notably, it's not embedded in ClusterStatus as this
type should not be used by anything else, other than kubeadm.
The InitConfiguration and JoinConfiguration fields do nothing with ObjectMeta
and don't store it anywhere.
In the ClusterConfiguration, `metadata.name` replaces the `clusterName` field.
The latter is therefore removed. Hence, the following v1beta2
ClusterConfiguration is upgraded to v1beta3 as such:

```yaml
apiVersion: kubeadm.k8s.io/v1beta2
kind: ClusterConfiguration
clusterName: LeCluster
```

```yaml
apiVersion: kubeadm.k8s.io/v1beta3
kind: ClusterConfiguration
metadata:
  name: LeCluster
```

Signed-off-by: Rostislav M. Georgiev <rostislavg@vmware.com>
This change introduces a list of addons to the ClusterConfiguration.

It is required for a couple of reasons:
- Users want to opt out of any of the kubeadm default addons. So far this has
  proven difficult and unreliable even with the help of phases.

- kubeadm has to advance its config closer to what is expected to be required
  by the cluster addons project.

If omitted the list is initialized by default with kube-proxy and CoreDNS with
automatically selected tags and repositories.
Possible addons to install are kube-proxy, kube-dns and CoreDNS. The latter two
can also have their image repository and image tag overwritten.
If no addons are to be installed, a user can simply initialize the addon list
as an empty array.
The existing DNS fields in the ClusterConfiguration of older configs are
superseded by the more general addons field and not present in v1beta3.

Signed-off-by: Rostislav M. Georgiev <rostislavg@vmware.com>
@rosti rosti force-pushed the rosti:kubeadm-v1beta3-addons branch from 50bad69 to c08f54a Nov 11, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 11, 2019

@rosti: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-verify c08f54a link /test pull-kubernetes-verify

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@rosti

This comment has been minimized.

Copy link
Member Author

rosti commented Nov 12, 2019

Closing this for now, as the approach does not seem like a final one and can definitely be done in a less rushed and more sensible way after a wider discussion and much deliberation (especially with cluster addons folks).

@rosti rosti closed this Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.