cdsbalancer: test cleanup part 2/N #6554
Conversation
easwars
force-pushed
the
cdsbalancer_test_cleanup_part_2
branch
from
5addb43
to
663f6d0
Compare
| // Equal reports whether the handshake info structs are identical (have the | ||
| // same pointer). This is sufficient as all subconns from one CDS balancer use | ||
| // the same one. |
There was a problem hiding this comment.
Please update this comment to what you changed this Equal() function to do.
|
|
||
| func (tcc *testCCWrapper) NewSubConn(addrs []resolver.Address, opts balancer.NewSubConnOptions) (balancer.SubConn, error) { |
There was a problem hiding this comment.
Can you mention that this requires exactly one address specified with handshake info present?
| } | ||
| t.Cleanup(xdsClose) | ||
|
|
||
| // Register a wrapped cds LB policy (child policy of the cds LB policy) for |
There was a problem hiding this comment.
cds LB policy is the child policy of cds LB policy?
| } | ||
| t.Cleanup(xdsClose) | ||
|
|
||
| // Register a wrapped cds LB policy (child policy of the cds LB policy) for |
There was a problem hiding this comment.
Same here about child policy.
| stub.Register(cdsName, stub.BalancerFuncs{ | ||
| Init: func(bd *stub.BalancerData) { | ||
| ccWrapper = &testCCWrapper{ | ||
| ClientConn: bd.ClientConn, | ||
| handshakeInfoCh: make(chan *xdscredsinternal.HandshakeInfo, 1), | ||
| } | ||
| bd.Data = cdsBuilder.Build(ccWrapper, bd.BuildOptions) | ||
| }, | ||
| ParseConfig: func(lbCfg json.RawMessage) (serviceconfig.LoadBalancingConfig, error) { | ||
| return cdsBuilder.(balancer.ConfigParser).ParseConfig(lbCfg) | ||
| }, | ||
| UpdateClientConnState: func(bd *stub.BalancerData, ccs balancer.ClientConnState) error { | ||
| bal := bd.Data.(balancer.Balancer) | ||
| return bal.UpdateClientConnState(ccs) | ||
| }, | ||
| Close: func(bd *stub.BalancerData) { | ||
| bal := bd.Data.(balancer.Balancer) | ||
| bal.Close() | ||
| }, |
There was a problem hiding this comment.
Same comment as previous PR. This seems to be common functionality (overriding cds balancer with testCCWrapper to get the NewSubConn handshake info passed in. Can you please pull into helper?
| t.Fatal(err) | ||
| } | ||
|
|
||
| // Verify that a successful RPC can be made. |
There was a problem hiding this comment.
Mention something about secure connection.
| // Make a NewSubConn and verify that attributes are added. | ||
| if _, err := makeNewSubConn(ctx, edsB.parentCC, tcc, false); err != nil { | ||
| t.Fatal(err) | ||
| // Verify that a successful RPC can be made. |
There was a problem hiding this comment.
Same here on a secure connection.
| // Wait for the connection to move to the new backend that expects | ||
| // connections without security. |
There was a problem hiding this comment.
For my understanding, how does this work? How does the switching logic know? Does it fail the first one since the client has no security and fallback to this backend which works because it expects no security?
There was a problem hiding this comment.
The new cluster resource pushed from here does not contain any security information. Once cds receives this, it is expected to not use the fallback credentials, which in our case is insecure creds.
It was hard to get the same old server (which was expecting a secure connection) to now expect an insecure connection. Hence I spun up a new one, and changed the EDS resource also to point to the new one.
The reason I have a while loop here is because it can take some time for the security configuration to be received and processed by the cds LB policy. And the corresponding EDS resource to be processed by the child policies down the tree.
| // bootstrap file contents. Verifies that the connection between the client and | ||
| // the server is secure. Subsequently, the cds LB policy receives a cluster | ||
| // resource that is NACKed by the xDS client. Test verifies that the cds LB | ||
| // policy continue to use the previous good configuration, but the error from |
| // Register a wrapped clusterresolver LB policy (child policy of the cds LB | ||
| // policy) for the duration of this test that makes the resolver error |
There was a problem hiding this comment.
This child policy makes sense :).
There was a problem hiding this comment.
Lol, switched this to the helper I added as part of the previous PR.
|
Thank you for the review. Addressed all the comments. |
This PR cleanup us tests in the cds balancer relating to security configuration. This is a pre-requisite for switching the cds balancer to use the generic xdsClient watch API.
RELEASE NOTES: none