Move the newClientCfg into clientv3 package so as to be reused by both etcdctl and v3discovery

[ahrtr]

Part of issues/13624, and it might be the last PR for this feature.

[codecov-commenter]

Codecov Report

Merging #13821 (b8104e0) into main (1b208aa) will decrease coverage by 0.22%.
The diff coverage is 65.57%.

@@            Coverage Diff             @@
##             main   #13821      +/-   ##
==========================================
- Coverage   72.63%   72.41%   -0.23%     
==========================================
  Files         467      468       +1     
  Lines       38280    38273       -7     
==========================================
- Hits        27806    27715      -91     
- Misses       8686     8752      +66     
- Partials     1788     1806      +18     

Flag	Coverage Δ
all	72.41% <65.57%> (-0.23%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
etcdctl/ctlv3/command/ep_command.go	45.97% <55.00%> (+0.35%)	⬆️
client/v3/config.go	66.66% <66.66%> (ø)
etcdctl/ctlv3/command/global.go	54.40% <100.00%> (-4.27%)	⬇️
server/etcdserver/api/v3discovery/discovery.go	79.16% <100.00%> (-1.12%)	⬇️
client/v3/txn.go	73.33% <0.00%> (-26.67%)	⬇️
server/auth/simple_token.go	80.00% <0.00%> (-8.47%)	⬇️
client/v3/leasing/util.go	91.66% <0.00%> (-6.67%)	⬇️
client/v3/concurrency/mutex.go	61.64% <0.00%> (-5.48%)	⬇️
raft/rafttest/node.go	95.00% <0.00%> (-5.00%)	⬇️
server/storage/mvcc/watchable_store.go	85.50% <0.00%> (-4.35%)	⬇️
... and 13 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b208aa...b8104e0. Read the comment docs.

[ahrtr]

cc @serathius @ptabor @spzala

[serathius]

Love the changes and I think we should go even further with them, however we need some tests.
This code seems simple at the first glance, however it's very fragile due to complicated dependencies.

Can you add a simple set of tests for newClientCfg that guarantees we don't make unintentional changes during refactor?

[ahrtr]

Added a unit test file, and all tests passed. PTAL, thanks. @serathius @spzala @ptabor

[serathius]

Added a unit test file, and all tests passed. PTAL, thanks. @serathius @spzala @ptabor

Just to confirm, have you run the tests without the refactor to confirm that it change output?

[ahrtr]

Just to confirm, have you run the tests without the refactor to confirm that it change output?

No. The function NewClientConfig has different signature as previous one newClientCfg. But both of them have the same logic, they only resolve the TLS, and all other fields should keep unchanged.

func NewClientConfig(confSpec *ConfigSpec, lg *zap.Logger) (*Config, error)
func newClientCfg(endpoints []string, dialTimeout, keepAliveTime, keepAliveTimeout time.Duration, scfg *clientv3.SecureConfig, acfg *clientv3.AuthConfig) (*clientv3.Config, error)

[ahrtr]

After second thought, I decided to completely remove the original newClientCfg function, because it doesn't make much sense to maintain a thin adapter.

PTAL, thanks. @serathius @spzala @ptabor

[serathius]

Just to confirm, have you run the tests without the refactor to confirm that it change output?

No. The function NewClientConfig has different signature as previous one newClientCfg. But both of them have the same logic, they only resolve the TLS, and all other fields should keep unchanged.

func NewClientConfig(confSpec *ConfigSpec, lg *zap.Logger) (*Config, error)
func newClientCfg(endpoints []string, dialTimeout, keepAliveTime, keepAliveTimeout time.Duration, scfg *clientv3.SecureConfig, acfg *clientv3.AuthConfig) (*clientv3.Config, error)

Just to clarify. A good rule about refactoring code is: don't refactor code that doesn't have tests. Add a test first, refactor, update test to new code. Problem with this PR is that it combines both adding test and refactor. This means that we are loosing the benefit of testing as we cannot verify if the refactor didn't change results. Refactor still needs to be human validated.

If we are not adding the tests for the original code, I would double check the refactored code it to make sure there is no weird edge case.

[ahrtr]

Thanks for the comment. I agree with you in principle.

To eliminate your concern, I just added a unit test for the original newClientCfg in another PR pull/13830. Note that the unit test in both PRs are almost identical, the only difference is as below.

// in the refactor PR
cfg, err := NewClientConfig(&tc.spec, lg)

// in this PR
cfg, err := newClientCfg(tc.spec.Endpoints, tc.spec.DialTimeout, tc.spec.KeepAliveTime, tc.spec.KeepAliveTimeout, tc.spec.Secure, tc.spec.Auth)

Note that the original newClientCfg implementation is indeed fragile, so I made minor change as well. The input parameter scfg *clientv3.SecureConfig is always a non-nil value for all use cases in the code base, so we do not see any issue so far. But when I intentionally pass a nil value in the unit test, then we will see issue.

Once the PR pull/13830 is merged, I will rebase this one.

[ahrtr]

Just rebased this PR. PTAL, thanks. @serathius @spzala @ptabor

[serathius]

cc @spzala @ptabor