Control plane node join #463

ashish-amarnath · 2018-12-25T02:40:48Z

What this PR does / why we need it:
Implements support for control plane node join

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #413

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

cluster-api-provider-aws now supports creating clusters with HA control plane using clusterctl phases.
Steps to create clusters with HA control plane are documented <Link to readme generated as a fix for #414 >

k8s-ci-robot · 2018-12-25T02:41:01Z

Hi @ashish-amarnath. Thanks for your PR.

I'm waiting for a kubernetes-sigs or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

ashish-amarnath · 2018-12-25T02:44:56Z

@chuckha Thanks for the pointers on #413
I took a stab at this to get the scaffolding and initial ideas in place.
Publishing this PR to get some feedback on the approach.

/assign @chuckha
cc @detiber

vincepri

Thanks for taking the lead on this! Left some early comments.

pkg/apis/awsprovider/v1alpha1/awsclusterproviderstatus_types.go

pkg/apis/awsprovider/v1alpha1/types.go

pkg/cloud/aws/actuators/machine/actuator.go

pkg/cloud/aws/actuators/machine_scope.go

pkg/cloud/aws/actuators/scope.go

pkg/cloud/aws/services/ec2/instances.go

chuckha

I think a little more discussion around why a counter is needed would help me understand this code more

pkg/cloud/aws/actuators/machine/actuator.go

ashish-amarnath · 2019-01-06T20:16:27Z

chuckha

This is definitely some good progress. Once the style errors are fixed I'll make another pass. Let me know when it's ready for testing and I'll run it through its paces.

pkg/cloud/aws/actuators/machine/actuator.go

pkg/cloud/aws/services/ec2/instances.go

detiber · 2019-01-09T15:42:13Z

pkg/cloud/aws/services/userdata/controlplane.go

@@ -55,6 +55,49 @@ nodeRegistration:
 EOF

 kubeadm init --config /tmp/kubeadm.yaml
