search

kubernetes-retired / cluster-api-bootstrap-provider-kubeadm

LEGACY REPO. NEW CODE IS https://github.com/kubernetes-sigs/cluster-api/tree/master/bootstrap/kubeadm
Apache License 2.0
62 stars 67 forks source link

⚠️ Certificate extraction #222

Closed chuckha closed 4 years ago

chuckha commented 4 years ago

What this PR does / why we need it: This PR extracts certificate management from the Reconciler keeping the scope of the reconciler more focused.

It fixes #217. Related to #59

This does not support external etcd yet because of the way kubeadm works. There will be a following PR to add support for external etcd.

This is a BREAKING API change.

I would like to see this project move to a non-library layout as this code was not intended to used as a library and we are not versioning it as such.

vincepri commented 4 years ago

How does all of this relate to https://github.com/kubernetes-sigs/cluster-api/tree/master/util/certs?

chuckha commented 4 years ago

@vincepri I could use those and delete (a lot) more code!

chuckha commented 4 years ago

/hold

need to run some local tests, will update with results

ncdc commented 4 years ago

I wouldn't call the package internal (I'm trying to get us to move away from generic names like internal and util). You could do internal/certs?

chuckha commented 4 years ago

@ncdc the logic behind the name was that it wasn't complex enough for its own package and certs.Certificates stutters a bit.

What about hmm, cluster?

ncdc commented 4 years ago

Not that this is a valid justification, but there is at least context.Context as precedent.

I'm fine with internal/cluster for the time being, though!

chuckha commented 4 years ago

/hold cancel

Looks good from my end (both docker and aws provider 👍)

/cc @ncdc

chuckha commented 4 years ago

/assign @fabriziopandini

ncdc commented 4 years ago

I will try to review later today

fabriziopandini commented 4 years ago

/approve Pending for @ncdc answer to comment before lgtm

k8s-ci-robot commented 4 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chuckha, fabriziopandini

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](https://github.com/kubernetes-sigs/cluster-api-bootstrap-provider-kubeadm/blob/master/OWNERS)~~ [chuckha,fabriziopandini] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
ncdc commented 4 years ago

@fabriziopandini over to you for lgtm (the f2f is keeping me busy) - thanks!

fabriziopandini commented 4 years ago

/lgtm