Address review comments to Peering Acceptor and Dialer.

hashicorp · Jun 14, 2022 · 485fd73 · 485fd73
1 parent 9ecfff4
commit 485fd73
Show file tree

Hide file tree

Showing 30 changed files with 531 additions and 494 deletions.
diff --git a/charts/consul/templates/connect-inject-clusterrole.yaml b/charts/consul/templates/connect-inject-clusterrole.yaml
@@ -24,14 +24,6 @@ rules:
   - "get"
   - "list"
   - "watch"
-- apiGroups: [ "" ]
-  resources: ["secrets"]
-  verbs:
-    - "get"
-    - "list"
-    - "watch"
-    - "create"
-    - "delete"
 - apiGroups:
   - coordination.k8s.io
   resources:
@@ -52,6 +44,15 @@ rules:
   - watch
   - patch
 {{- end }}
+{{- if .Values.global.peering.enabled }}
+- apiGroups: [ "" ]
+  resources: ["secrets"]
+  verbs:
+  - "get"
+  - "list"
+  - "watch"
+  - "create"
+  - "delete"
 - apiGroups: ["consul.hashicorp.com"]
   resources: ["peeringacceptors"]
   verbs:
@@ -88,6 +89,7 @@ rules:
     - get
     - patch
     - update
+{{- end }}
 {{- if .Values.global.enablePodSecurityPolicies }}
 - apiGroups: [ "policy" ]
   resources: [ "podsecuritypolicies" ]

diff --git a/charts/consul/templates/crd-exportedservices.yaml b/charts/consul/templates/crd-exportedservices.yaml
@@ -75,9 +75,9 @@ spec:
                             description: Partition is the admin partition to export
                               the service to.
                             type: string
-                          peerName:
-                            description: '[Experimental] PeerName is the name of the
-                              peer to export the service to.'
+                          peer:
+                            description: '[Experimental] Peer is the name of the peer
+                              to export the service to.'
                             type: string
                         type: object
                       type: array

diff --git a/control-plane/api/v1alpha1/exportedservices_types.go b/control-plane/api/v1alpha1/exportedservices_types.go
@@ -68,8 +68,8 @@ type ExportedService struct {
 type ServiceConsumer struct {
 	// Partition is the admin partition to export the service to.
 	Partition string `json:"partition,omitempty"`
-	// [Experimental] PeerName is the name of the peer to export the service to.
-	PeerName string `json:"peerName,omitempty"`
+	// [Experimental] Peer is the name of the peer to export the service to.
+	Peer string `json:"peer,omitempty"`
 }
 
 func (in *ExportedServices) GetObjectMeta() metav1.ObjectMeta {
@@ -165,11 +165,10 @@ func (in *ExportedServices) ToConsul(datacenter string) api.ConfigEntry {
 func (in *ExportedService) toConsul() capi.ExportedService {
 	var consumers []capi.ServiceConsumer
 	for _, consumer := range in.Consumers {
-		if consumer.PeerName != "" {
-			consumers = append(consumers, capi.ServiceConsumer{PeerName: consumer.PeerName})
-		} else {
-			consumers = append(consumers, capi.ServiceConsumer{Partition: consumer.Partition})
-		}
+		consumers = append(consumers, capi.ServiceConsumer{
+			Partition: consumer.Partition,
+			PeerName:  consumer.Peer,
+		})
 	}
 	return capi.ExportedService{
 		Name:      in.Name,
@@ -228,11 +227,11 @@ func (in *ExportedService) validate(path *field.Path, consulMeta common.ConsulMe
 }
 
 func (in *ServiceConsumer) validate(path *field.Path, consulMeta common.ConsulMeta) *field.Error {
-	if in.Partition != "" && in.PeerName != "" {
-		return field.Invalid(path, *in, "both partition and peerName cannot be specified.")
+	if in.Partition != "" && in.Peer != "" {
+		return field.Invalid(path, *in, "both partition and peer cannot be specified.")
 	}
-	if in.Partition == "" && in.PeerName == "" {
-		return field.Invalid(path, *in, "either partition or peerName must be specified.")
+	if in.Partition == "" && in.Peer == "" {
+		return field.Invalid(path, *in, "either partition or peer must be specified.")
 	}
 	if !consulMeta.PartitionsEnabled && in.Partition != "" {
 		return field.Invalid(path.Child("partitions"), in.Partition, "Consul Admin Partitions need to be enabled to specify partition.")

diff --git a/control-plane/api/v1alpha1/exportedservices_types_test.go b/control-plane/api/v1alpha1/exportedservices_types_test.go
@@ -54,7 +54,7 @@ func TestExportedServices_MatchesConsul(t *testing.T) {
 									Partition: "third",
 								},
 								{
-									PeerName: "second-peer",
+									Peer: "second-peer",
 								},
 							},
 						},
@@ -69,7 +69,7 @@ func TestExportedServices_MatchesConsul(t *testing.T) {
 									Partition: "fifth",
 								},
 								{
-									PeerName: "third-peer",
+									Peer: "third-peer",
 								},
 							},
 						},
@@ -178,7 +178,7 @@ func TestExportedServices_ToConsul(t *testing.T) {
 									Partition: "third",
 								},
 								{
-									PeerName: "second-peer",
+									Peer: "second-peer",
 								},
 							},
 						},
@@ -193,7 +193,7 @@ func TestExportedServices_ToConsul(t *testing.T) {
 									Partition: "fifth",
 								},
 								{
-									PeerName: "third-peer",
+									Peer: "third-peer",
 								},
 							},
 						},
@@ -253,8 +253,10 @@ func TestExportedServices_ToConsul(t *testing.T) {
 
 func TestExportedServices_Validate(t *testing.T) {
 	cases := map[string]struct {
-		input           *ExportedServices
-		expectedErrMsgs []string
+		input             *ExportedServices
+		namespaceEnabled  bool
+		partitionsEnabled bool
+		expectedErrMsgs   []string
 	}{
 		"valid": {
 			input: &ExportedServices{
@@ -271,14 +273,16 @@ func TestExportedServices_Validate(t *testing.T) {
 									Partition: "second",
 								},
 								{
-									PeerName: "second-peer",
+									Peer: "second-peer",
 								},
 							},
 						},
 					},
 				},
 			},
-			expectedErrMsgs: []string{},
+			namespaceEnabled:  true,
+			partitionsEnabled: true,
+			expectedErrMsgs:   []string{},
 		},
 		"no consumers specified": {
 			input: &ExportedServices{
@@ -295,6 +299,8 @@ func TestExportedServices_Validate(t *testing.T) {
 					},
 				},
 			},
