Skip to content

Clean up system role parsing#9756

Merged
zmb3 merged 3 commits intomasterfrom
zmb3/role-parser-capitalization
Jan 14, 2022
Merged

Clean up system role parsing#9756
zmb3 merged 3 commits intomasterfrom
zmb3/role-parser-capitalization

Conversation

@zmb3
Copy link
Copy Markdown
Collaborator

@zmb3 zmb3 commented Jan 12, 2022

Be more consistent in what inputs we accept when referring to system roles, such as in the tctl tokens add call.

Also add missing test coverage for this parser.

Fixes #9752

@zmb3 zmb3 force-pushed the zmb3/role-parser-capitalization branch from 65356ab to 8f2ae19 Compare January 12, 2022 20:39
@zmb3 zmb3 marked this pull request as ready for review January 12, 2022 20:43
@zmb3 zmb3 requested a review from webvictim January 12, 2022 20:43
@zmb3 zmb3 changed the title Add tests for ParseTeleportRoles Clean up system role parsing Jan 12, 2022
@zmb3 zmb3 requested a review from smallinsky January 12, 2022 20:47
@zmb3 zmb3 force-pushed the zmb3/role-parser-capitalization branch from 8f2ae19 to 43bca0f Compare January 12, 2022 21:25
Comment thread api/types/system_role.go Outdated
RoleTrustedCluster, LegacyClusterTokenType,
RoleSignup, RoleProxy, RoleNop, RoleKube, RoleWindowsDesktop:
RoleSignup, RoleProxy, RoleRemoteProxy,
RoleNop, RoleKube, RoleWindowsDesktop:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This approach have one main drawback the roleMappings map needs to be updated each time when a new Role is added also with the Check() case

What would you say about something like:

func (r *SystemRole)  Check() error {
    m := make(map[string]struct{})
    for _, v := range roleMappings {
        if v == r {
            return nil
         }
     }
   return trace.BadParameter("role %v is not registered", *r)
} 

so only we care about roleMappings

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, take a look at the latest commit. I like this much better.

Comment thread api/types/system_role.go
"node": RoleNode,
"proxy": RoleProxy,
"admin": RoleAdmin,
"provisiontoken": RoleProvisionToken,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is provision_token a valid input?

Copy link
Copy Markdown
Collaborator Author

@zmb3 zmb3 Jan 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it never has been.

We're a little more tolerant and allow _ separators for things that are commonly typed on the command line, like when generating tokens to allow a kube service or windows desktop service to join the cluster.

Since no one ever needs to generate provision tokens via the CLI, I didn't see a need to make provision_token valid.

In fact, after a quick search, it seems RoleProvisionToken is never used. Maybe we should delete it.

@zmb3 zmb3 force-pushed the zmb3/role-parser-capitalization branch from 1ea4618 to 017b986 Compare January 13, 2022 23:12
zmb3 added 3 commits January 13, 2022 16:53
Our original attempt at canonicalizing roles didn't work for system
roles using camelcase, resulting in an awkward user experience.

Here we maintain a mapping of allowed inputs to their corresponding
system roles, and perform a case-insensitive lookup. This allows us
to support camelcase roles, and has the advantage of permitting
_ word separators as well.

Fixes #9752
Rather than having to list each role here, we rely on the new
roleMappings set to validate the role.

Additionally, remove the LegacyClusterTokenType role. This change
is guaranteed to be backwards compatible because we check for
RoleTrustedCluster everywhere we were checking for
LegacyClusterTokenType, and our roleMappings will convert the old
string that represented LegacyClusterTokenType to RoleTrustedCluster.
@zmb3 zmb3 force-pushed the zmb3/role-parser-capitalization branch from 017b986 to 7c87c26 Compare January 13, 2022 23:53
@zmb3 zmb3 enabled auto-merge (squash) January 13, 2022 23:54
@zmb3 zmb3 merged commit 20c04df into master Jan 14, 2022
@zmb3 zmb3 deleted the zmb3/role-parser-capitalization branch January 14, 2022 00:14
zmb3 added a commit that referenced this pull request Jan 21, 2022
* Add tests for ParseTeleportRoles

Updates #9752

* Be more tolerant when parsing system roles.

Our original attempt at canonicalizing roles didn't work for system
roles using camelcase, resulting in an awkward user experience.

Here we maintain a mapping of allowed inputs to their corresponding
system roles, and perform a case-insensitive lookup. This allows us
to support camelcase roles, and has the advantage of permitting
_ word separators as well.

Fixes #9752

* Refactor *SystemRole.Check()

Rather than having to list each role here, we rely on the new
roleMappings set to validate the role.

Additionally, remove the LegacyClusterTokenType role. This change
is guaranteed to be backwards compatible because we check for
RoleTrustedCluster everywhere we were checking for
LegacyClusterTokenType, and our roleMappings will convert the old
string that represented LegacyClusterTokenType to RoleTrustedCluster.
zmb3 added a commit that referenced this pull request Jan 25, 2022
* Add tests for ParseTeleportRoles

Updates #9752

* Be more tolerant when parsing system roles.

Our original attempt at canonicalizing roles didn't work for system
roles using camelcase, resulting in an awkward user experience.

Here we maintain a mapping of allowed inputs to their corresponding
system roles, and perform a case-insensitive lookup. This allows us
to support camelcase roles, and has the advantage of permitting
_ word separators as well.

Fixes #9752

* Refactor *SystemRole.Check()

Rather than having to list each role here, we rely on the new
roleMappings set to validate the role.

Additionally, remove the LegacyClusterTokenType role. This change
is guaranteed to be backwards compatible because we check for
RoleTrustedCluster everywhere we were checking for
LegacyClusterTokenType, and our roleMappings will convert the old
string that represented LegacyClusterTokenType to RoleTrustedCluster.
zmb3 added a commit that referenced this pull request Jan 25, 2022
* Add tests for ParseTeleportRoles

Updates #9752

* Be more tolerant when parsing system roles.

Our original attempt at canonicalizing roles didn't work for system
roles using camelcase, resulting in an awkward user experience.

Here we maintain a mapping of allowed inputs to their corresponding
system roles, and perform a case-insensitive lookup. This allows us
to support camelcase roles, and has the advantage of permitting
_ word separators as well.

Fixes #9752

* Refactor *SystemRole.Check()

Rather than having to list each role here, we rely on the new
roleMappings set to validate the role.

Additionally, remove the LegacyClusterTokenType role. This change
is guaranteed to be backwards compatible because we check for
RoleTrustedCluster everywhere we were checking for
LegacyClusterTokenType, and our roleMappings will convert the old
string that represented LegacyClusterTokenType to RoleTrustedCluster.
@webvictim webvictim mentioned this pull request Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Windows Desktop token type name inconsistent with other services

3 participants