Assist - API implementation by jakule · Pull Request #25810 · gravitational/teleport

jakule · 2023-05-08T15:44:52Z

This is the first part of backports of Teleport Assist. This PR contains only API definitions and backend CRUD implementation.

For reviewers:
Teleport Assist is an AI bot-type bot that allows you to generate a Linux command and run it on multiple hosts. It has only WebUI interface. Because of that, all endpoints are specified in lib/web as web handlers.
From the backend perspective, each conversation with the bot creates a new conversation key and many messages tied with the same conversation ID. A message can contain a user message, bot response, Linux command generated by a user, or metadata information as an execution result.

jakule · 2023-05-08T18:10:39Z

The API will be backported in a separate PR

r0mant

Left a few suggestions but none should be blockers.

r0mant · 2023-05-08T20:01:00Z

Nit: Why not CreateAssistantMessage to keep consistent terminology?

r0mant · 2023-05-08T20:01:49Z

Nit: Can you add godocs on all public types?

r0mant · 2023-05-08T20:02:36Z

Nit: These comments will be applied to generated Go structs so they should follow Go field naming conventions. Same everywhere below.

Suggested change

// conversation_id it's the ID of a conversation.

// ConversationID it's the ID of a conversation.

The field is called ConversationId, but I get your point. https://github.com/gravitational/teleport/blob/e87cb3b2cae8bdfc8cd9bb56cd1586876a3bb1f4/api/client/proto/authservice.pb.go#L14052
We should probably stop using Go way of commenting on protobufs. If we ever generate a client in a different language all comments will be wrong.

r0mant · 2023-05-08T20:11:16Z

Just curious if it would make sense to split this out into a separate GRPC service. This seems to be "the way" lately although I don't have strong objections to keeping this part of auth service either for now.

Let me take a look at it. I was also thinking about it, but the idea "died" at some point. I don't think it will be so hard, but I could be wrong.

It took some time, but I was able to move it to a new service :)

r0mant · 2023-05-08T20:12:24Z

Nit: I'd clarify in the comment which assistant-related objects it governs. Conversations/messages/both?

r0mant · 2023-05-08T20:13:50Z

Why is username not a part of the "create" request? Would it make sense to make it a part of it? Same for other methods.

r0mant · 2023-05-08T20:17:52Z

Do you anticipate having to provide ability to update any other conversation properties in future?

If so, would it make more sense to have a more generic UpdateAssistantConversation method which would only allow updating title for now but can be extended in future?

I didn't want to change the API, but while addressing a different comment, I've also changed this.

zmb3

Thanks for breaking out the boilerplate. A few questions/comments about the data model and API organization, but nothing critical.

zmb3 · 2023-05-08T22:18:08Z

I feel like this would have made sense as a new GRPC service instead of putting it in the auth service, but I imagine that's too big of a change to make now.

Not "too big change". Moved

zmb3 · 2023-05-08T22:19:51Z

Suggested change

// conversationId it's the ID of a conversation.

// ConversationId identifies a conversation.

So that the generated godocs match the name of the generated type.

zmb3 · 2023-05-08T22:20:20Z

Suggested change

// conversationId used to tie all messages in a one conversation.

// ConversationId is used to tie all messages into a conversation.

zmb3 · 2023-05-08T22:20:43Z

Would an enum make more sense?

Yes and no. From the protobuf perspective, yes, but WebUI is also using this field, and to not pass super long strings to UI or keep re-mapping between values, I thought that string is just simpler.

zmb3 · 2023-05-08T22:21:06Z

Suggested change

// createdTime is the time when the even occurred.

// CreatedTime is the time when the event occurred.

zmb3 · 2023-05-08T22:25:57Z

Why are these attached to IdentityService?

// IdentityService is responsible for managing web users and currently // user accounts as well

This kind of stuff is hard to unwind, so getting the data model right seems important.

To speed up the initial development, I decided to piggyback most of the implementation on IdentityService. I moved the implementation now.

zmb3 · 2023-05-08T22:29:18Z

Do we have a constant for USER_MSG somewhere? Arbitrary hard-coded strings when there are only a few valid types seems like a bad idea.

This is a test, and in this case, the value doesn't make a huge difference. I can remove it if you think it's confusing.

zmb3 · 2023-05-08T22:30:32Z

Why do some of the API endpoints support the :site parameter and others don't? Shouldn't we be consistent?

