ref(*): remove CN from *envoy.Proxy (#4773)

Removes the CN from the proxy struct, as that is no 
longer a static identifier on the proxy itself.

This is part of the effort to allow rotating trust domains.

Signed-off-by: Sean Teeling <seanteeling@microsoft.com>

cmd/osm-healthcheck/osm-healthcheck_test.go

@@ -58,6 +58,8 @@ func TestGetHealthcheckHander(t *testing.T) {
 				//#nosec G307
 				defer listener.Close()
 
+				// required to avoid https://github.com/golang/go/wiki/CommonMistakes#using-goroutines-on-loop-iterator-variables
+				test := test
 				go func() {
 					conn, err := listener.Accept()
 					assert.Nil(err)

docs/release_notes.md

@@ -1,5 +1,19 @@
 # Release Notes
 
+## Release v1.2.0
+
+### Notable changes
+
+- Root certificate rotation is now supported through the new CRD, MeshRootCertificate.
+- Along with root certificate rotation we support custom trust domains, as well as rotating to new trust domains with no downtime.
+
+### Breaking Changes
+
+- The following metrics no longer use the label `common_name`, due to the fact that the common name's trust domain can rotate. Instead 2 new labels, `proxy_uuid` and `identity` have been added.
+  - `osm_proxy_response_send_success_count`
+  - `osm_proxy_response_send_error_count`
+  - `osm_proxy_xds_request_count`
+
 ## Release v1.1.0
 
 ### Notable changes

pkg/catalog/endpoint_test.go

@@ -192,9 +192,9 @@ func TestListAllowedUpstreamEndpointsForService(t *testing.T) {
 					}
 					pod.Status.PodIPs = podIps
 					pod.Spec.ServiceAccountName = sa.Name
-					_, err := kubeClient.CoreV1().Pods(tests.Namespace).Create(context.TODO(), &pod, metav1.CreateOptions{})
+					_, err := kubeClient.CoreV1().Pods(tests.Namespace).Create(context.TODO(), pod, metav1.CreateOptions{})
 					assert.Nil(err)
-					pods = append(pods, &pod)
+					pods = append(pods, pod)
 				}
 			}
 			mockKubeController.EXPECT().ListPods().Return(pods).AnyTimes()

pkg/catalog/fake/fake.go

@@ -27,15 +27,9 @@ import (
 
 // NewFakeMeshCatalog creates a new struct implementing catalog.MeshCataloger interface used for testing.
 func NewFakeMeshCatalog(kubeClient kubernetes.Interface, meshConfigClient configClientset.Interface) *catalog.MeshCatalog {
-	var (
-		mockCtrl             *gomock.Controller
-		mockKubeController   *k8s.MockController
-		mockPolicyController *policy.MockController
-	)
-
-	mockCtrl = gomock.NewController(ginkgo.GinkgoT())
-	mockKubeController = k8s.NewMockController(mockCtrl)
-	mockPolicyController = policy.NewMockController(mockCtrl)
+	mockCtrl := gomock.NewController(ginkgo.GinkgoT())
+	mockKubeController := k8s.NewMockController(mockCtrl)
+	mockPolicyController := policy.NewMockController(mockCtrl)
 
 	meshSpec := smiFake.NewFakeMeshSpecClient()
 

pkg/catalog/helpers_test.go

@@ -57,19 +57,19 @@ func newFakeMeshCatalogForRoutes(t *testing.T, testParams testParams) *MeshCatal
 
 	// Create a bookstoreV1 pod
 	bookstoreV1Pod := tests.NewPodFixture(tests.BookstoreV1Service.Namespace, tests.BookstoreV1Service.Name, tests.BookstoreServiceAccountName, tests.PodLabels)
-	if _, err := kubeClient.CoreV1().Pods(tests.BookstoreV1Service.Namespace).Create(context.TODO(), &bookstoreV1Pod, metav1.CreateOptions{}); err != nil {
+	if _, err := kubeClient.CoreV1().Pods(tests.BookstoreV1Service.Namespace).Create(context.TODO(), bookstoreV1Pod, metav1.CreateOptions{}); err != nil {
 		t.Fatalf("Error creating new pod: %s", err.Error())
 	}
 
 	// Create a bookstoreV2 pod
 	bookstoreV2Pod := tests.NewPodFixture(tests.BookstoreV2Service.Namespace, tests.BookstoreV2Service.Name, tests.BookstoreV2ServiceAccountName, tests.PodLabels)
-	if _, err := kubeClient.CoreV1().Pods(tests.BookstoreV2Service.Namespace).Create(context.TODO(), &bookstoreV2Pod, metav1.CreateOptions{}); err != nil {
+	if _, err := kubeClient.CoreV1().Pods(tests.BookstoreV2Service.Namespace).Create(context.TODO(), bookstoreV2Pod, metav1.CreateOptions{}); err != nil {
 		t.Fatalf("Error creating new pod: %s", err.Error())
 	}
 
 	// Create a bookbuyer pod
 	bookbuyerPod := tests.NewPodFixture(tests.BookbuyerService.Namespace, tests.BookbuyerService.Name, tests.BookbuyerServiceAccountName, tests.PodLabels)
-	if _, err := kubeClient.CoreV1().Pods(tests.BookbuyerService.Namespace).Create(context.TODO(), &bookbuyerPod, metav1.CreateOptions{}); err != nil {
+	if _, err := kubeClient.CoreV1().Pods(tests.BookbuyerService.Namespace).Create(context.TODO(), bookbuyerPod, metav1.CreateOptions{}); err != nil {
 		t.Fatalf("Error creating new pod: %s", err.Error())
 	}
 

pkg/catalog/traffictarget.go

@@ -139,8 +139,7 @@ func trafficTargetIdentityToSvcAccount(identitySubject smiAccess.IdentityBinding
 
 // trafficTargetIdentityToServiceIdentity returns an identity of the form <namespace>/<service-account>
 func trafficTargetIdentityToServiceIdentity(identitySubject smiAccess.IdentityBindingSubject) identity.ServiceIdentity {
-	svcAccount := trafficTargetIdentityToSvcAccount(identitySubject)
-	return identity.GetKubernetesServiceIdentity(svcAccount, identity.ClusterLocalTrustDomain)
+	return trafficTargetIdentityToSvcAccount(identitySubject).ToServiceIdentity()
 }
 
 // trafficTargetIdentitiesToSvcAccounts returns a list of Service Accounts from the given list of identities from a Traffic Target

pkg/debugger/proxy.go

@@ -51,23 +51,23 @@ func (ds DebugConfig) printProxies(w http.ResponseWriter) {
 		ts := proxy.GetConnectedAt()
 		proxyURL := fmt.Sprintf("/debug/proxy?%s=%s", uuidQueryKey, proxy.UUID)
 		configDumpURL := fmt.Sprintf("%s&%s=%t", proxyURL, proxyConfigQueryKey, true)
-		_, _ = fmt.Fprintf(w, `<tr><td>%d:</td><td>%s</td><td>%+v</td><td>(%+v ago)</td><td><a href="%s">certs</a></td><td><a href="%s">cfg</a></td></tr>`,
-			idx+1, proxy.Identity, ts, time.Since(ts), proxyURL, configDumpURL)
+		_, _ = fmt.Fprintf(w, `<tr><td>%d:</td><td>%s</td><td>%s</td><td>%+v</td><td>(%+v ago)</td><td><a href="%s">certs</a></td><td><a href="%s">cfg</a></td></tr>`,
+			idx+1, proxy.Identity, proxy.UUID, ts, time.Since(ts), proxyURL, configDumpURL)
 	}
 	_, _ = fmt.Fprint(w, `</table>`)
 }
 
 func (ds DebugConfig) getConfigDump(uuid string, w http.ResponseWriter) {
-	proxy, ok := ds.proxyRegistry.GetConnectedProxy(uuid)
-	if !ok {
+	proxy := ds.proxyRegistry.GetConnectedProxy(uuid)
+	if proxy != nil {
 		msg := fmt.Sprintf("Proxy for UUID %s not found, may have been disconnected", uuid)
 		log.Error().Msg(msg)
 		http.Error(w, msg, http.StatusNotFound)
 		return
 	}
-	pod, err := envoy.GetPodFromCertificate(proxy.GetCertificateCommonName(), ds.kubeController)
+	pod, err := ds.kubeController.GetPodForProxy(proxy)
 	if err != nil {
-		msg := fmt.Sprintf("Error getting Pod from certificate with CN=%s", proxy.GetCertificateCommonName())
+		msg := fmt.Sprintf("Error getting Pod from proxy %s", proxy.GetName())
 		log.Error().Err(err).Msg(msg)
 		http.Error(w, msg, http.StatusNotFound)
 		return
@@ -78,16 +78,16 @@ func (ds DebugConfig) getConfigDump(uuid string, w http.ResponseWriter) {
 }
 
 func (ds DebugConfig) getProxy(uuid string, w http.ResponseWriter) {
-	proxy, ok := ds.proxyRegistry.GetConnectedProxy(uuid)
-	if !ok {
+	proxy := ds.proxyRegistry.GetConnectedProxy(uuid)
+	if proxy == nil {
 		msg := fmt.Sprintf("Proxy for UUID %s not found, may have been disconnected", uuid)
 		log.Error().Msg(msg)
 		http.Error(w, msg, http.StatusNotFound)
 		return
 	}
-	pod, err := envoy.GetPodFromCertificate(proxy.GetCertificateCommonName(), ds.kubeController)
+	pod, err := ds.kubeController.GetPodForProxy(proxy)
 	if err != nil {
-		msg := fmt.Sprintf("Error getting Pod from certificate with CN=%s", proxy.GetCertificateCommonName())
+		msg := fmt.Sprintf("Error getting Pod from proxy %s", proxy.GetName())
 		log.Error().Err(err).Msg(msg)
 		http.Error(w, msg, http.StatusNotFound)
 		return

pkg/envoy/ads/cache_stream.go

@@ -12,10 +12,10 @@ import (
 	v1 "k8s.io/api/core/v1"
 
 	"github.com/openservicemesh/osm/pkg/announcements"
-	"github.com/openservicemesh/osm/pkg/certificate"
 	"github.com/openservicemesh/osm/pkg/constants"
 	"github.com/openservicemesh/osm/pkg/envoy"
 	"github.com/openservicemesh/osm/pkg/errcode"
+	"github.com/openservicemesh/osm/pkg/identity"
 )
 
 // Routine which fulfills listening to proxy broadcasts
@@ -61,9 +61,6 @@ func (s *Server) allPodUpdater() {
 // All verticals use the proxy structure to infer the pod later, so the actual only mandatory
 // data for the verticals to be functional is the common name, which links proxy <-> pod
 func GetProxyFromPod(pod *v1.Pod) (*envoy.Proxy, error) {
-	var serviceAccount string
-	var namespace string
-
 	uuidString, uuidFound := pod.Labels[constants.EnvoyUniqueIDLabelName]
 	if !uuidFound {
 		return nil, errors.Errorf("UUID not found for pod %s/%s, not a mesh pod", pod.Namespace, pod.Name)
@@ -73,27 +70,18 @@ func GetProxyFromPod(pod *v1.Pod) (*envoy.Proxy, error) {
 		return nil, errors.Errorf("Could not parse UUID label into UUID type (%s): %v", uuidString, err)
 	}
 
-	serviceAccount = pod.Spec.ServiceAccountName
-	namespace = pod.Namespace
-
-	// construct CN for this pod/proxy
-	// TODO: Infer proxy type from Pod
-	commonName := envoy.NewXDSCertCommonName(proxyUUID, envoy.KindSidecar, serviceAccount, namespace)
-	tempProxy, err := envoy.NewProxy(certificate.CommonName(commonName), "NoSerial", &net.IPAddr{IP: net.IPv4zero})
+	sa := pod.Spec.ServiceAccountName
+	namespace := pod.Namespace
 
-	return tempProxy, err
+	return envoy.NewProxy(envoy.KindSidecar, proxyUUID, identity.New(sa, namespace), &net.IPAddr{IP: net.IPv4zero}), nil
 }
 
 // RecordFullSnapshot stores a group of resources as a new Snapshot with a new version in the cache.
 // It also runs a consistency check on the snapshot (will warn if there are missing resources referenced in
 // the snapshot)
 func (s *Server) RecordFullSnapshot(proxy *envoy.Proxy, snapshotResources map[string][]types.Resource) error {
-	s.configVerMutex.Lock()
-	s.configVersion[proxy.GetCertificateCommonName().String()]++
-	s.configVerMutex.Unlock()
-
 	snapshot, err := cache.NewSnapshot(
-		fmt.Sprintf("%d", s.configVersion[proxy.GetCertificateCommonName().String()]),
+		fmt.Sprintf("%d", s.configVersion[proxy.UUID.String()]),
 		snapshotResources,
 	)
 	if err != nil {
@@ -104,5 +92,9 @@ func (s *Server) RecordFullSnapshot(proxy *envoy.Proxy, snapshotResources map[st
 		log.Warn().Err(err).Str("proxy", proxy.String()).Msgf("Snapshot for proxy not consistent")
 	}
 
-	return s.ch.SetSnapshot(context.TODO(), proxy.GetCertificateCommonName().String(), snapshot)
+	s.configVerMutex.Lock()
+	defer s.configVerMutex.Unlock()
+	s.configVersion[proxy.UUID.String()]++
+
+	return s.ch.SetSnapshot(context.TODO(), proxy.UUID.String(), snapshot)
 }

pkg/envoy/ads/cache_test.go

@@ -8,21 +8,18 @@ import (
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
-	"github.com/openservicemesh/osm/pkg/certificate"
 	"github.com/openservicemesh/osm/pkg/constants"
-	"github.com/openservicemesh/osm/pkg/envoy"
 )
 
 func TestGetProxyFromPod(t *testing.T) {
 	assert := tassert.New(t)
 
 	var (
 		// Default fixtures for various test variables
-		podName         = "pod"
-		namespace       = "namespace"
-		serviceAccount  = "serviceAccount"
-		validUUID       = uuid.New()
-		validCommonName = envoy.NewXDSCertCommonName(validUUID, envoy.KindSidecar, serviceAccount, namespace)
+		podName        = "pod"
+		namespace      = "namespace"
+		serviceAccount = "serviceAccount"
+		validUUID      = uuid.New()
 	)
 
 	testCases := []struct {
@@ -32,8 +29,7 @@ func TestGetProxyFromPod(t *testing.T) {
 		pod *v1.Pod
 
 		// Output check
-		expectErr  bool
-		commonName certificate.CommonName
+		expectErr bool
 	}{
 		{
 			testCaseName: "Pod with no label",
@@ -80,7 +76,6 @@ func TestGetProxyFromPod(t *testing.T) {
 					ServiceAccountName: serviceAccount,
 				},
 			},
-			commonName: validCommonName,
 		},
 	}
 
@@ -90,8 +85,8 @@ func TestGetProxyFromPod(t *testing.T) {
 		if testCase.expectErr {
 			assert.Error(err)
 		} else {
-			assert.Equal(proxyResult.GetCertificateCommonName(), testCase.commonName,
-				"%s: Did not return equal common name")
+			assert.NotNil(proxyResult)
+			assert.Equal(proxyResult.UUID, validUUID)
 		}
 	}
 }

pkg/envoy/ads/errors.go

@@ -10,3 +10,4 @@ var errGrpcClosed = errors.New("grpc closed")
 var errTooManyConnections = errors.New("too many connections")
 var errServiceAccountMismatch = errors.New("service account mismatch in nodeid vs xds certificate common name")
 var errUnsuportedXDSRequest = errors.New("Unsupported XDS server connection type")
+var errInvalidCertificateCN = errors.New("invalid cn")

pkg/envoy/ads/jobs.go

@@ -37,12 +37,5 @@ func (proxyJob *proxyResponseJob) Run() {
 
 // JobName implementation for this job, for logging purposes
 func (proxyJob *proxyResponseJob) JobName() string {
-	return fmt.Sprintf("sendJob-%s", proxyJob.proxy.GetCertificateSerialNumber())
-}
-
-// Hash implementation for this job to hash into the worker queues
-func (proxyJob *proxyResponseJob) Hash() uint64 {
-	// Uses proxy hash to always serialize work for the same proxy to the same worker,
-	// this avoid out-of-order mishandling of envoy updates by multiple workers
-	return proxyJob.proxy.GetHash()
+	return fmt.Sprintf("sendJob-%s", proxyJob.proxy.GetName())
 }

pkg/envoy/ads/profile.go

@@ -28,23 +28,23 @@ func xdsPathTimeTrack(startedAt time.Time, typeURI envoy.TypeURI, proxy *envoy.P
 		Observe(elapsed.Seconds())
 }
 
-func (s *Server) trackXDSLog(proxyName string, typeURL envoy.TypeURI) {
+func (s *Server) trackXDSLog(proxyUUID string, typeURL envoy.TypeURI) {
 	s.withXdsLogMutex(func() {
-		if _, ok := s.xdsLog[proxyName]; !ok {
-			s.xdsLog[proxyName] = make(map[envoy.TypeURI][]time.Time)
+		if _, ok := s.xdsLog[proxyUUID]; !ok {
+			s.xdsLog[proxyUUID] = make(map[envoy.TypeURI][]time.Time)
 		}
 
-		timeSlice, ok := s.xdsLog[proxyName][typeURL]
+		timeSlice, ok := s.xdsLog[proxyUUID][typeURL]
 		if !ok {
-			s.xdsLog[proxyName][typeURL] = []time.Time{time.Now()}
+			s.xdsLog[proxyUUID][typeURL] = []time.Time{time.Now()}
 			return
 		}
 
 		timeSlice = append(timeSlice, time.Now())
 		if len(timeSlice) > MaxXdsLogsPerProxy {
 			timeSlice = timeSlice[1:]
 		}
-		s.xdsLog[proxyName][typeURL] = timeSlice
+		s.xdsLog[proxyUUID][typeURL] = timeSlice
 	})
 }
 

pkg/envoy/ads/profile_test.go

@@ -1,7 +1,6 @@
 package ads
 
 import (
-	"fmt"
 	"testing"
 
 	xds_cluster "github.com/envoyproxy/go-control-plane/envoy/config/cluster/v3"
@@ -10,15 +9,14 @@ import (
 	"github.com/google/uuid"
 	tassert "github.com/stretchr/testify/assert"
 
-	"github.com/openservicemesh/osm/pkg/certificate"
 	"github.com/openservicemesh/osm/pkg/envoy"
+	"github.com/openservicemesh/osm/pkg/identity"
 	"github.com/openservicemesh/osm/pkg/tests"
 )
 
 func TestValidateResourcesRequestResponse(t *testing.T) {
 	assert := tassert.New(t)
-	proxy, err := envoy.NewProxy(certificate.CommonName(fmt.Sprintf("%s.sidecar.foo.bar", uuid.New())), certificate.SerialNumber("123"), tests.NewMockAddress("1.2.3.4"))
-	assert.Nil(err)
+	proxy := envoy.NewProxy(envoy.KindSidecar, uuid.New(), identity.New("foo", "bar"), tests.NewMockAddress("1.2.3.4"))
 
 	testCases := []struct {
 		request          *xds_discovery.DiscoveryRequest

pkg/envoy/ads/response.go

@@ -132,7 +132,7 @@ func (s *Server) SendDiscoveryResponse(proxy *envoy.Proxy, request *xds_discover
 		proto, err := anypb.New(res.(proto.Message))
 		if err != nil {
 			log.Error().Err(err).Str(errcode.Kind, errcode.GetErrCodeWithMetric(errcode.ErrMarshallingXDSResource)).
-				Msgf("Error marshalling resource %s for proxy %s", typeURI, proxy.GetCertificateSerialNumber())
+				Msgf("Error marshalling resource %s for proxy %s", typeURI, proxy.GetName())
 			continue
 		}
 		// Add resource to response
@@ -161,15 +161,15 @@ func (s *Server) SendDiscoveryResponse(proxy *envoy.Proxy, request *xds_discover
 
 	// Send the response
 	if err := (*server).Send(response); err != nil {
-		metricsstore.DefaultMetricsStore.ProxyResponseSendErrorCount.WithLabelValues(proxy.GetCertificateCommonName().String(), string(typeURI)).Inc()
+		metricsstore.DefaultMetricsStore.ProxyResponseSendErrorCount.WithLabelValues(proxy.UUID.String(), proxy.Identity.String(), string(typeURI)).Inc()
 		log.Error().Err(err).Str(errcode.Kind, errcode.GetErrCodeWithMetric(errcode.ErrSendingDiscoveryResponse)).
 			Str("proxy", proxy.String()).Msgf("Error sending response for typeURI %s to proxy", typeURI.Short())
 		return err
 	}
 
 	// Sending discovery response succeeded, record last resources sent
 	proxy.SetLastResourcesSent(typeURI, resourcesSent)
-	metricsstore.DefaultMetricsStore.ProxyResponseSendSuccessCount.WithLabelValues(proxy.GetCertificateCommonName().String(), string(typeURI)).Inc()
+	metricsstore.DefaultMetricsStore.ProxyResponseSendSuccessCount.WithLabelValues(proxy.UUID.String(), proxy.Identity.String(), string(typeURI)).Inc()
 
 	return nil
 }