Move networking in nodeup to dedicated subpackage #9137
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
area/nodeup
size/XL
|
/retest |
|
This is turning into a real pain to review. It would have been much easier to cross-check if there were a commit per CNI builder. |
upup/pkg/fi/nodeup/command.go
Outdated
| loader.Builders = append(loader.Builders, &model.SysctlBuilder{NodeupModelContext: modelContext}) | ||
| loader.Builders = append(loader.Builders, &model.KubeAPIServerBuilder{NodeupModelContext: modelContext}) | ||
| loader.Builders = append(loader.Builders, &model.KubeControllerManagerBuilder{NodeupModelContext: modelContext}) | ||
| loader.Builders = append(loader.Builders, &model.KubeSchedulerBuilder{NodeupModelContext: modelContext}) | ||
| loader.Builders = append(loader.Builders, &model.EtcdManagerTLSBuilder{NodeupModelContext: modelContext}) | ||
| loader.Builders = append(loader.Builders, &model.KubeProxyBuilder{NodeupModelContext: modelContext}) | ||
| loader.Builders = append(loader.Builders, &model.EtcdTLSBuilder{NodeupModelContext: modelContext}) |
There was a problem hiding this comment.
EtcdTLSBuilder needs to be changed to not act when networking is not Calico.
There was a problem hiding this comment.
Perhaps rename it to CalicoBuilder.
There was a problem hiding this comment.
Good catch. Will look into this
There was a problem hiding this comment.
This one is a bit misleading. It creates certs named calico, but it is used by any provider that accesses legacy etcd with TLS enabled. For Romana, this seems to be broken, for cilium, I have removed support for this. Calico could probably work if the user works really hard to to configure this. Would have to use calico V2 ++. I think we can just leave this as is and rather work on deprecating legacy etcd further.
There was a problem hiding this comment.
I do not want to start creating that cert when it previously was not. The appending of EtcdTLSBuilder was previously conditional on networking being set to Calico. So either the appending should remain conditional or the builder should only act for Calico.
There was a problem hiding this comment.
Comment resolved by subsequent commit.
|
That review took way too long. For future refactors, please submit a series of commits, each designed to permit a reviewer to see when logic changes and when it does not. Please see #9130 as an example. |
|
Sorry for putting all of this into one commit. It was quite a journey cleaning all of this up and the original commits reflected that. |
|
/retest |
1 similar comment
|
/retest |
|
/retest |
k8s-ci-robot
added
the
lgtm
|
/retest |
k8s-ci-robot
removed
the
lgtm
|
Thanks for spotting those @rdrgmnzs |
| @@ -22,7 +22,7 @@ import ( | |||
| "k8s.io/kops/upup/pkg/fi" | |||
| ) | |||
|
|
|||
| // KuberouterBuilder installs kube-router | |||
| // Kubenet installs kube-router | |||
There was a problem hiding this comment.
| // Kubenet installs kube-router | |
| // KubenetBuilder installs kube-router. |
| "k8s.io/kops/upup/pkg/fi" | ||
| ) | ||
|
|
||
| // KubenetBuilder installs Kubenet networking provider |
There was a problem hiding this comment.
nit:
| // KubenetBuilder installs Kubenet networking provider | |
| // KubenetBuilder installs the Kubenet networking provider. |
k8s-ci-robot
added
the
lgtm
|
/assign @rdrgmnzs |
* remove portmap as it is not used by kubenet * use generalised function for checking whether a provider uses kubenet
EtcdTLSBuilder is now only used in legacy configurations of calico so renaming appropriatly
k8s-ci-robot
removed
the
lgtm
|
Thanks for all the work you put into this @olemarkus! /lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: olemarkus, rdrgmnzs 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
As discussed in a previous PR, the networking builders should be in its own package. It went a bit further than intended when trying to clean up the NetworkBuilder. The CNI plugin installations were a bit messy. If a CNI requires kops to install plugins, it will be handled in specific builders instead.