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 static host user proto defs #44610

Merged
merged 1 commit into from
Jul 30, 2024
Merged

Add static host user proto defs #44610

merged 1 commit into from
Jul 30, 2024

Conversation

atburke
Copy link
Contributor

@atburke atburke commented Jul 25, 2024

This change adds the proto definition for the static host user resource (RFD 175) and its gRPC service.

Part of #42712.

@atburke atburke added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v16 labels Jul 25, 2024
@atburke atburke requested a review from rosstimothy July 25, 2024 00:05
@github-actions github-actions bot requested review from avatus and rudream July 25, 2024 00:06
@@ -0,0 +1,44 @@
syntax = "proto3";

package teleport.statichostuser.v1;
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about generalizing the package a bit to encompass all of auto user provisioning?

Suggested change
package teleport.statichostuser.v1;
package teleport.userprovisioning.v1;

Comment on lines 17 to 21
rpc CreateStaticHostUser(CreateStaticHostUserRequest) returns (CreateStaticHostUserResponse);
// UpdateStaticHostUser updates an existing static host user.
rpc UpdateStaticHostUser(UpdateStaticHostUserRequest) returns (UpdateStaticHostUserResponse);
// UpsertStaticHostUser creates a new static host user or forcefully updates an existing static host user.
rpc UpsertStaticHostUser(UpsertStaticHostUserRequest) returns (UpsertStaticHostUserResponse);
Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/gravitational/teleport/blob/master/rfd/0153-resource-guidelines.md#create
https://github.com/gravitational/teleport/blob/master/rfd/0153-resource-guidelines.md#update
https://github.com/gravitational/teleport/blob/master/rfd/0153-resource-guidelines.md#upsert

Suggested change
rpc CreateStaticHostUser(CreateStaticHostUserRequest) returns (CreateStaticHostUserResponse);
// UpdateStaticHostUser updates an existing static host user.
rpc UpdateStaticHostUser(UpdateStaticHostUserRequest) returns (UpdateStaticHostUserResponse);
// UpsertStaticHostUser creates a new static host user or forcefully updates an existing static host user.
rpc UpsertStaticHostUser(UpsertStaticHostUserRequest) returns (UpsertStaticHostUserResponse);
rpc CreateStaticHostUser(CreateStaticHostUserRequest) returns (StaticHostUser);
// UpdateStaticHostUser updates an existing static host user.
rpc UpdateStaticHostUser(UpdateStaticHostUserRequest) returns (StaticHostUser);
// UpsertStaticHostUser creates a new static host user or forcefully updates an existing static host user.
rpc UpsertStaticHostUser(UpsertStaticHostUserRequest) returns (StaticHostUser);

Comment on lines 72 to 78
if u.Spec.NodeLabels != nil {
for key, value := range u.Spec.NodeLabels.Values {
if key == types.Wildcard && !(len(value.Values) == 1 && value.Values[0] == types.Wildcard) {
return trace.BadParameter("selector *:<val> is not supported")
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trait templating doesn't apply here because when we create a host user, there won't be a user to pull traits from. I tried adding the expression validation, but it would require moving a LOT of stuff from lib/utils to api/utils and I think it would be much simpler to just do that in lib/services like we do for roles.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might make more sense for validation logic to be moved out of api and into a place that can make use of lib/utils in that case. If you want to omit it here and move it to lib/services or similar in a future PR that's fine with me.

Comment on lines 38 to 39
// node_labels is a map of node labels (used to dynamically grant access to
// nodes).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here and below, update the comment since the labels here are used to select nodes, not to grant access to nodes.

Suggested change
// node_labels is a map of node labels (used to dynamically grant access to
// nodes).
// node_labels is a map of node labels that will create a user
// for this resource

@public-teleport-github-review-bot public-teleport-github-review-bot bot removed the request for review from rudream July 30, 2024 02:58
This change adds the proto definition for the static host user resource
and its gRPC service.
@atburke atburke force-pushed the atburke/static-user-proto branch from fa529da to a134b2d Compare July 30, 2024 16:15
@atburke atburke enabled auto-merge July 30, 2024 16:15
@atburke atburke added this pull request to the merge queue Jul 30, 2024
Merged via the queue into master with commit ff68deb Jul 30, 2024
40 of 41 checks passed
@atburke atburke deleted the atburke/static-user-proto branch July 30, 2024 16:49
@public-teleport-github-review-bot

@atburke See the table below for backport results.

Branch Result
branch/v16 Failed

atburke added a commit that referenced this pull request Sep 10, 2024
This change adds the proto definition for the static host user resource
and its gRPC service.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v16 no-changelog Indicates that a PR does not require a changelog entry size/md
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants