Cloud API: Nexus Incoming/Outgoing Service Definitions #16

mastermanu · 2024-03-12T15:57:13Z

No description provided.

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

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

anekkanti

Can we make sure to make all the comments full sentences. Meaning starts with a capitalized word and ends with a period.

anekkanti · 2024-03-13T15:04:39Z

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

+    // Filter incoming services by the namespace they route to - optional.
+    string namespace = 3;
+
+    // Filter incoming services by the task queue they route to - optional.


nit: Do we really need to support filtering with task_queue? Can't think of a usecase.

My thought was that we might multiple services across different environments (test, stage, dev, prod) where the task queue is the same, but the namespace varies. @bergundy @prasek if this makes sense or not

spoke offline, we think this is useful to have this filter here

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

anekkanti · 2024-03-13T15:23:48Z

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

+
+    // The set of authorization policies for the service. Each request's caller
+    // must match with at least one of the specs to be accepted by the incoming service
+    repeated IncomingServiceAuthorizationSpec authorization_spec = 4;


Does the ordering of these specs mean anything? If yes, we need to make sure we can retrieve the same ordering that the user provides.

i think the ordering doesn't matter here (minus any form of short-circuit optimization), but cc @prasek @bergundy if you have thoughts

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

anekkanti · 2024-03-13T15:34:58Z

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

