feat: allow configuring min tls for grpc #6320
Conversation
or-shachar
force-pushed
the
ors/grpc-min-tls
branch
from
52b4a84 to
4e822dd
Compare
| @@ -50,8 +53,12 @@ func LoadGrpcTLSCredentials(certDir string, server bool) (credentials.TransportC | |||
| } | |||
|
|
|||
| // Create the credentials and return it | |||
| minTlsVersion, err := kedatls.GetMinGrpcTlsVersion() | |||
| if err != nil { | |||
| ctrl.Log.WithName("grpc_tls_setup").Info(err.Error()) | |||
There was a problem hiding this comment.
we should imho return err here
There was a problem hiding this comment.
I followed the previous behavior that didn't fail the process for invalid value, but selected the default value + returned an error. I can change the function signature to not return an error and perform the logging in the function. wdyt?
There was a problem hiding this comment.
hm, I am not sure if we shouldn't just fail here. I mean, the grpc connection wouldn't be functional -> KEDA wouldn't be fully functional either.
WDYT @kedacore/keda-maintainers
There was a problem hiding this comment.
Why not functional? The error only means that it fell back to default value because the configuration value was wrong.
There was a problem hiding this comment.
iiuc, if this returns error, KEDA will default back to TLS 1.3 as min for gRPC. So it might again cause difficulties with boring crypto?
I am also leaning towards failing here because that will tell the user their desired and configured min TLS version wasn't respected. They can then either fix the configuration or knowingly leave the default to KEDA.
There was a problem hiding this comment.
Ok. I'd make it fail here but I'll keep the same behavior for the HTTP client, because I don't want to introduce breaking change.
There was a problem hiding this comment.
this is still pending, correct?
or-shachar
force-pushed
the
ors/grpc-min-tls
branch
2 times, most recently
from
ae7defe to
873f7f5
Compare
Signed-off-by: Or Shachar <[email protected]>
or-shachar
force-pushed
the
ors/grpc-min-tls
branch
from
873f7f5 to
3e2e6fa
Compare
Supporting kedacore/keda#6320 Signed-off-by: Or Shachar <[email protected]>
Supporting kedacore/keda#6320 Signed-off-by: Or Shachar <[email protected]> Signed-off-by: Or Shachar <[email protected]>
|
/run-e2e internal |
There was a problem hiding this comment.
@or-shachar we should also document this, this is existing docs for HTTP TLS settings: https://keda.sh/docs/2.16/operate/cluster/#http-tls-min-version
| @@ -68,6 +68,7 @@ Here is an overview of all new **experimental** features: | |||
|
|
|||
| ### Improvements | |||
|
|
|||
| - **General**: Allow configuring minimum TLS version for GRPC client via `KEDA_GRPC_MIN_TLS_VERSION` ([#6320](https://github.com/kedacore/keda/pull/6320)) | |||
This will allow configuring grpc min grpc version very similarly to how we allow configuring the HTTP version.
Checklist
Docs: kedacore/keda-docs#1497
Fixes #6270