1.17 - Discovery resource version err (#10118) (#10135)

Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>

changelog/v1.17.11/discovery-watchlabels-bugfix.yaml

@@ -0,0 +1,7 @@
+changelog:
+  - type: FIX
+    issueLink: https://github.com/solo-io/gloo/issues/8635
+    description: >
+      Fix a bug that caused discovered Upstreams to not reflect the updated state of parent Services discovered using
+      watchLabels
+    resolvesIssue: false

projects/gloo/pkg/discovery/discovery.go

@@ -44,7 +44,6 @@ type UpstreamDiscovery struct {
 	discoveryPlugins       []DiscoveryPlugin
 	lock                   sync.Mutex
 	latestDesiredUpstreams map[DiscoveryPlugin]v1.UpstreamList
-	extraSelectorLabels    map[string]string
 }
 
 type EndpointDiscovery struct {
@@ -95,7 +94,7 @@ func NewUpstreamDiscovery(
 // launch a goroutine for all the UDS plugins
 func (d *UpstreamDiscovery) StartUds(opts clients.WatchOpts, discOpts Opts) (chan error, error) {
 	aggregatedErrs := make(chan error)
-	d.extraSelectorLabels = opts.Selector
+
 	for _, uds := range d.discoveryPlugins {
 		upstreams, errs, err := uds.DiscoverUpstreams(d.watchNamespaces, d.writeNamespace, opts, discOpts)
 		if err != nil {
@@ -136,12 +135,14 @@ func (d *UpstreamDiscovery) Resync(ctx context.Context) error {
 	for uds, desiredUpstreams := range d.latestDesiredUpstreams {
 		udsName := strings.Replace(reflect.TypeOf(uds).String(), "*", "", -1)
 		udsName = strings.Replace(udsName, ".", "", -1)
+
+		// selecting on the discovery plugin label will get all Upstreams created by Discovery
+		// there is no need to select on watchLabels as these are not present on Upstream metadata, nor are they
+		// necessary to identify Upstreams that may need to be resynced
 		selector := map[string]string{
 			"discovered_by": udsName,
 		}
-		for k, v := range d.extraSelectorLabels {
-			selector[k] = v
-		}
+
 		logger.Debugw("reconciling upstream details", zap.Any("upstreams", desiredUpstreams))
 		if err := d.upstreamReconciler.Reconcile(d.writeNamespace, desiredUpstreams, uds.UpdateUpstream, clients.ListOpts{
 			Ctx:      ctx,

test/kube2e/gateway/gateway_test.go

@@ -1061,6 +1061,7 @@ var _ = Describe("Kube2e: gateway", func() {
 		})
 	})
 
+	// as of v1.17 these tests are replaced by the discovery watchlabels suite in the new kubernetes e2e tests
 	Context("upstream discovery", func() {
 		var createdServices []string
 
@@ -1195,6 +1196,64 @@ var _ = Describe("Kube2e: gateway", func() {
 			}, "15s", "0.5s").ShouldNot(BeNil())
 		})
 
+		It("Modifies discovered Upstream when Service is modified", func() {
+			// This tests the fix for a bug whereby Discovery would fail to update discovered Upstreams on Service
+			// updates when watchLabels were enabled
+
+			watchedKey, watchedValue := "watchKey", "watchValue"
+			bonusKey, bonusValue, modifiedBonusValue := "foo", "bar", "modified-bar"
+
+			// set watchLabels for Discovery to select on
+			labels := map[string]string{
+				watchedKey: watchedValue,
+			}
+			setWatchLabels(labels)
+
+			// add an additional label which we'll modify later
+			labels[bonusKey] = bonusValue
+
+			svcName := "uds-test-service"
+			createServiceWithWatchedLabels(svcName, labels)
+
+			// confirm the Upstream has the Service's labels in its DiscoveryMetadata
+			Eventually(func() (map[string]string, error) {
+				us, err := getUpstream(svcName)
+				if err != nil {
+					return nil, err
+				}
+				return us.DiscoveryMetadata.GetLabels(), nil
+			}, "15s", "0.5s").Should(BeEquivalentTo(
+				map[string]string{
+					"gloo":     svcName,
+					watchedKey: watchedValue,
+					bonusKey:   bonusValue,
+				},
+			))
+
+			// get the service, modify its bonus label, and update it
+			svc, err := resourceClientset.KubeClients().CoreV1().Services(namespace).Get(ctx, svcName, metav1.GetOptions{})
+			Expect(err).NotTo(HaveOccurred())
+			svc.Labels[bonusKey] = modifiedBonusValue
+
+			_, err = resourceClientset.KubeClients().CoreV1().Services(namespace).Update(ctx, svc, metav1.UpdateOptions{})
+			Expect(err).NotTo(HaveOccurred())
+
+			// expect the discovered Upstream's DiscoveryMetadata to eventually reflect the updated label
+			Eventually(func() (map[string]string, error) {
+				us, err := getUpstream(svcName)
+				if err != nil {
+					return nil, err
+				}
+				return us.DiscoveryMetadata.GetLabels(), nil
+			}, "15s", "0.5s").Should(BeEquivalentTo(
+				map[string]string{
+					"gloo":     svcName,
+					watchedKey: watchedValue,
+					bonusKey:   modifiedBonusValue,
+				},
+			))
+		})
+
 		It("Does not discover upstream with no label when watched labels are set", func() {
 			watchedKey := "A"
 			watchedValue := "B"

test/kubernetes/e2e/debugging.md

@@ -6,7 +6,7 @@ This document describes workflows that may be useful when debugging e2e tests wi
 
 The entry point for an e2e test is a Go test function of the form `func TestXyz(t *testing.T)` which represents a top level suite against an installation mode of Gloo. For example, the `TestK8sGateway` function in [k8s_gw_test.go](/test/kubernetes/e2e/tests/k8s_gw_test.go) is a top-level suite comprising multiple feature specific suites that are invoked as subtests.
 
-Each feature suite is invoked as a subtest of the top level suite. The subtests use [testify](https://github.com/stretchr/testify) to structure the tests in the feature's test suite and make use of the libarary's assertions.
+Each feature suite is invoked as a subtest of the top level suite. The subtests use [testify](https://github.com/stretchr/testify) to structure the tests in the feature's test suite and make use of the library's assertions.
 
 ## Workflows
 

test/kubernetes/e2e/features/discovery_watchlabels/discovery_watchlabels_suite.go

@@ -0,0 +1,161 @@
+package discovery_watchlabels
+
+import (
+	"context"
+
+	"github.com/onsi/gomega"
+
+	v1 "github.com/solo-io/gloo/projects/gloo/pkg/api/v1/kube/apis/gloo.solo.io/v1"
+	"github.com/solo-io/gloo/projects/gloo/pkg/api/v1/options"
+	"github.com/solo-io/gloo/projects/gloo/pkg/plugins/kubernetes"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	"github.com/solo-io/gloo/projects/gloo/pkg/defaults"
+	"github.com/solo-io/gloo/test/kubernetes/e2e"
+	"github.com/solo-io/solo-kit/pkg/api/v1/clients"
+	"github.com/solo-io/solo-kit/pkg/api/v1/resources"
+	"github.com/solo-io/solo-kit/pkg/api/v1/resources/core"
+	"github.com/stretchr/testify/suite"
+)
+
+var _ e2e.NewSuiteFunc = NewDiscoveryWatchlabelsSuite
+
+// discoveryWatchlabelsSuite is the Suite of tests for validating Upstream discovery behavior when watchLabels are enabled
+// This suite replaces the "upstream discovery" Context block from kube2e gateway tests
+type discoveryWatchlabelsSuite struct {
+	suite.Suite
+
+	ctx context.Context
+
+	// testInstallation contains all the metadata/utilities necessary to execute a series of tests
+	// against an installation of Gloo Gateway
+	testInstallation *e2e.TestInstallation
+}
+
+func NewDiscoveryWatchlabelsSuite(ctx context.Context, testInst *e2e.TestInstallation) suite.TestingSuite {
+	return &discoveryWatchlabelsSuite{
+		ctx:              ctx,
+		testInstallation: testInst,
+	}
+}
+
+func (s *discoveryWatchlabelsSuite) TestDiscoverUpstreamMatchingWatchLabels() {
+	s.T().Cleanup(func() {
+		err := s.testInstallation.Actions.Kubectl().DeleteFile(s.ctx, serviceWithLabelsManifest, "-n", s.testInstallation.Metadata.InstallNamespace)
+		s.Assertions.NoError(err, "can delete service")
+
+		err = s.testInstallation.Actions.Kubectl().DeleteFile(s.ctx, serviceWithoutLabelsManifest, "-n", s.testInstallation.Metadata.InstallNamespace)
+		s.Assertions.NoError(err, "can delete service")
+
+		err = s.testInstallation.Actions.Kubectl().DeleteFile(s.ctx, serviceWithNoMatchingLabelsManifest, "-n", s.testInstallation.Metadata.InstallNamespace)
+		s.Assertions.NoError(err, "can delete service")
+	})
+
+	// add one service with labels matching our watchLabels
+	err := s.testInstallation.Actions.Kubectl().ApplyFile(s.ctx, serviceWithLabelsManifest, "-n", s.testInstallation.Metadata.InstallNamespace)
+	s.Assert().NoError(err, "can apply service")
+
+	// add one service without labels matching our watchLabels
+	err = s.testInstallation.Actions.Kubectl().ApplyFile(s.ctx, serviceWithoutLabelsManifest, "-n", s.testInstallation.Metadata.InstallNamespace)
+	s.Assert().NoError(err, "can apply service")
+
+	// add one service with a label matching our watchLabels but with an unwatched value
+	err = s.testInstallation.Actions.Kubectl().ApplyFile(s.ctx, serviceWithNoMatchingLabelsManifest, "-n", s.testInstallation.Metadata.InstallNamespace)
+	s.Assert().NoError(err, "can apply service")
+
+	// eventually an Upstream should be created for the Service with matching labels
+	labeledUsName := kubernetes.UpstreamName(s.testInstallation.Metadata.InstallNamespace, "example-svc", 8000)
+	s.testInstallation.Assertions.EventuallyResourceStatusMatchesState(
+		func() (resources.InputResource, error) {
+			return s.testInstallation.ResourceClients.UpstreamClient().Read(s.testInstallation.Metadata.InstallNamespace, labeledUsName, clients.ReadOpts{Ctx: s.ctx})
+		},
+		core.Status_Accepted,
+		defaults.GlooReporter,
+	)
+
+	// the Upstream should have DiscoveryMetadata labels matching the parent Service
+	us, err := s.testInstallation.ResourceClients.UpstreamClient().Read(s.testInstallation.Metadata.InstallNamespace, labeledUsName, clients.ReadOpts{Ctx: s.ctx})
+	s.Assert().NoError(err, "can read upstream")
+
+	s.Assert().Equal(map[string]string{
+		"watchedKey": "watchedValue",
+		"bonusKey":   "bonusValue",
+	}, us.GetDiscoveryMetadata().GetLabels())
+
+	// no Upstream should be created for the Service that does not have the watchLabels
+	noLabelsUsName := kubernetes.UpstreamName(s.testInstallation.Metadata.InstallNamespace, "example-svc-no-labels", 8000)
+	s.testInstallation.Assertions.ConsistentlyObjectsNotExist(
+		s.ctx, &v1.Upstream{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      noLabelsUsName,
+				Namespace: s.testInstallation.Metadata.InstallNamespace,
+			},
+		},
+	)
+
+	// no Upstream should be created for the Service that has a watched label without a watched value
+	noMatchingLabelsUsName := kubernetes.UpstreamName(s.testInstallation.Metadata.InstallNamespace, "example-svc-no-matching-labels", 8000)
+	s.testInstallation.Assertions.ConsistentlyObjectsNotExist(
+		s.ctx, &v1.Upstream{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      noMatchingLabelsUsName,
+				Namespace: s.testInstallation.Metadata.InstallNamespace,
+			},
+		},
+	)
+
+	// modify the non-watched label on the labeled service
+	err = s.testInstallation.Actions.Kubectl().ApplyFile(s.ctx, serviceWithModifiedLabelsManifest, "-n", s.testInstallation.Metadata.InstallNamespace)
+	s.Assert().NoError(err, "can re-apply service")
+
+	// expect the Upstream's DiscoveryMeta to eventually match the modified labels from the parent Service
+	s.testInstallation.Assertions.Gomega.Eventually(func() (map[string]string, error) {
+		us, err = s.testInstallation.ResourceClients.UpstreamClient().Read(s.testInstallation.Metadata.InstallNamespace, labeledUsName, clients.ReadOpts{Ctx: s.ctx})
+		return us.GetDiscoveryMetadata().GetLabels(), err
+	}).Should(gomega.Equal(map[string]string{
+		"watchedKey": "watchedValue",
+		"bonusKey":   "bonusValue-modified",
+	}))
+}
+
+func (s *discoveryWatchlabelsSuite) TestDiscoverySpecPreserved() {
+	s.T().Cleanup(func() {
+		err := s.testInstallation.Actions.Kubectl().DeleteFile(s.ctx, serviceWithLabelsManifest, "-n", s.testInstallation.Metadata.InstallNamespace)
+		s.Assertions.NoError(err, "can delete service")
+	})
+
+	// add one service with labels matching our watchLabels
+	err := s.testInstallation.Actions.Kubectl().ApplyFile(s.ctx, serviceWithLabelsManifest, "-n", s.testInstallation.Metadata.InstallNamespace)
+	s.Assert().NoError(err, "can apply service")
+
+	// eventually an Upstream should be created for the Service with matching labels
+	labeledUsName := kubernetes.UpstreamName(s.testInstallation.Metadata.InstallNamespace, "example-svc", 8000)
+	s.testInstallation.Assertions.EventuallyResourceStatusMatchesState(
+		func() (resources.InputResource, error) {
+			return s.testInstallation.ResourceClients.UpstreamClient().Read(s.testInstallation.Metadata.InstallNamespace, labeledUsName, clients.ReadOpts{Ctx: s.ctx})
+		},
+		core.Status_Accepted,
+		defaults.GlooReporter,
+	)
+
+	// the Upstream should have DiscoveryMetadata labels matching the parent Service
+	us, err := s.testInstallation.ResourceClients.UpstreamClient().Read(s.testInstallation.Metadata.InstallNamespace, labeledUsName, clients.ReadOpts{Ctx: s.ctx})
+	s.Assert().NoError(err, "can read upstream")
+
+	s.Assert().NotNil(us.GetKube())
+	s.Assert().Nil(us.GetKube().GetServiceSpec())
+
+	// modify the Upstream to have a ServiceSpec
+	us.GetKube().ServiceSpec = &options.ServiceSpec{
+		PluginType: &options.ServiceSpec_GrpcJsonTranscoder{},
+	}
+	updatedUs, err := s.testInstallation.ResourceClients.UpstreamClient().Write(us, clients.WriteOpts{Ctx: s.ctx, OverwriteExisting: true})
+	s.Assert().NoError(err, "can update upstream")
+	s.Assert().NotNil(updatedUs.GetKube().GetServiceSpec())
+
+	// expect the Upstream to consistently have the modified Spec
+	s.testInstallation.Assertions.Gomega.Consistently(func() (*options.ServiceSpec, error) {
+		us, err := s.testInstallation.ResourceClients.UpstreamClient().Read(us.GetMetadata().GetNamespace(), us.GetMetadata().GetName(), clients.ReadOpts{Ctx: s.ctx})
+		return us.GetKube().GetServiceSpec(), err
+	}).Should(gomega.Not(gomega.BeNil()))
+}

test/kubernetes/e2e/features/discovery_watchlabels/testdata/service-with-labels.yaml

@@ -0,0 +1,14 @@
+apiVersion: v1
+kind: Service
+metadata:
+  labels:
+    watchedKey: watchedValue
+    bonusKey: bonusValue
+  name: example-svc
+spec:
+  ports:
+    - name: http
+      port: 8000
+      targetPort: 8080
+  selector:
+    selectorKey: selectorValue

test/kubernetes/e2e/features/discovery_watchlabels/testdata/service-with-modified-labels.yaml

@@ -0,0 +1,14 @@
+apiVersion: v1
+kind: Service
+metadata:
+  labels:
+    watchedKey: watchedValue
+    bonusKey: bonusValue-modified
+  name: example-svc
+spec:
+  ports:
+    - name: http
+      port: 8000
+      targetPort: 8080
+  selector:
+    selectorKey: selectorValue

test/kubernetes/e2e/features/discovery_watchlabels/testdata/service-with-no-matching-labels.yaml

@@ -0,0 +1,13 @@
+apiVersion: v1
+kind: Service
+metadata:
+  labels:
+    watchedKey: unwatchedValue
+  name: example-svc-no-matching-labels
+spec:
+  ports:
+    - name: http
+      port: 8000
+      targetPort: 8080
+  selector:
+    selectorKey: selectorValue

test/kubernetes/e2e/features/discovery_watchlabels/testdata/service-without-labels.yaml

@@ -0,0 +1,11 @@
+apiVersion: v1
+kind: Service
+metadata:
+  name: example-svc-no-labels
+spec:
+  ports:
+    - name: http
+      port: 8000
+      targetPort: 8080
+  selector:
+    selectorKey: selectorValue

test/kubernetes/e2e/features/discovery_watchlabels/types.go

@@ -0,0 +1,14 @@
+package discovery_watchlabels
+
+import (
+	"path/filepath"
+
+	"github.com/solo-io/skv2/codegen/util"
+)
+
+var (
+	serviceWithLabelsManifest           = filepath.Join(util.MustGetThisDir(), "testdata/service-with-labels.yaml")
+	serviceWithModifiedLabelsManifest   = filepath.Join(util.MustGetThisDir(), "testdata/service-with-modified-labels.yaml")
+	serviceWithoutLabelsManifest        = filepath.Join(util.MustGetThisDir(), "testdata/service-without-labels.yaml")
+	serviceWithNoMatchingLabelsManifest = filepath.Join(util.MustGetThisDir(), "testdata/service-with-no-matching-labels.yaml")
+)