Would we ever want to support assistant across trusted clusters?

This is "implementation driven". /assistant and /execute endpoints with :site are using WithClusterAuth as this one passes reversetunnel.RemoteSite to the handler.
If you think that all should use :site I can add it.

I'm not an expert here, but I would think we either want to support remote sites for assist, or we don't want to. Supporting remote sites for half of the APIs and not the other half doesn't make sense to me.

@kimlisa ?

sorry... i'm not an expert either, but wherever :site is defined in this PR makes sense to me. all other endpoints are like CRUD operation on conversation, which if a user started conversation on root cluster, it makes sense to keep this conversation and its operations on root?

zmb3 · 2023-05-09T13:40:15Z

Ok, since it's not a role I would suggest this then.

// KindAssistant is used to program RBAC for // Teleport Assist resources.

zmb3 · 2023-05-09T13:43:19Z

I'm not an expert here, but I would think we either want to support remote sites for assist, or we don't want to. Supporting remote sites for half of the APIs and not the other half doesn't make sense to me.

@kimlisa ?

Fix comments

jakule · 2023-05-09T17:10:10Z

@zmb3 I don't think that the comment about remote sites is critical. I'll merge this PR, but I also create an issue to trace the discussion #25933

kimlisa · 2023-05-09T18:00:11Z

+	// Allows executing an arbitrary command on multiple nodes.
+	h.GET("/webapi/command/:site/execute", h.WithClusterAuth(h.executeCommand))


based on the comment, is it correct for this verb to be get? post sounds more correct but i also don't know what's going on under hood. also this endpoint doesn't follow the sites/:site convention

Suggested change

// Allows executing an arbitrary command on multiple nodes.

h.GET("/webapi/command/:site/execute", h.WithClusterAuth(h.executeCommand))

// Allows executing an arbitrary command on multiple nodes.

h.GET("/webapi/sites/:site/command/execute", h.WithClusterAuth(h.executeCommand))

#25810 added username and conversation ID to AssistantMessage. They aren't needed inside the message and can be moved out of it. This PR also fixes ` grpc: error while marshaling: proto: Marshal called with nil` in `CreateAssistantMessage`. Note: It's safe to change protobuf numbers as this code hasn't been released or backported yet.

* Move username and ID out of AssistantMessage #25810 added username and conversation ID to AssistantMessage. They aren't needed inside the message and can be moved out of it. This PR also fixes ` grpc: error while marshaling: proto: Marshal called with nil` in `CreateAssistantMessage`. Note: It's safe to change protobuf numbers as this code hasn't been released or backported yet. * Fix test * make grpc

* Assist - API implementation * Add Assistant CRUD test * Move assistant API to a separate file. * Rename GRPC methods Fix comments * Address PR comments * Update GRPC API - addressing code review comments * Move assist API to a new GRPC service * Add Assistant RBAC rules to this backport. * Add missing license * Update comment.

* Move username and ID out of AssistantMessage #25810 added username and conversation ID to AssistantMessage. They aren't needed inside the message and can be moved out of it. This PR also fixes ` grpc: error while marshaling: proto: Marshal called with nil` in `CreateAssistantMessage`. Note: It's safe to change protobuf numbers as this code hasn't been released or backported yet. * Fix test * make grpc

