Adds Nexus Endpoint #23

mastermanu · 2024-05-01T19:59:48Z

No description provided.

temporal/api/cloud/cloudservice/v1/service.proto

temporal/api/cloud/nexus/v1/message.proto

temporal/api/cloud/cloudservice/v1/request_response.proto

mastermanu · 2024-05-01T21:03:20Z

temporal/api/cloud/nexus/v1/message.proto

+message EndpointTargetSpec {
+    // The type of target that this endpoint routes Nexus requests to.
+    // Possible values are: task_queue
+    string target_type = 1;


note: this is essentially a poor person's version of oneof

I'm fine with this but I'd like to understand why we can't use what's standard.

will follow up with @anekkanti today just to have a solidified answer / reasoning here.

Same, seems unnecessary that you can't use the presence of the next field to determine type

Spoke with @anekkanti offline. as far as we can tell there are four options here:

use oneoff. I think we just decided not to use that because there were concerns about around how this manifests in other languages, and even the generated go code to read the field seems janky: https://protobuf.dev/reference/go/go-generated/#oneof. We were also worried that adding new types in the future would complicate versioning. Maybe both of these worries are unfounded. This is the first time we are adding something like this to the Cloud API, so worth laying the groundwork for this now. Curious if @cretz or @bergundy have thoughts on "oneoff" and cross-platform/language usage and any issues around that.

Don't use oneoff, but don't have the target_type field, just check that exactly one field is assigned a value. No reason we cannot do this. If I wanted to do a Get call for all endpoints of a specific target type, we would probably still need the string on that GetNexusEndpoints get request, but probably not a big deal.

Use target_type as an enum (I think there was a previous discussion about not using enums, we can re-open that if need be).

Use target_type as string (as is now). It's a bit more explicit, but agree its unnecessary. We'd probably still include it as an optional filter on GetNexusEndpoints() [but it can be added later down the line since there is only one type for now anyway]

Leaning towards option 2 (just get rid of target_type field). Any concerns with that?

I too lean towards option 2. Unrelated to this specifically, but I sure would like to re-open that never-use-proto-enum decision that was made long ago on this repo. I disagree with it.

ok cool. removed it completely for now. we have only one type so we can figure out if we want to add target_type later for the search/get

temporal/api/cloud/nexus/v1/message.proto

temporal/api/cloud/cloudservice/v1/request_response.proto

bergundy

Overall LGTM. I had a few small comments and one that I think we should discuss.

bergundy · 2024-05-03T13:47:34Z

temporal/api/cloud/cloudservice/v1/request_response.proto

+
+message GetNexusEndpointRequest {
+    // The id of the nexus endpoint to get.
+    string endpoint_id = 1;


I think you can just call this id, that's what I did in the operator API.

so I changed this to id, but this is now in-consistent with our other APIs related to user and namespace operations (where the request/path takes in user_id or namespace_id). am fine with having this as id, but will check with @anekkanti to be sure

We need to be consistent with the rest of the API here, that is more important than consistency with the self-hosted operator service

changed it back

bergundy · 2024-05-03T13:49:51Z

temporal/api/cloud/cloudservice/v1/request_response.proto

+    // The spec for the nexus endpoint.
+    temporal.api.cloud.nexus.v1.EndpointSpec spec = 1;
+
+    // The id to use for this async operation - optional.


Do we have guidance anywhere explaining why you'd want to specify this? I understand the concept but I'm not sure users do.

Agree that this probably needs to be fleshed out more here: https://docs.temporal.io/ops. The API itself is still in preview. I have seen other APIs have a section about "idempotency" which describes the usage/semantics around similar fields. We may want something similar her.

cc @anekkanti @jlacefie in case there is some other documentation I am missing here, or to potentially track ensuring docs cover this eventually

@rachfop since he is working on docs

bergundy · 2024-05-03T13:51:06Z

temporal/api/cloud/cloudservice/v1/service.proto

+    // Update a nexus endpoint
+    rpc UpdateNexusEndpoint(UpdateNexusEndpointRequest) returns (UpdateNexusEndpointResponse) {
+	option (google.api.http) = {
+            post: "/api/v1/nexus/endpoints/{endpoint_id}",


In OSS the route ends with /update to be consistent with other update methods.
Looks like Cloud took a different path.
CC @cretz

I almost missed this because it wasn't marked "ready for review" yet, heh.

We should combine these into one create-or-update, not sure the benefit of separating them. If you combine them you can use this URL, people expect POST to a resource like this to be able to create. If you must keep them separated, since we can't really support PUT accurately, I do think it deserves its own operation name.

Is there are good reason here or in operator service why we can't just have one mutation call?

So few things here:

User and Namespace have separate create and update endpoints. We can definitely combine it for Nexus, but then that is inconsistent with the rest of our current Cloud APIs

Having distinction between Create/Update has been generally useful for us given that it gives flexibility for the operations to have different auth permissions (and is actually the case for namespaces for instance). Note that from the Cloud Side, create generates an ID that must be passed for Update (same for namespace / user - we take the IDs not the name/email address). We can scrap the ID of the "id" and only rely on the user provided field, but having this decoupled ID seems better in the long-term.

Namespace and User have POST as well for update on the resource path itself. If we were to keep these separate, should we update the existing paths on those two types to have an /update suffix hanging off of the end as well? I would think so, but just double-checking.

cc @anekkanti to go over this as well

Synced with @anekkanti offline - we will add "update" suffix to the other update operation paths here and keep CREATE/UPDATE as separate operations for the reasons stated above.

Now that users can't provide their own ID, I think you should leave it as it is now. IIRC when I was reading before, it was a user-provided identifier which is why the ID-based URL made no sense to be update-only. Sorry if I misread and they never could provide their own ID. But since this is no longer a user-provided identifier, no suffix needed, the ID-based URL can clearly be for update. I hope that makes sense, sorry.

So no "update" suffix please. Match the other APIs in this repo.

temporal/api/cloud/nexus/v1/message.proto

bergundy · 2024-05-03T13:53:32Z

temporal/api/cloud/nexus/v1/message.proto

+message EndpointTargetSpec {
+    // The type of target that this endpoint routes Nexus requests to.
+    // Possible values are: task_queue
+    string target_type = 1;


I'm fine with this but I'd like to understand why we can't use what's standard.

temporal/api/cloud/nexus/v1/message.proto

bergundy · 2024-05-05T23:14:14Z

temporal/api/cloud/nexus/v1/message.proto

+    CloudTaskQueueSpec cloud_task_queue_spec = 2;
+}
+
+message CloudTaskQueueSpec {


In OSS I'm considering calling this Worker target.

changed it to CloudWorkerTargetSpec for now to be more aligned

bergundy · 2024-05-07T16:56:51Z

temporal/api/cloud/nexus/v1/message.proto

+    // The date and time when the endpoint was created.
+    google.protobuf.Timestamp created_time = 6;
+
+    // The date and time when the endpoint was last modified.
+    google.protobuf.Timestamp last_modified_time = 7;


@mastermanu can we also show the identity that created and modified this endpoint?

i think we can, although we'd probably want to consider adding that across all the objects we have (e.g. users, namespaces, etc).

would probably consider adding that as a separate effort.

cretz · 2024-05-07T20:02:03Z

temporal/api/cloud/cloudservice/v1/request_response.proto

+
+message GetNexusEndpointRequest {
+    // The id of the nexus endpoint to get.
+    string id = 1;


Suggested change

string id = 1;

string endpoint_id = 1;

Here and elsewhere to match what I see is already done in this repo

haha, yes, this is what it was before @bergundy commented, and gave the exact same response. happy to switch it back

We need to be consistent with ourselves, I updated that thread too

cretz · 2024-05-07T20:03:19Z

temporal/api/cloud/cloudservice/v1/request_response.proto

+    string page_token = 2;
+
+    // Filter endpoints based on their target specification - optional, treated as an AND if specified. Empty fields within the spec are treated as '*'.
+    temporal.api.cloud.nexus.v1.EndpointTargetSpec spec = 3;


I think to match what this API does, you should list out specific fields to filter on. I don't think it's consistent with other APIs here to accept an entire spec as a filter.

sure, can replace with flat fields

cretz · 2024-05-07T20:05:02Z

temporal/api/cloud/cloudservice/v1/service.proto

+       // Gets nexus endpoints
+    rpc GetNexusEndpoints(GetNexusEndpointsRequest) returns (GetNexusEndpointsResponse) {
+	option (google.api.http) = {
+            get: "/api/v1/nexus/endpoints",


Suggested change

get: "/api/v1/nexus/endpoints",

get: "/api/v1/cloud/nexus/endpoints",

Need to update the prefix to match others in this service (helps routing and may change soon anyways)

whoops, fixed

cretz · 2024-05-07T20:05:37Z

temporal/api/cloud/nexus/v1/message.proto

+    string name = 1;
+
+    // Indicates where the endpoint should forward received nexus requests to. 
+    EndpointTargetSpec endpoint_target_spec = 2;


Suggested change

EndpointTargetSpec endpoint_target_spec = 2;

EndpointTargetSpec target_spec = 2;

Inconsistent in whether you choose to prefix fields in this message with "endpoint_"

removed endpoint_ suffix

cretz · 2024-05-07T20:08:04Z

temporal/api/cloud/nexus/v1/message.proto

+    // Optional description. Can be in markdown for rendering in the UI.
+    // Note that this is an experimental field that may be deprecated in the future
+    string description = 4;


We support markdown-based descriptions in API elements? We have some other work trying to define the subset of markdown we're going to support. If we don't have description on namespaces or users or whatever, can we wait until we can consistently provide description on all of these types of resources instead of making Nexus endpoint the first? I support the idea, just think it should be generally applicable.

this is definitely open/experimental. we dont support description on any other resources (except for service accounts, which is also string description, and thats not released yet). Am fine removing this for now and adding it later. It's also why it was marked as "experimental" via a comment. @bergundy - any thoughts here?

I think we should remove pending a general discussion of description on resources (a good discussion to have, not sure Nexus PR is the place for that though)

removed for now. can add it in a separate PR after more discussion

I missed this conversation.

Our designs all include description in markdown, we have to keep this field.
I have requested that we make it a Payload so eventually, when we support account level codecs in the UI we can encrypt them.

cretz · 2024-05-07T20:08:46Z

temporal/api/cloud/nexus/v1/message.proto

+message EndpointPolicySpec {
+    // The policy set on this endpoint authorization spec.
+    // Possible values are: allowed_cloud_namespace
+    string policy_type = 1;


Is this necessary? Can the presence of the next field be enough to tell you what the policy type is?

cretz · 2024-05-07T20:09:09Z

temporal/api/cloud/nexus/v1/message.proto

+message EndpointTargetSpec {
+    // The type of target that this endpoint routes Nexus requests to.
+    // Possible values are: task_queue
+    string target_type = 1;


Same, seems unnecessary that you can't use the presence of the next field to determine type

temporal/api/cloud/cloudservice/v1/request_response.proto

bergundy · 2024-05-13T16:55:29Z

temporal/api/cloud/cloudservice/v1/request_response.proto

+message DeleteNexusEndpointResponse {
+    // The async operation
+    temporal.api.cloud.operation.v1.AsyncOperation async_operation = 1;
+}


nit: add empty line.

bergundy · 2024-05-13T16:56:12Z

temporal/api/cloud/nexus/v1/message.proto

+    CloudWorkerTargetSpec cloud_worker_target_spec = 1;
+}
+
+message CloudWorkerTargetSpec {


nit: I'd just call this Worker, it's not a cloud worker.

- Update endpoint name regex - Add endpoint description - Rename CloudWorkerTargetSpec to WorkerTargetSpec - Add next_page_token to GetNexusEndpointsResponse - Group nexus related request and responses together.

Sync Nexus Cloud API (temporalio#40)

Merge main + Remove /api/v1 prefix for HTTP paths

cretz · 2024-08-19T15:35:38Z

temporal/api/cloud/cloudservice/v1/service.proto

+	      option (google.api.http) = {
+            delete: "/cloud/nexus/endpoints/{endpoint_id}",
+        };
+    }


Suggested change

}

}

Is it time to add a formatter?

I think that can be discussed on the primary api repo and can apply to this if/when it moves there

cretz · 2024-08-19T15:36:12Z

temporal/api/cloud/cloudservice/v1/service.proto

@@ -179,6 +179,42 @@ service CloudService {
        };
    }

+    // Gets nexus endpoints
+    rpc GetNexusEndpoints(GetNexusEndpointsRequest) returns (GetNexusEndpointsResponse) {
+	      option (google.api.http) = {


Suggested change

option (google.api.http) = {

option (google.api.http) = {

Spacing off elsewhere for this too. Make sure you are not using tabs.

Fix spacing.

anekkanti · 2024-08-19T17:55:52Z

temporal/api/cloud/nexus/v1/message.proto

+    // The current state of the endpoint.
+    // Possible values: activating, activationfailed, active, updating, updatefailed, deleting, deletefailed, deleted
+    // For any failed state, reach out to Temporal Cloud support for remediation.
+    string state = 4;


Lets move to using an enum here.
@nikki-dag

We should reuse this: https://github.com/temporalio/api-cloud/pull/36/files#diff-b38afecfa7fefa137fb5dbe446fd43bd6603676748e6f061fcf1f03d2e386706R13

init checkin

c2976e4

mastermanu marked this pull request as draft May 1, 2024 19:59

mastermanu commented May 1, 2024

View reviewed changes

temporal/api/cloud/cloudservice/v1/service.proto Outdated Show resolved Hide resolved

mastermanu added 2 commits May 1, 2024 16:30

fixes stuff

d7c380d

fix build break

9081063

mastermanu requested a review from bergundy May 1, 2024 21:00

mastermanu assigned anekkanti and unassigned anekkanti May 1, 2024

mastermanu requested review from anekkanti, cretz and nikki-dag May 1, 2024 21:00

mastermanu commented May 1, 2024

View reviewed changes

temporal/api/cloud/nexus/v1/message.proto Show resolved Hide resolved

mastermanu commented May 1, 2024

View reviewed changes

temporal/api/cloud/cloudservice/v1/request_response.proto Outdated Show resolved Hide resolved

mastermanu commented May 1, 2024

View reviewed changes

temporal/api/cloud/nexus/v1/message.proto Outdated Show resolved Hide resolved

more changes

8e0dda3

bergundy reviewed May 1, 2024

View reviewed changes

temporal/api/cloud/cloudservice/v1/request_response.proto Outdated Show resolved Hide resolved

bergundy reviewed May 3, 2024

View reviewed changes

bergundy reviewed May 5, 2024

View reviewed changes

mastermanu added 4 commits May 7, 2024 09:59

change endpoint_id --> id

e2f3ed8

more changes

d80870a

more changes

0c99cbb

more changes

bdf4a13

mastermanu marked this pull request as ready for review May 7, 2024 14:18

bergundy reviewed May 7, 2024

View reviewed changes

cretz reviewed May 7, 2024

View reviewed changes

mastermanu added 4 commits May 7, 2024 16:36

address some of comments

c7ac7e6

more changes

b1af03b

more changes

dfa7aee

remove /update suffix

53bd255

mastermanu added 3 commits May 9, 2024 16:25

more changes

78eb26b

more changes

3a75ed7

more changes

7453e13

cretz approved these changes May 9, 2024

View reviewed changes

mastermanu added 2 commits May 9, 2024 21:07

changes

9760590

merge main

f7be5c4

bergundy reviewed May 13, 2024

View reviewed changes

temporal/api/cloud/cloudservice/v1/request_response.proto Show resolved Hide resolved

bergundy reviewed May 13, 2024

View reviewed changes

bergundy approved these changes May 13, 2024

View reviewed changes

cretz mentioned this pull request May 23, 2024

Nexus temporalio/cli#577

Merged

anekkanti mentioned this pull request Jun 4, 2024

[Breaking] Reorg the user-group apis with a explicit google group spec field. #33

Merged

nikki-dag and others added 5 commits August 16, 2024 11:39

Sync Nexus Cloud API (temporalio#40)

060abc0

- Update endpoint name regex - Add endpoint description - Rename CloudWorkerTargetSpec to WorkerTargetSpec - Add next_page_token to GetNexusEndpointsResponse - Group nexus related request and responses together.

Merge pull request #1 from temporalio/manu/nexus_endpoint

26c429c

Sync Nexus Cloud API (temporalio#40)

Merge remote-tracking branch 'upstream/main' into nikki/merge-main

5cb4b6b

Remove /api/v1 prefix for HTTP paths

80bc416

Merge pull request #2 from temporalio/nikki/merge-main

baa4151

Merge main + Remove /api/v1 prefix for HTTP paths

cretz approved these changes Aug 19, 2024

View reviewed changes

nikki-dag and others added 2 commits August 19, 2024 11:20

Fix spacing.

4bb0de1

Merge pull request #3 from nikki-dag/manu/nexus_endpoint

4d7f4da

Fix spacing.

anekkanti reviewed Aug 19, 2024

View reviewed changes

nikki-dag and others added 2 commits August 26, 2024 09:03

Nexus enum and oneof changes

2f131e2

Changes Nexus Endpoint state to enum and adds oneof variants

b281878

nikki-dag merged commit 0a01b74 into temporalio:main Aug 27, 2024
2 checks passed

	get: "/api/v1/nexus/endpoints",
	get: "/api/v1/cloud/nexus/endpoints",

	EndpointTargetSpec endpoint_target_spec = 2;
	EndpointTargetSpec target_spec = 2;

Adds Nexus Endpoint #23

Adds Nexus Endpoint #23

Conversation

mastermanu commented May 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mastermanu May 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergundy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz May 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz May 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mastermanu May 7, 2024 •

edited

Loading

cretz May 8, 2024 •

edited

Loading

cretz May 8, 2024 •

edited

Loading