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

fist pass on api key crud operations #25

Merged
merged 6 commits into from
May 22, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 77 additions & 0 deletions temporal/api/cloud/cloudservice/v1/request_response.proto
Original file line number Diff line number Diff line change
Expand Up @@ -234,3 +234,80 @@ message GetRegionResponse {
// The temporal cloud region.
temporal.api.cloud.region.v1.Region region = 1;
}

message GetAPIKeysRequest {
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 GetAPIKeysRequest {
message GetApiKeysRequest {

We're not making other acronyms like Mtls all-upper

// The requested size of the page to retrieve - optional.
// Cannot exceed 1000. Defaults to 100.
int32 page_size = 1;
// The page token if this is continuing from another response - optional.
string page_token = 2;
// Filter api keys by owner id - optional.
string owner_id = 3;
// Filter api keys by owner type - optional.
string owner_type = 4;
}

message GetAPIKeysResponse {
// The list of API keys in ascending ids order
Copy link
Member

@mastermanu mastermanu May 21, 2024

Choose a reason for hiding this comment

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

[Nit] "ascending ids order" --> "ascending id order"

repeated temporal.api.cloud.identity.v1.APIKey api_keys = 1;
Copy link
Member

@anekkanti anekkanti May 10, 2024

Choose a reason for hiding this comment

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

So in the list the user won't know the owner of each of the apikeys, right?
Maybe we should add identity type and id to the APIKey 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.

oh right let me add owner

Copy link
Member Author

Choose a reason for hiding this comment

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

done

// The next page's token
string next_page_token = 2;
}

message GetAPIKeyRequest {
// The id of the API key to get
string key_id = 1;
}

message GetAPIKeyResponse {
// The API key
temporal.api.cloud.identity.v1.APIKey api_key = 1;
}

message CreateAPIKeyRequest {
// The spec for the API key to invite
Copy link
Member

Choose a reason for hiding this comment

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

"the API key to invite" --> "the API key to create" ?

Copy link
Member

Choose a reason for hiding this comment

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

fixed

temporal.api.cloud.identity.v1.APIKeySpec spec = 1;
// The id to use for this async operation - optional
string async_operation_id = 2;
}

message CreateAPIKeyResponse {
// The id of the API Key created
string key_id = 1;
// The secret of the API Key created
string secret = 2;
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 we should call this token to make it clearer that this is the API key.

// The async operation
temporal.api.cloud.operation.v1.AsyncOperation async_operation = 3;
}

message UpdateAPIKeyRequest {
// The id of the API key to update
string key_id = 1;
// The new API key specification
temporal.api.cloud.identity.v1.APIKeySpec spec = 2;
// The version of the API key for which this update is intended for
// The latest version can be found in the GetAPIKey operation response
Copy link
Member

Choose a reason for hiding this comment

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

[Nit] Does the casing of this 'GetAPIKey' reference in the comment match the actual operation name casing?

Copy link
Member

Choose a reason for hiding this comment

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

Fixed.

string resource_version = 3;
// The id to use for this async operation - optional
string async_operation_id = 4;
}

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

message DeleteAPIKeyRequest {
// The id of the API key to delete
string key_id = 1;
// The version of the API key for which this delete is intended for
// The latest version can be found in the GetAPIKey operation response
string resource_version = 2;
// The id to use for this async operation - optional
string async_operation_id = 3;
}

message DeleteAPIKeyResponse {
// The async operation
temporal.api.cloud.operation.v1.AsyncOperation async_operation = 1;
}
37 changes: 37 additions & 0 deletions temporal/api/cloud/cloudservice/v1/service.proto
Original file line number Diff line number Diff line change
Expand Up @@ -125,4 +125,41 @@ service CloudService {
get: "/api/v1/cloud/regions/{region}",
};
}

// Get all known API keys
rpc GetAPIKeys (GetAPIKeysRequest) returns (GetAPIKeysResponse) {
option (google.api.http) = {
get: "/api/v1/cloud/api-keys",
};
}

// Get an API key
rpc GetAPIKey (GetAPIKeyRequest) returns (GetAPIKeyResponse) {
option (google.api.http) = {
get: "/api/v1/cloud/api-keys/{key_id}",
};
}

// Create an API key
rpc CreateAPIKey (CreateAPIKeyRequest) returns (CreateAPIKeyResponse) {
option (google.api.http) = {
post: "/api/v1/cloud/api-keys",
body: "*"
};
}

// Update an API key
rpc UpdateAPIKey (UpdateAPIKeyRequest) returns (UpdateAPIKeyResponse) {
option (google.api.http) = {
post: "/api/v1/cloud/api-keys/{key_id}",
body: "*"
};
}

// Delete an API key
rpc DeleteAPIKey (DeleteAPIKeyRequest) returns (DeleteAPIKeyResponse) {
option (google.api.http) = {
delete: "/api/v1/cloud/api-keys/{key_id}",
};
}
}
34 changes: 34 additions & 0 deletions temporal/api/cloud/identity/v1/message.proto
Original file line number Diff line number Diff line change
Expand Up @@ -69,3 +69,37 @@ message User {
// Will not be set if the user has never been modified.
google.protobuf.Timestamp last_modified_time = 8;
}

message APIKey {
// The id of the API Key
string key_id = 1;
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 key_id = 1;
string id = 1;

Other entities are not prefixing something to the ID here

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

Choose a reason for hiding this comment

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

Since we're not using proper enumerates, may need to define in docs the string literals expected here

Copy link
Member

Choose a reason for hiding this comment

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

addressed.

string state = 4;
// The id of the async operation that is creating/updating/deleting the API key, if any
string async_operation_id = 5;
// The date and time when the API key was created
google.protobuf.Timestamp created_time = 6;
// The date and time when the API key was last modified
// Will not be set if the API key has never been modified.
google.protobuf.Timestamp last_modified_time = 7;
}

message APIKeySpec {
// The id of the owner the api key belongs to
string owner_id = 1;
Copy link
Member

Choose a reason for hiding this comment

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

We should mark the owner_id, owner_type and expiry as immutable in the comments.

Copy link
Member

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

Also we should mention that the id here will be the id of the service account when the type is service account.

Copy link
Member

Choose a reason for hiding this comment

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

added comments.

// The type of the owner the api key belongs to
string owner_type = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since owner is not update-able, I think we should put the owner_id and owner_type into the APIKey message and then allow it as optional parameters into CreateAPIKey.

Copy link
Member

Choose a reason for hiding this comment

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

Talked about this offline, we will keep the owner_id and owner_type in the Spec to keep consistent with the the other apis, where all user provided info is in the spec, irrespective of their mutability.

// The display name of the API key
string display_name = 3;
// The description of the API key
string description = 4;
// The expiry time of the API key
google.protobuf.Timestamp expiry_time = 5;
// True if the API key is disabled
bool disabled = 6;
}
Loading