[v18] fix: enhance multi-resource editing in tctl edit

tool/tctl/common/edit_command.go

@@ -19,12 +19,14 @@
 package common
 
 import (
+	"bytes"
 	"context"
 	"crypto/sha256"
 	"encoding/hex"
 	"errors"
 	"fmt"
 	"io"
+	"log/slog"
 	"os"
 	"os/exec"
 	"strings"
@@ -149,11 +151,19 @@ func (e *EditCommand) editResource(ctx context.Context, client *authclient.Clien
 		return trace.Wrap(err)
 	}
 
-	originalName, err := resourceName(f.Name())
+	originalResources, err := editResources(f.Name())
 	if err != nil {
 		return trace.Wrap(err)
 	}
 
+	key := func(r services.UnknownResource) string {
+		return fmt.Sprintf("%s/%s", r.Kind, r.GetName())
+	}
+	originalResourcesMap := make(map[string][]byte)
+	for _, r := range originalResources {
+		originalResourcesMap[key(r)] = r.Raw
+	}
+
 	if err := e.runEditor(ctx, f.Name()); err != nil {
 		return trace.Wrap(err)
 	}
@@ -169,46 +179,51 @@ func (e *EditCommand) editResource(ctx context.Context, client *authclient.Clien
 		return nil
 	}
 
-	newName, err := resourceName(f.Name())
+	newResources, err := editResources(f.Name())
 	if err != nil {
 		return trace.Wrap(err)
 	}
 
-	if originalName != newName {
-		return trace.NotImplemented("renaming resources is not supported with tctl edit")
+	if len(newResources) != len(originalResources) {
+		return trace.BadParameter("one or more resources were added or removed, renaming resources is not supported with tctl edit")
 	}
 
-	f, err = utils.OpenFileAllowingUnsafeLinks(rc.filename)
-	if err != nil {
-		return trace.Wrap(err)
-	}
-	defer f.Close()
+	for _, newResource := range newResources {
+		// Ensure the resource name and kind was not changed.
+		origRaw, ok := originalResourcesMap[key(newResource)]
+		if !ok {
+			return trace.BadParameter("resource %s/%s was added or removed, renaming resources is not supported with tctl edit", newResource.Kind, newResource.Metadata.Name)
+		}
 
-	decoder := kyaml.NewYAMLOrJSONDecoder(f, defaults.LookaheadBufSize)
-	var raw services.UnknownResource
-	if err := decoder.Decode(&raw); err != nil {
-		if errors.Is(err, io.EOF) {
-			return trace.BadParameter("no resources found, empty input?")
+		if bytes.Equal(origRaw, newResource.Raw) {
+			// Nothing changed for this resource, continue to the next one.
+			slog.DebugContext(ctx, "Resource was not modified", "resource", key(newResource))
+			continue
 		}
-		return trace.Wrap(err)
-	}
 
-	// Use the UpdateHandler if the resource has one, otherwise fallback to using
-	// the CreateHandler. UpdateHandlers are preferred over CreateHandler because an update
-	// will not forcibly overwrite a resource unlike with create which requires the force
-	// flag to be set to update an existing resource.
-	updator, found := rc.UpdateHandlers[ResourceKind(raw.Kind)]
-	if found {
-		return trace.Wrap(updator(ctx, client, raw))
-	}
+		// Use the UpdateHandler if the resource has one, otherwise fallback to using
+		// the CreateHandler. UpdateHandlers are preferred over CreateHandler because an update
+		// will not forcibly overwrite a resource unlike with create which requires the force
+		// flag to be set to update an existing resource.
+		if updator, found := rc.UpdateHandlers[ResourceKind(newResource.Kind)]; found {
+			if err := updator(ctx, client, newResource); err != nil {
+				return trace.Wrap(err)
+			}
+			continue
+		}
 
-	// TODO(tross) remove the fallback to CreateHandlers once all the resources
-	// have been updated to implement an UpdateHandler.
-	if creator, found := rc.CreateHandlers[ResourceKind(raw.Kind)]; found {
-		return trace.Wrap(creator(ctx, client, raw))
+		// TODO(tross) remove the fallback to CreateHandlers once all the resources
+		// have been updated to implement an UpdateHandler.
+		if creator, found := rc.CreateHandlers[ResourceKind(newResource.Kind)]; found {
+			if err := creator(ctx, client, newResource); err != nil {
+				return trace.Wrap(err)
+			}
+			continue
+		}
+		return trace.BadParameter("updating resources of type %q is not supported", newResource.Kind)
 	}
 
-	return trace.BadParameter("updating resources of type %q is not supported", raw.Kind)
+	return nil
 }
 
 // getTextEditor returns the text editor to be used for editing the resource.
@@ -236,19 +251,26 @@ func checksum(filename string) (string, error) {
 	return hex.EncodeToString(h.Sum(nil)), nil
 }
 
-func resourceName(filename string) (string, error) {
+func editResources(filename string) ([]services.UnknownResource, error) {
 	f, err := utils.OpenFileAllowingUnsafeLinks(filename)
 	if err != nil {
-		return "", trace.Wrap(err)
+		return nil, trace.Wrap(err)
 	}
 	defer f.Close()
 
 	decoder := kyaml.NewYAMLOrJSONDecoder(f, defaults.LookaheadBufSize)
 
-	var raw services.UnknownResource
-	if err := decoder.Decode(&raw); err != nil {
-		return "", trace.Wrap(err)
-	}
+	names := make([]services.UnknownResource, 0)
+	for {
+		var raw services.UnknownResource
+		switch err := decoder.Decode(&raw); {
+		case errors.Is(err, io.EOF):
+			return names, nil
+		case err != nil:
+			return nil, trace.Wrap(err)
+		default:
+			names = append(names, raw)
+		}
 
-	return raw.GetName(), nil
+	}
 }

tool/tctl/common/edit_command_test.go

@@ -21,6 +21,7 @@ package common
 import (
 	"context"
 	"os"
+	"slices"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -147,7 +148,6 @@ func testEditGithubConnector(t *testing.T, clt *authclient.Client) {
 
 		collection := &connectorsCollection{github: []types.GithubConnector{expected}}
 		return trace.NewAggregate(writeYAML(collection, f), f.Close())
-
 	}
 
 	// Edit the connector and validate that the expected field is updated.
@@ -185,7 +185,6 @@ func testEditRole(t *testing.T, clt *authclient.Client) {
 
 		collection := &roleCollection{roles: []types.Role{expected}}
 		return trace.NewAggregate(writeYAML(collection, f), f.Close())
-
 	}
 
 	// Edit the role and validate that the expected field is updated.
@@ -225,7 +224,6 @@ func testEditUser(t *testing.T, clt *authclient.Client) {
 
 		collection := &userCollection{users: []types.User{expected}}
 		return trace.NewAggregate(writeYAML(collection, f), f.Close())
-
 	}
 
 	// Edit the user and validate that the expected field is updated.
@@ -263,7 +261,6 @@ func testEditClusterNetworkingConfig(t *testing.T, clt *authclient.Client) {
 
 		collection := &netConfigCollection{netConfig: expected}
 		return trace.NewAggregate(writeYAML(collection, f), f.Close())
-
 	}
 
 	// Edit the cnc and validate that the expected field is updated.
@@ -302,7 +299,6 @@ func testEditAuthPreference(t *testing.T, clt *authclient.Client) {
 
 		collection := &authPrefCollection{authPref: expected}
 		return trace.NewAggregate(writeYAML(collection, f), f.Close())
-
 	}
 
 	// Edit the cap and validate that the expected field is updated.
@@ -340,7 +336,6 @@ func testEditSessionRecordingConfig(t *testing.T, clt *authclient.Client) {
 
 		collection := &recConfigCollection{recConfig: expected}
 		return trace.NewAggregate(writeYAML(collection, f), f.Close())
-
 	}
 
 	// Edit the src and validate that the expected field is updated.
@@ -438,7 +433,6 @@ func testEditOIDCConnector(t *testing.T, clt *authclient.Client) {
 
 		collection := &connectorsCollection{oidc: []types.OIDCConnector{expected}}
 		return trace.NewAggregate(writeYAML(collection, f), f.Close())
-
 	}
 
 	// Edit the connector and validate that the expected field is updated.
@@ -507,7 +501,6 @@ func testEditSAMLConnector(t *testing.T, clt *authclient.Client) {
 
 		collection := &connectorsCollection{saml: []types.SAMLConnector{expected}}
 		return trace.NewAggregate(writeYAML(collection, f), f.Close())
-
 	}
 
 	// Edit the connector and validate that the expected field is updated.
@@ -691,3 +684,67 @@ func testEditDynamicWindowsDesktop(t *testing.T, clt *authclient.Client) {
 	expected.SetRevision(actual.GetRevision())
 	require.Empty(t, cmp.Diff(expected, actual, protocmp.Transform()))
 }
+
+func TestMultipleRoles(t *testing.T) {
+	t.Parallel()
+
+	ctx := context.Background()
+	log := logtest.NewLogger()
+	process, err := testenv.NewTeleportProcess(t.TempDir(), testenv.WithLogger(log))
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		require.NoError(t, process.Close())
+		require.NoError(t, process.Wait())
+	})
+	rootClient, err := testenv.NewDefaultAuthClient(process)
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = rootClient.Close() })
+
+	roleNames := []string{"test-role1", "test-role2"}
+	for _, name := range roleNames {
+		expected, err := types.NewRole(name, types.RoleSpecV6{})
+		require.NoError(t, err, "creating initial role resource")
+		_, err = rootClient.CreateRole(ctx, expected.(*types.RoleV6))
+		require.NoError(t, err, "persisting initial role resource")
+	}
+
+	roles, err := rootClient.GetRoles(ctx)
+	require.NoError(t, err)
+
+	editor := func(name string) error {
+		f, err := os.Create(name)
+		if err != nil {
+			return trace.Wrap(err, "opening file to edit")
+		}
+		for _, role := range roles {
+			if !slices.Contains(roleNames, role.GetName()) {
+				continue
+			}
+			role.SetLogins(types.Allow, []string{"abcdef"})
+		}
+
+		collection := &roleCollection{roles: roles}
+		return trace.NewAggregate(writeYAML(collection, f), f.Close())
+	}
+
+	// Edit the role and validate that the expected field is updated.
+	_, err = runEditCommand(t, rootClient, []string{"edit", "roles"},
+		withEditor(editor),
+	)
+	require.NoError(t, err, "expected editing role to succeed")
+
+	for _, role := range roles {
+		actual, err := rootClient.GetRole(ctx, role.GetName())
+		require.NoError(t, err, "retrieving role after edit")
+
+		assert.Equal(t, role.GetLogins(types.Allow), actual.GetLogins(types.Allow), "logins should have been modified by edit")
+		require.Empty(t, cmp.Diff(role, actual, cmpopts.IgnoreFields(types.Metadata{}, "Revision")))
+
+		switch {
+		case !slices.Contains(roleNames, role.GetName()):
+			require.Equal(t, role.GetRevision(), actual.GetRevision(), "revision should not have been modified by edit")
+		default:
+			require.NotEqual(t, role.GetRevision(), actual.GetRevision(), "revision should have been modified by edit")
+		}
+	}
+}