add tls min/max version to grpc proxy #18816
Conversation
tjungblu
force-pushed
the
mintls_grpcproxy
branch
from
98eb3ab
to
3149bcf
Compare
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files
... and 22 files with indirect coverage changes @@ Coverage Diff @@
## main #18816 +/- ##
==========================================
+ Coverage 68.72% 68.74% +0.02%
==========================================
Files 420 420
Lines 35532 35558 +26
==========================================
+ Hits 24418 24444 +26
Misses 9681 9681
Partials 1433 1433 Continue to review full report in Codecov by Sentry.
|
|
happy to add some in a separate PR, because for grpc proxy we have almost nothing 🙀 |
tjungblu
force-pushed
the
mintls_grpcproxy
branch
from
3149bcf
to
463eef9
Compare
|
just added a basic e2e that those flags work for starting the proxy process |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahrtr, jmhbnz, tjungblu The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
tests/e2e/etcd_grpcproxy_test.go
Outdated
| @@ -90,6 +90,44 @@ func TestGrpcProxyAutoSync(t *testing.T) { | |||
| assert.Equal(t, []testutils.KV{{Key: "k1", Val: "v1"}}, kvs) | |||
| } | |||
|
|
|||
| func TestGrpcProxyTlsVersions(t *testing.T) { | |||
There was a problem hiding this comment.
Minor nit: this should be named TestGRPCProxyTLSVersions
| func TestGrpcProxyTlsVersions(t *testing.T) { | |
| func TestGRPCProxyTLSVersions(t *testing.T) { |
There was a problem hiding this comment.
Agreed. But unfortunately it isn't detected by the linters.
There was a problem hiding this comment.
scratch my previous comments, the linter doesn't seem to run on the e2e folder?
There was a problem hiding this comment.
The tests is already in the list. Anyway, could anyone raise a followup task and get it sorted out? thx
Line 169 in 12b47c2
There was a problem hiding this comment.
fixed the naming manually now. I'm not able to even run the linter locally without OOMing my 64gig machine LOL
[tjungblu ~/git/etcd/tests]$ golangci-lint run --config /home/tjungblu/git/etcd/tools/.golangci.yaml
Killed
There was a problem hiding this comment.
Sorry, I was checking this and forgot to finish my reply. I'm unsure why golangci-lint is getting OOMKilled in your machine. It works for me. However, for some reason, it does not pick up the names of the functions inside test files. I'll follow up this on an issue soon.
There was a problem hiding this comment.
Raised mgechev/revive#1124. The issue seems to be in the revive linter.
There was a problem hiding this comment.
Heads up: The linter won't check test file functions, as mentioned in mgechev/revive#1124 (comment), and there doesn't seem an interest by the maintainers to support them. I raised PR mgechev/revive#1134 to document this behavior (already merged).
This adds the min and max TLS version support from etcd-io#13506 and etcd-io#15156 to the grpc proxy. Fixes etcd-io#13506 Signed-off-by: Thomas Jungblut <[email protected]>
tjungblu
force-pushed
the
mintls_grpcproxy
branch
from
463eef9
to
617f357
Compare
|
Thank you both, I'd like to point you to the 3.5 backport as well: |
This adds the min and max TLS version support from #13506 and #15156 to the grpc proxy.
Fixes #13506
Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.