Add DynamicWindowsDesktop to proto #46984
Conversation
|
The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with |
probakowski
added
the
no-changelog
There was a problem hiding this comment.
I think we should follow most of the guidance defined in RFD 153 for this new resource. The only deviation that might be warranted here is to define the dynamic desktop resource type in the legacy types package so that it can still have gogo tags and be added to List[Unified]Resources.
However, I think the rest of the CRUD service would be better off defined in it's own service and follow the API semantics laid out in RFD 153 for consistency and to reduce the footprint of the auth service proto.
…r-resources-proto
|
@zmb3 @rosstimothy friendly ping |
| // DynamicWindowsDesktopV1 represents a dynamic windows host for desktop access. | ||
| message DynamicWindowsDesktopV1 { |
There was a problem hiding this comment.
Do we intend for this type to be added to any existing legacy APIs(Inventory Control Stream, List Resources, List Unified Resources, etc.)?
There was a problem hiding this comment.
We don't need it in List Resources and such because they are only used to create WindowsDesktops that will show up in those APIs already. We do want to add it to terraform provider and operator, though.
There was a problem hiding this comment.
What about heartbeats via the Inventory Control Stream? I'm asking because if it will not be heartbeated via the ICS or added to one of the ListResources variants, then this type should likely be defined outside of legacy/types/types and follow the guidance in RFD 153 with respect to resource naming and definition.
@hugoShaka has recently made changes so that RFD 153 resources are available in the TF provider, though I'm not sure if the operator supports them yet.
There was a problem hiding this comment.
For the operator there's open issue that prevents us from using 153-style resources there. That's one of the reasons to put this into legacy package.
There was a problem hiding this comment.
Do we need support for the operator immediately? Can it wait until we can properly support RFD 153 resources?
There was a problem hiding this comment.
We'd like to support Terraform and the Kube operator, but if I had to pick one then Terraform is probably more important.
I think it makes sense to keep dynamic registration as similar as we can across all resources. If Dynamic registration for apps and databases doesn't use 153-style resources then we might as well be consistent.
There was a problem hiding this comment.
IMO the only reason not to switch to a pure 153 resource is if the resource needs to be used in a legacy API. I'm fairly certain we'll want to heartbeat these via ICS so that we can support more than 998 of them, but I've not gotten an answer about that yet.
There was a problem hiding this comment.
I don't think that we will heartbeat on them TBH, they are meant to be created/managed by user (or terraform but still) not by our service so they probably don't need heartbeat unless I miss something
There was a problem hiding this comment.
In that case I'd vote for this to not be a legacy type but will yield if you all feel strongly it should belong here to not hold up progress.
Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>
…r-resources-proto
…r-resources-proto
public-teleport-github-review-bot
bot
removed the request for review
from atburke
* Add DynamicWindowsDesktop to proto * move rpc to separate server * e * remove dynamic windows from paginated resource * Update api/proto/teleport/legacy/types/types.proto Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com> --------- Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>
This PR adds new resource to proto files: DynamicWindowsDesktop.
It will be used for dynamic registration of windows desktops (#37103)
This is part of splitting #46738 into smaller, easier to review, PRs