Add UpsertTrustedClusterV2 RPC #49789
Conversation
This supersedes UpsertTrustedCluster rpc. V2 performs resource name validation.
bernardjkim
added
the
no-changelog
|
If a trusted cluster is upserted and labels are added/changed/modified, would that also propagate the label change to the corresponding |
According to the documentation, labels are not support on |
- Remove unnecessary ping - Update error messages - Use skipNameValidation consts - Validate cluster name before establishing trust - Do not reveal cluster name in error message - Use BadParameter instead of CompareFailed
|
Thanks for putting this feature together! |
I believe it is already possible to update the role mapping of a TC without having to rejoin or having an active join token. This PR should not affect this behavior. |
public-teleport-github-review-bot
bot
removed the request for review
from gzdunek
|
I've updated the PR to also include |
We should probably replace all of the V2 RPCs introduced here with new RPCs to the trust service. |
bernardjkim
force-pushed
the
bernard/upsert-trusted-cluster-v2
branch
from
0401fcc to
4a4bb8d
Compare
|
I've moved the V2 RPCs to the trust service. I've left the logic in auth service for now to limit the size and scope of this PR. |
lib/auth/trustedcluster.go
Outdated
| if existingCluster == nil { | ||
| return a.createTrustedCluster(ctx, tc) | ||
| return a.createTrustedCluster(ctx, tc, validateName) | ||
| } | ||
|
|
||
| if err := existingCluster.CanChangeStateTo(tc); err != nil { | ||
| return nil, trace.Wrap(err) | ||
| } | ||
| updated, err := a.updateTrustedCluster(ctx, tc, existingCluster) | ||
| return updated, trace.Wrap(err) |
There was a problem hiding this comment.
Should we give this a handful of tries? UpsertFoo should basically never fail due to concurrent writes, but this logic is liable to fail on a race between an upsert and a delete, or two upserts on a new trusted cluster.
There was a problem hiding this comment.
That is a good point. I think we can defer this to a separate PR though. We'd also be modifying the behavior of the existing Upsert method.
|
Thanks for all the good feedback! |
|
@bernardjkim See the table below for backport results.
|
Closes #48309
Supports #22474
This PR introduces the
UpsertTrustedClusterV2rpc. This new rpc supersedesUpsertTrustedCluster. V2 performs resource name validation for trusted_clusters. This is required to support trusted_cluster resources with the kubernetes operator.Approaches Considered
Previously in #49169, there was discussion about introducing a new TrustedClusterV3 to address #48309. This approach could work, but it is unnecessary because V3 would not introduce any breaking changes in the resource itself. We would also need to be careful not to break backwards compatibility while introducing this new resource version.
We also considered using the resource origin or label to identify if resource name validation is required. This would allow the auth server to only validate the resource name if the trusted_cluster origin is kubernetes. However, this approach would not be able to ensure name validation if the kubernetes operator connects to an older auth server.
We've decided that a new endpoint was the more appropriate solution. This approach enables the auth server to ensure trusted_cluster resource names are validated when created by the kubernetes operator. The kubernetes operator would also not be able to bypass the resource name validation when connected to an older auth server.
Deprecation/Migration
No current timeline to migrate other clients to use the
UpsertTrustedClusterV2.UpsertTrustedClusterwill be supported indefinitely for now.