+			namespaceEnabled:  true,
+			partitionsEnabled: true,
 			expectedErrMsgs: []string{
 				`spec.services[0]: Invalid value: []v1alpha1.ServiceConsumer{}: service must have at least 1 consumer.`,
 			},
@@ -312,15 +318,17 @@ func TestExportedServices_Validate(t *testing.T) {
 							Consumers: []ServiceConsumer{
 								{
 									Partition: "second",
-									PeerName:  "second-peer",
+									Peer:      "second-peer",
 								},
 							},
 						},
 					},
 				},
 			},
+			namespaceEnabled:  true,
+			partitionsEnabled: true,
 			expectedErrMsgs: []string{
-				`spec.services[0].consumers[0]: Invalid value: v1alpha1.ServiceConsumer{Partition:"second", PeerName:"second-peer"}: both partition and peerName cannot be specified.`,
+				`spec.services[0].consumers[0]: Invalid value: v1alpha1.ServiceConsumer{Partition:"second", Peer:"second-peer"}: both partition and peer cannot be specified.`,
 			},
 		},
 		"neither partition nor peer name specified": {
@@ -340,8 +348,60 @@ func TestExportedServices_Validate(t *testing.T) {
 					},
 				},
 			},
+			namespaceEnabled:  true,
+			partitionsEnabled: true,
 			expectedErrMsgs: []string{
-				`spec.services[0].consumers[0]: Invalid value: v1alpha1.ServiceConsumer{Partition:"", PeerName:""}: either partition or peerName must be specified.`,
+				`spec.services[0].consumers[0]: Invalid value: v1alpha1.ServiceConsumer{Partition:"", Peer:""}: either partition or peer must be specified.`,
+			},
+		},
+		"partition provided when partitions are disabled": {
+			input: &ExportedServices{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: common.DefaultConsulPartition,
+				},
+				Spec: ExportedServicesSpec{
+					Services: []ExportedService{
+						{
+							Name:      "service-frontend",
+							Namespace: "frontend",
+							Consumers: []ServiceConsumer{
+								{
+									Partition: "test-partition",
+								},
+							},
+						},
+					},
+				},
+			},
+			namespaceEnabled:  true,
+			partitionsEnabled: false,
+			expectedErrMsgs: []string{
+				`spec.services[0].consumers[0].partitions: Invalid value: "test-partition": Consul Admin Partitions need to be enabled to specify partition.`,
+			},
+		},
+		"namespace provided when namespaces are disabled": {
+			input: &ExportedServices{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: common.DefaultConsulPartition,
+				},
+				Spec: ExportedServicesSpec{
+					Services: []ExportedService{
+						{
+							Name:      "service-frontend",
+							Namespace: "frontend",
+							Consumers: []ServiceConsumer{
+								{
+									Peer: "test-peer",
+								},
+							},
+						},
+					},
+				},
+			},
+			namespaceEnabled:  false,
+			partitionsEnabled: false,
+			expectedErrMsgs: []string{
+				`spec.services[0]: Invalid value: "frontend": Consul Namespaces must be enabled to specify service namespace.`,
 			},
 		},
 		"multiple errors": {
@@ -357,24 +417,26 @@ func TestExportedServices_Validate(t *testing.T) {
 							Consumers: []ServiceConsumer{
 								{
 									Partition: "second",
-									PeerName:  "second-peer",
+									Peer:      "second-peer",
 								},
 								{},
 							},
 						},
 					},
 				},
 			},
