From 9b3a86a00af61a8a4030fea60365853d54350c1a Mon Sep 17 00:00:00 2001 From: Tiago Silva Date: Mon, 8 Sep 2025 19:21:16 +0100 Subject: [PATCH 1/2] fix: enhance multi-resource editing in tctl edit This PR enhances tctl edit command to support multiple resources such as when editing all roles. `tctl edit roles` Now, all updated roles are reflected into the backend and not only the first one. Signed-off-by: Tiago Silva --- tool/tctl/common/edit_command.go | 94 ++++++++++++++++++++------------ 1 file changed, 58 insertions(+), 36 deletions(-) diff --git a/tool/tctl/common/edit_command.go b/tool/tctl/common/edit_command.go index a0704a5bcdab1..00eec18de8567 100644 --- a/tool/tctl/common/edit_command.go +++ b/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 + } } From 39131ca927c0c1fbce25bd8cfc12e34a0e7a10c0 Mon Sep 17 00:00:00 2001 From: Tiago Silva Date: Mon, 8 Sep 2025 21:22:02 +0100 Subject: [PATCH 2/2] add multi int tests --- tool/tctl/common/edit_command_test.go | 73 ++++++++++++++++++++++++--- 1 file changed, 65 insertions(+), 8 deletions(-) diff --git a/tool/tctl/common/edit_command_test.go b/tool/tctl/common/edit_command_test.go index 28599d2e79c83..90928cc49641f 100644 --- a/tool/tctl/common/edit_command_test.go +++ b/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") + } + } +}