Add dynamic windows desktops #46738

probakowski · 2024-09-18T20:57:51Z

This change adds new resource: dynamic windows desktop.
The difference between this new type and current windows desktop is that it doesn't have host UUID - it is meant to be picked up by windows desktop services by matching labels similar to dynamic registration of apps and databases.

…r-resources4

zmb3

This is a pretty big PR. It might make sense to do the proto changes in a dedicated. PR.

zmb3 · 2024-09-18T20:59:03Z

api/proto/teleport/legacy/client/proto/authservice.proto

+// desktop host. If HostID is not specified, all Windows desktops with
+// specified Name will be deleted


Remove the info about HostID, as there is no HostID here.

zmb3 · 2024-09-18T21:00:00Z

api/proto/teleport/legacy/types/types.proto

+}
+
+// DynamicWindowsDesktopFilter are filters to apply when searching for windows desktops.
+message DynamicWindowsDesktopFilter {


Do we need to duplicate this? Can we use the existing filter message and just ignore the HostID?

zmb3 · 2024-09-18T21:00:56Z

api/types/desktop.go

+	if d.Spec.Addr == "" {
+		return trace.BadParameter("DynamicWindowsDesktopV1.Spec missing Addr field")
+	}
+
+	// We use SNI to identify the desktop to route a connection to,
+	// and '.' will add an extra subdomain, preventing Teleport from
+	// correctly establishing TLS connections.
+	if name := d.GetName(); strings.Contains(name, ".") {
+		return trace.BadParameter("invalid name %q: desktop names cannot contain periods", name)
+	}
+
+	d.setStaticFields()
+	if err := d.ResourceHeader.CheckAndSetDefaults(); err != nil {
+		return trace.Wrap(err)
+	}
+
+	if d.Spec.ScreenSize != nil {
+		if d.Spec.ScreenSize.Width > MaxRDPScreenWidth || d.Spec.ScreenSize.Height > MaxRDPScreenHeight {
+			return trace.BadParameter("invalid screen size %dx%d (maximum %dx%d)",
+				d.Spec.ScreenSize.Width, d.Spec.ScreenSize.Height, MaxRDPScreenWidth, MaxRDPScreenHeight)
+		}
+	}
+


This is a lot of copy-paste from the original desktop resource. I'm worried about the two diverging. Any suggestions for sharing more code?

zmb3 · 2024-09-18T21:03:38Z

api/types/desktop.go

+}
+
+// GetFieldVals returns list of select field values.
+func (s DynamicWindowsDesktops) GetFieldVals(field string) ([]string, error) {


What is this used for?

zmb3 · 2024-09-18T21:04:29Z

constants.go

@@ -262,6 +262,9 @@ const (
 	// ComponentWindowsDesktop is a Windows desktop access server.
 	ComponentWindowsDesktop = "windows_desktop"

+	// ComponentDynamicWindowsDesktop is a Windows desktop access server.


This comment is exactly the same as the one on ComponentWindowsDesktop. I would expect the godoc to help me understand what is different about a dynamic desktop.

Then again, if this is just used for logging, then you can probably use the original component field.

zmb3 · 2024-09-18T21:06:53Z

lib/config/fileconf.go

@@ -2385,7 +2385,8 @@ type WindowsDesktopService struct {
 	// HostLabels optionally applies labels to Windows hosts for RBAC.
 	// A host can match multiple rules and will get a union of all
 	// the matched labels.
-	HostLabels []WindowsHostLabelRule `yaml:"host_labels,omitempty"`
+	HostLabels       []WindowsHostLabelRule `yaml:"host_labels,omitempty"`
+	ResourceMatchers []ResourceMatcher      `yaml:"resources,omitempty"`


Add godoc for the new field?

rosstimothy

Agree with Zac, there is a lot going on here. It might be best to split this into PRs that operate on a single layer: proto, backend storage, rpc implementation, tctl, resource watcher, windows desktop server changes.

rosstimothy · 2024-09-19T12:16:22Z

api/proto/teleport/legacy/client/proto/authservice.proto

+  // GetDynamicWindowsDesktops returns all registered Windows desktop hosts matching the supplied filter.
+  rpc GetDynamicWindowsDesktops(types.DynamicWindowsDesktopFilter) returns (GetDynamicWindowsDesktopsResponse);


Can we use a List RPC instead of a GetAll RPC that returns an array of desktops? Is this separate RPC needed? Should these resource be Listed via ListResources or ListUnifiedResources?

rosstimothy · 2024-09-19T12:17:05Z

api/proto/teleport/legacy/client/proto/authservice.proto

+  rpc CreateDynamicWindowsDesktop(types.DynamicWindowsDesktopV1) returns (google.protobuf.Empty);
+  // UpdateDynamicWindowsDesktop updates an existing Windows desktop host.
+  rpc UpdateDynamicWindowsDesktop(types.DynamicWindowsDesktopV1) returns (google.protobuf.Empty);
+  // UpsertDynamicWindowsDesktop updates a Windows desktop host, creating it if it doesn't exist.
+  rpc UpsertDynamicWindowsDesktop(types.DynamicWindowsDesktopV1) returns (google.protobuf.Empty);


Create, Update, and Upsert should all return the resource instead of empty.

Suggested change

rpc CreateDynamicWindowsDesktop(types.DynamicWindowsDesktopV1) returns (google.protobuf.Empty);

// UpdateDynamicWindowsDesktop updates an existing Windows desktop host.

rpc UpdateDynamicWindowsDesktop(types.DynamicWindowsDesktopV1) returns (google.protobuf.Empty);

// UpsertDynamicWindowsDesktop updates a Windows desktop host, creating it if it doesn't exist.

rpc UpsertDynamicWindowsDesktop(types.DynamicWindowsDesktopV1) returns (google.protobuf.Empty);

rpc CreateDynamicWindowsDesktop(types.DynamicWindowsDesktopV1) returns (types.DynamicWindowsDesktopV1);

// UpdateDynamicWindowsDesktop updates an existing Windows desktop host.

rpc UpdateDynamicWindowsDesktop(types.DynamicWindowsDesktopV1) returns (types.DynamicWindowsDesktopV1);

// UpsertDynamicWindowsDesktop updates a Windows desktop host, creating it if it doesn't exist.

rpc UpsertDynamicWindowsDesktop(types.DynamicWindowsDesktopV1) returns (types.DynamicWindowsDesktopV1);

rosstimothy · 2024-09-19T12:18:11Z

api/proto/teleport/legacy/client/proto/authservice.proto

+  // DeleteAllDynamicWindowsDesktops removes all registered Windows desktop hosts.
+  rpc DeleteAllDynamicWindowsDesktops(google.protobuf.Empty) returns (google.protobuf.Empty);


This should not need to be exposed via an RPC. The main purpose of DeleteAll is to satisfy the cache interface, and that can all be done with the storage service.

Suggested change

// DeleteAllDynamicWindowsDesktops removes all registered Windows desktop hosts.

rpc DeleteAllDynamicWindowsDesktops(google.protobuf.Empty) returns (google.protobuf.Empty);

rosstimothy · 2024-09-19T12:20:06Z

api/proto/teleport/legacy/types/types.proto

+// DynamicWindowsDesktopV1 represents a dynamic windows host for desktop access.
+message DynamicWindowsDesktopV1 {


The name DynamicWindowsDesktopV1 made me initially think that something about this desktop was dynamic, not that it is a windows desktop host that is discovered dynamically. All of the comments refer to this as a dynamic windows desktop host - perhaps we should consider renaming this to WindowsDesktopHostV1?

rosstimothy · 2024-09-19T12:21:34Z

lib/auth/auth_with_roles.go

+func (a *ServerWithRoles) CreateDynamicWindowsDesktop(ctx context.Context, s types.DynamicWindowsDesktop) error {
+	if err := a.action(apidefaults.Namespace, types.KindDynamicWindowsDesktop, types.VerbCreate); err != nil {
+		return trace.Wrap(err)
+	}
+	return a.authServer.CreateDynamicWindowsDesktop(ctx, s)
+}
+
+// UpdateDynamicWindowsDesktop updates an existing dynamic windows desktop host.
+func (a *ServerWithRoles) UpdateDynamicWindowsDesktop(ctx context.Context, s types.DynamicWindowsDesktop) error {
+	if err := a.action(apidefaults.Namespace, types.KindDynamicWindowsDesktop, types.VerbUpdate); err != nil {
+		return trace.Wrap(err)
+	}
+
+	existing, err := a.authServer.GetDynamicWindowsDesktops(ctx,
+		types.DynamicWindowsDesktopFilter{Name: s.GetName()})
+	if err != nil {
+		return trace.Wrap(err)
+	}
+	if len(existing) == 0 {
+		return trace.NotFound("no dynamic windows desktops with Name %s", s.GetName())
+	}
+
+	if err := a.checkAccessToDynamicWindowsDesktop(existing[0]); err != nil {
+		return trace.Wrap(err)
+	}
+	if err := a.checkAccessToDynamicWindowsDesktop(s); err != nil {
+		return trace.Wrap(err)
+	}
+	return a.authServer.UpdateDynamicWindowsDesktop(ctx, s)
+}
+
+// UpsertDynamicWindowsDesktop updates a dynamic windows desktop resource, creating it if it doesn't exist.
+func (a *ServerWithRoles) UpsertDynamicWindowsDesktop(ctx context.Context, s types.DynamicWindowsDesktop) error {


Are these be called by humans or by a desktop_service? Should there be an admin actions check?

rosstimothy · 2024-09-19T12:34:10Z

lib/srv/desktop/discovery.go

+			select {
+			case desktops := <-watcher.DynamicWindowsDesktopsC:


Do we need to handle when the watcher is closed?

Suggested change

select {

case desktops := <-watcher.DynamicWindowsDesktopsC:

select {

case <-watcher.Done():

case desktops := <-watcher.DynamicWindowsDesktopsC:

rosstimothy · 2024-09-19T12:35:43Z

lib/srv/desktop/discovery.go

+	labels := make(map[string]string)
+	maps.Copy(labels, dynamicDesktop.GetAllLabels())
+	labels[types.OriginLabel] = types.OriginDynamic


Suggested change

labels := make(map[string]string)

maps.Copy(labels, dynamicDesktop.GetAllLabels())

labels[types.OriginLabel] = types.OriginDynamic

desktopLabels := dynamicDesktop.GetAllLabels()

labels := make(map[string]string, len(desktopLabels)+1)

maps.Copy(labels, desktopLabels)

labels[types.OriginLabel] = types.OriginDynamic

rosstimothy · 2024-09-19T12:37:16Z

tool/tctl/common/resource_command.go

+
+	fmt.Printf("labels %q", wd.GetAllLabels())
+
+	if err := client.UpsertDynamicWindowsDesktop(ctx, wd); err != nil {


This should call CreateDynamicWindowsDesktop unless --force has been provided.

rosstimothy · 2024-09-19T12:37:45Z

tool/tctl/common/resource_command.go

@@ -153,6 +153,7 @@ func (rc *ResourceCommand) Initialize(app *kingpin.Application, config *servicec
 		types.KindOktaImportRule:           rc.createOktaImportRule,
 		types.KindIntegration:              rc.createIntegration,
 		types.KindWindowsDesktop:           rc.createWindowsDesktop,
+		types.KindDynamicWindowsDesktop:    rc.createDynamicWindowsDesktop,


Please add an equivalent update handler that calls UpdatDynamicWindowsDesktop when tctl edit is used for this resource.

rosstimothy · 2024-09-19T12:39:47Z

tool/tctl/common/resource_command.go

@@ -882,6 +883,22 @@ func (rc *ResourceCommand) createWindowsDesktop(ctx context.Context, client *aut
 	return nil
 }

+func (rc *ResourceCommand) createDynamicWindowsDesktop(ctx context.Context, client *authclient.Client, raw services.UnknownResource) error {


Please also update the TestCreateResources and TestEditResources tests for this new resource

probakowski · 2024-10-18T19:04:10Z

It was splitted to smaller PRs, closing this one

probakowski added 13 commits September 9, 2024 20:47

initial proto and yaml

4253d25

interface

1a75966

service

23fef01

api

662ad47

tctl

57f74c3

???

f56d74c

Merge remote-tracking branch 'origin/master' into probakowski/registe…

afcb7ea

…r-resources4

add DynamicWindowsDesktop to events

62ffb68

startDynamicReconciler

a6c9231

move startDynamicReconciler

7eb58c2

refactoring

42f75e3

Merge remote-tracking branch 'origin/master' into probakowski/registe…

c7aad94

…r-resources4

cleanup

e752466

zmb3 reviewed Sep 18, 2024

View reviewed changes

zmb3 requested a review from rosstimothy September 18, 2024 21:09

rosstimothy reviewed Sep 19, 2024

View reviewed changes

probakowski closed this Oct 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add dynamic windows desktops #46738

Add dynamic windows desktops #46738

probakowski commented Sep 18, 2024

zmb3 left a comment

zmb3 Sep 18, 2024

zmb3 Sep 18, 2024

zmb3 Sep 18, 2024

zmb3 Sep 18, 2024

zmb3 Sep 18, 2024

zmb3 Sep 18, 2024

rosstimothy left a comment

rosstimothy Sep 19, 2024

rosstimothy Sep 19, 2024

rosstimothy Sep 19, 2024

rosstimothy Sep 19, 2024

rosstimothy Sep 19, 2024

rosstimothy Sep 19, 2024

rosstimothy Sep 19, 2024

rosstimothy Sep 19, 2024

rosstimothy Sep 19, 2024

rosstimothy Sep 19, 2024

probakowski commented Oct 18, 2024

		// desktop host. If HostID is not specified, all Windows desktops with
		// specified Name will be deleted

		// GetDynamicWindowsDesktops returns all registered Windows desktop hosts matching the supplied filter.
		rpc GetDynamicWindowsDesktops(types.DynamicWindowsDesktopFilter) returns (GetDynamicWindowsDesktopsResponse);

		// DeleteAllDynamicWindowsDesktops removes all registered Windows desktop hosts.
		rpc DeleteAllDynamicWindowsDesktops(google.protobuf.Empty) returns (google.protobuf.Empty);

		// DynamicWindowsDesktopV1 represents a dynamic windows host for desktop access.
		message DynamicWindowsDesktopV1 {


		fmt.Printf("labels %q", wd.GetAllLabels())

		if err := client.UpsertDynamicWindowsDesktop(ctx, wd); err != nil {

Add dynamic windows desktops #46738

Add dynamic windows desktops #46738

Conversation

probakowski commented Sep 18, 2024

zmb3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosstimothy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

probakowski commented Oct 18, 2024