Conversation
| "joinToken": token, | ||
| } | ||
|
|
||
| if err := ctrlClient.Patch(ctx, existing, patch); err != nil { |
There was a problem hiding this comment.
Since the old implementation used Update(), and that is generally considered better practice, why is it necessary and important now to use Patch() instead?
| // TeleportKubeTokenValidity is the TTL for all join tokens generated by this | ||
| // operator. Reduced from 720h to 1h to limit the blast radius of a leaked token. | ||
| // Tokens are renewed proactively via TokenRenewalThreshold before they expire. | ||
| TeleportKubeTokenValidity = 1 * time.Hour |
There was a problem hiding this comment.
I like 1hr in theory, but I want to be sure I understand the scope here. This is the validity of the WC join token, right? If teleport-operator loses connectivity for 6 hours (but WCs are still connected), does this mean WCs will be disconnected, or do they use a different token after the initial join?
I'm worried about what happens in an incident or network outage, so I just want to make sure this won't boot WCs out and will self-heal if TO can't reach teleport for some reason for a few hours. Once it regains connectivity, would oncallers need to manually do anything to restore TKA access?
| // to make the name unique across cluster re-creations with the same name. | ||
| // An empty clusterUID falls back to the legacy two-component format to preserve | ||
| // backwards compatibility with already-registered clusters. | ||
| func GetRegisterName(managementClusterName, clusterName, clusterUID string) string { |
There was a problem hiding this comment.
What is the change to GetRegisterName() for?
This seems extremely likely to break things. For example, tsh kube login would then need to know the uid of the target cluster, right?
| if !cluster.DeletionTimestamp.IsZero() { | ||
| // Delete teleport token for the cluster | ||
| if err := r.Teleport.DeleteToken(ctx, log, registerName); err != nil { | ||
| // Read token names from K8s resources before deleting them so we can |
There was a problem hiding this comment.
just to clarify - all the deletion changes besides using DeleteByTokenName are only necessary if changing the register name, right? Or is there a second purpose here?
| // secretLabels are applied to every Secret this operator manages so that | ||
| // ownership can be determined without relying on OwnerReferences, and so | ||
| // that secrets can be scoped to a specific cluster UID for auditability. | ||
| secretLabels := map[string]string{ | ||
| "app.kubernetes.io/managed-by": "teleport-operator", | ||
| "teleport-operator/cluster-uid": string(cluster.UID), | ||
| } | ||
|
|
||
| // Check and update Secret if necessary. | ||
| // The Secret uses the same roles as the ConfigMap (kube + optionally app) | ||
| // because the node role is not required for kube-agent registration and | ||
| // granting excess roles violates least-privilege. |
There was a problem hiding this comment.
- What is the difference between the ConfigMap and Secret that we create here?
- If we're adding labels, let's apply the same labels to both CM and Secret
2 participants
Towards: https://github.com/giantswarm/giantswarm/issues/35198
What this PR does / why we need it
This PR:
Checklist