Add vtctldclient TLS support to vtadmin GRPC #18556
Conversation
- Reused same utilities from vtctlclient - Registers vtadmin to use `grpcclientcommon` - Updates vtadmin proxy dial options to use grpc secure dial options when possible Signed-off-by: Kyle Johnson <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
vitess-bot
bot
added
NeedsBackportReason
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #18556 +/- ##
==========================================
+ Coverage 67.49% 67.55% +0.06%
==========================================
Files 1607 1607
Lines 263104 263337 +233
==========================================
+ Hits 177569 177889 +320
+ Misses 85535 85448 -87 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@corbantek Can you also add a test for this? |
@dbussink Do you have any specifics on what you'd like to be tested? I honestly didn't change anything other than what's already being used by Vitess in vtctldclient and others... Let me poke around in the meantime, but any advice it appreciated. |
go/cmd/vtadmin/main_test.go - Tests for the vtadmin main.go changes - Validates that TLS-related flags are properly registered when grpcclientcommon.RegisterFlags() is called - Tests flag registration functionality and command structure go/vt/vtadmin/vtctldclient/proxy_test.go - Added tests for the proxy dial method changes - Tests TLS configuration via grpcclientcommon.SecureDialOption - Tests credential handling with the new TLS option ordering - Tests error scenarios for SecureDialOption - default behavior uses insecure connection: Tests that the new grpcclientcommon.SecureDialOption() approach works the same as the original insecure.NewCredentials() approach when no TLS flags are configured - backward compatibility with existing cluster configurations: Creates a real server connection using the standard New() function to verify existing vtadmin cluster configurations continue to work - insecure connection behavior is preserved: Tests that grpcclientcommon.SecureDialOption() returns valid dial options with default parameters - dial works with default configuration: Verifies the dial method works exactly the same as before the changes, ensuring backward compatibility for existing deployments - SecureDialOption maintains compatibility: Confirms that the replacement function works as expected in the default case (no TLS configuration provided) go/vt/vtctl/grpcclientcommon/dial_option_test.go - Tests for the grpcclientcommon package changes - Tests flag registration functionality - Tests SecureDialOption() function behavior - Tests the vtadmin registration in the init function - Tests global variable handling and flag integration - default behavior is insecure for backward compatibility: Tests that when TLS flags are empty (default state), SecureDialOption() returns insecure credentials, maintaining backward compatibility - vtadmin registration in init function: Documents and tests that vtadmin was properly added to the init function to enable TLS flag registration go/vt/vtadmin/vtctldclient/grpc_integration_test.go - Integration tests for the complete flow - Tests the integration between vtadmin's proxy and grpcclientcommon.SecureDialOption - Tests dial option ordering (TLS first, then credentials, then resolver) - Tests error handling scenarios Signed-off-by: Kyle Johnson <[email protected]>
Nevermind, I tried sticking an agent on this and had it create/add tests for everything I changed (even added some backwards compatible tests for non-TLS) |
Signed-off-by: Kyle Johnson <[email protected]>
This generally means you need to clean things up there. It has the tendency to add way too much, violate formatting etc. So it can be a useful starting point, but let's make sure to reduce it then to a minimal useful test. I now see a big wall of text that does way more than it needs to. Do any of the other binaries have tests to verify the minimal necessary thing here? |
Actually re-reviewing some of these test and tracing them, they are crap. Let me look at this again tomorrow and see if I can figure out a way to actually test. Some of these tests looked like they were building up mocks but when I actually traced they were just testing themselves... I find testing my change to proxy.go the most difficult... |
Signed-off-by: Kyle Johnson <[email protected]>
corbantek
force-pushed
the
add-tls-grpc-to-vtadmin-external-pr
branch
from
761d754 to
d7bedd8
Compare
Signed-off-by: Kyle Johnson <[email protected]>
…on() Signed-off-by: Kyle Johnson <[email protected]>
Signed-off-by: Kyle Johnson <[email protected]>
go/cmd/vtadmin/main_test.go
Outdated
| t.Run("grpc tls flags are registered", func(t *testing.T) { | ||
| certFlag := testCmd.Flags().Lookup("vtctld-grpc-cert") | ||
| require.NotNil(t, certFlag, "vtctld-grpc-cert flag should be registered") | ||
| assert.Equal(t, "", certFlag.DefValue, "vtctld-grpc-cert should have empty default value") |
There was a problem hiding this comment.
Style wise we generally don't add all the string descriptions to testify assertions, since they are very often superfluous. Only if they really provide very explicit value, we should add them. This applies here to all the assertions we have.
|
|
||
| // Test that grpcclient.SecureDialOption fails with these files | ||
| _, err = grpcclient.SecureDialOption(tmpCert.Name(), tmpKey.Name(), "", "", "") | ||
| assert.Error(t, err, "SecureDialOption should fail with invalid cert/key files") |
There was a problem hiding this comment.
Why is this testing SecureDialOption here? That seems not really a concern for testing in this package?
There was a problem hiding this comment.
If SecureDialOption somehow changed this would catch that here so we don't have a bogus test below that always passes...
I can remove if you feel its not needed
There was a problem hiding this comment.
@corbantek Testing for SecureDialOption needs to be in the package where that function lives really. Not outside of it here. I'm not really sure how this test is also then related to the original issue fixed?
There was a problem hiding this comment.
I only added changed these lines in the original PR:
tlsOpt, err := grpcclientcommon.SecureDialOption()
if err != nil {
return err
}You asked me to write unit tests for this PRs changes...
The current unit tests for proxy.go already covered this entire file EXCEPT
if err != nil {
return err
}This test builds up a scenario where the arguments to SecureDialOption are invalid causing it throw an error and reach return err (the only line not covered by tests)
Signed-off-by: Kyle Johnson <[email protected]>
- Remove unneeded assert text - Remove safe-guard for grpcclient.SecureDialOption error possibly changing Signed-off-by: Kyle Johnson <[email protected]>
corbantek
changed the title
|
This is ready for re-review. All comments addressed and all updated lines covered by unit tests. |
|
@dbussink Bumping this for review. Anything else you'd like me to address? |
dbussink
added
Type: Enhancement
|
I'm not able to re-run that [onlineddl_vrepl_suite] test suite. I feel its just flakey because I didn't change anything in that area of code. Could someone please re-run and merge on sucess? |
Description
Why
vtadminwithout vtctldclient TLSWhat
grpcclientcommonRelated Issue(s)
Feature Request: expose TLS related flags for vtadmin config
Checklist
Deployment Notes