Machine ID: Introduce BotService by strideynet · Pull Request #35441 · gravitational/teleport

strideynet · 2023-12-06T19:00:19Z

Quite a few deprecations in this one. Deprecated RPCs and perms now emit a warning log message which should encourage most folks to migrate prior to v16.0.0.

Fairly large diff - in part due to about 1800 lines of tests that expand the coverage of bot related code significantly. Some of these tests are for the legacy/deprecated endpoints that weren't covered previously but I wanted to make sure they behaved properly in their deprecated state.

changelog: SEE PR

Machine ID Bots can now be managed as yaml with the typical tctl commands, e.g:

tctl get bot foo
tctl get bots
tctl create -f bot.yaml

kind: bot
version: v1
metadata:
  name: test
spec:
  roles:
  - editor
  traits:
  - name: logins
    values:
    - root

The ability to specifically control access to managing Bots has been added to Teleport's RBAC engine. Users managing Bots should now be explicitly granted permissions to manage the bot resource. eg:

spec:
  allow:
    rules:
    - resources:
      - bot
      verbs:
      - list
      - create
      - read
      - update
      - delete

The previous required permissions, a combination of access to user and role, will no longer grant access to Bot RPCs in Teleport 16.0.0.

Additionally, three new audit events are now emitted:

bot.create
bot.update
bot.delete

For those using the Teleport API, the following RPCs are deprecated and will stop functioning in Teleport 16.0.0:

proto.AuthService.CreateBot - switch to machineid.v1.BotService.CreateBot
proto.AuthService.DeleteBot - switch to machineid.v1.BotService.DeleteBot
proto.AuthService.GetBotUsers - switch tomachineid.v1.BotService.ListBots

github-actions · 2023-12-12T11:11:48Z

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

public-teleport-github-review-bot · 2023-12-12T11:12:07Z

@strideynet - this PR will require admin approval to merge due to its size. Consider breaking it up into a series smaller changes.

codingllama

Drive-by review, just regarding that one comment.

ravicious

Gave it just a cursory look as I'm not the best person to review this, but I see that you already requested a review from Tim.

ravicious · 2023-12-13T16:28:34Z

  // CreateBot creates a new bot user.
-  rpc CreateBot(CreateBotRequest) returns (CreateBotResponse);
+  //
+  // Deprecated: Use [teleport.machineid.v1.BotService] instead.


How can I ensure that a doc link like this is valid?

This comment ends up in api/client/proto/authservice.pb.go. I was under the impression that if machineid.v1 is not directly imported in that file, then a doc link must use the full import path, which I assume would be github.com/gravitational/teleport/…. Isaiah pointed it out in one of my PRs but I've never tried to actually verify if my doc links are correct.

https://tip.golang.org/doc/comment#doclinks

I know it doesn't matter much as most of the time those "doc links" will not end up in any docs and are just for internal reference. Still, it's been bugging me since it was pointed out to me so I figured maybe you know the answer.

Ah - I suppose it's a little odd here since my intention was to "comment on the proto" and therefore here I've referred to the proto service rather than the generated code.

You can:

$ go install golang.org/x/pkgsite/cmd/pkgsite@latest $ pkgsite .

And then navigate to http://localhost:8080/github.com/gravitational/teleport#section-directories to see the docs rendered locally.

ibeckermayer · 2023-12-13T20:03:29Z

