Adding logic to Patch ETCD apiserver client crt secret on rollout #56
Conversation
|
LGTM, I'll wait for one more person to review this. |
pokearu
force-pushed
the
etcd-client-crt-renew
branch
from
8d1370a to
c16c9ec
Compare
| @@ -87,7 +88,16 @@ func (r *EtcdadmClusterReconciler) generateCAandClientCertSecrets(ctx context.Co | |||
| Generated: true, | |||
| } | |||
| s := apiServerClientCertKeyPair.AsSecret(client.ObjectKey{Name: cluster.Name, Namespace: cluster.Namespace}, *metav1.NewControllerRef(etcdCluster, etcdv1.GroupVersion.WithKind("EtcdadmCluster"))) | |||
| if err := r.Client.Create(ctx, s); err != nil && !apierrors.IsAlreadyExists(err) { | |||
| secretToPatch := s.DeepCopy() | |||
There was a problem hiding this comment.
i dont quite understand why we need to have a deep copy of the secret
There was a problem hiding this comment.
So basically its for how CreateOrPatch works. If the object is found, its expected that the changes needed should be redone in a call back func.
So on line 98 s.Data = secretToPatch.Data i am updating the secret data again using the deep copied object.
controllers/controller.go
Outdated
| // The default case is hit right after ScaleUp of ETCD nodes is completed and before the first CP comes up. | ||
| // If EtcdCertificatesAvailable is False, this means we need to update the certs. | ||
| // EtcdCertificatesAvailable is set to True once the certs are updated. | ||
| if conditions.Has(ep.EC, etcdv1.EtcdCertificatesAvailableCondition) && |
There was a problem hiding this comment.
do we need conditions.Has(ep.EC, etcdv1.EtcdCertificatesAvailableCondition)? or conditions.IsFalse(ep.EC, etcdv1.EtcdCertificatesAvailableCondition) is sufficient?
There was a problem hiding this comment.
Um that might work. I followed the pattern on top line 322 where we checked if conditions.Has(ep.EC, etcdv1.EtcdMachinesSpecUpToDateCondition) before setting to True.
But it might work with just IsFalse since it returns False if the condition is not found.
There was a problem hiding this comment.
yea it will return false if condition is nil so i dont think you need the Has check
| if conditions.Has(ep.EC, etcdv1.EtcdCertificatesAvailableCondition) && | ||
| conditions.IsFalse(ep.EC, etcdv1.EtcdCertificatesAvailableCondition) { | ||
| log.Info("Updating Etcd client certs") | ||
| if err := r.generateCAandClientCertSecrets(ctx, cluster, etcdCluster); err != nil { |
There was a problem hiding this comment.
the actual external etcd CA cert + key pair are not regenerated after the etcd machines are rolled out right?
There was a problem hiding this comment.
So the ETCD certs on the ETCD nodes do get updated but thats with etcdadm, its not related to what we do here.
That was working, the problem was only that the secret on the cluster was not updated and thereby the cert on the CP was not updated.
| // But if an object is found, the function expects to reconcile the fields we want patched in a callback func. | ||
| // Hence we keep a copy of the newly generated secret and update the secret Data field in a callback func. | ||
| // Ex; https://github.com/kubernetes-sigs/controller-runtime/blob/v0.14.5/pkg/controller/controllerutil/example_test.go | ||
| if _, err := controllerutil.CreateOrPatch(ctx, r.Client, s, func() error { |
There was a problem hiding this comment.
there is another secret create in
etcdadm-controller/controllers/certs.go
Line 120 in c16c9ec
what does it do and why we do not need to make similar change like this api-server one?
pokearu
force-pushed
the
etcd-client-crt-renew
branch
from
c16c9ec to
5faa2b2
Compare
|
/lgtm |
Description of changes:
When ETCD machines are rolled out, we did not update the ETCD API Server client cert secret on the cluster. Namely
<cluster-name>-apiserver-etcd-clientHence CAPI was not updating the etcd client cert on the CP nodes, which is found in path
/etc/kubernetes/pki/apiserver-etcd-client.crtThis prevented the certs on the CP from being renewed during node rollouts. This meant that once the certificates expired, the cluster would no longer be in a healthy state.
The changes modify the ETCD nodes rollout workflow to have a condition to patch the ETCD API server client cert secret on the cluster. This happens once after the new ETCD nodes have been rolled out and before the first CP node comes up.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.