Merge pull request #2524 from snehachhabria/edsCleanup

ref(pkg/envoy/eds/*): Remove redundant code from EDS and update test

pkg/envoy/eds/response.go

@@ -17,14 +17,6 @@ import (
 
 // NewResponse creates a new Endpoint Discovery Response.
 func NewResponse(meshCatalog catalog.MeshCataloger, proxy *envoy.Proxy, _ *xds_discovery.DiscoveryRequest, _ configurator.Configurator, _ certificate.Manager) (*xds_discovery.DiscoveryResponse, error) {
-	svcList, err := meshCatalog.GetServicesFromEnvoyCertificate(proxy.GetCertificateCommonName())
-	if err != nil {
-		log.Error().Err(err).Msgf("Error looking up MeshService for Envoy with SerialNumber=%s on Pod with UID=%s", proxy.GetCertificateSerialNumber(), proxy.GetPodUID())
-		return nil, err
-	}
-	// Github Issue #1575
-	proxyServiceName := svcList[0]
-
 	proxyIdentity, err := catalog.GetServiceAccountFromProxyCertificate(proxy.GetCertificateCommonName())
 	if err != nil {
 		log.Error().Err(err).Msgf("Error looking up proxy identity for proxy with SerialNumber=%s on Pod with UID=%s", proxy.GetCertificateSerialNumber(), proxy.GetPodUID())
@@ -41,14 +33,14 @@ func NewResponse(meshCatalog catalog.MeshCataloger, proxy *envoy.Proxy, _ *xds_d
 		outboundServicesEndpoints[dstSvc] = endpoints
 	}
 
-	log.Trace().Msgf("Outbound service endpoints for proxy %s: %v", proxyServiceName, outboundServicesEndpoints)
+	log.Trace().Msgf("Outbound service endpoints for proxy with SerialNumber=%s on Pod with UID=%s: %v", proxy.GetCertificateSerialNumber(), proxy.GetPodUID(), outboundServicesEndpoints)
 
 	var protos []*any.Any
 	for svc, endpoints := range outboundServicesEndpoints {
 		loadAssignment := cla.NewClusterLoadAssignment(svc, endpoints)
 		proto, err := ptypes.MarshalAny(loadAssignment)
 		if err != nil {
-			log.Error().Err(err).Msgf("Error marshalling EDS payload for proxy %s: %+v", proxyServiceName, loadAssignment)
+			log.Error().Err(err).Msgf("Error marshalling EDS payload for proxy with SerialNumber=%s on Pod with UID=%s: %+v", proxy.GetCertificateSerialNumber(), proxy.GetPodUID(), loadAssignment)
 			continue
 		}
 		protos = append(protos, proto)

pkg/envoy/eds/response_test.go

@@ -1,16 +1,13 @@
 package eds
 
 import (
-	"context"
 	"fmt"
-
-	"github.com/google/uuid"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	testclient "k8s.io/client-go/kubernetes/fake"
+	"testing"
 
 	"github.com/golang/mock/gomock"
-	. "github.com/onsi/ginkgo"
-	. "github.com/onsi/gomega"
+	tassert "github.com/stretchr/testify/assert"
+	"k8s.io/client-go/kubernetes"
+	testclient "k8s.io/client-go/kubernetes/fake"
 
 	"github.com/openservicemesh/osm/pkg/catalog"
 	"github.com/openservicemesh/osm/pkg/certificate"
@@ -20,71 +17,50 @@ import (
 	"github.com/openservicemesh/osm/pkg/tests"
 )
 
-var _ = Describe("Test EDS response", func() {
-	var (
-		mockCtrl         *gomock.Controller
-		mockConfigurator *configurator.MockConfigurator
-	)
-
-	mockCtrl = gomock.NewController(GinkgoT())
-	mockConfigurator = configurator.NewMockConfigurator(mockCtrl)
-
+func getProxy(kubeClient kubernetes.Interface) (*envoy.Proxy, error) {
+	podLabels := map[string]string{
+		tests.SelectorKey:                tests.BookbuyerService.Name,
+		constants.EnvoyUniqueIDLabelName: tests.ProxyUUID,
+	}
+	if _, err := tests.MakePod(kubeClient, tests.Namespace, tests.BookbuyerServiceName, tests.BookbuyerServiceAccountName, podLabels); err != nil {
+		return nil, err
+	}
+
+	selectors := map[string]string{
+		tests.SelectorKey: tests.BookbuyerServiceName,
+	}
+	if _, err := tests.MakeService(kubeClient, tests.BookbuyerServiceName, selectors); err != nil {
+		return nil, err
+	}
+
+	for _, svcName := range []string{tests.BookstoreApexServiceName, tests.BookstoreV1ServiceName, tests.BookstoreV2ServiceName} {
+		selectors := map[string]string{
+			tests.SelectorKey: "bookstore",
+		}
+		if _, err := tests.MakeService(kubeClient, svcName, selectors); err != nil {
+			return nil, err
+		}
+	}
+
+	certCommonName := certificate.CommonName(fmt.Sprintf("%s.%s.%s", tests.ProxyUUID, tests.BookbuyerServiceAccountName, tests.Namespace))
+	certSerialNumber := certificate.SerialNumber("123456")
+	proxy := envoy.NewProxy(certCommonName, certSerialNumber, nil)
+	return proxy, nil
+}
+
+func TestEndpointConfiguration(t *testing.T) {
+	assert := tassert.New(t)
+	mockCtrl := gomock.NewController(t)
+	mockConfigurator := configurator.NewMockConfigurator(mockCtrl)
 	kubeClient := testclient.NewSimpleClientset()
-	catalog := catalog.NewFakeMeshCatalog(kubeClient)
-
-	Context("Test eds.NewResponse", func() {
-		It("Correctly returns an response for endpoints when the certificate and service are valid", func() {
-			// Initialize the proxy service
-			proxyServiceName := tests.BookbuyerServiceName
-			proxyServiceAccountName := tests.BookbuyerServiceAccountName
-			proxyUUID := uuid.New()
-
-			// The format of the CN matters
-			xdsCertificate := certificate.CommonName(fmt.Sprintf("%s.%s.%s.foo.bar", proxyUUID, proxyServiceAccountName, tests.Namespace))
-			certSerialNumber := certificate.SerialNumber("123456")
-			proxy := envoy.NewProxy(xdsCertificate, certSerialNumber, nil)
-
-			{
-				// Create a pod to match the CN
-				podName := fmt.Sprintf("pod-0-%s", uuid.New())
-				pod := tests.NewPodFixture(tests.Namespace, podName, proxyServiceAccountName, tests.PodLabels)
-
-				pod.Labels[constants.EnvoyUniqueIDLabelName] = proxyUUID.String() // This is what links the Pod and the Certificate
-				_, err := kubeClient.CoreV1().Pods(tests.Namespace).Create(context.TODO(), &pod, metav1.CreateOptions{})
-				Expect(err).ToNot(HaveOccurred())
-			}
-
-			{
-				// Create a service for the pod created above
-				selectors := map[string]string{
-					// These need to match the POD created above
-					tests.SelectorKey: tests.SelectorValue,
-				}
-				// The serviceName must match the SMI
-				service := tests.NewServiceFixture(proxyServiceName, tests.Namespace, selectors)
-				_, err := kubeClient.CoreV1().Services(tests.Namespace).Create(context.TODO(), service, metav1.CreateOptions{})
-				Expect(err).ToNot(HaveOccurred())
-			}
-
-			_, err := NewResponse(catalog, proxy, nil, mockConfigurator, nil)
-			Expect(err).ToNot(HaveOccurred())
-		})
-
-		It("Correctly returns an error response for endpoints when the proxy isn't associated with a MeshService", func() {
-			// Initialize the proxy service
-			proxyServiceAccountName := "non-existent-service-account"
-			proxyUUID := uuid.New()
-
-			// The format of the CN matters
-			xdsCertificate := certificate.CommonName(fmt.Sprintf("%s.%s.%s.foo.bar", proxyUUID, proxyServiceAccountName, tests.Namespace))
-			certSerialNumber := certificate.SerialNumber("123456")
-			proxy := envoy.NewProxy(xdsCertificate, certSerialNumber, nil)
+	meshCatalog := catalog.NewFakeMeshCatalog(kubeClient)
 
-			// Don't create a pod/service for this proxy, this should result in an error when the
-			// service is being looked up based on the proxy's certificate
+	proxy, err := getProxy(kubeClient)
+	assert.Empty(err)
+	assert.NotNil(meshCatalog)
+	assert.NotNil(proxy)
 
-			_, err := NewResponse(catalog, proxy, nil, mockConfigurator, nil)
-			Expect(err).To(HaveOccurred())
-		})
-	})
-})
+	actual, err := NewResponse(meshCatalog, proxy, nil, mockConfigurator, nil)
+	assert.Empty(err)
+	assert.NotNil(actual)
+}