+	return bot, nil
+}
+
+func (bs *BotService) UpdateBot(


Throughout the codebase we have these UpdateX functions which semantically mean "if this resource already exists, overwrite it completely (a-la Upsert), otherwise error". AFAICT it's very unusual to need this semantic:
If you're calling UpdateX it almost always means that you've just called GetX, changed a field or two, and are now updating the object. This makes the check for "if this resource already exists" superfluous -- you already know it exists because you just got it with GetX.

Note that HTTP doesn't have any such semantic:

CREATE: CreateX

GET: GetX

DELETE: DeleteX

PUT: UpsertX

PATCH: no equivalent except an odd UpdateX like UpdateRemoteCluster

POST: according to the RFC:

The POST method requests that the target resource process the
representation enclosed in the request according to the resource's
own specific semantics.

Arguably our UpdateX is this miscellaneous type, however as I pointed out previously it's an odd semantic to implement.

Given this line of reasoning, I think we should aim to avoid adding these UpdateX by default. Usually an Upsert seems to do the job.

One loose end I have on this topic is: how do Upserts behave with the new optimistic locking behavior? If a resource has a Revision, will it be checked and rejected if it doesn't match what's in the database on an Upsert? If yes, then Upsert already implements the current Update semantic (and Updates can be replaced with Upserts). If that's not how it works, and Upserts just ignore Revision, then the current Updates could be seen as means of distributed synchronization to prevent a case like

My code calls GetX

Some other part of the system calls DeleteX on that object

My code updates a field and calls UpdateX on that object

However I don't think this was an intentional design pattern.

cc @rosstimothy @espadolini

Fwiw: my implementation here is following the recently proposed RFD 153 on the design of resource APIs. So it may be worth following up with some of these concerns there - #34103 .

This makes the check for "if this resource already exists" superfluous

I don't think this is true - we're operating in a distributed system. I can think of a few scenarios where writing to a resource which has been modified or deleted since you read it risks harming consistency and whilst we don't have "real" locking this seems like the most sensible alternative.

Given this line of reasoning, I think we should aim to avoid adding these UpdateX by default. Usually an Upsert seems to do the job.

I believe if anything long term goal is to do the opposite of what you suggest, and eliminate upsert in favour of create and update. Right now, several things rely on the ability to upsert (e.g tctl create -f / the terraform provider / the kubernetes operator). Once we reach a point where all of those no longer rely on this behaviour, then I think we can almost globally deprecate upsert RPCs.

One loose end I have on this topic is: how do Upserts behave with the new optimistic locking behavior?

IIRC the behaviour of upsert RPCs has not been changed. Optimistic locking is entirely ignored.

However I don't think this was an intentional design pattern.

It was my understanding that this was the entire motivation behind adding optimistic locking to the update RPC.

In addition to this,update RPCs are encouraged to support update masks where appropriate. At least in this case, the UpdateBot RPC implements this to allow selective modification of the roles or traits associated with the Bot without knowledge of the state of other fields.

Upsert is by definition unconditional, and it's required (at least as a primitive operation, it doesn't have to be exposed through grpc necessarily) for the cache to work.

The old Update primitive backend operation has no right to exist, and it has the semantics that @ibeckermayer mentioned. ConditionalUpdate is the one that you want to use, and you use exactly like that. You get the resource, you make some decisions and calculate the new resource, you ConditionalUpdate the resource with a constraint on the revision you got, and your update succeeds if and only if nothing has written the underlying backend item since you read it.

Whether the revision should be exposed to the client over grpc is a choice that should be done depending on the resource type and the semantics we want to give to each operation; it might make sense sometimes to expose an "update" function that lets the caller update some fields with no regard to what was there in the first place, but even in that case the auth server should 100% use the conditional update operation internally.

