Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add V4 Roles #7118

Merged
merged 21 commits into from
Jun 10, 2021
Merged

Add V4 Roles #7118

merged 21 commits into from
Jun 10, 2021

Conversation

nklaassen
Copy link
Contributor

@nklaassen nklaassen commented Jun 1, 2021

Issue #5513

This PR adds V4 roles, which are identical to V3 roles except that they have stricter defaults for their allow labels.

Label RoleV3 Default RoleV4 Default
NodeLabels [{"*": "*"}] if the user has any logins, else [] nil
AppLabels [{"*": "*"}] nil
KubernetesLabels [{"*": "*"}] nil
DatabaseLabels [{"*": "*"}] nil

Defaults are set for all nil label sets (and sometimes for empty label sets, see Downgrading Roles), not only when the role is first created but also at various other points (e.g. after serializing/deserializing).

Backward Compatibility

Existing V3 roles will be unaffected. Users can continue to create V3 roles without changes.

Because V4 roles have an identical structure to V3, they are completely compatible except that older clients will not recognize the V4 version tag and throw an error.

Because we want V4 roles to be usable on a cluster with teleport instances running a version which does not have RoleV4 support, the teleport gRPC server will downgrade all V4 roles to V3 before sending them to older clients.

Detecting Client Version

In order for the server to be able to detect the client version, I have added a version tag to the gRPC metadata that will be sent with all requests from the gRPC client - see changes in api/metadata/metadata.go and api/client/client.go. If no version metadata is received with a gRPC request, the server will assume that the client is running an older teleport version and send the downgraded V3 role (see downgradeRole in lib/auth/grpcserver.go).

Downgrading Roles

Two things need to be done to downgrade a V4 role to V3:

  1. The version tag needs to be changed to v3. Older instances will not recognize a newer version tag, and throw an error.
  2. If any of the label sets are currently empty, set a placeholder label so that the older clients will not mistakenly set the V3 defaults.
    At first I thought we could just set the labels to an empty list rather than nil, but distinguishing an empty vs a nil list is always error prone - and because we currently serialize labels with omitempty they may be impossible to distinguish. For this reason, I set a placeholder label for any empty label set during the downgrade (see DowngradeRoleToV3 in lib/services/role.go, the current placeholder is {"__teleport_no_labels": <uuid>}).

Rolling Back / Downgrading Teleport

If a user creates any V4 roles, they will not be able to downgrade to a teleport version without V4 role support without first deleting all of the V4 roles, or manually changing them to V3.

RoleV3 Type Rename

As part of this change RoleV3 and RoleSpecV3 should probably also be renamed to RoleV4 and RoleSpecV4. To avoid creating too much noise in this diff, I've put this in a following PR at #7132

Related Issues

#5513

@nklaassen nklaassen force-pushed the nklaassen/roles-v4 branch 2 times, most recently from 2842d02 to b289858 Compare June 1, 2021 02:20
api/metadata/metadata.go Outdated Show resolved Hide resolved
lib/services/types.go Outdated Show resolved Hide resolved
Comment on lines 2337 to 2339
case V4:
// V4 roles are identical to V3 except for their defaults
fallthrough
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm not mistaken, resource versions always line up with the VX appended to the struct name. It's main purpose right now is to unmarshal data into the correct struct version.

I think we should avoid conflating logic this logic with the version field. If the main purpose of the V4 roles is to set defaults in a backwards compatible way, a solution similar to ResourceWithOrigin might be more suitable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting idea, so this would look more like adding a field to the struct to decide which defaults you get, rather than incrementing the version? I think that could potentially work (with similar "downgrade" steps). I'm curious what others think though

api/types/role.go Outdated Show resolved Hide resolved
@nklaassen nklaassen force-pushed the nklaassen/roles-v4 branch from b289858 to f9684e5 Compare June 1, 2021 20:05
@nklaassen nklaassen force-pushed the nklaassen/roles-v4 branch from f9684e5 to 77c704a Compare June 1, 2021 20:06
}
}

minSupportedVersionForV4Roles := semver.New("6.3.0-aa") // "aa" is included so that this compares before v6.3.0-alpha
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will need to update this to the first version which will support V4 roles before merging

@nklaassen nklaassen marked this pull request as ready for review June 1, 2021 21:52
@nklaassen nklaassen requested review from fspmarshall and tcsc June 1, 2021 21:53
api/types/role.go Outdated Show resolved Hide resolved
api/types/role.go Outdated Show resolved Hide resolved

const (
// TODO(Joerger): change this to generated value
Version = "7.0.0-dev"
Copy link
Collaborator

Choose a reason for hiding this comment

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

We already have version in version.go file. Is this in addition to that? Does this mean we'd need to bump version in 3 places now when doing releases (Makefile, version.go and here)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

teleport/api/ doesn't depend on teleport/ which is why I can't really use the value from version.go. It sounds like @Joerger is working on something so that api/ will have access to the generated client version, the suggestion was to copy this here for now and let him fill in the generated value when his changes come in. I'm not sure how quickly those changes are coming or if we should put in something to get the generated value for this PR @Joerger ? fyi I think we want this going out in the 6.2 train

Copy link
Contributor

@Joerger Joerger Jun 3, 2021

Choose a reason for hiding this comment

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

Here's the PR for the client version generation - #7157
If this PR is merged before that one, I'll update it to use the new generated version in #7157

api/metadata/metadata.go Outdated Show resolved Hide resolved
api/types/role.go Outdated Show resolved Hide resolved
lib/services/role.go Outdated Show resolved Hide resolved
nklaassen added 3 commits June 2, 2021 10:20
move downgrade func to services/ to avoid uuid dep in api/
don't check list for nil, len() will return 0
add comments
@nklaassen nklaassen requested a review from smallinsky June 3, 2021 02:09
api/metadata/metadata.go Outdated Show resolved Hide resolved
api/metadata/metadata.go Outdated Show resolved Hide resolved
@nklaassen nklaassen added the ux label Jun 8, 2021
@nklaassen nklaassen requested a review from tcsc June 8, 2021 19:18
lib/services/role.go Outdated Show resolved Hide resolved
@nklaassen nklaassen requested a review from inertial-frame as a code owner June 9, 2021 18:02
Copy link
Contributor

@inertial-frame inertial-frame left a comment

Choose a reason for hiding this comment

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

Docs approved - thanks!

Copy link
Contributor

@klizhentas klizhentas left a comment

Choose a reason for hiding this comment

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

The logic and UX looks good. How would our existing preset roles behave with this change, in particular, "access" preset. Would it continue working?

@nklaassen
Copy link
Contributor Author

The logic and UX looks good. How would our existing preset roles behave with this change, in particular, "access" preset. Would it continue working?

@klizhentas Yes, existing built-in and preset roles are remaining on V3 for now, there should be no noticeable change. In #7132 all of the go types are changing to RoleV4, but the version tag and thus the defaults are staying on V3, this change should be completely invisible to users.

The "access" preset explicitly sets all labels to wildcard-allow anyways, and the "editor" preset does not set any label values and appears to rely on the V3 wildcard defaults to be set.

The only roles that should be affected here are ones explicitly created by users on V4. We may want to migrate built-in and preset roles (and possibly existing user roles) in a future major release.

@nklaassen nklaassen requested a review from alex-kovoy as a code owner June 10, 2021 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants