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

Namespace Management APIs #8

Merged
merged 18 commits into from
Nov 17, 2023
97 changes: 95 additions & 2 deletions temporal/api/cloud/cloudservice/v1/request_response.proto
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,14 @@ option go_package = "go.temporal.io/api/cloud/cloudservice/v1;cloudservice";

import "temporal/api/cloud/operation/v1/message.proto";
import "temporal/api/cloud/identity/v1/message.proto";
import "temporal/api/cloud/namespace/v1/message.proto";

message GetUsersRequest {
// The requested size of the page to retrieve
int32 page_size = 1;
// The page token
string page_token = 2;
// Optional field to filter users by email address
// Filter users by email address - optional
string email = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

Completely unrelated, feel free to punt this comment for another day.

Should we have allowed a list of emails? Then this can operate as a batch Get operation (as opposed to just single email). Ditto for IDs.

Copy link
Member

Choose a reason for hiding this comment

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

👍 At the very least this should clarify whether it's exact match or partial match

}

Expand Down Expand Up @@ -94,7 +95,7 @@ message SetUserNamespaceAccessRequest {
}

message SetUserNamespaceAccessResponse {
// The request status of the update operation
// The async operation
temporal.api.cloud.operation.v1.AsyncOperation async_operation = 1;
}

Expand All @@ -107,3 +108,95 @@ message GetAsyncOperationResponse {
// The async operation
temporal.api.cloud.operation.v1.AsyncOperation async_operation = 1;
}

message CreateNamespaceRequest {
// The prefix to use for the namespace
// will create a namespace that's available at '<namespace_prefix>.<account>.tmprl.cloud:7233'
Copy link
Member

Choose a reason for hiding this comment

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

You are inconsistent with capitalization of comments. Is will here supposed to start a new phrase/sentence? Also, most proto doc generators do not consider a newline as a sentence break so this will just run on (as will others). I mentioned on last review, I would strongly recommend complete sentences and proper documentation.

string namespace_prefix = 1;
Copy link

@jlacefie jlacefie Oct 13, 2023

Choose a reason for hiding this comment

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

why is the namespace_prefix not a part of the namespace spec?
how/where will users receive the "fully qualified" namespace name in this API spec as indicated in the comment of the prefix field?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes makes sense to have the namespace_prefix in the spec. Moving to spec.
The fully qualified namespace name will be in the create-namespace response.

// The namespace specification
temporal.api.cloud.namespace.v1.NamespaceSpec spec = 2;
// The id to use for this async operation - optional
string async_operation_id = 3;
}

message CreateNamespaceResponse {
anekkanti marked this conversation as resolved.
Show resolved Hide resolved
// The namespace that was created
string namespace = 1;
// The async operation
temporal.api.cloud.operation.v1.AsyncOperation async_operation = 1;
}

message GetNamespacesRequest {
// The requested size of the page to retrieve
Copy link
Member

Choose a reason for hiding this comment

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

May want to clarify the default (but don't have to)

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a default and a max to the comment.

int32 page_size = 1;
// The page token
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// The page token
// The page token if this is continuing from another response

Not obvious to users that this is not required

Copy link
Member Author

Choose a reason for hiding this comment

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

added as suggested.

string page_token = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Will we add more filters in a follow-up? Things like region (or regions) etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Absolutely with time.

}

message GetNamespacesResponse {
// The list of namespaces
repeated temporal.api.cloud.namespace.v1.Namespace namespaces = 1;
anekkanti marked this conversation as resolved.
Show resolved Hide resolved
// The next page's token
string next_page_token = 2;
}

message GetNamespaceRequest {
// The namespace to get
string namespace = 1;
}

message GetNamespaceResponse {
// The namespace
temporal.api.cloud.namespace.v1.Namespace namespace = 1;
}

message UpdateNamespaceRequest {
// The namespace to update
string namespace = 1;
// The new namespace specification
temporal.api.cloud.namespace.v1.NamespaceSpec spec = 2;
anekkanti marked this conversation as resolved.
Show resolved Hide resolved
// The version of the namespace for which this update is intended for
// The latest version can be found in the namespace status
string resource_version = 3;
// The id to use for this async operation - optional
string async_operation_id = 4;
}

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

message RenameCustomSearchAttributeRequest {
anekkanti marked this conversation as resolved.
Show resolved Hide resolved
// The namespace to rename the custom search attribute for
string namespace = 1;
// The existing name of the custom search attribute to be renamed
string existing_custom_search_attribute_name = 2;
// The new name of the custom search attribute
string new_custom_search_attribute_name = 3;
// The version of the namespace for which this update is intended for
// The latest version can be found in the namespace status
string resource_version = 4;
// The id to use for this async operation - optional
string async_operation_id = 5;
}

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

message DeleteNamespaceRequest {
// The namespace to delete
string namespace = 1;
// The version of the namespace for which this delete is intended for
// The latest version can be found in the namespace status
string resource_version = 2;
// The id to use for this async operation - optional
string async_operation_id = 3;
}

message DeleteNamespaceResponse {
// The async operation
temporal.api.cloud.operation.v1.AsyncOperation async_operation = 1;
}
45 changes: 45 additions & 0 deletions temporal/api/cloud/cloudservice/v1/service.proto
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,49 @@ service CloudService {
get: "/api/v1/operations/{async_operation_id}",
};
}

// Create a new namespace
rpc CreateNamespace (CreateNamespaceRequest) returns (CreateNamespaceResponse) {
option (google.api.http) = {
post: "/api/v1/namespaces",
body: "*"
};
}

// Get all namespaces
rpc GetNamespaces (GetNamespacesRequest) returns (GetNamespacesResponse) {
option (google.api.http) = {
get: "/api/v1/namespaces",
};
}

// Get a namespace
rpc GetNamespace (GetNamespaceRequest) returns (GetNamespaceResponse) {
option (google.api.http) = {
get: "/api/v1/namespaces/{namespace}",
};
}

// Update a namespace
rpc UpdateNamespace (UpdateNamespaceRequest) returns (UpdateNamespaceResponse) {
option (google.api.http) = {
post: "/api/v1/namespaces/{namespace}",
body: "*"
};
}

// Rename an existing customer search attribute
rpc RenameCustomSearchAttribute (RenameCustomSearchAttributeRequest) returns (RenameCustomSearchAttributeResponse) {
option (google.api.http) = {
post: "/api/v1/namespaces/{namespace}/rename-custom-search-attribute",
body: "*"
};
}

// Delete a namespace
rpc DeleteNamespace (DeleteNamespaceRequest) returns (DeleteNamespaceResponse) {
option (google.api.http) = {
delete: "/api/v1/namespaces/{namespace}",
};
}
}
93 changes: 93 additions & 0 deletions temporal/api/cloud/namespace/v1/message.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
syntax = "proto3";

package temporal.api.cloud.namespace.v1;

option go_package = "go.temporal.io/api/cloud/namespace/v1;namespace";

import "temporal/api/cloud/sink/v1/message.proto";

import "google/protobuf/timestamp.proto";

message CertificateFilterSpec {
Copy link
Member

Choose a reason for hiding this comment

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

Clarify whether fields are optional and how the filter works (exact string match for all present fields?)

Copy link
Member

Choose a reason for hiding this comment

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

Bump, unsure if this does exact string matches or partial string matches, and unsure whether any field must match or all fields must match.

jlacefie marked this conversation as resolved.
Show resolved Hide resolved
// The common_name in the certificate
string common_name = 1;
// The organization in the certificate
string organization = 2;
// The organizational_unit in the certificate
string organizational_unit = 3;
// The subject_alternative_name in the certificate
string subject_alternative_name = 4;
}

