⚠️ Certificate extraction #222
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
size/XXL
|
How does all of this relate to https://github.com/kubernetes-sigs/cluster-api/tree/master/util/certs? |
|
@vincepri I could use those and delete (a lot) more code! |
chuckha
force-pushed
the
cert-refactor
branch
3 times, most recently
from
5568e0b to
a467d8c
Compare
|
/hold need to run some local tests, will update with results |
k8s-ci-robot
added
the
do-not-merge/hold
|
I wouldn't call the package |
|
@ncdc the logic behind the name was that it wasn't complex enough for its own package and What about hmm, |
|
Not that this is a valid justification, but there is at least I'm fine with |
chuckha
force-pushed
the
cert-refactor
branch
from
a467d8c to
6a23e88
Compare
|
/hold cancel Looks good from my end (both docker and aws provider 👍) /cc @ncdc |
k8s-ci-robot
removed
the
do-not-merge/hold
|
/assign @fabriziopandini |
|
I will try to review later today |
chuckha
force-pushed
the
cert-refactor
branch
from
6a23e88 to
eb9ef5b
Compare
Signed-off-by: Chuck Ha <[email protected]>
chuckha
force-pushed
the
cert-refactor
branch
5 times, most recently
from
3daa5d8 to
0854478
Compare
chuckha
force-pushed
the
cert-refactor
branch
from
0854478 to
1e837f9
Compare
chuckha
force-pushed
the
cert-refactor
branch
from
1e837f9 to
595fd70
Compare
internal/cluster/certificates.go
Outdated
|
|
||
| // AsFiles converts a slice of certificates into bootstrap files. | ||
| func (c Certificates) AsFiles() []bootstrapv1.File { | ||
| clusterCA := c.GetByName(ClusterCAName) |
There was a problem hiding this comment.
What if this func is called before the certificates are read/generated?
Should we add a check to prevent this condition?
| } | ||
|
|
||
| // AsSecret converts a single certificate into a Kubernetes secret. | ||
| func (c *Certificate) AsSecret(cluster *clusterv1.Cluster, config *bootstrapv1.KubeadmConfig) *corev1.Secret { |
There was a problem hiding this comment.
What if this func is called before the certificates are read/generated?
Should we add a check to prevent this condition?
There was a problem hiding this comment.
We certainly could. Would it be ok to do this in a follow up issue so we can discuss behavior expectations?
It might be worth mentioning this package is internal so no one outside of CABPK will be able to use it, but I'm sure it would still be more friendly to return an error or produce an empty secret.
chuckha
force-pushed
the
cert-refactor
branch
from
595fd70 to
54bb858
Compare
Signed-off-by: Chuck Ha <[email protected]>
chuckha
force-pushed
the
cert-refactor
branch
from
54bb858 to
74d2ff2
Compare
|
[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:
Approvers can indicate their approval by writing |
|
@fabriziopandini over to you for lgtm (the f2f is keeping me busy) - thanks! |
|
/lgtm |
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.