From cab700f0aa5f725d841cc5cd58bd29c21ff29689 Mon Sep 17 00:00:00 2001 From: Ethan Date: Thu, 9 Sep 2021 12:00:51 -0400 Subject: [PATCH] Backport change to use correct map key when deleting endpoints in sanitizer (#5307) * Use the correct map key to delete endpoints in upstream_removing_sanitizer * remove unused test * Clean up teat and add changelog * Merge branch 'master' into missing-kube-service * codegen, remove debug printing and log level changes * codegen again * default endpoint name --- changelog/v1.8.15/upstream-sanitizer-fix.yaml | 5 ++++ .../pkg/syncer/envoy_translator_syncer.go | 5 ++-- .../sanitizer/upstream_removing_sanitizer.go | 20 +++++++++---- .../upstream_removing_sanitizer_test.go | 30 +++++++++++++++---- 4 files changed, 47 insertions(+), 13 deletions(-) create mode 100644 changelog/v1.8.15/upstream-sanitizer-fix.yaml diff --git a/changelog/v1.8.15/upstream-sanitizer-fix.yaml b/changelog/v1.8.15/upstream-sanitizer-fix.yaml new file mode 100644 index 00000000000..14fb5c77ea0 --- /dev/null +++ b/changelog/v1.8.15/upstream-sanitizer-fix.yaml @@ -0,0 +1,5 @@ +changelog: + - type: FIX + issueLink: https://github.com/solo-io/gloo/issues/5022 + resolvesIssue: true + description: Ensure that endpoints for invalid upstreams are deleted from snapshots and invalid upstreams are not accepted. diff --git a/projects/gloo/pkg/syncer/envoy_translator_syncer.go b/projects/gloo/pkg/syncer/envoy_translator_syncer.go index 30e8b05a2ac..912ae50bdf4 100644 --- a/projects/gloo/pkg/syncer/envoy_translator_syncer.go +++ b/projects/gloo/pkg/syncer/envoy_translator_syncer.go @@ -137,14 +137,14 @@ func (s *translatorSyncer) syncEnvoy(ctx context.Context, snap *v1.ApiSnapshot, sanitizedSnapshot, err := s.sanitizer.SanitizeSnapshot(ctx, snap, xdsSnapshot, reports) if err != nil { - logger.Warnf("proxy %v was rejected due to invalid config: %v\n"+ + logger.Errorf("proxy %v was rejected due to invalid config: %v\n"+ "Attempting to update only EDS information", proxy.GetMetadata().Ref().Key(), err) // If the snapshot is invalid, attempt at least to update the EDS information. This is important because // endpoints are relatively ephemeral entities and the previous snapshot Envoy got might be stale by now. sanitizedSnapshot, err = s.updateEndpointsOnly(key, xdsSnapshot) if err != nil { - logger.Warnf("endpoint update failed. xDS snapshot for proxy %v will not be updated. "+ + logger.Errorf("endpoint update failed. xDS snapshot for proxy %v will not be updated. "+ "Error is: %s", proxy.GetMetadata().Ref().Key(), err) continue } @@ -153,7 +153,6 @@ func (s *translatorSyncer) syncEnvoy(ctx context.Context, snap *v1.ApiSnapshot, // Merge reports after sanitization to capture changes made by the sanitizers allReports.Merge(reports) - if err := s.xdsCache.SetSnapshot(key, sanitizedSnapshot); err != nil { err := eris.Wrapf(err, "failed while updating xDS snapshot cache") logger.DPanicw("", zap.Error(err)) diff --git a/projects/gloo/pkg/syncer/sanitizer/upstream_removing_sanitizer.go b/projects/gloo/pkg/syncer/sanitizer/upstream_removing_sanitizer.go index ee5c71be197..e7708671fcf 100644 --- a/projects/gloo/pkg/syncer/sanitizer/upstream_removing_sanitizer.go +++ b/projects/gloo/pkg/syncer/sanitizer/upstream_removing_sanitizer.go @@ -3,15 +3,15 @@ package sanitizer import ( "context" + envoy_config_cluster_v3 "github.com/envoyproxy/go-control-plane/envoy/config/cluster/v3" "github.com/solo-io/gloo/pkg/utils" - "github.com/solo-io/gloo/projects/gloo/pkg/syncer/stats" - "github.com/solo-io/solo-kit/pkg/api/v1/control-plane/resource" - v1 "github.com/solo-io/gloo/projects/gloo/pkg/api/v1" + "github.com/solo-io/gloo/projects/gloo/pkg/syncer/stats" "github.com/solo-io/gloo/projects/gloo/pkg/translator" "github.com/solo-io/gloo/projects/gloo/pkg/xds" "github.com/solo-io/go-utils/contextutils" envoycache "github.com/solo-io/solo-kit/pkg/api/v1/control-plane/cache" + "github.com/solo-io/solo-kit/pkg/api/v1/control-plane/resource" "github.com/solo-io/solo-kit/pkg/api/v2/reporter" ) @@ -46,16 +46,26 @@ func (s *UpstreamRemovingSanitizer) SanitizeSnapshot( clusters := xdsSnapshot.GetResources(resource.ClusterTypeV3) endpoints := xdsSnapshot.GetResources(resource.EndpointTypeV3) - var removed int64 // Find all the errored upstreams and remove them from the xDS snapshot for _, up := range glooSnapshot.Upstreams.AsInputResources() { + if reports[up].Errors != nil { + clusterName := translator.UpstreamToClusterName(up.GetMetadata().Ref()) + endpointName := clusterName + cluster, _ := clusters.Items[clusterName].ResourceProto().(*envoy_config_cluster_v3.Cluster) + if cluster.GetType() == envoy_config_cluster_v3.Cluster_EDS { + if cluster.GetEdsClusterConfig().GetServiceName() != "" { + endpointName = cluster.GetEdsClusterConfig().GetServiceName() + } else { + endpointName = cluster.GetName() + } + } // remove cluster and endpoints delete(clusters.Items, clusterName) - delete(endpoints.Items, clusterName) + delete(endpoints.Items, endpointName) removed++ } } diff --git a/projects/gloo/pkg/syncer/sanitizer/upstream_removing_sanitizer_test.go b/projects/gloo/pkg/syncer/sanitizer/upstream_removing_sanitizer_test.go index 8e98711e05f..ec78ce48346 100644 --- a/projects/gloo/pkg/syncer/sanitizer/upstream_removing_sanitizer_test.go +++ b/projects/gloo/pkg/syncer/sanitizer/upstream_removing_sanitizer_test.go @@ -3,6 +3,7 @@ package sanitizer_test import ( "context" + envoy_config_cluster_v3 "github.com/envoyproxy/go-control-plane/envoy/config/cluster/v3" envoyclusterapi "github.com/envoyproxy/go-control-plane/envoy/config/cluster/v3" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" @@ -26,9 +27,19 @@ var _ = Describe("UpstreamRemovingSanitizer", func() { Namespace: "upstream", }, } + clusterType = &envoy_config_cluster_v3.Cluster_Type{ + Type: envoy_config_cluster_v3.Cluster_EDS, + } goodClusterName = translator.UpstreamToClusterName(us.Metadata.Ref()) goodCluster = &envoyclusterapi.Cluster{ - Name: goodClusterName, + Name: goodClusterName, + ClusterDiscoveryType: clusterType, + EdsClusterConfig: &envoy_config_cluster_v3.Cluster_EdsClusterConfig{ + ServiceName: goodClusterName + "-123", + }, + } + goodEndpoint = &envoyclusterapi.Cluster{ + Name: goodClusterName + "-123", } badUs = &v1.Upstream{ @@ -39,21 +50,30 @@ var _ = Describe("UpstreamRemovingSanitizer", func() { } badClusterName = translator.UpstreamToClusterName(badUs.Metadata.Ref()) badCluster = &envoyclusterapi.Cluster{ - Name: badClusterName, + Name: badClusterName, + ClusterDiscoveryType: clusterType, + EdsClusterConfig: &envoy_config_cluster_v3.Cluster_EdsClusterConfig{ + ServiceName: badClusterName + "-123", + }, + } + badEndpoint = &envoyclusterapi.Cluster{ + Name: badClusterName + "-123", } ) It("removes upstreams whose reports have an error, and changes the error to a warning", func() { xdsSnapshot := xds.NewSnapshotFromResources( - envoycache.NewResources("", nil), - envoycache.NewResources("clusters", []envoycache.Resource{ + envoycache.NewResources("unit_test", []envoycache.Resource{ + resource.NewEnvoyResource(goodEndpoint), + resource.NewEnvoyResource(badEndpoint), + }), + envoycache.NewResources("unit_test", []envoycache.Resource{ resource.NewEnvoyResource(goodCluster), resource.NewEnvoyResource(badCluster), }), envoycache.NewResources("", nil), envoycache.NewResources("", nil), ) - sanitizer := NewUpstreamRemovingSanitizer() reports := reporter.ResourceReports{