+			namespaceEnabled:  true,
+			partitionsEnabled: true,
 			expectedErrMsgs: []string{
-				`spec.services[0].consumers[0]: Invalid value: v1alpha1.ServiceConsumer{Partition:"second", PeerName:"second-peer"}: both partition and peerName cannot be specified.`,
-				`spec.services[0].consumers[1]: Invalid value: v1alpha1.ServiceConsumer{Partition:"", PeerName:""}: either partition or peerName must be specified.`,
+				`spec.services[0].consumers[0]: Invalid value: v1alpha1.ServiceConsumer{Partition:"second", Peer:"second-peer"}: both partition and peer cannot be specified.`,
+				`spec.services[0].consumers[1]: Invalid value: v1alpha1.ServiceConsumer{Partition:"", Peer:""}: either partition or peer must be specified.`,
 			},
 		},
 	}
 
 	for name, testCase := range cases {
 		t.Run(name, func(t *testing.T) {
-			err := testCase.input.Validate(common.ConsulMeta{NamespacesEnabled: true, PartitionsEnabled: true, Partition: common.DefaultConsulPartition})
+			err := testCase.input.Validate(common.ConsulMeta{NamespacesEnabled: testCase.namespaceEnabled, PartitionsEnabled: testCase.partitionsEnabled, Partition: common.DefaultConsulPartition})
 			if len(testCase.expectedErrMsgs) != 0 {
 				require.Error(t, err)
 				for _, s := range testCase.expectedErrMsgs {

diff --git a/control-plane/config/crd/bases/consul.hashicorp.com_exportedservices.yaml b/control-plane/config/crd/bases/consul.hashicorp.com_exportedservices.yaml
@@ -68,9 +68,9 @@ spec:
                             description: Partition is the admin partition to export
                               the service to.
                             type: string
-                          peerName:
-                            description: '[Experimental] PeerName is the name of the
-                              peer to export the service to.'
+                          peer:
+                            description: '[Experimental] Peer is the name of the peer
+                              to export the service to.'
                             type: string
                         type: object
                       type: array

diff --git a/control-plane/connect-inject/annotations.go b/control-plane/connect-inject/annotations.go
@@ -136,7 +136,7 @@ const (
 	annotationTransparentProxyOverwriteProbes = "consul.hashicorp.com/transparent-proxy-overwrite-probes"
 
 	// annotationOriginalPod is the value of the pod before being overwritten by the consul
-	// webhook/handler.
+	// webhook/meshWebhook.
 	annotationOriginalPod = "consul.hashicorp.com/original-pod"
 
 	// labelServiceIgnore is a label that can be added to a service to prevent it from being

diff --git a/control-plane/connect-inject/consul_sidecar.go b/control-plane/connect-inject/consul_sidecar.go
@@ -11,7 +11,7 @@ import (
 // the metrics merging server when metrics merging feature is enabled.
 // It always disables service registration because for connect we no longer
 // need to keep services registered as this is handled in the endpoints-controller.
-func (w *ConnectWebhook) consulSidecar(pod corev1.Pod) (corev1.Container, error) {
+func (w *MeshWebhook) consulSidecar(pod corev1.Pod) (corev1.Container, error) {
 	metricsPorts, err := w.MetricsConfig.mergedMetricsServerConfiguration(pod)
 	if err != nil {
 		return corev1.Container{}, err
@@ -48,7 +48,7 @@ func (w *ConnectWebhook) consulSidecar(pod corev1.Pod) (corev1.Container, error)
 	}, nil
 }
 
-func (w *ConnectWebhook) consulSidecarResources(pod corev1.Pod) (corev1.ResourceRequirements, error) {
+func (w *MeshWebhook) consulSidecarResources(pod corev1.Pod) (corev1.ResourceRequirements, error) {
 	resources := corev1.ResourceRequirements{
 		Limits:   corev1.ResourceList{},
 		Requests: corev1.ResourceList{},