+    CloudIncomingServiceSpec cloud_incoming_service_spec = 2;
+}
+
+message CloudIncomingServiceSpec {


Suggested change

message CloudIncomingServiceSpec {

message TargetCloudIncomingServiceSpec {

Otherwise its very confusing with the IncomingServiceSpec that we already have.

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

mastermanu · 2024-03-13T16:14:34Z

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

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


mention eventual consistency here?

bergundy · 2024-03-13T17:56:29Z

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

@@ -229,3 +230,187 @@ message GetRegionResponse {
    // The temporal cloud region.
    temporal.api.cloud.region.v1.Region region = 1;
 }
+
+message GetIncomingServicesRequest {


Wonder why not call this "list" instead of "get", I find this confusing but I figure it's consistent with other cloud APIs.

Please mention if these are AND or OR filters in the comment for this message.

yeah the list vs get is for consistency with existing cloud apis. could have been list.

these are AND filters. will update comments

bergundy · 2024-03-13T18:03:22Z

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

+
+message GetIncomingServiceRequest {
+    // The id of the incoming service to get
+    string incoming_service_id = 1;


nit: can we just call this id? it's already in the context of an GetIncomingServiceRequest.

we can. I was trying to be consistent with this one: https://github.com/temporalio/api-cloud/blob/main/temporal/api/cloud/cloudservice/v1/request_response.proto#L33 but I definitely wish that used id

will leave as-is for now unless there is strong feeling about it

bergundy · 2024-03-13T18:22:10Z

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

+    string next_page_token = 2;
+}
+
+message GetIncomingServiceRequest {


Suggested change

message GetIncomingServiceRequest {

message GetIncomingServiceByIdRequest {

Just in case we want to add a "by name" request?
The list approach is also fine, just a bit harder to use IMHO.

Can it simply be 'GetIncomingServiceRequest' and if/when we add by name, we'd add 'GetIncomingServiceByNameRequest'? Or is this against a convention for the 'ById' suffix we already use? I see that in

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

Line 31 in 4fff2fd

message GetUserRequest {

we named a similar message 'GetUserRequest' even though it takes a user ID.

agreeing with sergey here. am inclined towards leaving this as is for now (e.g. GetIncomingServiceRequest) just to be consistent with existing cloud user apis

the weirdness is that when we translate this to HTTP path, we would end up creating a different path at that time probably, but not the end of the world if we have to introduce that

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

bergundy · 2024-03-13T18:32:28Z

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

+    // The name of the outgoing service to filter on - optional. Specifying this will result in zero or one results.
+    string name = 4;


Not really useful as a filter, this is basically a Get request.

right, this is what we did for our Users api (users have a static id, but you call GetUsersRequset and can supply an email address), so this is just an attempt to stay consistent.

@anekkanti - thoughts? we basically followed the users model, which relies on this

bergundy · 2024-03-13T18:49:35Z

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

+    // Gets all known incoming services
+    rpc GetIncomingServices(GetIncomingServicesRequest) returns (GetIncomingServicesResponse) {
+	option (google.api.http) = {
+            get: "/api/v1/nexus/incomingservices",


Can we add a /by-id or /id suffix to this URL just in case we want to allow looking up by name eventually?

since I am following what we do for users, I've kept the same format (which doesn't have by-id or id either..). @anekkanti , thoughts?

I think for the default behavior (by ID) we don't need to add a suffix.

yes, am inclined to leave as-is as well. can always add suffix later if needed

bergundy · 2024-03-13T18:50:52Z

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

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


Why not make this an enum, I'm curious?

@anekkanti can comment here. We appear to have stayed away from enums elsewhere (and are using strings instead), so this is just being consistent with the existing cloud APIs

bergundy · 2024-03-13T18:52:00Z

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

+    // The name of the outgoing service. Must be unique within the namespace.
+    // The name must match `[a-zA-Z_][a-zA-Z0-9_]*`.
+    // This name is immutable.
+    string name = 1;


This is the identifier, doesn't belong in the spec AFAICT.

that seems to be what we have done for namespace and for outgoing service. it is a property a user can set (albeit only on create), so we happened to put it on spec. can try and move it off, but it does create some inconsistency with the other cloud apis

bergundy · 2024-03-13T18:52:56Z

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

+    // The namespace this outgoing service is attached to.
+    string namespace = 2;


Should this be = 1? It's essentially the first part of the identifier.

good point. reordered

…ming_service_draft_nexus

sergeybykov

Added a couple of minor comments. Otherwise, LGTM.

sergeybykov · 2024-03-13T20:08:12Z

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

+    string next_page_token = 2;
+}
+
+message GetIncomingServiceRequest {


Can it simply be 'GetIncomingServiceRequest' and if/when we add by name, we'd add 'GetIncomingServiceByNameRequest'? Or is this against a convention for the 'ById' suffix we already use? I see that in

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

Line 31 in 4fff2fd

message GetUserRequest {

we named a similar message 'GetUserRequest' even though it takes a user ID.

sergeybykov · 2024-03-13T20:12:24Z

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

+    // Gets all known incoming services
+    rpc GetIncomingServices(GetIncomingServicesRequest) returns (GetIncomingServicesResponse) {
+	option (google.api.http) = {
+            get: "/api/v1/nexus/incomingservices",


I think for the default behavior (by ID) we don't need to add a suffix.

bergundy · 2024-03-18T22:59:48Z

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

+    google.protobuf.Timestamp created_time = 6;
+
+    // The date and time when the service was last modified.
+    // Will not be set if the service has never been modified.


Curious why not default this to created_time? Is this for consistency with other APIs?

yeah, this is just for consistency. could have definitely been defaulted to created_time here

…ming_service_draft_nexus

bergundy · 2024-04-04T17:32:47Z

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

+    // The set of authorization policies for the service. Each request's caller
+    // must match with at least one of the specs to be accepted by the incoming service.
+    repeated IncomingServiceAuthorizationSpec authorization_specs = 4;


I think maybe instead of auth spec, we might want to have a general message to also add limits (rate, concurrency, etc..) and other properties for every caller (only namespaces for now).

cretz

I have no real comments/opinions on this one. From a high-level LGTM, but would defer to those with more familiarity with the behavior.

mastermanu · 2024-05-22T15:03:39Z

abandoning as there is replacement PR

anekkanti and others added 6 commits December 4, 2023 14:15

account get/udpate apis

82dfc84

address review comments

6336346

address review comments

bdc5c85

address review comments

ae200d8

init checkin

0c888a4

adds white space

a344890

mastermanu commented Mar 12, 2024

View reviewed changes

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

mastermanu commented Mar 12, 2024

View reviewed changes

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

mastermanu commented Mar 12, 2024

View reviewed changes

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

mastermanu commented Mar 12, 2024

View reviewed changes

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

mastermanu commented Mar 12, 2024

View reviewed changes

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

more changes

ca41698

mastermanu changed the title ~~[DRAFT] Nexus Incoming Service~~ [DRAFT] Nexus Incoming/Outgoing Service definitions Mar 12, 2024

more changes

fd0e337

mastermanu commented Mar 12, 2024

View reviewed changes

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

mastermanu commented Mar 12, 2024

View reviewed changes

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

mastermanu added 2 commits March 12, 2024 15:00

more changes

a5c6138

more changes

5a75261

mastermanu commented Mar 12, 2024

View reviewed changes

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

mastermanu requested review from anekkanti and nikki-dag March 12, 2024 19:08

mastermanu commented Mar 12, 2024

View reviewed changes

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

mastermanu commented Mar 12, 2024

View reviewed changes

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

mastermanu commented Mar 12, 2024

View reviewed changes

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

mastermanu added 5 commits March 13, 2024 10:46

more changes

9acae7e

remove extra .

57ff662

rename service_name --> service

45c4d09

comment cleanup

c622166

comment cleanup

88e7ec3

anekkanti reviewed Mar 13, 2024

View reviewed changes

mastermanu added 2 commits March 13, 2024 11:51

more changes

968eae8

more changse

0f16196

mastermanu commented Mar 13, 2024

View reviewed changes

mastermanu requested review from MichaelSnowden, sergeybykov and cretz and removed request for sergeybykov March 13, 2024 16:29

bergundy reviewed Mar 13, 2024

View reviewed changes

mastermanu added 3 commits March 13, 2024 15:40

Merge branch 'main' of github.com:temporalio/api-cloud into manu/inco…

b4aaedc

…ming_service_draft_nexus

add missing commas

9ccf3fb

addressed PR feedback

b62fc9c

sergeybykov approved these changes Mar 13, 2024

View reviewed changes

mastermanu changed the title ~~[DRAFT] Nexus Incoming/Outgoing Service definitions~~ Cloud API: Nexus Incoming/Outgoing Service Definitions Mar 13, 2024

bergundy reviewed Mar 18, 2024

View reviewed changes

mastermanu added 3 commits March 21, 2024 10:58

Merge branch 'main' of github.com:temporalio/api-cloud into manu/inco…

bf9771e

…ming_service_draft_nexus

more changes

db6d017

more changes

8bf55f9

bergundy reviewed Apr 4, 2024

View reviewed changes

cretz approved these changes Apr 4, 2024

View reviewed changes

mastermanu closed this May 22, 2024

	message CloudIncomingServiceSpec {
	message TargetCloudIncomingServiceSpec {

	message GetIncomingServiceRequest {
	message GetIncomingServiceByIdRequest {

		// The name of the outgoing service to filter on - optional. Specifying this will result in zero or one results.
		string name = 4;

		// The namespace this outgoing service is attached to.
		string namespace = 2;

Cloud API: Nexus Incoming/Outgoing Service Definitions #16

Cloud API: Nexus Incoming/Outgoing Service Definitions #16

Conversation

mastermanu commented Mar 12, 2024

anekkanti 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

mastermanu Mar 13, 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

sergeybykov 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

cretz left a comment

Choose a reason for hiding this comment

mastermanu commented May 22, 2024

mastermanu Mar 13, 2024 •

edited

Loading