Skip to content

Commit

Permalink
[release-1.7] delegate vs change could not trigger push to sidecars (#…
Browse files Browse the repository at this point in the history 
…29293)

* Fix bug: delegate vs change could not trigger push to sidecars (#29193)

* Fix bug: delegate vs change could not trigger push to sidecars

* Fix comments

* add test

* update

* fix lint

* lint

* add release-note

* Fix

* Fix lint
  • Loading branch information
@hzxuzhonghu
hzxuzhonghu authored Dec 7, 2020
1 parent 35ffee3 commit 74a8d16
Show file tree
Hide file tree
Showing 8 changed files with 179 additions and 34 deletions.
17 changes: 14 additions & 3 deletions pilot/pkg/model/push_context.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ type PushContext struct {
privateVirtualServicesByNamespaceAndGateway map[string]map[string][]Config
// This contains all virtual services whose exportTo is "*", keyed by gateway
publicVirtualServicesByGateway map[string][]Config
// root vs namespace/name ->delegate vs virtualservice gvk/namespace/name
delegateVirtualServices map[ConfigKey][]ConfigKey

// destination rules are of three types:
// namespaceLocalDestRules: all public/private dest rules pertaining to a service defined in a given namespace
Expand Down Expand Up @@ -613,7 +615,7 @@ func (ps *PushContext) ServiceForHostname(proxy *Proxy, hostname host.Name) *Ser
return nil
}

// VirtualServices lists all virtual services bound to the specified gateways
// VirtualServicesForGateway lists all virtual services bound to the specified gateways
// This replaces store.VirtualServices. Used only by the gateways
// Sidecars use the egressListener.VirtualServices().
func (ps *PushContext) VirtualServicesForGateway(proxy *Proxy, gateway string) []Config {
Expand All @@ -623,6 +625,16 @@ func (ps *PushContext) VirtualServicesForGateway(proxy *Proxy, gateway string) [
return res
}

// DelegateVirtualServicesConfigKey lists all the delegate virtual services configkeys associated with the provided virtual services
func (ps *PushContext) DelegateVirtualServicesConfigKey(vses []Config) []ConfigKey {
var out []ConfigKey
for _, vs := range vses {
out = append(out, ps.delegateVirtualServices[ConfigKey{Kind: gvk.VirtualService, Namespace: vs.Namespace, Name: vs.Name}]...)
}

return out
}

// getSidecarScope returns a SidecarScope object associated with the
// proxy. The SidecarScope object is a semi-processed view of the service
// registry, and config state associated with the sidecar crd. The scope contains
Expand Down Expand Up @@ -1115,8 +1127,7 @@ func (ps *PushContext) initVirtualServices(env *Environment) error {
// the RDS code. See separateVSHostsAndServices in route/route.go
sortConfigByCreationTime(vservices)

vservices = mergeVirtualServicesIfNeeded(vservices, ps.defaultVirtualServiceExportTo)

vservices, ps.delegateVirtualServices = mergeVirtualServicesIfNeeded(vservices, ps.defaultVirtualServiceExportTo)
// convert all shortnames in virtual services into FQDNs
for _, r := range vservices {
resolveVirtualServiceShortnames(r.Spec.(*networking.VirtualService), r.ConfigMeta)
Expand Down
11 changes: 10 additions & 1 deletion pilot/pkg/model/sidecar.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,11 @@ func DefaultSidecarScopeForNamespace(ps *PushContext, configNamespace string) *S
}

for _, el := range out.EgressListeners {
// add dependencies on delegate virtual services
delegates := ps.DelegateVirtualServicesConfigKey(el.virtualServices)
for _, delegate := range delegates {
out.AddConfigDependencies(delegate)
}
for _, vs := range el.virtualServices {
out.AddConfigDependencies(ConfigKey{
Kind: gvk.VirtualService,
Expand Down Expand Up @@ -284,7 +289,11 @@ func ConvertToSidecarScope(ps *PushContext, sidecarConfig *Config, configNamespa
for _, s := range listener.services {
addService(s)
}

// add dependencies on delegate virtual services
delegates := ps.DelegateVirtualServicesConfigKey(listener.virtualServices)
for _, delegate := range delegates {
out.AddConfigDependencies(delegate)
}
// Infer more possible destinations from virtual services
// Services chosen here will not override services explicitly requested in listener.services.
// That way, if there is ambiguity around what hostname to pick, a user can specify the one they
Expand Down
29 changes: 19 additions & 10 deletions pilot/pkg/model/virtualservice.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (

"istio.io/istio/pilot/pkg/features"
"istio.io/istio/pkg/config/constants"
"istio.io/istio/pkg/config/schema/gvk"
"istio.io/istio/pkg/config/visibility"
)

Expand Down Expand Up @@ -87,8 +88,11 @@ func resolveVirtualServiceShortnames(rule *networking.VirtualService, meta Confi
}
}

func mergeVirtualServicesIfNeeded(vServices []Config, defaultExportTo map[visibility.Instance]bool) (out []Config) {
out = make([]Config, 0, len(vServices))
// Return merged virtual services and the root->delegate vs map
func mergeVirtualServicesIfNeeded(
vServices []Config,
defaultExportTo map[visibility.Instance]bool) ([]Config, map[ConfigKey][]ConfigKey) {
out := make([]Config, 0, len(vServices))
delegatesMap := map[string]Config{}
delegatesExportToMap := map[string]map[visibility.Instance]bool{}
// root virtualservices with delegate
Expand Down Expand Up @@ -130,33 +134,38 @@ func mergeVirtualServicesIfNeeded(vServices []Config, defaultExportTo map[visibi
// If `PILOT_ENABLE_VIRTUAL_SERVICE_DELEGATE` feature disabled,
// filter out invalid vs(root or delegate), this can happen after enable -> disable
if !features.EnableVirtualServiceDelegate {
return
return out, nil
}

delegatesByRoot := make(map[ConfigKey][]ConfigKey, len(rootVses))

// 2. merge delegates and root
for _, root := range rootVses {
rootConfigKey := ConfigKey{Kind: gvk.VirtualService, Name: root.Name, Namespace: root.Namespace}
rootVs := root.Spec.(*networking.VirtualService)
mergedRoutes := []*networking.HTTPRoute{}
for _, route := range rootVs.Http {
// it is root vs with delegate
if route.Delegate != nil {
delegate, ok := delegatesMap[key(route.Delegate.Name, route.Delegate.Namespace)]
if delegate := route.Delegate; delegate != nil {
delegateConfigKey := ConfigKey{Kind: gvk.VirtualService, Name: delegate.Name, Namespace: delegate.Namespace}
delegatesByRoot[rootConfigKey] = append(delegatesByRoot[rootConfigKey], delegateConfigKey)
delegateVS, ok := delegatesMap[key(delegate.Name, delegate.Namespace)]
if !ok {
log.Debugf("delegate virtual service %s/%s of %s/%s not found",
route.Delegate.Namespace, route.Delegate.Name, root.Namespace, root.Name)
delegate.Namespace, delegate.Name, root.Namespace, root.Name)
// delegate not found, ignore only the current HTTP route
continue
}
// make sure that the delegate is visible to root virtual service's namespace
exportTo := delegatesExportToMap[key(route.Delegate.Name, route.Delegate.Namespace)]
exportTo := delegatesExportToMap[key(delegate.Name, delegate.Namespace)]
if !exportTo[visibility.Public] && !exportTo[visibility.Instance(root.Namespace)] {
log.Debugf("delegate virtual service %s/%s of %s/%s is not exported to %s",
route.Delegate.Namespace, route.Delegate.Name, root.Namespace, root.Name, root.Namespace)
delegate.Namespace, delegate.Name, root.Namespace, root.Name, root.Namespace)
continue
}
// DeepCopy to prevent mutate the original delegate, it can conflict
// when multiple routes delegate to one single VS.
copiedDelegate := delegate.DeepCopy()
copiedDelegate := delegateVS.DeepCopy()
vs := copiedDelegate.Spec.(*networking.VirtualService)
merged := mergeHTTPRoutes(route, vs.Http)
mergedRoutes = append(mergedRoutes, merged...)
Expand All @@ -173,7 +182,7 @@ func mergeVirtualServicesIfNeeded(vServices []Config, defaultExportTo map[visibi
out = append(out, root)
}

return
return out, delegatesByRoot
}

// merge root's route with delegate's and the merged route number equals the delegate's.
Expand Down
2 changes: 1 addition & 1 deletion pilot/pkg/model/virtualservice_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -617,7 +617,7 @@ func TestMergeVirtualServices(t *testing.T) {

for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
got := mergeVirtualServicesIfNeeded(tc.virtualServices, map[visibility.Instance]bool{visibility.Public: true})
got, _ := mergeVirtualServicesIfNeeded(tc.virtualServices, map[visibility.Instance]bool{visibility.Public: true})
if !reflect.DeepEqual(got, tc.expectedVirtualServices) {
t.Errorf("expected vs %v, but got %v,\n diff: %s ", len(tc.expectedVirtualServices), len(got), cmp.Diff(tc.expectedVirtualServices, got))
}
Expand Down
136 changes: 123 additions & 13 deletions pilot/pkg/xds/ads_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,28 +19,26 @@ import (
"testing"
"time"

xdsapi "github.com/envoyproxy/go-control-plane/envoy/api/v2"
route "github.com/envoyproxy/go-control-plane/envoy/config/route/v3"
discovery "github.com/envoyproxy/go-control-plane/envoy/service/discovery/v3"
"github.com/golang/protobuf/proto"

mesh "istio.io/api/mesh/v1alpha1"
networking "istio.io/api/networking/v1alpha3"
"istio.io/istio/pilot/pkg/features"
"istio.io/istio/pilot/pkg/model"
"istio.io/istio/pilot/pkg/xds"
v2 "istio.io/istio/pilot/pkg/xds/v2"
v3 "istio.io/istio/pilot/pkg/xds/v3"
istioagent "istio.io/istio/pkg/istio-agent"
"istio.io/istio/pkg/security"

"istio.io/istio/pkg/adsc"
"istio.io/istio/pkg/config/host"
"istio.io/istio/pkg/config/protocol"
"istio.io/istio/pkg/config/schema/collections"
"istio.io/istio/pkg/config/schema/gvk"

istioagent "istio.io/istio/pkg/istio-agent"
"istio.io/istio/pkg/security"
"istio.io/istio/tests/util"

xdsapi "github.com/envoyproxy/go-control-plane/envoy/api/v2"
route "github.com/envoyproxy/go-control-plane/envoy/config/route/v3"
discovery "github.com/envoyproxy/go-control-plane/envoy/service/discovery/v3"
)

const (
Expand Down Expand Up @@ -500,6 +498,10 @@ func TestAdsClusterUpdate(t *testing.T) {
func TestAdsPushScoping(t *testing.T) {
server, tearDown := initLocalPilotTestEnv(t)
defer tearDown()
features.EnableVirtualServiceDelegate = true
defer func() {
features.EnableVirtualServiceDelegate = false
}()

const (
svcSuffix = ".testPushScoping.com"
Expand Down Expand Up @@ -599,6 +601,67 @@ func TestAdsPushScoping(t *testing.T) {
removeVirtualService := func(i int) {
server.EnvoyXdsServer.MemConfigController.Delete(gvk.VirtualService, fmt.Sprintf("vs%d", i), model.IstioDefaultConfigNamespace)
}

addDelegateVirtualService := func(i int, hosts []string) {
if _, err := server.EnvoyXdsServer.MemConfigController.Create(model.Config{
ConfigMeta: model.ConfigMeta{
GroupVersionKind: gvk.VirtualService,
Name: fmt.Sprintf("rootvs%d", i), Namespace: model.IstioDefaultConfigNamespace},
Spec: &networking.VirtualService{
Hosts: hosts,
Http: []*networking.HTTPRoute{{
Name: "dest-foo",
Delegate: &networking.Delegate{
Name: fmt.Sprintf("delegatevs%d", i),
Namespace: model.IstioDefaultConfigNamespace,
},
}},
ExportTo: nil,
},
}); err != nil {
t.Fatal(err)
}

if _, err := server.EnvoyXdsServer.MemConfigController.Create(model.Config{
ConfigMeta: model.ConfigMeta{
GroupVersionKind: gvk.VirtualService,
Name: fmt.Sprintf("delegatevs%d", i), Namespace: model.IstioDefaultConfigNamespace},
Spec: &networking.VirtualService{
Http: []*networking.HTTPRoute{{Redirect: &networking.HTTPRedirect{
Uri: "example.org",
Authority: "some-authority.default.svc.cluster.local",
RedirectCode: 308,
}}},
ExportTo: nil,
},
}); err != nil {
t.Fatal(err)
}
}

updateDelegateVirtualService := func(i int) {
if _, err := server.EnvoyXdsServer.MemConfigController.Update(model.Config{
ConfigMeta: model.ConfigMeta{
GroupVersionKind: gvk.VirtualService,
Name: fmt.Sprintf("delegatevs%d", i), Namespace: model.IstioDefaultConfigNamespace},
Spec: &networking.VirtualService{
Http: []*networking.HTTPRoute{{Redirect: &networking.HTTPRedirect{
Uri: "example.org",
Authority: "new-authority.default.svc.cluster.local",
RedirectCode: 308,
}}},
ExportTo: nil,
},
}); err != nil {
t.Fatal(err)
}
}

removeDelegateVirtualService := func(i int) {
server.EnvoyXdsServer.MemConfigController.Delete(gvk.VirtualService, fmt.Sprintf("rootvs%d", i), model.IstioDefaultConfigNamespace)
server.EnvoyXdsServer.MemConfigController.Delete(gvk.VirtualService, fmt.Sprintf("delegatevs%d", i), model.IstioDefaultConfigNamespace)
}

addDestinationRule := func(i int, host string) {
if _, err := server.EnvoyXdsServer.MemConfigController.Create(model.Config{
ConfigMeta: model.ConfigMeta{
Expand Down Expand Up @@ -650,6 +713,10 @@ func TestAdsPushScoping(t *testing.T) {
index int
hosts []string
}
delegatevsIndexes []struct {
index int
hosts []string
}
drIndexes []struct {
index int
host string
Expand Down Expand Up @@ -711,7 +778,7 @@ func TestAdsPushScoping(t *testing.T) {
drIndexes: []struct {
index int
host string
}{{index: 4}},
}{{4, fmt.Sprintf("svc%d%s", 4, svcSuffix)}},
expectUpdates: []string{"cds"},
},
{
Expand Down Expand Up @@ -781,6 +848,33 @@ func TestAdsPushScoping(t *testing.T) {
unexpectUpdates: []string{"cds"},
timeout: time.Second,
},
{
desc: "Add delegation virtual service for scoped service with transitively scoped dest svc",
ev: model.EventAdd,
delegatevsIndexes: []struct {
index int
hosts []string
}{{index: 4, hosts: []string{fmt.Sprintf("svc%d%s", 4, svcSuffix)}}},
expectUpdates: []string{"lds", "rds"},
},
{
desc: "Update delegate virtual service should trigger full push",
ev: model.EventUpdate,
delegatevsIndexes: []struct {
index int
hosts []string
}{{index: 4, hosts: []string{fmt.Sprintf("svc%d%s", 4, svcSuffix)}}},
expectUpdates: []string{"lds", "rds"},
},
{
desc: "Delete delegate virtual service for scoped service with transitively scoped dest svc",
ev: model.EventDelete,
delegatevsIndexes: []struct {
index int
hosts []string
}{{index: 4}},
expectUpdates: []string{"lds", "rds"},
},
{
desc: "Remove a scoped service",
ev: model.EventDelete,
Expand Down Expand Up @@ -808,11 +902,9 @@ func TestAdsPushScoping(t *testing.T) {

for i, c := range svcCases {
fmt.Printf("begin %d case(%s) %v\n", i, c.desc, c)

var wantUpdates []string
wantUpdates = append(wantUpdates, c.expectUpdates...)
wantUpdates = append(wantUpdates, c.unexpectUpdates...)

switch c.ev {
case model.EventAdd:
if len(c.svcIndexes) > 0 {
Expand All @@ -831,11 +923,22 @@ func TestAdsPushScoping(t *testing.T) {
addVirtualService(vsIndex.index, vsIndex.hosts...)
}
}
if len(c.delegatevsIndexes) > 0 {
for _, vsIndex := range c.delegatevsIndexes {
addDelegateVirtualService(vsIndex.index, vsIndex.hosts)
}
}
if len(c.drIndexes) > 0 {
for _, drIndex := range c.drIndexes {
addDestinationRule(drIndex.index, drIndex.host)
}
}
case model.EventUpdate:
if len(c.delegatevsIndexes) > 0 {
for _, vsIndex := range c.delegatevsIndexes {
updateDelegateVirtualService(vsIndex.index)
}
}
case model.EventDelete:
if len(c.svcIndexes) > 0 {
removeService(c.ns, c.svcIndexes...)
Expand All @@ -848,6 +951,12 @@ func TestAdsPushScoping(t *testing.T) {
removeVirtualService(vsIndex.index)
}
}
if len(c.delegatevsIndexes) > 0 {
for _, vsIndex := range c.delegatevsIndexes {
removeDelegateVirtualService(vsIndex.index)
}
}

if len(c.drIndexes) > 0 {
for _, drIndex := range c.drIndexes {
removeDestinationRule(drIndex.index)
Expand All @@ -863,14 +972,15 @@ func TestAdsPushScoping(t *testing.T) {
timeout = c.timeout
}
upd, _ := adscConn.Wait(timeout, wantUpdates...) // XXX slow for unexpect ...

for _, expect := range c.expectUpdates {
if !contains(upd, expect) {
t.Fatalf("expect %s but not contains (%v) for case %v", expect, upd, c)
t.Fatalf("expect %s but not contains (%v) for case %v", expect, upd, c.desc)
}
}
for _, unexpect := range c.unexpectUpdates {
if contains(upd, unexpect) {
t.Fatalf("unexpect %s but contains (%v) for case %v", unexpect, upd, c)
t.Fatalf("unexpect %s but contains (%v) for case %v", unexpect, upd, c.desc)
}
}
}
Expand Down
Loading

0 comments on commit 74a8d16

Please sign in to comment.