+
+tar -cvzf /etc/kubernetes/pki.tar.gz /etc/kubernetes/pki/*
+kubectl -n kube-system --kubeconfig /etc/kubernetes/admin.conf create secret generic kubeadm-certs --from-file=/etc/kubernetes/pki.tar.gz


This is an interesting approach for managing the secrets, but it would probably be better if we took a different approach. I can see a couple of different ways of solving this without adding a secret post-init:

pre-generate the etcd ca keypair, front-proxy ca keypair, and sa(service account) keypair in the cluster controller and store in the Cluster ProviderSpec

inject them into the init config like we are currently doing with the CA cert/key

pre-write the keypairs to the filesystem before calling kubeadm join

if other certificates are needed prior to calling kubeadm join, pregenerate them using kubeadm phases (kubeadm join should only really require the CAs, it would be a bug otherwise)

Instead of modifying the cluster actuator, it would be possible to fetch the required certificates using the kubernetes client from within the machine actuator, then inject the certificates using the init script on the host prior to running kubeadm join.

i like the simplicity of this approach. can you elaborate on why you think it would be better if we took a different approach?

i'm guessing security issues? secrets aren't actually secret and it would be far too easy to get these secrets and do bad things. But it's not that different if we store the values in the cluster provider spec. That's still plaintext secret storage in etcd.

I would expect operators to use envelope encryption for secrets in their bootstrap / manager clusters, so it'll be justifiable to store ca certificates as secrets, less so as values in the provider spec.

We can't pre-generate the non-authoritative certs yet because we don't know the IP addresses of the nodes beforehand, and this is particularly important for etcd, but reasonable to do so for the certificate authorities and service account key-pair.

For a solution that will also support autoscaling groups, the ideal way would be use the instance identity document as a host attestation mechanism, where a verifiable CSR is made, that the controller can sign.

So, there are a couple of different reasons why I'm suggesting adding it to the provider spec:

It automatically provides an avenue for users to override the CA and SA keypairs to use custom values

While we are embedding the certificates in the spec today, we can later update the spec to allow for either embedding or referencing an external source at a later point

I want to make sure that we are considering UX when we start to look at more secure private key storage, by allowing direct embedding of the cert material, we avoid having to pre-generate any additional secrets as part of the workflow and we the spec is updated to support remote references for secure material (k8s secrets or leveraging something AWS native), we can have the actuator move the cert material to the proper place as part of the workflow, or even have an admission webhook that does the job to avoid ever storing the cert material in etcd).

Happy with this approach. So we need to change the spec a bit now, at least, to hold all the key pairs we need for the HA control plane?

I agree with the UX and the extendability aspect. However, IMO, we have a dependency on kubeadm here for generating the certs on when the first controlplane node runs the init. From reading the kubeadm docs, there seems to be a way to skip cert generation and run the other "phases". By doing this we will be deviating from the standard kubeadm init flow. Which, IMO, is OK as long as we call that out for UX.

@ashish-amarnath If the keypair files already exist on disk in the default locations that kubeadm would use for creating them and are not close to expiration they will be picked up and used by kubeadm without having to run phases.

Also, kubeadm will start handling the cert distribution for controlplane join in v.1.14. More info kubernetes/kubeadm#1200
With this in mind and we aren't GA yet, I think the cert distribution fix I have should be a stop-gap till we go to 1.14. WDYT?

Even if kubeadm handles the certificate copying for v1.14+ we would still benefit from having the ability for users to specify custom keypairs here.

I've decoupled the certs into their individual secrets so that they can be each updated independent of the other secrets.

+1 thanks for the reasoning @detiber. totally agree that changing the spec is the right way to go.

So, I propose that we keep the spec change out of this PR and get the controlplane machine join scenario worked out. I'll follow this PR with the spec change.

Created #492 to address this.

ashish-amarnath · 2019-01-14T07:02:20Z

/ok-to-test

k8s-ci-robot · 2019-01-14T07:02:46Z

@ashish-amarnath: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

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.

justaugustus · 2019-01-14T07:04:33Z

/ok-to-test

* pass kubeconfig into joining controlplane node userdata * use kubeconfig tofetch kubeadm-certs secret * create kubeadm generated certs as a secret in kube-system as kubeadm-certs * fetch kubeadm-certs secret on controlplane join machine and unpack it * run kubeadm join post kubeadm-cert secrets unpacking

pkg/cloud/aws/actuators/machine/actuator.go

chuckha · 2019-01-17T19:43:09Z

/ok-to-test

chuckha · 2019-01-17T19:58:52Z

/lgtm
/appove

chuckha · 2019-01-17T19:59:30Z

let's type better

/approve

k8s-ci-robot · 2019-01-17T19:59:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashish-amarnath, chuckha

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~~ [chuckha]

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

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 25, 2018

k8s-ci-robot requested review from chuckha and vincepri December 25, 2018 02:40

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 25, 2018

k8s-ci-robot assigned chuckha Dec 25, 2018

ashish-amarnath force-pushed the control-plane-node-join branch from 46ae5c7 to 20493a6 Compare December 31, 2018 16:59

vincepri reviewed Jan 1, 2019

View reviewed changes

chuckha reviewed Jan 2, 2019

View reviewed changes

ashish-amarnath closed this Jan 6, 2019

ashish-amarnath reopened this Jan 6, 2019

chuckha reviewed Jan 8, 2019

View reviewed changes

detiber reviewed Jan 9, 2019

View reviewed changes

chuckha mentioned this pull request Jan 10, 2019

Add machine actuator support for control plane join #413

Closed

ashish-amarnath force-pushed the control-plane-node-join branch from 99fed99 to 0f18c7f Compare January 11, 2019 10:06

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 11, 2019

ashish-amarnath changed the title ~~[WIP] Control plane node join~~ Control plane node join Jan 14, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 14, 2019

ashish-amarnath mentioned this pull request Jan 14, 2019

Include CA and service account keypairs in cluster spec. #492

Closed

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 14, 2019

refactor

bfb76da

ashish-amarnath added 17 commits January 14, 2019 19:57

remove debug logs

903e22d

fix copy-pasta

44980fc

save work

c83d0f0

save work before test

626c6d1

remove controlPlane type to store cluster control plane status

8ff04dc

save node join incomplete work

8e471d9

update controlplane join userdata

6ec5024

fixes and updates to controlplane join userdata

3a3d3b4

distribute only required certs

e79eccb

cleanup

8b4f61b

controlplane machines join nodes when no controlplane machine exists

73c5aec

make existing unit tests pass

0ab2d90

add '.vscode' to list of skipped dirs for header verification

ff1d321

refactor and add unit tests

3ca9e1e

remove stale comments

1e20d3e

use errors package and wrap errors

f660c5d

daniel-garcia reviewed Jan 16, 2019

View reviewed changes

pkg/cloud/aws/actuators/machine/actuator.go Outdated Show resolved Hide resolved

fix typo

66ecf7f

ashish-amarnath force-pushed the control-plane-node-join branch from c32aab2 to 66ecf7f Compare January 17, 2019 18:15

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 17, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 17, 2019

k8s-ci-robot merged commit b8ef509 into kubernetes-sigs:master Jan 17, 2019

ashish-amarnath deleted the control-plane-node-join branch January 17, 2019 20:04

ashish-amarnath mentioned this pull request Feb 11, 2019

REQUEST: New membership for ashish-amarnath kubernetes/org#465

Closed

6 tasks

ashish-amarnath mentioned this pull request Mar 5, 2019

Cluster deployer should support creation of clusters with HA controlplane kubernetes-sigs/cluster-api#682

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Control plane node join #463

Control plane node join #463

ashish-amarnath commented Dec 25, 2018 •

edited

Loading

k8s-ci-robot commented Dec 25, 2018

ashish-amarnath commented Dec 25, 2018

vincepri left a comment

chuckha left a comment

ashish-amarnath commented Jan 6, 2019 •

edited

Loading

chuckha left a comment

detiber Jan 9, 2019

chuckha Jan 9, 2019

randomvariable Jan 9, 2019

detiber Jan 10, 2019

randomvariable Jan 10, 2019

detiber Jan 10, 2019

ashish-amarnath Jan 10, 2019

chuckha Jan 10, 2019

ashish-amarnath Jan 11, 2019

ashish-amarnath Jan 14, 2019

ashish-amarnath commented Jan 14, 2019

k8s-ci-robot commented Jan 14, 2019

justaugustus commented Jan 14, 2019

chuckha commented Jan 17, 2019

chuckha commented Jan 17, 2019

chuckha commented Jan 17, 2019

k8s-ci-robot commented Jan 17, 2019

Control plane node join #463

Control plane node join #463

Conversation

ashish-amarnath commented Dec 25, 2018 • edited Loading

k8s-ci-robot commented Dec 25, 2018

ashish-amarnath commented Dec 25, 2018

vincepri left a comment

Choose a reason for hiding this comment

chuckha left a comment

Choose a reason for hiding this comment

ashish-amarnath commented Jan 6, 2019 • edited Loading

Current status:

ToDo:

Developer notes:

Testing

Test 1

Test 2

chuckha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashish-amarnath commented Jan 14, 2019

k8s-ci-robot commented Jan 14, 2019

justaugustus commented Jan 14, 2019

chuckha commented Jan 17, 2019

chuckha commented Jan 17, 2019

chuckha commented Jan 17, 2019

k8s-ci-robot commented Jan 17, 2019

ashish-amarnath commented Dec 25, 2018 •

edited

Loading

ashish-amarnath commented Jan 6, 2019 •

edited

Loading