options to deactivate legacy wildcard #1323
Conversation
| } | ||
|
|
||
| func TestSotwSubscriptionsWithDeactivatedLegacyWildcardForTypes(t *testing.T) { | ||
| t.Run("deactivate for specific type", func(t *testing.T) { |
There was a problem hiding this comment.
Can you also add explicitly subscribing to wildcard in this test?
There was a problem hiding this comment.
I removed this test because it's already covered by the logic of the multiple types test. I added explict wildcard test to all of them, and added delta versions of the test.
zhiyanfoo
force-pushed
the
zfs/legacy-wildcard
branch
from
58fd17a to
f280781
Compare
pkg/server/stream/v3/subscription.go
Outdated
|
|
||
| // newSubscription initializes a subscription state. | ||
| func newSubscription(wildcard bool, initialResourceVersions map[string]string) Subscription { | ||
| func newSubscription(emptyRequest bool, initialResourceVersions map[string]string, opts config.Opts, typeURL string) Subscription { |
There was a problem hiding this comment.
I don't think passing Opts here is needed. Directly passing if legacy is deactivated is sufficient
pkg/server/config/config.go
Outdated
| } | ||
|
|
||
| // LegacyWildcardDeactivated returns whether legacy wildcard mode is deactivated for all resource types | ||
| func (o Opts) LegacyWildcardDeactivated() bool { |
There was a problem hiding this comment.
I would remove those two methods and replace them with IsLegacyWildcardActive(typeURL)
This would allow extending/evolving the option interface without impacting code in the rest of the modules
pkg/server/stream/v3/subscription.go
Outdated
| // As soon as a resource or an explicit subscription to wildcard is provided, | ||
| // this flag will be set to false | ||
| // allowLegacyWildcard indicates that an empty request should be treated as a wildcard request. | ||
| // If in the stream at some point subscribes explicitly a resource or a explicitly makes wildcard |
There was a problem hiding this comment.
Comment to rewrite. The sentence is inconsistent
zhiyanfoo
force-pushed
the
zfs/legacy-wildcard
branch
2 times, most recently
from
fd6324b to
06e64e6
Compare
pkg/server/stream/v3/subscription.go
Outdated
|
|
||
| func NewSotwSubscription(subscribed []string) Subscription { | ||
| sub := newSubscription(len(subscribed) == 0, nil) | ||
| func NewSotwSubscription(subscribed []string, opts config.Opts, typeURL string) Subscription { |
There was a problem hiding this comment.
Why is this passed in this interface? This is still leaking parameters here that have no value on the subscription
| // When deactivated, empty requests are treated as a request with no subscriptions to any resource. | ||
| // This is recommended for when you are using the go-control-plane to serve grpc-xds clients. | ||
| // These clients never want to treat an empty request as a wildcard subscription. | ||
| func DeactivateLegacyWildcard() XDSOption { |
There was a problem hiding this comment.
Minor: can you map them in sotw and delta to allow cleaner import as reported at line 46
Signed-off-by: Zhiyan Foo <[email protected]>
Signed-off-by: Zhiyan Foo <[email protected]>
Signed-off-by: Zhiyan Foo <[email protected]>
Signed-off-by: Zhiyan Foo <[email protected]>
Signed-off-by: Zhiyan Foo <[email protected]>
…y mode is true Signed-off-by: Zhiyan Foo <[email protected]>
Signed-off-by: Zhiyan Foo <[email protected]>
zhiyanfoo
force-pushed
the
zfs/legacy-wildcard
branch
from
2c15074 to
6c34080
Compare
3 participants
This PR add two new public facing xds options
DeactivateLegacyWildcardandDeactivateLegacyWildcardForTypes.This will allow users to disable legacy wildcard mode. This allows us to avoid returning all resources when envoy makes a VHDS request with an empty subscription list. It'll also allows us to ensure that we never return all resources to grpc-xds clients.