* Assist - API implementation (#25810) * Assist - API implementation * Add Assistant CRUD test * Move assistant API to a separate file. * Rename GRPC methods Fix comments * Address PR comments * Update GRPC API - addressing code review comments * Move assist API to a new GRPC service * Add Assistant RBAC rules to this backport. * Add missing license * Update comment. * Move username and ID out of AssistantMessage (#25964) * Move username and ID out of AssistantMessage #25810 added username and conversation ID to AssistantMessage. They aren't needed inside the message and can be moved out of it. This PR also fixes ` grpc: error while marshaling: proto: Marshal called with nil` in `CreateAssistantMessage`. Note: It's safe to change protobuf numbers as this code hasn't been released or backported yet. * Fix test * make grpc

* Assist - API implementation (#25810) * Assist - API implementation * Add Assistant CRUD test * Move assistant API to a separate file. * Rename GRPC methods Fix comments * Address PR comments * Update GRPC API - addressing code review comments * Move assist API to a new GRPC service * Add Assistant RBAC rules to this backport. * Add missing license * Update comment. * Move username and ID out of AssistantMessage (#25964) * Move username and ID out of AssistantMessage gravitational/teleport#25810 added username and conversation ID to AssistantMessage. They aren't needed inside the message and can be moved out of it. This PR also fixes ` grpc: error while marshaling: proto: Marshal called with nil` in `CreateAssistantMessage`. Note: It's safe to change protobuf numbers as this code hasn't been released or backported yet. * Fix test * make grpc

* Assist - API implementation * Add Assistant CRUD test * Move assistant API to a separate file. * Rename GRPC methods Fix comments * Address PR comments * Update GRPC API - addressing code review comments * Move assist API to a new GRPC service * Add Assistant RBAC rules to this backport. * Add missing license * Update comment.

* Move username and ID out of AssistantMessage #25810 added username and conversation ID to AssistantMessage. They aren't needed inside the message and can be moved out of it. This PR also fixes ` grpc: error while marshaling: proto: Marshal called with nil` in `CreateAssistantMessage`. Note: It's safe to change protobuf numbers as this code hasn't been released or backported yet. * Fix test * make grpc

* Assist - API implementation (#25810) * Assist - API implementation * Add Assistant CRUD test * Move assistant API to a separate file. * Rename GRPC methods Fix comments * Address PR comments * Update GRPC API - addressing code review comments * Move assist API to a new GRPC service * Add Assistant RBAC rules to this backport. * Add missing license * Update comment. * Move username and ID out of AssistantMessage (#25964) * Move username and ID out of AssistantMessage #25810 added username and conversation ID to AssistantMessage. They aren't needed inside the message and can be moved out of it. This PR also fixes ` grpc: error while marshaling: proto: Marshal called with nil` in `CreateAssistantMessage`. Note: It's safe to change protobuf numbers as this code hasn't been released or backported yet. * Fix test * make grpc * Assist - Configuration and usage (#25953) * Assist - Configuration and usage * Add network config test * Add config test * Run GCI * Address review comments * Assist - OpenAI library port (#25948) * Assist - OpenAI library port * Add tests * Address code review comments * Added comment * Partial backport of #26058 * Move AI messages to a new file * Prevent blocking on error * Add comments. Fix typo * Assist - Execution web endpoint (#25955) * Assist - Execution web endpoint * Add test Clean up code a bit * Add missing username * Address review comments * Make more implementation shared between Terminal and Command Web Handlers * Address review comments * Address review comments * Fixes after rebase Add comments * Add comments Fix linter * Add TELEPORT to Teleport related environment variable. * Assist - Web endpoints (#26046) * Assist - Web endpoints * GCI * proto rpc * enforce endpoint checks * disable in ui if disallowed by auth * add comment * unwrap stack * Refactor code --------- Co-authored-by: Joel Wejdenstål <jwejdenstal@icloud.com> * [Assist] Fix random user selection (#26183) Currently, after selecting a user for command execution in Teleport Assist the user can randomly change. This PR fixes this behavior. * Add rate limiting to Assist (#26011) * Add rate limiting to Assist * Only rate limit Assist in Cloud * Add a comment to assistantLimiter * Fixes after rebase * Add 'rate-limited' test case to assistant_test * Handle CHAT_MESSAGE_UI in Assist web UI * Add godoc * CHAT_MESSAGE_UI -> CHAT_MESSAGE_ERROR * Run assistant test cases in parallel * Rename `ChatGPT` to `GPT-4` (#26272) * Rename `ChatGPT` to `GPT4` ChatGPT is a user friendly name, but is technically inaccurate. * Apply suggestions from code review Co-authored-by: Reed Loden <reed@goteleport.com> * Lint fix * Additional ChatGPT --> OpenAI GPT-4 fixes --------- Co-authored-by: Reed Loden <reed@goteleport.com> * Fix Assist rate-limiting in Cloud (#26342) When Proxy is separate from Auth, Proxy 'modules' will not contain meaningful data. Instead, one must use ClusterFeatures fetched from the Auth server * Assist UI improvements (#26365) * Update assist warning wording, add link to ToS (#26396) * Always render the portal for the assist title to go into (#26733) * Stop using TokenPath for API key in Assist (#26671) * [Assist] MFA support (#26719) * Initial support for MFA in Assist * UI webauth handler * WebUI - WIP * Run prettier * Perform MFA ceremony only once. * Cleanup JS * Remove hacky WS logic * Add cancel MFA logic * [Assist] Prevent creating messages without conversation (#26797) * Add parallel execution to assist. (#26563) * Add parallel execution to assist. * Extract execution logic to a new function. * Add test * Switch uber to std * Address code review comments * Fix display of Assist command executions with empty output (#27010) * Fix display of Assist executions with empty output * Lint * Nit * Improve display of commands that failed with code * Address lint * [Assist] Allow removing assist conversations (#26788) * [Assist] Allow removing assist conversations * Display landing page after the conversion is removed * Improve styling and add a confirmation dialog * Change the icon opacity to copy the main navigation * Remove unused minus icon * Add missing trace.wrap --------- Co-authored-by: Ryan Clark <ryan.clark@goteleport.com> * Backport fixes * Regenerate go.sum to make the linter happy. * Sync files after rebase * After rebase fixes --------- Co-authored-by: Joel Wejdenstål <jwejdenstal@icloud.com> Co-authored-by: Justinas Stankevičius <justinas@users.noreply.github.com> Co-authored-by: Mike Jensen <jentfoo@users.noreply.github.com> Co-authored-by: Reed Loden <reed@goteleport.com> Co-authored-by: Ryan Clark <ryan.clark@goteleport.com>

jakule added the assist label May 8, 2023

jakule force-pushed the jakule/assist-api-backport branch 4 times, most recently from e9606ef to ead0cea Compare May 8, 2023 16:38

jakule marked this pull request as ready for review May 8, 2023 16:45

jakule requested a review from r0mant May 8, 2023 16:45

github-actions Bot requested review from fspmarshall and gabrielcorado May 8, 2023 16:45

github-actions Bot added the size/lg label May 8, 2023

jakule force-pushed the jakule/assist-api-backport branch from ead0cea to e87cb3b Compare May 8, 2023 16:47

jakule commented May 8, 2023

View reviewed changes

Comment thread lib/web/assistant.go Outdated

Copy link
Copy Markdown

Contributor Author

jakule May 8, 2023

Choose a reason for hiding this comment

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

The API will be backported in a separate PR

r0mant approved these changes May 8, 2023

View reviewed changes

r0mant requested review from tigrato and zmb3 and removed request for fspmarshall, gabrielcorado and tigrato May 8, 2023 22:09

zmb3 reviewed May 8, 2023

View reviewed changes

jakule force-pushed the jakule/assist-api-backport branch from 1489245 to e07370d Compare May 9, 2023 02:12

jakule requested review from r0mant and zmb3 May 9, 2023 02:34

zmb3 approved these changes May 9, 2023

View reviewed changes

jakule added 7 commits May 9, 2023 13:02

Assist - API implementation

5bb1824

Add Assistant CRUD test

d890cf8

Move assistant API to a separate file.

a265291

Rename GRPC methods

9d9d437

Fix comments

Address PR comments

5840e67

Update GRPC API - addressing code review comments

20b1687

Move assist API to a new GRPC service

00b4caa

jakule force-pushed the jakule/assist-api-backport branch from 939152a to aea37a3 Compare May 9, 2023 17:05

Update comment.

6c8c768

jakule mentioned this pull request May 9, 2023

Remote site endpoints for Teleport Assist #25933

Closed

jakule enabled auto-merge May 9, 2023 17:10

jakule added this pull request to the merge queue May 9, 2023

kimlisa reviewed May 9, 2023

View reviewed changes

Merged via the queue into master with commit bc71a79 May 9, 2023

jakule deleted the jakule/assist-api-backport branch May 9, 2023 18:09

jakule mentioned this pull request May 9, 2023

[v12] Assist - API implementation #25956

Merged

jakule mentioned this pull request May 9, 2023

Move username and ID out of AssistantMessage #25964

Merged

	// conversation_id it's the ID of a conversation.
	// ConversationID it's the ID of a conversation.

	// conversationId it's the ID of a conversation.
	// ConversationId identifies a conversation.

	// conversationId used to tie all messages in a one conversation.
	// ConversationId is used to tie all messages into a conversation.

	// createdTime is the time when the even occurred.
	// CreatedTime is the time when the event occurred.

		// Allows executing an arbitrary command on multiple nodes.
		h.GET("/webapi/command/:site/execute", h.WithClusterAuth(h.executeCommand))

Conversation

jakule commented May 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

r0mant left a comment

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!

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!

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!

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!

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!

zmb3 left a comment

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!

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!

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!

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!

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!

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!

jakule commented May 8, 2023 •

edited

Loading