message CodecServerPropertySpec {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
message CodecServerPropertySpec {
message CodecServerSpec {

Not sure what the "property" word means with regards to codec servers. Also, consider setting this as UICodecServerSpec (and changing field) if it only applies to the UI.

Choose a reason for hiding this comment

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

+1 for removing property
The Cloud UI referes to this field as Codec Server only.
Recommend adding a comment to clarify the CodecServer purpose like in the UI

Enter a codec server endpoing to decode payloads for all users interacting with this Namespace in the UI.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to CodecServerSpec as suggested.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated comment. Please review.

// Server endpoints

Choose a reason for hiding this comment

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

nit - codec server endpoint

Also, does this take in 1 endpoint or many? the plural endpoints in the comment is ambiguous

Copy link
Member Author

Choose a reason for hiding this comment

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

Its just one, fixed the comments.

string endpoint = 1;
// Whether to pass access token, i.e. jwt

Choose a reason for hiding this comment

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

nit - comment should align to the UI to indicate where the JWT ends up.
UI comment - Pass the user access token with your endpoint.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack fixed, as mentioned in the UI.

bool pass_access_token = 2;
// Whether to include credentials
bool include_credentials = 3;
Copy link
Member

@cretz cretz Oct 10, 2023

Choose a reason for hiding this comment

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

Suggested change
bool include_credentials = 3;
bool include_cross_origin_credentials = 3;

Is that correct, this is for some kind of CORS credentials? I tried to read https://docs.temporal.io/dataconversion#cors but it was unclear. Can you explain (can do here, don't have to update proto) what these "credentials" are? EDIT: Looks like https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Credentials explains it, should still change the name

Choose a reason for hiding this comment

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

+1 ^ CORS should be explicit. Pls update the comment as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed.

}

message NamespaceSpec {
Copy link
Member

Choose a reason for hiding this comment

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

It is unclear what is and isn't required here on every field. Do I have to provide a region for my namespace? What about an accepted client CA?

Copy link
Member Author

Choose a reason for hiding this comment

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

Marked each field if they are optional or immutable.

Copy link
Member

Choose a reason for hiding this comment

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

Do I assume if it doesn't say optional then it's required? Arguably also saying something is required is simpler than me having to infer it by a lack of "optional".

Choose a reason for hiding this comment

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

nit. indicate the number of regions supported in the comment with the understanding that the comment can change as we open up the possibility of more regions. For context, the plural name indicates more than 1

Copy link
Member Author

Choose a reason for hiding this comment

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

@jlacefie we don't support all regions for all accounts. We plan on adding a GetRegions account endpoint that the developer can use to list all supported regions for their account.

jlacefie marked this conversation as resolved.
Show resolved Hide resolved
// The region where the namespace is (or will be) active
string region = 1;
// The base64 encoded ca cert(s) that the clients can use for authentication and authorization
string accepted_client_ca = 2;
Copy link
Member

@cretz cretz Oct 10, 2023

Choose a reason for hiding this comment

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

Auth needs to be broken off of this message because I don't believe we will always require MTLS forever so it'll become optional (not to mention other auth we may configure at this level such as UI SSO, cross-signing keys for API keys (though unlikely), etc). For example:

MtlsAuth mtls_auth = 2;

and

message MtlsAuth {
    // Accepted CAs, PEM formatted. Currently this can/must only be one value, but the
    // CA can have a chain.
    repeated string accepted_client_ca = 1;

    // Filters that must match for auth to succeed. If unset, all client certificates match that
    // are issued from one of the CAs.
    repeated CertificateFilterSpec certificate_filters = 2;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack, like the restructuring. Fixed.

// The number of days the workflows data will be retained for

Choose a reason for hiding this comment

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

comment nit - align with the UI comment to indicate the impact of changing retention period
UI description
Changes to the retention period may impact your storage costs.
Any changes to the retention period will be applied to all new running workflows.

Copy link
Member Author

Choose a reason for hiding this comment

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

ack fixed as suggested.

int32 retention_days = 3;
jlacefie marked this conversation as resolved.
Show resolved Hide resolved
// The custom search attributes to use for the namespace
// The name of the attribute is the key and the type is the value
// Supported attribute types: text, keyword, int, double, bool, datetime, keyword_list
map<string, string> search_attributes = 4;
Copy link
Member

Choose a reason for hiding this comment

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

If/when combined with regular API, one might expect this value to be temporal.api.enums.v1.IndexedValueType

Choose a reason for hiding this comment

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

recommend custom_search_attributes to align with the UI and docs.
Also, to Chad's point above, are search_attributes referenced in line 39 the same as what the OSS API refers to as custom_search_attribute_aliases?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes they will be mapped to the temporal.api.enums.v1.IndexedValueType

// Certificate filters which, if specified, only allow connections from client certificates
// whose distinguished name properties match at least one of the filters
repeated CertificateFilterSpec certificate_filters = 5;
// Environment of the namespace. - optional
// NOTE: currently there is no additional SLA or functional guarantee implied by the value of this field.
// supported environments: dev, test, prod
string environment = 6;
Copy link
Member

Choose a reason for hiding this comment

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

Can you help me understand what "environment" means? Just a link to some docs would work. I am trying to understand why we have 3 fixed values instead of any arbitrary string and where these 3 values are used.

Choose a reason for hiding this comment

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

May we not include environment yet please? This seems premature and something that can be added later after we are aligned with the way we will expose hierarchy containers and metadata to users.

// Codec server property spec needed for user to set and retrieve - optional
CodecServerPropertySpec codec_spec = 7;
Copy link
Member

Choose a reason for hiding this comment

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

You are inconsistent with your field and message naming. Sometimes when you have Spec in the message name, you put _spec in the field name, and sometimes you don't. Sometimes you match the non-Spec parts of the message name in the field and sometimes you don't. Would strongly encourage extreme consistency with API naming.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to coded_server.

// The regions where the namespace is (or will be) located - optional
repeated string passive_regions = 8;
Copy link
Member

Choose a reason for hiding this comment

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

Fields are a bit scattered here. You have something about a region at the top then more down here, you have some CA stuff at the top then more CA stuff a few fields later. Try to combine like fields at least adjacent (if not off in their own message) just so users can at least understand all options for a feature/component.

Choose a reason for hiding this comment

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

is this for Global Namespace? If so, is passive_region the right term here? This feels like a one-way door.

Also, will we offer multiple regions in the future? If so, passive seems restricting as it implies one option.
This item in particular needs more thought IMO.

// The export sink specifications keyed on the sink name - optional
map<string, temporal.api.cloud.sink.v1.ExportSinkSpec> export_sinks = 9;
Copy link
Member Author

Choose a reason for hiding this comment

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

Do we want to make the export sinks a sub resource?

Copy link

@robzienert robzienert Oct 9, 2023

Choose a reason for hiding this comment

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

For my particular use case, it would be nice to be able to reference a common export sink config since we use the same config globally across all namespaces (as history export is a platform-managed feature here). I don't feel particularly strongly here, however.

Copy link
Member Author

@anekkanti anekkanti Oct 10, 2023

Choose a reason for hiding this comment

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

@robzienert thanks for this feedback.
We are tracking this as a feature as part of the "hierarchy" story where various configurations like ca-certs, search-attrs, export configs can trickle down a hierarchy sub-tree as defaults...
cc: @jlacefie @sergeybykov

Choose a reason for hiding this comment

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

TY - Rob. We released the first Export per Namespace with the understanding that some Namespace owners would like isolation at the S3 bucket. In a subsequent release we can provide Account level affordances for a default.

Copy link
Member

Choose a reason for hiding this comment

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

Are sink specs expected to be referenced/used outside of namespace spec? Meaning, do they deserve their own package if this is their only use?

Copy link
Member

Choose a reason for hiding this comment

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

Arguably "export" is too generic of a word for this. Is this just history export? Or is this also log export, audit export, user export, and all other exports we may support henceforth?

}

message NamespaceURI {
Copy link
Member

Choose a reason for hiding this comment

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

This isn't a URI, this is a message that only some of the fields happen to be URIs. Consider either just inlining these fields into the namespace or changing this to "Endpoints" or something.

Copy link
Member Author

Choose a reason for hiding this comment

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

renamed to NamespaceEndpoints

Choose a reason for hiding this comment

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

what is the purpose of this message?

Copy link
Member Author

Choose a reason for hiding this comment

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

Its the information that the customer will needs to locate the namespace's UI and the URL that the temporal client should connect to.

// The web ui address
string web = 1;
// The grpc address
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// The grpc address
// The grpc host:port

Just so people don't think it's a real URL with a scheme like web will be. Arguably this shouldn't even be named "grpc" because many users don't know/care that that happens to be our API protocol today, but meh.

string grpc = 2;
// The list of private links
repeated string vpc_endpoint_service_names = 3;
}

message NamespaceEnvelope {
// The namespace may be throttled if its APS exceeds the limit
int32 actions_per_second_limit = 1;
}

message Namespace {
// The namespace name
string namespace = 1;
Copy link
Member

Choose a reason for hiding this comment

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

One might appreciate the different pieces of the namespace broken out (i.e. in addition to this, also the namespace prefix, account ID). But I'm ok leaving like this if we promise it's always a single . separating the two pieces.

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't think we plan on having any other format.
But even if we do, don't know if the customer should really know. For them this should be a opaque string that they should use while operating the namespace.
I like the idea of keeping it opaque, so we have the flexibility to change the format, however unlikely it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if calling namespace in all these requests as id is clearer. Just because of the confusion around namespace name vs <name>.<account_id>.

That said, I understand that this aligns with what we call it in the OSS APIs.

Copy link
Member

Choose a reason for hiding this comment

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

It's much less clear to try to use id because "namespace ID" is another thing. It is good that namespace aligns across all Temporal APIs.

// The current version of the namespace specification
// The next update operation will have to include this version
string resource_version = 2;
// The namespace specification
NamespaceSpec spec = 3;
// The current state of the namespace
string state = 4;
Copy link
Member

Choose a reason for hiding this comment

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

While I disagree with not using enums for enums, every place you use these "stringly-typed enums" you should at least document all enumerate possibilities

Choose a reason for hiding this comment

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

@anekkanti is there a reason for not using enums as described here ^ is that an explicit decision?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is an explicit decision that we took based on our experience with the existing internal APIs.

// The id of the async operation that is creating/updating/deleting the namespace, if any
string async_operation_id = 5;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
string async_operation_id = 5;
string last_async_operation_id = 5;

Just a bit clearer to me, but not required

// The web uri for the namespace
NamespaceURI uri = 6;
// The envelope is a list of service level agreements (SLAs) that can be provided around a given namespace
NamespaceEnvelope envelope = 7;

Choose a reason for hiding this comment

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

Awesome.

Choose a reason for hiding this comment

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

@robzienert TY for the comment here. What is exciting about this one for you? Also, do you have an opinion on the term Envelope in this context?

Copy link
Member

@cretz cretz Oct 10, 2023

Choose a reason for hiding this comment

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

I don't think "Envelope" is a good word here (that term has many meanings in the API/RPC world), can we think of another? Maybe "Limits"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why?
We do talk about envelops when talking about our cloud offering:
https://docs.temporal.io/cloud/operating-envelope

Copy link
Member

@cretz cretz Oct 16, 2023

Choose a reason for hiding this comment

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

I think it probably wasn't necessarily a good choice of words there either (in code, an envelope is often a "wrapper" or container something is in when making calls), but if this is general SaaS nomenclature for limits, ok (we are the first I see on sla envelope Google search). I was just confused as a dev and I fear we made up a term here. Maybe at least make it OperatingEnvelope.

// Allowed principals is a list of principals that allowed to access the private links on the namespace
repeated string allowed_principals = 8;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
repeated string allowed_principals = 8;
repeated string allowed_private_link_principals = 8;

The field name was as if you were limiting principal across the namespace and not specific for private link. Also, consider colocating common fields (e.g. ones about private links).

Choose a reason for hiding this comment

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

This field is confusing for 2 reasons:

  1. what chad stated. comment states Private Link are these specific to Private Link?
  2. we do not have a concept of a principal in Temporal yet. We have users. What is a principal? I know this term has been discussed in the context of Service Accounts but it's not defined yet.

Choose a reason for hiding this comment

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

It looks like main includes Identity. Could we use Identity here if that's the intent of this field, please?

// The export sink status keyed on the sink name
map<string, temporal.api.cloud.sink.v1.ExportSink> export_sinks = 9;
// The date and time when the namespace was last modified
google.protobuf.Timestamp last_modified_time = 10;
}
45 changes: 45 additions & 0 deletions temporal/api/cloud/sink/v1/message.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
syntax = "proto3";

package temporal.api.cloud.sink.v1;

option go_package = "go.temporal.io/api/cloud/sink/v1;sink";

import "google/protobuf/timestamp.proto";

message ExportSinkSpec {
Copy link
Member

Choose a reason for hiding this comment

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

Consider a unique, user-provided identifier for a sink (e.g. a name) if you want to support updating of these inside a list. Not required of course, but can be easier on you, the UI, and the user.

Choose a reason for hiding this comment

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

Where do users apply the sink message to a Namespace? It doesn't appear to be used in either the service or namespace protos? Please make sink an option when creating a namespace and provide users the ability to add a sink to a namespace at a later time.

// Whether the sink is enabled
bool enabled = 1;
// The destination of the sink
oneof destination {
// The AWS S3 destination spec
S3Spec s3_spec = 2;
}
}

message ExportSink {
// The state of the sink
Copy link
Member

Choose a reason for hiding this comment

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

Mentioned before (so will stop mentioning), but please provide all possible string enumerate values if we're set on not using properly defined enums

string state = 1;
// The health of the sink
string health = 2;
// The error message of the sink if any
string error_message = 3;
// The latest data export time
google.protobuf.Timestamp latest_data_export_time = 4;
// The latest health check time
google.protobuf.Timestamp last_health_check_time = 5;
}

message S3Spec{
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
message S3Spec{
message S3Spec {

// The role to be created that Temporal Cloud assumes for writing records to customer's S3 bucket
string role_name = 1;
// Destination S3 bucket name for us to send data to.
string bucket_name = 2;
// The region of the S3 bucket
string region = 3;
// The kms key ARN used for encryption
string kms_arn = 4;
// The aws account id of s3 bucket and assumed role
string aws_account_id = 5;
}


Loading