(and yeah, "i just read the item successfully so the item exists" is 100% bogus and it's a pattern that we should start removing from Teleport whenever possible)

@strideynet

It was my understanding that this was the entire motivation behind adding optimistic locking to the update RPC.

I was referring to the state of things pre-optimistic-locking.

I believe if anything long term goal is to do the opposite of what you suggest, and eliminate upsert in favour of create and update. Right now, several things rely on the ability to upsert (e.g tctl create -f / the terraform provider / the kubernetes operator). Once we reach a point where all of those no longer rely on this behaviour, then I think we can almost globally deprecate upsert RPCs.

Seems like an odd goal... Is create -f not the user explicitly asking for an upsert? I can imagine implementing it with a get + update, but I'm not currently seeing what the point of that would be.

@espadolini

Upsert is by definition unconditional

Granted, dumb question.

The old Update primitive backend operation has no right to exist, and it has the semantics that @ibeckermayer mentioned. ConditionalUpdate is the one that you want to use, and you use exactly like that. You get the resource, you make some decisions and calculate the new resource, you ConditionalUpdate the resource with a constraint on the revision you got, and your update succeeds if and only if nothing has written the underlying backend item since you read it.

Is this not essentially what Update has become, presuming it now uses the optimistic locking mechanism?

I can see several plausible update-like semantics:

update the entire object irrespective of revision (error if object dne)

update the entire object on a specific revision (error if revision/object dne)

patch a few fields irrespective of revision (error if object dne)

patch a few fields on a specific revision (error if revision/object dne)

write an entire object irrespective of revision/existence (upsert)

analysis:

is the status quo

is presumably the primary reason we implemented optimistic locking

we have in a few places (HTTP patch semantic)

seems practically useless -- if we have the object's revision, that typically means we have the object, so we may as well just use 2

seems useful to me, although there seems to be some discussion of getting rid of it

Feel free to enlighten me on where I might be confused about how our optimistic locking system works (highly plausible), what we need it to enforce (I'm mostly ignorant except in very general terms) or what I'm missing about upsert.

Thanks for pointing out that PR @strideynet, this whole discussion is more suited to that RFD so let's continue it over there: #34103 (comment)

@ibeckermayer do you think this blocks this PR moving forward ? I'm a little concerned about how long discussing this will run and I have some time pressure to get this merged as it's blocking other work.

Not necessarily, I can review the rest and we can just consider this potentially-pending-a-future-PR.

Since you did go through the trouble of adding a field mask, it's worth following this line of inquiry through to its conclusion.

timothyb89

Just a few comments after playing with the branch for a while. The implementation turned out pretty nice! The separate service and RFD 153 resources make this much tidier than I remember.

I didn't notice any compatibility issues but haven't yet tested the Terraform provider integration (which probably could use an update anyway, now that we have sane bot objects).

Co-authored-by: Tim Buckley <tim@goteleport.com>

strideynet · 2023-12-14T14:13:52Z

FYI @timothyb89 - I've addressed your comments if you'd like to re-review.

timothyb89

Looks good, I'm excited to finally see this!

…d-service

strideynet · 2023-12-19T11:44:01Z

Flaky Test Detector is failing due to test length - it looks like the TestAdminActionMFA test takes about 7.67 seconds to run on my mac, which at 100 repetitions passes the 10 minute threshold. As far as I can tell, the changes I've made are only adding about 200-400ms to this test - I'm not sure there's another way I can approach this without adding that overhead and the test is already pretty slow. CC @Joerger

Joerger · 2023-12-19T18:21:46Z

@strideynet This test will be skipped by the flaky test detector momentarily - #35882

strideynet · 2023-12-19T20:23:00Z

Merged master in to pick up joerger's fix for the flaky-test failure

zmb3 · 2023-12-19T21:05:30Z

  // CreateBot creates a new bot user.
-  rpc CreateBot(CreateBotRequest) returns (CreateBotResponse);
+  //
+  // Deprecated: Use [teleport.machineid.v1.BotService] instead.


You can:

$ go install golang.org/x/pkgsite/cmd/pkgsite@latest $ pkgsite .

And then navigate to http://localhost:8080/github.com/gravitational/teleport#section-directories to see the docs rendered locally.

zmb3 · 2023-12-19T21:09:33Z

 // IsBot returns true if the user is a bot.
 func (u UserV2) IsBot() bool {
-	_, ok := u.GetMetadata().Labels[BotGenerationLabel]
+	_, ok := u.GetMetadata().Labels[BotLabel]


Does creating the new bot resource still create a user and role in the backend, or are those replaced by the bot resource?

The Bot resource is still just transformed into a User/Role - eventually we'll remove this but that'll involve delving into the core of Teleport's RBAC 😓

This change here is just to make this a little less fragile. I'm not quite sure why we used the Generation label here - it stays at "0" for bots using delegated joining and could be removed in future. The BotLabel is a much better indicator of a user/role being linked to a bot.

zmb3 · 2023-12-20T19:27:59Z

+// BotResourceName returns the default name for resources associated with the
+// given named bot.
+func BotResourceName(botName string) string {
+	return "bot-" + strings.ReplaceAll(botName, " ", "-")


Do we need to remove / as well?

At the moment, this has just moved the previous implementation of BotResourceName from lib/auth into lib/auth/machineid/machineidv1 - I do agree that maybe we ought to replace / but I feel it's out of scope for this PR and I don't want to introduce additional risk by changing this.

…d-service

Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com>

public-teleport-github-review-bot · 2023-12-21T12:19:32Z

@strideynet See the table below for backport results.

Branch	Result
branch/v14	Failed

strideynet force-pushed the strideynet/machineid-service branch 2 times, most recently from 480dd85 to eea5e1d Compare December 11, 2023 22:20

strideynet added the machine-id label Dec 11, 2023

strideynet requested a review from timothyb89 December 11, 2023 22:58

strideynet added the backport/branch/v14 label Dec 12, 2023

Start sketching out BotService handler impl

1cbbe8c

strideynet force-pushed the strideynet/machineid-service branch from c6b2863 to 1cbbe8c Compare December 12, 2023 10:59

strideynet marked this pull request as ready for review December 12, 2023 11:11

github-actions Bot requested review from ibeckermayer and ravicious December 12, 2023 11:11

github-actions Bot added audit-log Issues related to Teleports Audit Log size/xl tctl tctl - Teleport admin tool ui labels Dec 12, 2023

strideynet changed the title ~~Introduce BotService~~ Machine ID: Introduce BotService Dec 12, 2023

strideynet commented Dec 12, 2023

View reviewed changes

Comment thread tool/tctl/common/bots_command.go

Allow bots to be created without roles

4e448ed

strideynet mentioned this pull request Dec 12, 2023

Add docs for new bot resource #35651

Merged

strideynet commented Dec 12, 2023

View reviewed changes

Comment thread lib/auth/init.go Outdated

strideynet added 2 commits December 12, 2023 14:49

Fix up tests

f217d94

Add warning about adding a role to the bot

d2e4c95

codingllama reviewed Dec 12, 2023

View reviewed changes

Comment thread lib/auth/init.go Outdated

Comment thread lib/auth/init.go Outdated

Comment thread lib/auth/init.go Outdated

codingllama reviewed Dec 12, 2023

View reviewed changes

Improve resource type handling

effb495

ravicious reviewed Dec 13, 2023

View reviewed changes

ibeckermayer reviewed Dec 13, 2023

View reviewed changes

timothyb89 reviewed Dec 14, 2023

View reviewed changes

Update tool/tctl/common/bots_command.go

f4cf615

Co-authored-by: Tim Buckley <tim@goteleport.com>

strideynet added 2 commits December 14, 2023 13:38

Don't display traits with zero length values

c2fc752

Preserve json style output for tctl bots add

d552a14

strideynet requested a review from timothyb89 December 14, 2023 14:13

Unbreak tests broken by CreatedBy changes

d439892

timothyb89 approved these changes Dec 14, 2023

View reviewed changes

Merge remote-tracking branch 'origin/master' into strideynet/machinei…

e23ccce

…d-service

mcbattirola mentioned this pull request Dec 15, 2023

Add bot resource to user ACL #35774

Merged

ibeckermayer mentioned this pull request Dec 16, 2023

RFD 0153 Resource Guidelines #34103

Merged

ibeckermayer approved these changes Dec 18, 2023

View reviewed changes

Comment thread tool/tctl/common/bots_command.go Outdated

Comment thread tool/tctl/common/bots_command.go

Comment thread tool/tctl/common/bots_command.go

strideynet added 3 commits December 19, 2023 09:53

Clarify deprecation/deletion

e472f48

Improve error message when no proxies

3b27833

Merge remote-tracking branch 'origin/master' into strideynet/machinei…

9c9a2f0

…d-service

Merge branch 'master' into strideynet/machineid-service

00233f2

zmb3 approved these changes Dec 20, 2023

View reviewed changes

strideynet and others added 6 commits December 21, 2023 10:40

Merge remote-tracking branch 'origin/master' into strideynet/machinei…

7217691

…d-service

An -> a

bd4abe9

Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com>

Further clarify GoDoc

6da3b2a

Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com>

Remove redundant case statements

83272c9

Remove unused import

88abec0

Use slices

191930b

strideynet added this pull request to the merge queue Dec 21, 2023

Merged via the queue into master with commit e3191b0 Dec 21, 2023

strideynet deleted the strideynet/machineid-service branch December 21, 2023 12:18

timothyb89 mentioned this pull request Jan 5, 2024

Use GetRole in the user login state generator, fail if not found. #36314

Merged

Conversation

strideynet commented Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Dec 12, 2023

Uh oh!

public-teleport-github-review-bot Bot commented Dec 12, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codingllama left a comment

Choose a reason for hiding this comment

Uh oh!

ravicious left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

strideynet Dec 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

espadolini Dec 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibeckermayer Dec 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

I can see several plausible update-like semantics:

analysis:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibeckermayer Dec 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

timothyb89 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

strideynet commented Dec 14, 2023

Uh oh!

timothyb89 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

strideynet commented Dec 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Joerger commented Dec 19, 2023

Uh oh!

strideynet commented Dec 19, 2023

Uh oh!

strideynet commented Dec 6, 2023 •

edited

Loading

strideynet Dec 14, 2023 •

edited

Loading

espadolini Dec 14, 2023 •

edited

Loading

ibeckermayer Dec 16, 2023 •

edited

Loading

ibeckermayer Dec 18, 2023 •

edited

Loading

strideynet commented Dec 19, 2023 •

edited

Loading