💥 [BREAKING] enable TLS if api key specified#2126
Conversation
cretz
left a comment
cretz
left a comment
There was a problem hiding this comment.
(leaving main review to Go SDK owners, just a couple of notes here)
| // TLS configures connection level security credentials. | ||
| TLS *tls.Config | ||
|
|
||
| // TLSDisabled explicitly disables TLS. When true, TLS will not be used even |
There was a problem hiding this comment.
When true, TLS will not be used
Reading the code here, this is not true, TLS will still be enable if the TLS field is set. IMO you should either 1) disallow both this being true and the TLS field being set (my preference), or 2) document that this being true means anything in the TLS field is ignored, and then respect that in code.
There was a problem hiding this comment.
Ah I should've tested this. Added validation to make it mutually exclusive + test
|
|
||
| func (apiKeyCredentials) applyToOptions(*ConnectionOptions) error { return nil } | ||
| func (apiKeyCredentials) applyToOptions(opts *ConnectionOptions) error { | ||
| // Auto-enable TLS when API key is provided and TLS is not explicitly set/disabled |
There was a problem hiding this comment.
Might as well document to users that we do this where API key is set by users (i.e. in the Godoc on the NewAPIKey* functions)
There was a problem hiding this comment.
added a little line to the new api key methods noting the auto TLS behavior
|
I'll note there is another approach where you instead add a method to |
Added validation to make |
cretz
left a comment
cretz
left a comment
There was a problem hiding this comment.
LGTM, but would want to wait for Go SDK owner(s) approval. No preference on approach @Quinn-With-Two-Ns said vs what's here (hopefully it's rarely/never used anyways, heh)
… methods that api key can enable TLS
THardy98
force-pushed
the
enable-tls-api-key
branch
from
f148251 to
2a0a678
Compare
When using API key authentication with Temporal Cloud, KEDA was explicitly creating a TLS config and passing it to the Temporal SDK. This overrode the SDK's built-in auto-TLS behavior for API key auth, causing 'tls: certificate required' errors because the explicit config lacked the client certificate that Temporal Cloud's mTLS endpoint expects. The Temporal Go SDK (since temporalio/sdk-go#2126) auto-enables TLS with the correct configuration when an API key is provided and TLS is not explicitly set. By only creating an explicit TLS config when the user has provided custom TLS settings (CA cert, server name, or unsafeSsl), we allow the SDK to handle the standard Temporal Cloud case correctly. Fixes kedacore#7443 Signed-off-by: rohansood10 <[email protected]>
4 participants
What was changed
DWISOTT
Part of Assume TLS enabled if API key provided in SDKs features#687
How was this tested:
internals/credentials_test.goAny docs updates needed?
Maybe