Account get and udpate apis #14
Conversation
| bool enabled = 1; | ||
| // The base64 encoded ca cert(s) in PEM format that clients connecting to the metrics endpoint can use for authentication. | ||
| // This must only be one value, but the CA can have a chain. | ||
| string accepted_client_ca = 2; |
There was a problem hiding this comment.
| string accepted_client_ca = 2; | |
| bytes accepted_client_ca = 2; |
Why not just accept the PEM instead of base64 encoded (which is what proto JSON will be anyways FWIW)
There was a problem hiding this comment.
We are accepting the cert as a string for the namespace spec.
https://github.com/temporalio/api-cloud/blob/main/temporal/api/cloud/namespace/v1/message.proto#L27
I would rather keep them consistent.
There was a problem hiding this comment.
This was my fault for not checking that well enough. PEM is already mostly base64, it's a bit weird to double encode. But I guess it's up to y'all.
There was a problem hiding this comment.
If we keep the double-encoding, we should add to the comment about this. Regardless of bytes or strings, I would have expected that base64 encoded ca cert(s) means PEM format.
There was a problem hiding this comment.
I think the idea with base64 encoding is to make it safer to pass pem formatted certs which has multi-line content that could cause all kind of issues if not handled correctly.
There was a problem hiding this comment.
Why is base64 string here easier than bytes for that use case? Of course when using via HTTP API or proto JSON, bytes is just base64 string. If you have concerns about character encoding like newlines, you should use bytes. I think it's unfortunate that because we didn't accept bytes before that's being used to disallow proper use of that data type.
There was a problem hiding this comment.
if we don't want to change the namespace spec (since I guess it's public preview), the other alternative is to just add another field which takes bytes to both account/namespace and then eventually deprecate the string field?
There was a problem hiding this comment.
Okay, changed the type to []byte. Which expects the raw pem certs.
| // The next update operation will have to include this version. | ||
| string resource_version = 2; | ||
| // The current state of the account. | ||
| // Possible values: activating, activationfailed, active, updating, updatefailed, deleting, deletefailed, deleted, suspending, suspendfailed, suspended. |
There was a problem hiding this comment.
remember, let's remove suspendfailed, suspending from here and other places
| @@ -60,6 +60,8 @@ message User { | |||
| // The user specification | |||
| UserSpec spec = 3; | |||
| // The current state of the user | |||
| // Possible values: activating, activationfailed, active, updating, updatefailed, deleting, deletefailed, deleted, suspending, suspendfailed, suspended. | |||
| // For any failed state, reach out to Temporal Cloud support for remediation. | |||
| string state = 4; | |||
There was a problem hiding this comment.
We should consider making this an enum, but up to y'all.
There was a problem hiding this comment.
This is becoming an enum in a different PR.
No description provided.