diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 18e6a7e192..aff9311354 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -146,7 +146,7 @@ jobs: working-directory: control-plane run: | mkdir -p $HOME/bin - wget https://github.com/ndhanushkodi/binaries/releases/download/v3oss/consul -O consulbin && \ + wget https://github.com/ndhanushkodi/binaries/releases/download/v4oss/consul -O consulbin && \ mv consulbin $HOME/bin/consul && chmod +x $HOME/bin/consul @@ -194,7 +194,7 @@ jobs: working-directory: control-plane run: | mkdir -p $HOME/bin - wget https://github.com/ndhanushkodi/binaries/releases/download/v3ent/consul -O consulbin && \ + wget https://github.com/ndhanushkodi/binaries/releases/download/v4ent/consul -O consulbin && \ mv consulbin $HOME/bin/consul && chmod +x $HOME/bin/consul diff --git a/acceptance/go.mod b/acceptance/go.mod index c6282e61a1..2cc9576f61 100644 --- a/acceptance/go.mod +++ b/acceptance/go.mod @@ -5,9 +5,10 @@ go 1.17 require ( github.com/gruntwork-io/terratest v0.31.2 github.com/hashicorp/consul-k8s/control-plane v0.0.0-20211207212234-aea9efea5638 - github.com/hashicorp/consul/api v1.12.0 + github.com/hashicorp/consul/api v1.10.1-0.20220614213650-6453375ab228 github.com/hashicorp/consul/sdk v0.9.0 github.com/hashicorp/go-uuid v1.0.3 + github.com/hashicorp/go-version v1.2.0 github.com/hashicorp/vault/api v1.2.0 github.com/stretchr/testify v1.7.0 gopkg.in/yaml.v2 v2.4.0 @@ -34,7 +35,7 @@ require ( github.com/gogo/protobuf v1.3.2 // indirect github.com/golang/protobuf v1.5.2 // indirect github.com/golang/snappy v0.0.1 // indirect - github.com/google/go-cmp v0.5.6 // indirect + github.com/google/go-cmp v0.5.7 // indirect github.com/google/gofuzz v1.1.0 // indirect github.com/google/uuid v1.1.2 // indirect github.com/googleapis/gnostic v0.5.5 // indirect @@ -50,7 +51,6 @@ require ( github.com/hashicorp/go-secure-stdlib/parseutil v0.1.1 // indirect github.com/hashicorp/go-secure-stdlib/strutil v0.1.1 // indirect github.com/hashicorp/go-sockaddr v1.0.2 // indirect - github.com/hashicorp/go-version v1.2.0 // indirect github.com/hashicorp/golang-lru v0.5.3 // indirect github.com/hashicorp/hcl v1.0.0 // indirect github.com/hashicorp/serf v0.9.6 // indirect diff --git a/acceptance/go.sum b/acceptance/go.sum index 4625fbf9f0..48012f8c78 100644 --- a/acceptance/go.sum +++ b/acceptance/go.sum @@ -333,8 +333,9 @@ github.com/google/go-cmp v0.5.0/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/ github.com/google/go-cmp v0.5.4/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= -github.com/google/go-cmp v0.5.6 h1:BKbKCqvP6I+rmFHt06ZmyQtvB8xAkWdhFyr0ZUNZcxQ= github.com/google/go-cmp v0.5.6/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= +github.com/google/go-cmp v0.5.7 h1:81/ik6ipDQS2aGcBfIN5dHDB36BwrStyeAQquSYCV4o= +github.com/google/go-cmp v0.5.7/go.mod h1:n+brtR0CgQNWTVd5ZUFpTBC8YFBDLK/h/bpaJ8/DtOE= github.com/google/go-containerregistry v0.0.0-20200110202235-f4fb41bf00a3/go.mod h1:2wIuQute9+hhWqvL3vEI7YB0EKluF4WcPzI1eAliazk= github.com/google/go-querystring v0.0.0-20170111101155-53e6ce116135/go.mod h1:odCYkC5MyYFN7vkCjXpyrEuKhc/BUO6wN/zVPAxq5ck= github.com/google/go-querystring v1.0.0/go.mod h1:odCYkC5MyYFN7vkCjXpyrEuKhc/BUO6wN/zVPAxq5ck= @@ -387,8 +388,8 @@ github.com/hashicorp/consul-k8s/control-plane v0.0.0-20211207212234-aea9efea5638 github.com/hashicorp/consul-k8s/control-plane v0.0.0-20211207212234-aea9efea5638/go.mod h1:7ZeaiADGbvJDuoWAT8UKj6KCcLsFUk+34OkUGMVtdXg= github.com/hashicorp/consul/api v1.1.0/go.mod h1:VmuI/Lkw1nC05EYQWNKwWGbkg+FbDBtguAZLlVdkD9Q= github.com/hashicorp/consul/api v1.10.1-0.20211116182834-e6956893fb6f/go.mod h1:6pVBMo0ebnYdt2S3H87XhekM/HHrUoTD2XXb/VrZVy0= -github.com/hashicorp/consul/api v1.12.0 h1:k3y1FYv6nuKyNTqj6w9gXOx5r5CfLj/k/euUeBXj1OY= -github.com/hashicorp/consul/api v1.12.0/go.mod h1:6pVBMo0ebnYdt2S3H87XhekM/HHrUoTD2XXb/VrZVy0= +github.com/hashicorp/consul/api v1.10.1-0.20220614213650-6453375ab228 h1:BqzKe5O+75uYcFfJI0mJz3rhCgdVztvEj3rEs4xpPr0= +github.com/hashicorp/consul/api v1.10.1-0.20220614213650-6453375ab228/go.mod h1:ZlVrynguJKcYr54zGaDbaL3fOvKC9m72FhPvA8T35KQ= github.com/hashicorp/consul/sdk v0.1.1/go.mod h1:VKf9jXwCTEY1QZP2MOLRhb5i/I/ssyNV1vwHyQBF0x8= github.com/hashicorp/consul/sdk v0.8.0/go.mod h1:GBvyrGALthsZObzUGsfgHZQDXjg4lOjagTIwIR1vPms= github.com/hashicorp/consul/sdk v0.9.0 h1:NGSHAU7X3yDCjo8WBUbNOtD3BSqv8u0vu3+zNxgmxQI= diff --git a/charts/consul/templates/connect-inject-clusterrole.yaml b/charts/consul/templates/connect-inject-clusterrole.yaml index 00e2e23a50..1165782c4b 100644 --- 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 index 6ed5406cba..27d03dbb06 100644 --- 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 index 477574d945..3087379c62 100644 --- 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 index c7c1df6b62..0810f8edaf 100644 --- 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,7 +417,7 @@ func TestExportedServices_Validate(t *testing.T) { Consumers: []ServiceConsumer{ { Partition: "second", - PeerName: "second-peer", + Peer: "second-peer", }, {}, }, @@ -365,16 +425,18 @@ func TestExportedServices_Validate(t *testing.T) { }, }, }, + 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 index ddcc51b2b8..da1a66fd74 100644 --- 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 index d1c59d19dc..1139642a49 100644 --- 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 index 2f2bac34b8..a19eebb5ef 100644 --- 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{}, diff --git a/control-plane/connect-inject/consul_sidecar_test.go b/control-plane/connect-inject/consul_sidecar_test.go index fac2454991..bafaad104a 100644 --- a/control-plane/connect-inject/consul_sidecar_test.go +++ b/control-plane/connect-inject/consul_sidecar_test.go @@ -13,7 +13,7 @@ import ( // Test that if the conditions for running a merged metrics server are true, // that we pass the metrics flags to consul sidecar. func TestConsulSidecar_MetricsFlags(t *testing.T) { - handler := ConnectWebhook{ + meshWebhook := MeshWebhook{ Log: logrtest.TestLogger{T: t}, ImageConsulK8S: "hashicorp/consul-k8s:9.9.9", MetricsConfig: MetricsConfig{ @@ -21,7 +21,7 @@ func TestConsulSidecar_MetricsFlags(t *testing.T) { DefaultEnableMetricsMerging: true, }, } - container, err := handler.consulSidecar(corev1.Pod{ + container, err := meshWebhook.consulSidecar(corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ annotationMergedMetricsPort: "20100", @@ -53,13 +53,13 @@ func TestHandlerConsulSidecar_Resources(t *testing.T) { zero := resource.MustParse("0") cases := map[string]struct { - handler ConnectWebhook + meshWebhook MeshWebhook annotations map[string]string expResources corev1.ResourceRequirements expErr string }{ "no defaults, no annotations": { - handler: ConnectWebhook{ + meshWebhook: MeshWebhook{ Log: logrtest.TestLogger{T: t}, ImageConsulK8S: "hashicorp/consul-k8s:9.9.9", MetricsConfig: MetricsConfig{ @@ -78,7 +78,7 @@ func TestHandlerConsulSidecar_Resources(t *testing.T) { }, }, "all defaults, no annotations": { - handler: ConnectWebhook{ + meshWebhook: MeshWebhook{ Log: logrtest.TestLogger{T: t}, ImageConsulK8S: "hashicorp/consul-k8s:9.9.9", MetricsConfig: MetricsConfig{ @@ -113,7 +113,7 @@ func TestHandlerConsulSidecar_Resources(t *testing.T) { }, }, "no defaults, all annotations": { - handler: ConnectWebhook{ + meshWebhook: MeshWebhook{ Log: logrtest.TestLogger{T: t}, ImageConsulK8S: "hashicorp/consul-k8s:9.9.9", MetricsConfig: MetricsConfig{ @@ -142,7 +142,7 @@ func TestHandlerConsulSidecar_Resources(t *testing.T) { }, }, "annotations override defaults": { - handler: ConnectWebhook{ + meshWebhook: MeshWebhook{ Log: logrtest.TestLogger{T: t}, ImageConsulK8S: "hashicorp/consul-k8s:9.9.9", MetricsConfig: MetricsConfig{ @@ -181,7 +181,7 @@ func TestHandlerConsulSidecar_Resources(t *testing.T) { }, }, "defaults set to zero, no annotations": { - handler: ConnectWebhook{ + meshWebhook: MeshWebhook{ Log: logrtest.TestLogger{T: t}, ImageConsulK8S: "hashicorp/consul-k8s:9.9.9", MetricsConfig: MetricsConfig{ @@ -216,7 +216,7 @@ func TestHandlerConsulSidecar_Resources(t *testing.T) { }, }, "annotations set to 0": { - handler: ConnectWebhook{ + meshWebhook: MeshWebhook{ Log: logrtest.TestLogger{T: t}, ImageConsulK8S: "hashicorp/consul-k8s:9.9.9", MetricsConfig: MetricsConfig{ @@ -245,7 +245,7 @@ func TestHandlerConsulSidecar_Resources(t *testing.T) { }, }, "invalid cpu request": { - handler: ConnectWebhook{ + meshWebhook: MeshWebhook{ Log: logrtest.TestLogger{T: t}, ImageConsulK8S: "hashicorp/consul-k8s:9.9.9", MetricsConfig: MetricsConfig{ @@ -262,7 +262,7 @@ func TestHandlerConsulSidecar_Resources(t *testing.T) { expErr: "parsing annotation consul.hashicorp.com/consul-sidecar-cpu-request:\"invalid\": quantities must match the regular expression", }, "invalid cpu limit": { - handler: ConnectWebhook{ + meshWebhook: MeshWebhook{ Log: logrtest.TestLogger{T: t}, ImageConsulK8S: "hashicorp/consul-k8s:9.9.9", MetricsConfig: MetricsConfig{ @@ -279,7 +279,7 @@ func TestHandlerConsulSidecar_Resources(t *testing.T) { expErr: "parsing annotation consul.hashicorp.com/consul-sidecar-cpu-limit:\"invalid\": quantities must match the regular expression", }, "invalid memory request": { - handler: ConnectWebhook{ + meshWebhook: MeshWebhook{ Log: logrtest.TestLogger{T: t}, ImageConsulK8S: "hashicorp/consul-k8s:9.9.9", MetricsConfig: MetricsConfig{ @@ -296,7 +296,7 @@ func TestHandlerConsulSidecar_Resources(t *testing.T) { expErr: "parsing annotation consul.hashicorp.com/consul-sidecar-memory-request:\"invalid\": quantities must match the regular expression", }, "invalid memory limit": { - handler: ConnectWebhook{ + meshWebhook: MeshWebhook{ Log: logrtest.TestLogger{T: t}, ImageConsulK8S: "hashicorp/consul-k8s:9.9.9", MetricsConfig: MetricsConfig{ @@ -330,7 +330,7 @@ func TestHandlerConsulSidecar_Resources(t *testing.T) { }, }, } - container, err := c.handler.consulSidecar(pod) + container, err := c.meshWebhook.consulSidecar(pod) if c.expErr != "" { require.NotNil(err) require.Contains(err.Error(), c.expErr) diff --git a/control-plane/connect-inject/container_env.go b/control-plane/connect-inject/container_env.go index f20976d5f9..4ad2b156cd 100644 --- a/control-plane/connect-inject/container_env.go +++ b/control-plane/connect-inject/container_env.go @@ -8,7 +8,7 @@ import ( corev1 "k8s.io/api/core/v1" ) -func (w *ConnectWebhook) containerEnvVars(pod corev1.Pod) []corev1.EnvVar { +func (w *MeshWebhook) containerEnvVars(pod corev1.Pod) []corev1.EnvVar { raw, ok := pod.Annotations[annotationUpstreams] if !ok || raw == "" { return []corev1.EnvVar{} diff --git a/control-plane/connect-inject/container_env_test.go b/control-plane/connect-inject/container_env_test.go index 41eb45d71b..cb29b6e742 100644 --- a/control-plane/connect-inject/container_env_test.go +++ b/control-plane/connect-inject/container_env_test.go @@ -28,8 +28,8 @@ func TestContainerEnvVars(t *testing.T) { t.Run(tt.Name, func(t *testing.T) { require := require.New(t) - var h ConnectWebhook - envVars := h.containerEnvVars(corev1.Pod{ + var w MeshWebhook + envVars := w.containerEnvVars(corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ annotationService: "foo", diff --git a/control-plane/connect-inject/container_init.go b/control-plane/connect-inject/container_init.go index 3c4974784a..66ab836863 100644 --- a/control-plane/connect-inject/container_init.go +++ b/control-plane/connect-inject/container_init.go @@ -93,7 +93,7 @@ type initContainerCommandData struct { // initCopyContainer returns the init container spec for the copy container which places // the consul binary into the shared volume. -func (w *ConnectWebhook) initCopyContainer() corev1.Container { +func (w *MeshWebhook) initCopyContainer() corev1.Container { // Copy the Consul binary from the image to the shared volume. cmd := "cp /bin/consul /consul/connect-inject/consul" container := corev1.Container{ @@ -123,7 +123,7 @@ func (w *ConnectWebhook) initCopyContainer() corev1.Container { // containerInit returns the init container spec for connect-init that polls for the service and the connect proxy service to be registered // so that it can save the proxy service id to the shared volume and boostrap Envoy with the proxy-id. -func (w *ConnectWebhook) containerInit(namespace corev1.Namespace, pod corev1.Pod, mpi multiPortInfo) (corev1.Container, error) { +func (w *MeshWebhook) containerInit(namespace corev1.Namespace, pod corev1.Pod, mpi multiPortInfo) (corev1.Container, error) { // Check if tproxy is enabled on this pod. tproxyEnabled, err := transparentProxyEnabled(namespace, pod, w.EnableTransparentProxy) if err != nil { @@ -283,7 +283,7 @@ func (w *ConnectWebhook) containerInit(namespace corev1.Namespace, pod corev1.Po // constructDNSServiceHostName use the resource prefix and the DNS Service hostname suffix to construct the // key of the env variable whose value is the cluster IP of the Consul DNS Service. // It translates "resource-prefix" into "RESOURCE_PREFIX_DNS_SERVICE_HOST". -func (w *ConnectWebhook) constructDNSServiceHostName() string { +func (w *MeshWebhook) constructDNSServiceHostName() string { upcaseResourcePrefix := strings.ToUpper(w.ResourcePrefix) upcaseResourcePrefixWithUnderscores := strings.ReplaceAll(upcaseResourcePrefix, "-", "_") return strings.Join([]string{upcaseResourcePrefixWithUnderscores, dnsServiceHostEnvSuffix}, "_") diff --git a/control-plane/connect-inject/container_init_test.go b/control-plane/connect-inject/container_init_test.go index 1954ea2f61..f97868f603 100644 --- a/control-plane/connect-inject/container_init_test.go +++ b/control-plane/connect-inject/container_init_test.go @@ -46,7 +46,7 @@ func TestHandlerContainerInit(t *testing.T) { cases := []struct { Name string Pod func(*corev1.Pod) *corev1.Pod - Handler ConnectWebhook + Webhook MeshWebhook Cmd string // Strings.Contains test CmdNot string // Not contains }{ @@ -58,7 +58,7 @@ func TestHandlerContainerInit(t *testing.T) { pod.Annotations[annotationService] = "web" return pod }, - ConnectWebhook{}, + MeshWebhook{}, `/bin/sh -ec export CONSUL_HTTP_ADDR="${HOST_IP}:8500" export CONSUL_GRPC_ADDR="${HOST_IP}:8502" @@ -85,7 +85,7 @@ consul-k8s-control-plane connect-init -pod-name=${POD_NAME} -pod-namespace=${POD } return pod }, - ConnectWebhook{ + MeshWebhook{ AuthMethod: "an-auth-method", ConsulAPITimeout: 5 * time.Second, }, @@ -118,7 +118,7 @@ consul-k8s-control-plane connect-init -pod-name=${POD_NAME} -pod-namespace=${POD pod.Annotations[annotationPrometheusScrapePath] = "/scrape-path" return pod }, - ConnectWebhook{ + MeshWebhook{ ConsulAPITimeout: 5 * time.Second, }, `# Generate the envoy bootstrap code @@ -135,7 +135,7 @@ consul-k8s-control-plane connect-init -pod-name=${POD_NAME} -pod-namespace=${POD t.Run(tt.Name, func(t *testing.T) { require := require.New(t) - h := tt.Handler + h := tt.Webhook pod := *tt.Pod(minimal()) container, err := h.containerInit(testNS, pod, multiPortInfo{}) require.NoError(err) @@ -287,7 +287,7 @@ func TestHandlerContainerInit_transparentProxy(t *testing.T) { } for name, c := range cases { t.Run(name, func(t *testing.T) { - h := ConnectWebhook{ + w := MeshWebhook{ EnableTransparentProxy: c.globalEnabled, ConsulAPITimeout: 5 * time.Second, } @@ -305,7 +305,7 @@ func TestHandlerContainerInit_transparentProxy(t *testing.T) { } ns := testNS ns.Labels = c.namespaceLabel - container, err := h.containerInit(ns, *pod, multiPortInfo{}) + container, err := w.containerInit(ns, *pod, multiPortInfo{}) require.NoError(t, err) actualCmd := strings.Join(container.Command, " ") @@ -384,7 +384,7 @@ func TestHandlerContainerInit_consulDNS(t *testing.T) { } for name, c := range cases { t.Run(name, func(t *testing.T) { - h := ConnectWebhook{ + w := MeshWebhook{ EnableConsulDNS: c.globalEnabled, EnableTransparentProxy: true, ResourcePrefix: "consul-consul", @@ -398,7 +398,7 @@ func TestHandlerContainerInit_consulDNS(t *testing.T) { ns := testNS ns.Labels = c.namespaceLabel - container, err := h.containerInit(ns, *pod, multiPortInfo{}) + container, err := w.containerInit(ns, *pod, multiPortInfo{}) require.NoError(t, err) actualCmd := strings.Join(container.Command, " ") @@ -428,8 +428,8 @@ func TestHandler_constructDNSServiceHostName(t *testing.T) { for _, c := range cases { t.Run(c.prefix, func(t *testing.T) { - h := ConnectWebhook{ResourcePrefix: c.prefix, ConsulAPITimeout: 5 * time.Second} - require.Equal(t, c.result, h.constructDNSServiceHostName()) + w := MeshWebhook{ResourcePrefix: c.prefix, ConsulAPITimeout: 5 * time.Second} + require.Equal(t, c.result, w.constructDNSServiceHostName()) }) } } @@ -469,7 +469,7 @@ func TestHandlerContainerInit_namespacesAndPartitionsEnabled(t *testing.T) { cases := []struct { Name string Pod func(*corev1.Pod) *corev1.Pod - Handler ConnectWebhook + Webhook MeshWebhook Cmd string // Strings.Contains test }{ { @@ -478,7 +478,7 @@ func TestHandlerContainerInit_namespacesAndPartitionsEnabled(t *testing.T) { pod.Annotations[annotationService] = "web" return pod }, - ConnectWebhook{ + MeshWebhook{ EnableNamespaces: true, ConsulDestinationNamespace: "default", ConsulPartition: "", @@ -503,7 +503,7 @@ consul-k8s-control-plane connect-init -pod-name=${POD_NAME} -pod-namespace=${POD pod.Annotations[annotationService] = "web" return pod }, - ConnectWebhook{ + MeshWebhook{ EnableNamespaces: true, ConsulDestinationNamespace: "default", ConsulPartition: "default", @@ -530,7 +530,7 @@ consul-k8s-control-plane connect-init -pod-name=${POD_NAME} -pod-namespace=${POD pod.Annotations[annotationService] = "web" return pod }, - ConnectWebhook{ + MeshWebhook{ EnableNamespaces: true, ConsulDestinationNamespace: "non-default", ConsulPartition: "", @@ -555,7 +555,7 @@ consul-k8s-control-plane connect-init -pod-name=${POD_NAME} -pod-namespace=${POD pod.Annotations[annotationService] = "web" return pod }, - ConnectWebhook{ + MeshWebhook{ EnableNamespaces: true, ConsulDestinationNamespace: "non-default", ConsulPartition: "non-default-part", @@ -582,7 +582,7 @@ consul-k8s-control-plane connect-init -pod-name=${POD_NAME} -pod-namespace=${POD pod.Annotations[annotationService] = "" return pod }, - ConnectWebhook{ + MeshWebhook{ AuthMethod: "auth-method", EnableNamespaces: true, ConsulDestinationNamespace: "non-default", @@ -616,7 +616,7 @@ consul-k8s-control-plane connect-init -pod-name=${POD_NAME} -pod-namespace=${POD pod.Annotations[annotationService] = "" return pod }, - ConnectWebhook{ + MeshWebhook{ AuthMethod: "auth-method", EnableNamespaces: true, ConsulDestinationNamespace: "non-default", // Overridden by mirroring @@ -651,7 +651,7 @@ consul-k8s-control-plane connect-init -pod-name=${POD_NAME} -pod-namespace=${POD pod.Annotations[annotationService] = "web" return pod }, - ConnectWebhook{ + MeshWebhook{ EnableNamespaces: true, ConsulDestinationNamespace: "default", ConsulPartition: "", @@ -683,7 +683,7 @@ consul-k8s-control-plane connect-init -pod-name=${POD_NAME} -pod-namespace=${POD pod.Annotations[annotationService] = "web" return pod }, - ConnectWebhook{ + MeshWebhook{ EnableNamespaces: true, ConsulPartition: "default", ConsulDestinationNamespace: "non-default", @@ -719,7 +719,7 @@ consul-k8s-control-plane connect-init -pod-name=${POD_NAME} -pod-namespace=${POD pod.Annotations[annotationService] = "web" return pod }, - ConnectWebhook{ + MeshWebhook{ AuthMethod: "auth-method", EnableNamespaces: true, ConsulPartition: "non-default", @@ -763,8 +763,8 @@ consul-k8s-control-plane connect-init -pod-name=${POD_NAME} -pod-namespace=${POD t.Run(tt.Name, func(t *testing.T) { require := require.New(t) - h := tt.Handler - container, err := h.containerInit(testNS, *tt.Pod(minimal()), multiPortInfo{}) + w := tt.Webhook + container, err := w.containerInit(testNS, *tt.Pod(minimal()), multiPortInfo{}) require.NoError(err) actual := strings.Join(container.Command, " ") require.Equal(tt.Cmd, actual) @@ -818,7 +818,7 @@ func TestHandlerContainerInit_Multiport(t *testing.T) { cases := []struct { Name string Pod func(*corev1.Pod) *corev1.Pod - Handler ConnectWebhook + Webhook MeshWebhook NumInitContainers int MultiPortInfos []multiPortInfo Cmd []string // Strings.Contains test @@ -828,7 +828,7 @@ func TestHandlerContainerInit_Multiport(t *testing.T) { func(pod *corev1.Pod) *corev1.Pod { return pod }, - ConnectWebhook{ConsulAPITimeout: 5 * time.Second}, + MeshWebhook{ConsulAPITimeout: 5 * time.Second}, 2, []multiPortInfo{ { @@ -876,7 +876,7 @@ consul-k8s-control-plane connect-init -pod-name=${POD_NAME} -pod-namespace=${POD func(pod *corev1.Pod) *corev1.Pod { return pod }, - ConnectWebhook{ + MeshWebhook{ AuthMethod: "auth-method", ConsulAPITimeout: 5 * time.Second, }, @@ -938,9 +938,9 @@ consul-k8s-control-plane connect-init -pod-name=${POD_NAME} -pod-namespace=${POD t.Run(tt.Name, func(t *testing.T) { require := require.New(t) - h := tt.Handler + w := tt.Webhook for i := 0; i < tt.NumInitContainers; i++ { - container, err := h.containerInit(testNS, *tt.Pod(minimal()), tt.MultiPortInfos[i]) + container, err := w.containerInit(testNS, *tt.Pod(minimal()), tt.MultiPortInfos[i]) require.NoError(err) actual := strings.Join(container.Command, " ") require.Equal(tt.Cmd[i], actual) @@ -951,7 +951,7 @@ consul-k8s-control-plane connect-init -pod-name=${POD_NAME} -pod-namespace=${POD func TestHandlerContainerInit_authMethod(t *testing.T) { require := require.New(t) - h := ConnectWebhook{ + w := MeshWebhook{ AuthMethod: "release-name-consul-k8s-auth-method", ConsulAPITimeout: 5 * time.Second, } @@ -978,7 +978,7 @@ func TestHandlerContainerInit_authMethod(t *testing.T) { ServiceAccountName: "foo", }, } - container, err := h.containerInit(testNS, *pod, multiPortInfo{}) + container, err := w.containerInit(testNS, *pod, multiPortInfo{}) require.NoError(err) actual := strings.Join(container.Command, " ") require.Contains(actual, ` @@ -998,7 +998,7 @@ consul-k8s-control-plane connect-init -pod-name=${POD_NAME} -pod-namespace=${POD // and CA cert should be set as env variable. func TestHandlerContainerInit_WithTLS(t *testing.T) { require := require.New(t) - h := ConnectWebhook{ + w := MeshWebhook{ ConsulCACert: "consul-ca-cert", ConsulAPITimeout: 5 * time.Second, } @@ -1017,7 +1017,7 @@ func TestHandlerContainerInit_WithTLS(t *testing.T) { }, }, } - container, err := h.containerInit(testNS, *pod, multiPortInfo{}) + container, err := w.containerInit(testNS, *pod, multiPortInfo{}) require.NoError(err) actual := strings.Join(container.Command, " ") require.Contains(actual, ` @@ -1034,7 +1034,7 @@ export CONSUL_GRPC_ADDR="${HOST_IP}:8502"`) func TestHandlerContainerInit_Resources(t *testing.T) { require := require.New(t) - h := ConnectWebhook{ + w := MeshWebhook{ InitContainerResources: corev1.ResourceRequirements{ Requests: corev1.ResourceList{ corev1.ResourceCPU: resource.MustParse("10m"), @@ -1062,7 +1062,7 @@ func TestHandlerContainerInit_Resources(t *testing.T) { }, }, } - container, err := h.containerInit(testNS, *pod, multiPortInfo{}) + container, err := w.containerInit(testNS, *pod, multiPortInfo{}) require.NoError(err) require.Equal(corev1.ResourceRequirements{ Limits: corev1.ResourceList{ @@ -1082,9 +1082,9 @@ func TestHandlerInitCopyContainer(t *testing.T) { for _, openShiftEnabled := range openShiftEnabledCases { t.Run(fmt.Sprintf("openshift enabled: %t", openShiftEnabled), func(t *testing.T) { - h := ConnectWebhook{EnableOpenShift: openShiftEnabled, ConsulAPITimeout: 5 * time.Second} + w := MeshWebhook{EnableOpenShift: openShiftEnabled, ConsulAPITimeout: 5 * time.Second} - container := h.initCopyContainer() + container := w.initCopyContainer() if openShiftEnabled { require.Nil(t, container.SecurityContext) diff --git a/control-plane/connect-inject/container_volume.go b/control-plane/connect-inject/container_volume.go index c9e794b45b..53ba985f3e 100644 --- a/control-plane/connect-inject/container_volume.go +++ b/control-plane/connect-inject/container_volume.go @@ -10,7 +10,7 @@ const volumeName = "consul-connect-inject-data" // containerVolume returns the volume data to add to the pod. This volume // is used for shared data between containers. -func (w *ConnectWebhook) containerVolume() corev1.Volume { +func (w *MeshWebhook) containerVolume() corev1.Volume { return corev1.Volume{ Name: volumeName, VolumeSource: corev1.VolumeSource{ diff --git a/control-plane/connect-inject/endpoints_controller.go b/control-plane/connect-inject/endpoints_controller.go index 2fdd912b16..f91b306571 100644 --- a/control-plane/connect-inject/endpoints_controller.go +++ b/control-plane/connect-inject/endpoints_controller.go @@ -405,7 +405,7 @@ func getProxyServiceID(pod corev1.Pod, serviceEndpoints corev1.Endpoints) string func (r *EndpointsController) createServiceRegistrations(pod corev1.Pod, serviceEndpoints corev1.Endpoints) (*api.AgentServiceRegistration, *api.AgentServiceRegistration, error) { // If a port is specified, then we determine the value of that port // and register that port for the host service. - // The handler will always set the port annotation if one is not provided on the pod. + // The meshWebhook will always set the port annotation if one is not provided on the pod. var consulServicePort int if raw, ok := pod.Annotations[annotationPort]; ok && raw != "" { if multiPort := strings.Split(raw, ","); len(multiPort) > 1 { @@ -807,6 +807,62 @@ func (r *EndpointsController) deleteACLTokensForServiceInstance(client *api.Clie return nil } +// processUpstreams reads the list of upstreams from the Pod annotation and converts them into a list of api.Upstream +// objects. +func (r *EndpointsController) processUpstreams(pod corev1.Pod, endpoints corev1.Endpoints) ([]api.Upstream, error) { + // In a multiport pod, only the first service's proxy should have upstreams configured. This skips configuring + // upstreams on additional services on the pod. + mpIdx := getMultiPortIdx(pod, endpoints) + if mpIdx > 0 { + return []api.Upstream{}, nil + } + + var upstreams []api.Upstream + if raw, ok := pod.Annotations[annotationUpstreams]; ok && raw != "" { + for _, raw := range strings.Split(raw, ",") { + var upstream api.Upstream + + // parts separates out the port, and determines whether it's a prepared query or not, since parts[0] would + // be "prepared_query" if it is. + parts := strings.SplitN(raw, ":", 3) + + // serviceParts helps determine which format of upstream we're processing, + // [service-name].[service-namespace].[service-partition]:[port]:[optional datacenter] + // or + // [service-name].svc.[service-namespace].ns.[service-peer].peer:[port] + // [service-name].svc.[service-namespace].ns.[service-partition].ap:[port] + // [service-name].svc.[service-namespace].ns.[service-datacenter].dc:[port] + labeledFormat := false + serviceParts := strings.Split(parts[0], ".") + if len(serviceParts) >= 2 { + if serviceParts[1] == "svc" { + labeledFormat = true + } + } + + if strings.TrimSpace(parts[0]) == "prepared_query" { + upstream = processPreparedQueryUpstream(pod, raw) + } else if labeledFormat { + var err error + upstream, err = r.processLabeledUpstream(pod, raw) + if err != nil { + return []api.Upstream{}, err + } + } else { + var err error + upstream, err = r.processUnlabeledUpstream(pod, raw) + if err != nil { + return []api.Upstream{}, err + } + } + + upstreams = append(upstreams, upstream) + } + } + + return upstreams, nil +} + // getTokenMetaFromDescription parses JSON metadata from token's description. func getTokenMetaFromDescription(description string) (map[string]string, error) { re := regexp.MustCompile(`.*({.+})`) @@ -855,9 +911,9 @@ func processPreparedQueryUpstream(pod corev1.Pod, rawUpstream string) api.Upstre return upstream } -// processNonAnnotatedUpstream processes an upstream in the format: +// processUnlabeledUpstream processes an upstream in the format: // [service-name].[service-namespace].[service-partition]:[port]:[optional datacenter]. -func (r *EndpointsController) processNonAnnotatedUpstream(pod corev1.Pod, rawUpstream string) (api.Upstream, error) { +func (r *EndpointsController) processUnlabeledUpstream(pod corev1.Pod, rawUpstream string) (api.Upstream, error) { var datacenter, serviceName, namespace, partition, peer string var port int32 var upstream api.Upstream @@ -921,11 +977,11 @@ func (r *EndpointsController) processNonAnnotatedUpstream(pod corev1.Pod, rawUps } -// processAnnotatedUpstream processes an upstream in the format: +// processLabeledUpstream processes an upstream in the format: // [service-name].svc.[service-namespace].ns.[service-peer].peer:[port] // [service-name].svc.[service-namespace].ns.[service-partition].ap:[port] // [service-name].svc.[service-namespace].ns.[service-datacenter].dc:[port]. -func (r *EndpointsController) processAnnotatedUpstream(pod corev1.Pod, rawUpstream string) (api.Upstream, error) { +func (r *EndpointsController) processLabeledUpstream(pod corev1.Pod, rawUpstream string) (api.Upstream, error) { var datacenter, serviceName, namespace, partition, peer string var port int32 var upstream api.Upstream @@ -1004,64 +1060,6 @@ func (r *EndpointsController) processAnnotatedUpstream(pod corev1.Pod, rawUpstre } -// processUpstreams reads the list of upstreams from the Pod annotation and converts them into a list of api.Upstream -// objects. -func (r *EndpointsController) processUpstreams(pod corev1.Pod, endpoints corev1.Endpoints) ([]api.Upstream, error) { - // In a multiport pod, only the first service's proxy should have upstreams configured. This skips configuring - // upstreams on additional services on the pod. - mpIdx := getMultiPortIdx(pod, endpoints) - if mpIdx > 0 { - return []api.Upstream{}, nil - } - - var upstreams []api.Upstream - if raw, ok := pod.Annotations[annotationUpstreams]; ok && raw != "" { - for _, raw := range strings.Split(raw, ",") { - //var datacenter, serviceName, namespace, partition, peer string - //var port int32 - var upstream api.Upstream - - // parts separates out the port, and determines whether it's a prepared query or not, since parts[0] would - // be "prepared_query" if it is. - parts := strings.SplitN(raw, ":", 3) - - // serviceParts helps determine which format of upstream we're processing, - // [service-name].[service-namespace].[service-partition]:[port]:[optional datacenter] - // or - // [service-name].svc.[service-namespace].ns.[service-peer].peer:[port] - // [service-name].svc.[service-namespace].ns.[service-partition].ap:[port] - // [service-name].svc.[service-namespace].ns.[service-datacenter].dc:[port] - annotatedFormat := false - serviceParts := strings.Split(parts[0], ".") - if len(serviceParts) >= 2 { - if serviceParts[1] == "svc" { - annotatedFormat = true - } - } - - if strings.TrimSpace(parts[0]) == "prepared_query" { - upstream = processPreparedQueryUpstream(pod, raw) - } else if annotatedFormat { - var err error - upstream, err = r.processAnnotatedUpstream(pod, raw) - if err != nil { - return []api.Upstream{}, err - } - } else { - var err error - upstream, err = r.processNonAnnotatedUpstream(pod, raw) - if err != nil { - return []api.Upstream{}, err - } - } - - upstreams = append(upstreams, upstream) - } - } - - return upstreams, nil -} - // remoteConsulClient returns an *api.Client that points at the consul agent local to the pod for a provided namespace. func (r *EndpointsController) remoteConsulClient(ip string, namespace string) (*api.Client, error) { newAddr := fmt.Sprintf("%s://%s:%s", r.ConsulScheme, ip, r.ConsulPort) diff --git a/control-plane/connect-inject/endpoints_controller_test.go b/control-plane/connect-inject/endpoints_controller_test.go index c1ded649e8..741c7571ee 100644 --- a/control-plane/connect-inject/endpoints_controller_test.go +++ b/control-plane/connect-inject/endpoints_controller_test.go @@ -443,6 +443,39 @@ func TestProcessUpstreams(t *testing.T) { consulNamespacesEnabled: false, consulPartitionsEnabled: false, }, + { + name: "annotated upstream error: both peer and partition provided", + pod: func() *corev1.Pod { + pod1 := createPod("pod1", "1.2.3.4", true, true) + pod1.Annotations[annotationUpstreams] = "upstream1.svc.ns1.ns.part1.partition.peer1.peer:1234" + return pod1 + }, + expErr: "upstream structured incorrectly: upstream1.svc.ns1.ns.part1.partition.peer1.peer:1234", + consulNamespacesEnabled: true, + consulPartitionsEnabled: true, + }, + { + name: "annotated upstream error: both peer and dc provided", + pod: func() *corev1.Pod { + pod1 := createPod("pod1", "1.2.3.4", true, true) + pod1.Annotations[annotationUpstreams] = "upstream1.svc.ns1.ns.peer1.peer.dc1.dc:1234" + return pod1 + }, + expErr: "upstream structured incorrectly: upstream1.svc.ns1.ns.peer1.peer.dc1.dc:1234", + consulNamespacesEnabled: true, + consulPartitionsEnabled: false, + }, + { + name: "annotated upstream error: both dc and partition provided", + pod: func() *corev1.Pod { + pod1 := createPod("pod1", "1.2.3.4", true, true) + pod1.Annotations[annotationUpstreams] = "upstream1.svc.ns1.ns.part1.partition.dc1.dc:1234" + return pod1 + }, + expErr: "upstream structured incorrectly: upstream1.svc.ns1.ns.part1.partition.dc1.dc:1234", + consulNamespacesEnabled: true, + consulPartitionsEnabled: true, + }, { name: "upstream with datacenter with ProxyDefaults and mesh gateway is in local mode", pod: func() *corev1.Pod { @@ -5754,7 +5787,7 @@ func TestCreateServiceRegistrations_withTransparentProxy(t *testing.T) { pod.Spec.Containers = c.podContainers } - // We set these annotations explicitly as these are set by the handler and we + // We set these annotations explicitly as these are set by the meshWebhook and we // need these values to determine which port to use for the service registration. pod.Annotations[annotationPort] = "tcp" diff --git a/control-plane/connect-inject/envoy_sidecar.go b/control-plane/connect-inject/envoy_sidecar.go index 716b4d4ef5..074855f2ce 100644 --- a/control-plane/connect-inject/envoy_sidecar.go +++ b/control-plane/connect-inject/envoy_sidecar.go @@ -10,7 +10,7 @@ import ( "k8s.io/apimachinery/pkg/api/resource" ) -func (w *ConnectWebhook) envoySidecar(namespace corev1.Namespace, pod corev1.Pod, mpi multiPortInfo) (corev1.Container, error) { +func (w *MeshWebhook) envoySidecar(namespace corev1.Namespace, pod corev1.Pod, mpi multiPortInfo) (corev1.Container, error) { resources, err := w.envoySidecarResources(pod) if err != nil { return corev1.Container{}, err @@ -64,7 +64,7 @@ func (w *ConnectWebhook) envoySidecar(namespace corev1.Namespace, pod corev1.Pod return corev1.Container{}, fmt.Errorf("pod security context cannot have the same uid as envoy: %v", envoyUserAndGroupID) } } - // Ensure that none of the user's containers have the same UID as Envoy. At this point in injection the handler + // Ensure that none of the user's containers have the same UID as Envoy. At this point in injection the meshWebhook // has only injected init containers so all containers defined in pod.Spec.Containers are from the user. for _, c := range pod.Spec.Containers { // User container and Envoy container cannot have the same UID. @@ -82,7 +82,7 @@ func (w *ConnectWebhook) envoySidecar(namespace corev1.Namespace, pod corev1.Pod return container, nil } -func (w *ConnectWebhook) getContainerSidecarCommand(pod corev1.Pod, multiPortSvcName string, multiPortSvcIdx int) ([]string, error) { +func (w *MeshWebhook) getContainerSidecarCommand(pod corev1.Pod, multiPortSvcName string, multiPortSvcIdx int) ([]string, error) { bootstrapFile := "/consul/connect-inject/envoy-bootstrap.yaml" if multiPortSvcName != "" { bootstrapFile = fmt.Sprintf("/consul/connect-inject/envoy-bootstrap-%s.yaml", multiPortSvcName) @@ -122,7 +122,7 @@ func (w *ConnectWebhook) getContainerSidecarCommand(pod corev1.Pod, multiPortSvc return cmd, nil } -func (w *ConnectWebhook) envoySidecarResources(pod corev1.Pod) (corev1.ResourceRequirements, error) { +func (w *MeshWebhook) envoySidecarResources(pod corev1.Pod) (corev1.ResourceRequirements, error) { resources := corev1.ResourceRequirements{ Limits: corev1.ResourceList{}, Requests: corev1.ResourceList{}, diff --git a/control-plane/connect-inject/envoy_sidecar_test.go b/control-plane/connect-inject/envoy_sidecar_test.go index d631b0f481..e907ae6533 100644 --- a/control-plane/connect-inject/envoy_sidecar_test.go +++ b/control-plane/connect-inject/envoy_sidecar_test.go @@ -12,7 +12,7 @@ import ( func TestHandlerEnvoySidecar(t *testing.T) { require := require.New(t) - h := ConnectWebhook{} + w := MeshWebhook{} pod := corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ @@ -28,7 +28,7 @@ func TestHandlerEnvoySidecar(t *testing.T) { }, }, } - container, err := h.envoySidecar(testNS, pod, multiPortInfo{}) + container, err := w.envoySidecar(testNS, pod, multiPortInfo{}) require.NoError(err) require.Equal(container.Command, []string{ "envoy", @@ -45,7 +45,7 @@ func TestHandlerEnvoySidecar(t *testing.T) { func TestHandlerEnvoySidecar_Multiport(t *testing.T) { require := require.New(t) - h := ConnectWebhook{} + w := MeshWebhook{} pod := corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ @@ -79,7 +79,7 @@ func TestHandlerEnvoySidecar_Multiport(t *testing.T) { 1: {"envoy", "--config-path", "/consul/connect-inject/envoy-bootstrap-web-admin.yaml", "--base-id", "1"}, } for i := 0; i < 2; i++ { - container, err := h.envoySidecar(testNS, pod, multiPortInfos[i]) + container, err := w.envoySidecar(testNS, pod, multiPortInfos[i]) require.NoError(err) require.Equal(expCommand[i], container.Command) @@ -136,7 +136,7 @@ func TestHandlerEnvoySidecar_withSecurityContext(t *testing.T) { } for name, c := range cases { t.Run(name, func(t *testing.T) { - h := ConnectWebhook{ + w := MeshWebhook{ EnableTransparentProxy: c.tproxyEnabled, EnableOpenShift: c.openShiftEnabled, } @@ -155,7 +155,7 @@ func TestHandlerEnvoySidecar_withSecurityContext(t *testing.T) { }, }, } - ec, err := h.envoySidecar(testNS, pod, multiPortInfo{}) + ec, err := w.envoySidecar(testNS, pod, multiPortInfo{}) require.NoError(t, err) require.Equal(t, c.expSecurityContext, ec.SecurityContext) }) @@ -163,10 +163,10 @@ func TestHandlerEnvoySidecar_withSecurityContext(t *testing.T) { } // Test that if the user specifies a pod security context with the same uid as `envoyUserAndGroupID` that we return -// an error to the handler. +// an error to the meshWebhook. func TestHandlerEnvoySidecar_FailsWithDuplicatePodSecurityContextUID(t *testing.T) { require := require.New(t) - h := ConnectWebhook{} + w := MeshWebhook{} pod := corev1.Pod{ Spec: corev1.PodSpec{ Containers: []corev1.Container{ @@ -179,18 +179,18 @@ func TestHandlerEnvoySidecar_FailsWithDuplicatePodSecurityContextUID(t *testing. }, }, } - _, err := h.envoySidecar(testNS, pod, multiPortInfo{}) + _, err := w.envoySidecar(testNS, pod, multiPortInfo{}) require.Error(err, fmt.Sprintf("pod security context cannot have the same uid as envoy: %v", envoyUserAndGroupID)) } // Test that if the user specifies a container with security context with the same uid as `envoyUserAndGroupID` that we -// return an error to the handler. If a container using the envoy image has the same uid, we don't return an error +// return an error to the meshWebhook. If a container using the envoy image has the same uid, we don't return an error // because in multiport pod there can be multiple envoy sidecars. func TestHandlerEnvoySidecar_FailsWithDuplicateContainerSecurityContextUID(t *testing.T) { cases := []struct { name string pod corev1.Pod - handler ConnectWebhook + webhook MeshWebhook expErr bool expErrMessage error }{ @@ -217,7 +217,7 @@ func TestHandlerEnvoySidecar_FailsWithDuplicateContainerSecurityContextUID(t *te }, }, }, - handler: ConnectWebhook{}, + webhook: MeshWebhook{}, expErr: true, expErrMessage: fmt.Errorf("container app has runAsUser set to the same uid %q as envoy which is not allowed", envoyUserAndGroupID), }, @@ -244,7 +244,7 @@ func TestHandlerEnvoySidecar_FailsWithDuplicateContainerSecurityContextUID(t *te }, }, }, - handler: ConnectWebhook{ + webhook: MeshWebhook{ ImageEnvoy: "envoy", }, expErr: false, @@ -253,7 +253,7 @@ func TestHandlerEnvoySidecar_FailsWithDuplicateContainerSecurityContextUID(t *te for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - _, err := tc.handler.envoySidecar(testNS, tc.pod, multiPortInfo{}) + _, err := tc.webhook.envoySidecar(testNS, tc.pod, multiPortInfo{}) if tc.expErr { require.Error(t, err, tc.expErrMessage) } else { @@ -341,7 +341,7 @@ func TestHandlerEnvoySidecar_EnvoyExtraArgs(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - h := ConnectWebhook{ + h := MeshWebhook{ ImageConsul: "hashicorp/consul:latest", ImageEnvoy: "hashicorp/consul-k8s:latest", EnvoyExtraArgs: tc.envoyExtraArgs, @@ -362,13 +362,13 @@ func TestHandlerEnvoySidecar_Resources(t *testing.T) { zero := resource.MustParse("0") cases := map[string]struct { - handler ConnectWebhook + webhook MeshWebhook annotations map[string]string expResources corev1.ResourceRequirements expErr string }{ "no defaults, no annotations": { - handler: ConnectWebhook{}, + webhook: MeshWebhook{}, annotations: nil, expResources: corev1.ResourceRequirements{ Limits: corev1.ResourceList{}, @@ -376,7 +376,7 @@ func TestHandlerEnvoySidecar_Resources(t *testing.T) { }, }, "all defaults, no annotations": { - handler: ConnectWebhook{ + webhook: MeshWebhook{ DefaultProxyCPURequest: cpu1, DefaultProxyCPULimit: cpu2, DefaultProxyMemoryRequest: mem1, @@ -395,7 +395,7 @@ func TestHandlerEnvoySidecar_Resources(t *testing.T) { }, }, "no defaults, all annotations": { - handler: ConnectWebhook{}, + webhook: MeshWebhook{}, annotations: map[string]string{ annotationSidecarProxyCPURequest: "100m", annotationSidecarProxyMemoryRequest: "100Mi", @@ -414,7 +414,7 @@ func TestHandlerEnvoySidecar_Resources(t *testing.T) { }, }, "annotations override defaults": { - handler: ConnectWebhook{ + webhook: MeshWebhook{ DefaultProxyCPURequest: zero, DefaultProxyCPULimit: zero, DefaultProxyMemoryRequest: zero, @@ -438,7 +438,7 @@ func TestHandlerEnvoySidecar_Resources(t *testing.T) { }, }, "defaults set to zero, no annotations": { - handler: ConnectWebhook{ + webhook: MeshWebhook{ DefaultProxyCPURequest: zero, DefaultProxyCPULimit: zero, DefaultProxyMemoryRequest: zero, @@ -457,7 +457,7 @@ func TestHandlerEnvoySidecar_Resources(t *testing.T) { }, }, "annotations set to 0": { - handler: ConnectWebhook{}, + webhook: MeshWebhook{}, annotations: map[string]string{ annotationSidecarProxyCPURequest: "0", annotationSidecarProxyMemoryRequest: "0", @@ -476,28 +476,28 @@ func TestHandlerEnvoySidecar_Resources(t *testing.T) { }, }, "invalid cpu request": { - handler: ConnectWebhook{}, + webhook: MeshWebhook{}, annotations: map[string]string{ annotationSidecarProxyCPURequest: "invalid", }, expErr: "parsing annotation consul.hashicorp.com/sidecar-proxy-cpu-request:\"invalid\": quantities must match the regular expression", }, "invalid cpu limit": { - handler: ConnectWebhook{}, + webhook: MeshWebhook{}, annotations: map[string]string{ annotationSidecarProxyCPULimit: "invalid", }, expErr: "parsing annotation consul.hashicorp.com/sidecar-proxy-cpu-limit:\"invalid\": quantities must match the regular expression", }, "invalid memory request": { - handler: ConnectWebhook{}, + webhook: MeshWebhook{}, annotations: map[string]string{ annotationSidecarProxyMemoryRequest: "invalid", }, expErr: "parsing annotation consul.hashicorp.com/sidecar-proxy-memory-request:\"invalid\": quantities must match the regular expression", }, "invalid memory limit": { - handler: ConnectWebhook{}, + webhook: MeshWebhook{}, annotations: map[string]string{ annotationSidecarProxyMemoryLimit: "invalid", }, @@ -521,7 +521,7 @@ func TestHandlerEnvoySidecar_Resources(t *testing.T) { }, }, } - container, err := c.handler.envoySidecar(testNS, pod, multiPortInfo{}) + container, err := c.webhook.envoySidecar(testNS, pod, multiPortInfo{}) if c.expErr != "" { require.NotNil(err) require.Contains(err.Error(), c.expErr) diff --git a/control-plane/connect-inject/connect_webhook.go b/control-plane/connect-inject/mesh_webhook.go similarity index 95% rename from control-plane/connect-inject/connect_webhook.go rename to control-plane/connect-inject/mesh_webhook.go index f2653c2bcc..21c710d5b5 100644 --- a/control-plane/connect-inject/connect_webhook.go +++ b/control-plane/connect-inject/mesh_webhook.go @@ -31,8 +31,8 @@ var ( kubeSystemNamespaces = mapset.NewSetWith(metav1.NamespaceSystem, metav1.NamespacePublic) ) -// Handler is the HTTP handler for admission webhooks. -type ConnectWebhook struct { +// Webhook is the HTTP meshWebhook for admission webhooks. +type MeshWebhook struct { ConsulClient *api.Client Clientset kubernetes.Interface @@ -116,7 +116,7 @@ type ConnectWebhook struct { DefaultProxyMemoryLimit resource.Quantity // MetricsConfig contains metrics configuration from the inject-connect command and has methods to determine whether - // configuration should come from the default flags or annotations. The handler uses this to configure prometheus + // configuration should come from the default flags or annotations. The meshWebhook uses this to configure prometheus // annotations and the merged metrics server. MetricsConfig MetricsConfig @@ -167,10 +167,10 @@ type multiPortInfo struct { serviceName string } -// Handle is the admission.Handler implementation that actually handles the +// Handle is the admission.Webhook implementation that actually handles the // webhook request for admission control. This should be registered or // served via the controller runtime manager. -func (w *ConnectWebhook) Handle(ctx context.Context, req admission.Request) admission.Response { +func (w *MeshWebhook) Handle(ctx context.Context, req admission.Request) admission.Response { var pod corev1.Pod // Decode the pod from the request @@ -383,7 +383,7 @@ func (w *ConnectWebhook) Handle(ctx context.Context, req admission.Request) admi return admission.Errored(http.StatusBadRequest, err) } - // Create a patches based on the Pod that was received by the handler + // Create a patches based on the Pod that was received by the meshWebhook // and the desired Pod spec. patches, err := jsonpatch.CreatePatch(origPodJson, updatedPodJson) if err != nil { @@ -402,7 +402,7 @@ func (w *ConnectWebhook) Handle(ctx context.Context, req admission.Request) admi } // Return a Patched response along with the patches we intend on applying to the - // Pod received by the handler. + // Pod received by the meshWebhook. return admission.Patched(fmt.Sprintf("valid %s request", pod.Kind), patches...) } @@ -418,7 +418,7 @@ func shouldOverwriteProbes(pod corev1.Pod, globalOverwrite bool) (bool, error) { // overwriteProbes overwrites readiness/liveness probes of this pod when // both transparent proxy is enabled and overwrite probes is true for the pod. -func (w *ConnectWebhook) overwriteProbes(ns corev1.Namespace, pod *corev1.Pod) error { +func (w *MeshWebhook) overwriteProbes(ns corev1.Namespace, pod *corev1.Pod) error { tproxyEnabled, err := transparentProxyEnabled(ns, *pod, w.EnableTransparentProxy) if err != nil { return err @@ -449,7 +449,7 @@ func (w *ConnectWebhook) overwriteProbes(ns corev1.Namespace, pod *corev1.Pod) e return nil } -func (w *ConnectWebhook) injectVolumeMount(pod corev1.Pod) { +func (w *MeshWebhook) injectVolumeMount(pod corev1.Pod) { containersToInject := splitCommaSeparatedItemsFromAnnotation(annotationInjectMountVolumes, pod) for index, container := range pod.Spec.Containers { @@ -462,7 +462,7 @@ func (w *ConnectWebhook) injectVolumeMount(pod corev1.Pod) { } } -func (w *ConnectWebhook) shouldInject(pod corev1.Pod, namespace string) (bool, error) { +func (w *MeshWebhook) shouldInject(pod corev1.Pod, namespace string) (bool, error) { // Don't inject in the Kubernetes system namespaces if kubeSystemNamespaces.Contains(namespace) { return false, nil @@ -494,7 +494,7 @@ func (w *ConnectWebhook) shouldInject(pod corev1.Pod, namespace string) (bool, e return !w.RequireAnnotation, nil } -func (w *ConnectWebhook) defaultAnnotations(pod *corev1.Pod, podJson string) error { +func (w *MeshWebhook) defaultAnnotations(pod *corev1.Pod, podJson string) error { if pod.Annotations == nil { pod.Annotations = make(map[string]string) } @@ -518,7 +518,7 @@ func (w *ConnectWebhook) defaultAnnotations(pod *corev1.Pod, podJson string) err // prometheusAnnotations sets the Prometheus scraping configuration // annotations on the Pod. -func (w *ConnectWebhook) prometheusAnnotations(pod *corev1.Pod) error { +func (w *MeshWebhook) prometheusAnnotations(pod *corev1.Pod) error { enableMetrics, err := w.MetricsConfig.enableMetrics(*pod) if err != nil { return err @@ -540,11 +540,11 @@ func (w *ConnectWebhook) prometheusAnnotations(pod *corev1.Pod) error { // consulNamespace returns the namespace that a service should be // registered in based on the namespace options. It returns an // empty string if namespaces aren't enabled. -func (w *ConnectWebhook) consulNamespace(ns string) string { +func (w *MeshWebhook) consulNamespace(ns string) string { return namespaces.ConsulNamespace(ns, w.EnableNamespaces, w.ConsulDestinationNamespace, w.EnableK8SNSMirroring, w.K8SNSMirroringPrefix) } -func (w *ConnectWebhook) validatePod(pod corev1.Pod) error { +func (w *MeshWebhook) validatePod(pod corev1.Pod) error { if _, ok := pod.Annotations[annotationProtocol]; ok { return fmt.Errorf("the %q annotation is no longer supported. Instead, create a ServiceDefaults resource (see www.consul.io/docs/k8s/crds/upgrade-to-crds)", annotationProtocol) @@ -609,7 +609,7 @@ func findServiceAccountVolumeMount(pod corev1.Pod, multiPort bool, multiPortSvcN return volumeMount, "/var/run/secrets/kubernetes.io/serviceaccount/token", nil } -func (w *ConnectWebhook) annotatedServiceNames(pod corev1.Pod) []string { +func (w *MeshWebhook) annotatedServiceNames(pod corev1.Pod) []string { var annotatedSvcNames []string if anno, ok := pod.Annotations[annotationService]; ok { annotatedSvcNames = strings.Split(anno, ",") @@ -617,7 +617,7 @@ func (w *ConnectWebhook) annotatedServiceNames(pod corev1.Pod) []string { return annotatedSvcNames } -func (w *ConnectWebhook) checkUnsupportedMultiPortCases(ns corev1.Namespace, pod corev1.Pod) error { +func (w *MeshWebhook) checkUnsupportedMultiPortCases(ns corev1.Namespace, pod corev1.Pod) error { tproxyEnabled, err := transparentProxyEnabled(ns, pod, w.EnableTransparentProxy) if err != nil { return fmt.Errorf("couldn't check if tproxy is enabled: %s", err) @@ -642,7 +642,7 @@ func (w *ConnectWebhook) checkUnsupportedMultiPortCases(ns corev1.Namespace, pod return nil } -func (w *ConnectWebhook) InjectDecoder(d *admission.Decoder) error { +func (w *MeshWebhook) InjectDecoder(d *admission.Decoder) error { w.decoder = d return nil } diff --git a/control-plane/connect-inject/connect_webhook_ent_test.go b/control-plane/connect-inject/mesh_webhook_ent_test.go similarity index 95% rename from control-plane/connect-inject/connect_webhook_ent_test.go rename to control-plane/connect-inject/mesh_webhook_ent_test.go index 24e5ce7d11..5faaf4e0c1 100644 --- a/control-plane/connect-inject/connect_webhook_ent_test.go +++ b/control-plane/connect-inject/mesh_webhook_ent_test.go @@ -22,7 +22,7 @@ import ( ) // This tests the checkAndCreate namespace function that is called -// in handler.Mutate. Patch generation is tested in the non-enterprise +// in meshWebhook.Mutate. Patch generation is tested in the non-enterprise // tests. Other namespace-specific logic is tested directly in the // specific methods (shouldInject, consulNamespace). func TestHandler_MutateWithNamespaces(t *testing.T) { @@ -42,13 +42,13 @@ func TestHandler_MutateWithNamespaces(t *testing.T) { cases := []struct { Name string - Handler ConnectWebhook + Webhook MeshWebhook Req admission.Request ExpectedNamespaces []string }{ { Name: "single destination namespace 'default' from k8s 'default'", - Handler: ConnectWebhook{ + Webhook: MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSet("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -70,7 +70,7 @@ func TestHandler_MutateWithNamespaces(t *testing.T) { { Name: "single destination namespace 'default' from k8s 'non-default'", - Handler: ConnectWebhook{ + Webhook: MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSet("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -92,7 +92,7 @@ func TestHandler_MutateWithNamespaces(t *testing.T) { { Name: "single destination namespace 'dest' from k8s 'default'", - Handler: ConnectWebhook{ + Webhook: MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSet("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -114,7 +114,7 @@ func TestHandler_MutateWithNamespaces(t *testing.T) { { Name: "single destination namespace 'dest' from k8s 'non-default'", - Handler: ConnectWebhook{ + Webhook: MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSet("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -136,7 +136,7 @@ func TestHandler_MutateWithNamespaces(t *testing.T) { { Name: "mirroring from k8s 'default'", - Handler: ConnectWebhook{ + Webhook: MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSet("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -159,7 +159,7 @@ func TestHandler_MutateWithNamespaces(t *testing.T) { { Name: "mirroring from k8s 'dest'", - Handler: ConnectWebhook{ + Webhook: MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSet("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -182,7 +182,7 @@ func TestHandler_MutateWithNamespaces(t *testing.T) { { Name: "mirroring with prefix from k8s 'default'", - Handler: ConnectWebhook{ + Webhook: MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSet("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -206,7 +206,7 @@ func TestHandler_MutateWithNamespaces(t *testing.T) { { Name: "mirroring with prefix from k8s 'dest'", - Handler: ConnectWebhook{ + Webhook: MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSet("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -245,11 +245,11 @@ func TestHandler_MutateWithNamespaces(t *testing.T) { }) require.NoError(err) - // Add the client to the test's handler - tt.Handler.ConsulClient = client + // Add the client to the test's meshWebhook + tt.Webhook.ConsulClient = client // Mutate! - resp := tt.Handler.Handle(context.Background(), tt.Req) + resp := tt.Webhook.Handle(context.Background(), tt.Req) require.Equal(resp.Allowed, true) // Check all the namespace things @@ -298,13 +298,13 @@ func TestHandler_MutateWithNamespaces_ACLs(t *testing.T) { cases := []struct { Name string - Handler ConnectWebhook + Webhook MeshWebhook Req admission.Request ExpectedNamespaces []string }{ { Name: "acls + single destination namespace 'default' from k8s 'default'", - Handler: ConnectWebhook{ + Webhook: MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSet("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -327,7 +327,7 @@ func TestHandler_MutateWithNamespaces_ACLs(t *testing.T) { { Name: "acls + single destination namespace 'default' from k8s 'non-default'", - Handler: ConnectWebhook{ + Webhook: MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSet("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -350,7 +350,7 @@ func TestHandler_MutateWithNamespaces_ACLs(t *testing.T) { { Name: "acls + single destination namespace 'dest' from k8s 'default'", - Handler: ConnectWebhook{ + Webhook: MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSet("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -373,7 +373,7 @@ func TestHandler_MutateWithNamespaces_ACLs(t *testing.T) { { Name: "acls + single destination namespace 'dest' from k8s 'non-default'", - Handler: ConnectWebhook{ + Webhook: MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSet("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -396,7 +396,7 @@ func TestHandler_MutateWithNamespaces_ACLs(t *testing.T) { { Name: "acls + mirroring from k8s 'default'", - Handler: ConnectWebhook{ + Webhook: MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSet("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -420,7 +420,7 @@ func TestHandler_MutateWithNamespaces_ACLs(t *testing.T) { { Name: "acls + mirroring from k8s 'dest'", - Handler: ConnectWebhook{ + Webhook: MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSet("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -444,7 +444,7 @@ func TestHandler_MutateWithNamespaces_ACLs(t *testing.T) { { Name: "acls + mirroring with prefix from k8s 'default'", - Handler: ConnectWebhook{ + Webhook: MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSet("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -469,7 +469,7 @@ func TestHandler_MutateWithNamespaces_ACLs(t *testing.T) { { Name: "acls + mirroring with prefix from k8s 'dest'", - Handler: ConnectWebhook{ + Webhook: MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSet("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -525,8 +525,8 @@ func TestHandler_MutateWithNamespaces_ACLs(t *testing.T) { }) require.NoError(t, err) - // Add the client to the test's handler - tt.Handler.ConsulClient = client + // Add the client to the test's meshWebhook + tt.Webhook.ConsulClient = client // Create cross namespace policy // This would have been created by the acl bootstrapper in the @@ -550,7 +550,7 @@ func TestHandler_MutateWithNamespaces_ACLs(t *testing.T) { require.NoError(t, err) // Mutate! - resp := tt.Handler.Handle(context.Background(), tt.Req) + resp := tt.Webhook.Handle(context.Background(), tt.Req) require.Equal(t, resp.Allowed, true) // Check all the namespace things @@ -640,7 +640,7 @@ func TestHandler_MutateWithNamespaces_Annotation(t *testing.T) { }) require.NoError(err) - handler := ConnectWebhook{ + webhook := MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSet("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -671,7 +671,7 @@ func TestHandler_MutateWithNamespaces_Annotation(t *testing.T) { Namespace: sourceKubeNS, }, } - resp := handler.Handle(context.Background(), request) + resp := webhook.Handle(context.Background(), request) require.Equal(resp.Allowed, true) // Check that the annotation was added as a patch. diff --git a/control-plane/connect-inject/connect_webhook_test.go b/control-plane/connect-inject/mesh_webhook_test.go similarity index 97% rename from control-plane/connect-inject/connect_webhook_test.go rename to control-plane/connect-inject/mesh_webhook_test.go index 92bae3d872..bbebd48c30 100644 --- a/control-plane/connect-inject/connect_webhook_test.go +++ b/control-plane/connect-inject/mesh_webhook_test.go @@ -41,14 +41,14 @@ func TestHandlerHandle(t *testing.T) { cases := []struct { Name string - Handler ConnectWebhook + Webhook MeshWebhook Req admission.Request Err string // expected error string, not exact Patches []jsonpatch.Operation }{ { "kube-system namespace", - ConnectWebhook{ + MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSetWith("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -68,7 +68,7 @@ func TestHandlerHandle(t *testing.T) { { "already injected", - ConnectWebhook{ + MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSetWith("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -92,7 +92,7 @@ func TestHandlerHandle(t *testing.T) { { "empty pod basic", - ConnectWebhook{ + MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSetWith("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -134,7 +134,7 @@ func TestHandlerHandle(t *testing.T) { { "pod with upstreams specified", - ConnectWebhook{ + MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSetWith("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -189,7 +189,7 @@ func TestHandlerHandle(t *testing.T) { { "empty pod with injection disabled", - ConnectWebhook{ + MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSetWith("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -215,7 +215,7 @@ func TestHandlerHandle(t *testing.T) { { "empty pod with injection truthy", - ConnectWebhook{ + MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSetWith("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -266,7 +266,7 @@ func TestHandlerHandle(t *testing.T) { { "pod with empty volume mount annotation", - ConnectWebhook{ + MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSetWith("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -316,7 +316,7 @@ func TestHandlerHandle(t *testing.T) { }, { "pod with volume mount annotation", - ConnectWebhook{ + MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSetWith("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -387,7 +387,7 @@ func TestHandlerHandle(t *testing.T) { { "pod with service annotation", - ConnectWebhook{ + MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSetWith("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -438,7 +438,7 @@ func TestHandlerHandle(t *testing.T) { { "pod with existing label", - ConnectWebhook{ + MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSetWith("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -489,7 +489,7 @@ func TestHandlerHandle(t *testing.T) { { "when metrics merging is enabled, we should inject the consul-sidecar and add prometheus annotations", - ConnectWebhook{ + MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSetWith("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -567,7 +567,7 @@ func TestHandlerHandle(t *testing.T) { { "tproxy with overwriteProbes is enabled", - ConnectWebhook{ + MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSetWith("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -648,7 +648,7 @@ func TestHandlerHandle(t *testing.T) { }, { "multi port pod", - ConnectWebhook{ + MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSetWith("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -706,7 +706,7 @@ func TestHandlerHandle(t *testing.T) { t.Run(tt.Name, func(t *testing.T) { require := require.New(t) ctx := context.Background() - resp := tt.Handler.Handle(ctx, tt.Req) + resp := tt.Webhook.Handle(ctx, tt.Req) if (tt.Err == "") != resp.Allowed { t.Fatalf("allowed: %v, expected err: %v", resp.Allowed, tt.Err) } @@ -760,7 +760,7 @@ func TestHandler_ErrorsOnDeprecatedAnnotations(t *testing.T) { decoder, err := admission.NewDecoder(s) require.NoError(err) - handler := ConnectWebhook{ + webhook := MeshWebhook{ Log: logrtest.TestLogger{T: t}, AllowK8sNamespacesSet: mapset.NewSetWith("*"), DenyK8sNamespacesSet: mapset.NewSet(), @@ -785,7 +785,7 @@ func TestHandler_ErrorsOnDeprecatedAnnotations(t *testing.T) { }, } - response := handler.Handle(context.Background(), request) + response := webhook.Handle(context.Background(), request) require.False(response.Allowed) require.Equal(c.expErr, response.Result.Message) }) @@ -917,8 +917,8 @@ func TestHandlerDefaultAnnotations(t *testing.T) { podJson, err := json.Marshal(tt.Pod) require.NoError(err) - var h ConnectWebhook - err = h.defaultAnnotations(tt.Pod, string(podJson)) + var w MeshWebhook + err = w.defaultAnnotations(tt.Pod, string(podJson)) if (tt.Err != "") != (err != nil) { t.Fatalf("actual: %v, expected err: %v", err, tt.Err) } @@ -939,12 +939,12 @@ func TestHandlerDefaultAnnotations(t *testing.T) { func TestHandlerPrometheusAnnotations(t *testing.T) { cases := []struct { Name string - Handler ConnectWebhook + Webhook MeshWebhook Expected map[string]string }{ { Name: "Sets the correct prometheus annotations on the pod if metrics are enabled", - Handler: ConnectWebhook{ + Webhook: MeshWebhook{ MetricsConfig: MetricsConfig{ DefaultEnableMetrics: true, DefaultPrometheusScrapePort: "20200", @@ -959,7 +959,7 @@ func TestHandlerPrometheusAnnotations(t *testing.T) { }, { Name: "Does not set annotations if metrics are not enabled", - Handler: ConnectWebhook{ + Webhook: MeshWebhook{ MetricsConfig: MetricsConfig{ DefaultEnableMetrics: false, DefaultPrometheusScrapePort: "20200", @@ -973,7 +973,7 @@ func TestHandlerPrometheusAnnotations(t *testing.T) { for _, tt := range cases { t.Run(tt.Name, func(t *testing.T) { require := require.New(t) - h := tt.Handler + h := tt.Webhook pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{}}} err := h.prometheusAnnotations(pod) @@ -1157,14 +1157,14 @@ func TestConsulNamespace(t *testing.T) { t.Run(tt.Name, func(t *testing.T) { require := require.New(t) - h := ConnectWebhook{ + w := MeshWebhook{ EnableNamespaces: tt.EnableNamespaces, ConsulDestinationNamespace: tt.ConsulDestinationNamespace, EnableK8SNSMirroring: tt.EnableK8SNSMirroring, K8SNSMirroringPrefix: tt.K8SNSMirroringPrefix, } - ns := h.consulNamespace(tt.K8sNamespace) + ns := w.consulNamespace(tt.K8sNamespace) require.Equal(tt.Expected, ns) }) @@ -1459,14 +1459,14 @@ func TestShouldInject(t *testing.T) { t.Run(tt.Name, func(t *testing.T) { require := require.New(t) - h := ConnectWebhook{ + w := MeshWebhook{ RequireAnnotation: false, EnableNamespaces: tt.EnableNamespaces, AllowK8sNamespacesSet: tt.AllowK8sNamespacesSet, DenyK8sNamespacesSet: tt.DenyK8sNamespacesSet, } - injected, err := h.shouldInject(*tt.Pod, tt.K8sNamespace) + injected, err := w.shouldInject(*tt.Pod, tt.K8sNamespace) require.Equal(nil, err) require.Equal(tt.Expected, injected) @@ -1765,11 +1765,11 @@ func TestOverwriteProbes(t *testing.T) { pod.ObjectMeta.Annotations = c.additionalAnnotations } - h := ConnectWebhook{ + w := MeshWebhook{ EnableTransparentProxy: c.tproxyEnabled, TProxyOverwriteProbes: c.overwriteProbes, } - err := h.overwriteProbes(corev1.Namespace{}, pod) + err := w.overwriteProbes(corev1.Namespace{}, pod) require.NoError(t, err) for i, container := range pod.Spec.Containers { if container.ReadinessProbe != nil { @@ -1811,10 +1811,10 @@ func TestHandler_checkUnsupportedMultiPortCases(t *testing.T) { } for _, tt := range cases { t.Run(tt.name, func(t *testing.T) { - h := ConnectWebhook{} + w := MeshWebhook{} pod := minimal() pod.Annotations = tt.annotations - err := h.checkUnsupportedMultiPortCases(corev1.Namespace{}, *pod) + err := w.checkUnsupportedMultiPortCases(corev1.Namespace{}, *pod) require.Error(t, err) require.Equal(t, tt.expErr, err.Error()) }) diff --git a/control-plane/connect-inject/metrics_configuration.go b/control-plane/connect-inject/metrics_configuration.go index cdfc5d0b6e..fc8c5d574a 100644 --- a/control-plane/connect-inject/metrics_configuration.go +++ b/control-plane/connect-inject/metrics_configuration.go @@ -35,7 +35,7 @@ func (mc MetricsConfig) mergedMetricsServerConfiguration(pod corev1.Pod) (metric return metricsPorts{}, err } - // This should never happen because we only call this function in the handler if + // This should never happen because we only call this function in the meshWebhook if // we need to run the metrics merging server. This check is here just in case. if !run { return metricsPorts{}, errors.New("metrics merging should be enabled in order to return the metrics server configuration") @@ -61,7 +61,7 @@ func (mc MetricsConfig) mergedMetricsServerConfiguration(pod corev1.Pod) (metric return metricsPorts, nil } -// enableMetrics returns whether metrics are enabled either via the default value in the handler, or if it's been +// enableMetrics returns whether metrics are enabled either via the default value in the meshWebhook, or if it's been // overridden via the annotation. func (mc MetricsConfig) enableMetrics(pod corev1.Pod) (bool, error) { enabled := mc.DefaultEnableMetrics @@ -76,7 +76,7 @@ func (mc MetricsConfig) enableMetrics(pod corev1.Pod) (bool, error) { } // enableMetricsMerging returns whether metrics merging functionality is enabled either via the default value in the -// handler, or if it's been overridden via the annotation. +// meshWebhook, or if it's been overridden via the annotation. func (mc MetricsConfig) enableMetricsMerging(pod corev1.Pod) (bool, error) { enabled := mc.DefaultEnableMetricsMerging if raw, ok := pod.Annotations[annotationEnableMetricsMerging]; ok && raw != "" { @@ -89,19 +89,19 @@ func (mc MetricsConfig) enableMetricsMerging(pod corev1.Pod) (bool, error) { return enabled, nil } -// mergedMetricsPort returns the port to run the merged metrics server on, either via the default value in the handler, +// mergedMetricsPort returns the port to run the merged metrics server on, either via the default value in the meshWebhook, // or if it's been overridden via the annotation. It also validates the port is in the unprivileged port range. func (mc MetricsConfig) mergedMetricsPort(pod corev1.Pod) (string, error) { return determineAndValidatePort(pod, annotationMergedMetricsPort, mc.DefaultMergedMetricsPort, false) } -// prometheusScrapePort returns the port for Prometheus to scrape from, either via the default value in the handler, or +// prometheusScrapePort returns the port for Prometheus to scrape from, either via the default value in the meshWebhook, or // if it's been overridden via the annotation. It also validates the port is in the unprivileged port range. func (mc MetricsConfig) prometheusScrapePort(pod corev1.Pod) (string, error) { return determineAndValidatePort(pod, annotationPrometheusScrapePort, mc.DefaultPrometheusScrapePort, false) } -// prometheusScrapePath returns the path for Prometheus to scrape from, either via the default value in the handler, or +// prometheusScrapePath returns the path for Prometheus to scrape from, either via the default value in the meshWebhook, or // if it's been overridden via the annotation. func (mc MetricsConfig) prometheusScrapePath(pod corev1.Pod) string { if raw, ok := pod.Annotations[annotationPrometheusScrapePath]; ok && raw != "" { diff --git a/control-plane/connect-inject/metrics_configuration_test.go b/control-plane/connect-inject/metrics_configuration_test.go index 9fcbccc336..9564b2190c 100644 --- a/control-plane/connect-inject/metrics_configuration_test.go +++ b/control-plane/connect-inject/metrics_configuration_test.go @@ -18,7 +18,7 @@ func TestMetricsConfigEnableMetrics(t *testing.T) { Err string }{ { - Name: "Metrics enabled via handler", + Name: "Metrics enabled via meshWebhook", Pod: func(pod *corev1.Pod) *corev1.Pod { return pod }, @@ -80,7 +80,7 @@ func TestMetricsConfigEnableMetricsMerging(t *testing.T) { Err string }{ { - Name: "Metrics merging enabled via handler", + Name: "Metrics merging enabled via meshWebhook", Pod: func(pod *corev1.Pod) *corev1.Pod { return pod }, @@ -221,7 +221,7 @@ func TestMetricsConfigPrometheusScrapePath(t *testing.T) { Expected string }{ { - Name: "Defaults to the handler's value", + Name: "Defaults to the meshWebhook's value", Pod: func(pod *corev1.Pod) *corev1.Pod { return pod }, diff --git a/control-plane/connect-inject/peering_acceptor_controller.go b/control-plane/connect-inject/peering_acceptor_controller.go index 522f8b9ade..ca14b2f89c 100644 --- a/control-plane/connect-inject/peering_acceptor_controller.go +++ b/control-plane/connect-inject/peering_acceptor_controller.go @@ -74,16 +74,15 @@ func (r *PeeringAcceptorController) Reconcile(ctx context.Context, req ctrl.Requ } else { if containsString(acceptor.Finalizers, FinalizerName) { r.Log.Info("PeeringAcceptor was deleted, deleting from Consul", "name", req.Name, "ns", req.Namespace) - if err := r.deletePeering(ctx, req.Name); err != nil { - return ctrl.Result{}, err - } + err := r.deletePeering(ctx, req.Name) if acceptor.Secret().Backend == "kubernetes" { - if err := r.deleteK8sSecret(ctx, acceptor); err != nil { - return ctrl.Result{}, err - } + err = r.deleteK8sSecret(ctx, acceptor) + } + if err != nil { + return ctrl.Result{}, err } controllerutil.RemoveFinalizer(acceptor, FinalizerName) - err := r.Update(ctx, acceptor) + err = r.Update(ctx, acceptor) return ctrl.Result{}, err } } @@ -112,10 +111,11 @@ func (r *PeeringAcceptorController) Reconcile(ctx context.Context, req ctrl.Requ // If the peering doesn't exist in Consul, generate a new token, and store it in the specified backend. Store the // current state in the status. if peering == nil { - r.Log.Info("peering doesn't exist in Consul", "name", acceptor.Name) + r.Log.Info("peering doesn't exist in Consul; creating new peering", "name", acceptor.Name) if statusSecretSet { if existingStatusSecret != nil { + r.Log.Info("stale secret in status; deleting stale secret", "name", acceptor.Name) err := r.Client.Delete(ctx, existingStatusSecret) if err != nil { r.updateStatusError(ctx, acceptor, err) @@ -130,7 +130,7 @@ func (r *PeeringAcceptorController) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, err } if acceptor.Secret().Backend == "kubernetes" { - secretResourceVersion, err = r.createK8sSecret(ctx, acceptor, resp) + secretResourceVersion, err = r.createOrUpdateK8sSecret(ctx, acceptor, resp) if err != nil { r.updateStatusError(ctx, acceptor, err) return ctrl.Result{}, err @@ -147,7 +147,7 @@ func (r *PeeringAcceptorController) Reconcile(ctx context.Context, req ctrl.Requ // TODO(peering): Verify that the existing peering in Consul is an acceptor peer. If it is a dialing peer, an error should be thrown. // If the peering does exist in Consul, figure out whether to generate and store a new token by comparing the secret - // in the status to the actual contents of the secret. If no secret is specified in the status, shouldGenerate will + // in the status to the resource version of the secret. If no secret is specified in the status, shouldGenerate will // be set to true. var shouldGenerate bool var nameChanged bool @@ -168,7 +168,7 @@ func (r *PeeringAcceptorController) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, err } if acceptor.Secret().Backend == "kubernetes" { - secretResourceVersion, err = r.createK8sSecret(ctx, acceptor, resp) + secretResourceVersion, err = r.createOrUpdateK8sSecret(ctx, acceptor, resp) if err != nil { return ctrl.Result{}, err } @@ -193,8 +193,7 @@ func (r *PeeringAcceptorController) Reconcile(ctx context.Context, req ctrl.Requ } // shouldGenerateToken returns whether a token should be generated, and whether the name of the secret has changed. It -// compares the spec secret's name/key/backend and contents to the status secret's name/key/backend and contents. The -// contents are compared by taking a SHA256 sum of the secret. +// compares the spec secret's name/key/backend and resource version with the name/key/backend and resource version of the status secret's. func shouldGenerateToken(acceptor *consulv1alpha1.PeeringAcceptor, existingStatusSecret *corev1.Secret) (shouldGenerate bool, nameChanged bool, err error) { if acceptor.SecretRef() == nil { return false, false, errors.New("shouldGenerateToken was called with an empty fields in the existing status") @@ -210,8 +209,8 @@ func shouldGenerateToken(acceptor *consulv1alpha1.PeeringAcceptor, existingStatu if acceptor.SecretRef().Backend != acceptor.Secret().Backend { return false, false, errors.New("PeeringAcceptor backend cannot be changed") } - // Compare the existing secret hash. - // Get the secret specified by the status, make sure it matches the status' secret.latestHash. + // Compare the existing secret resource version. + // Get the secret specified by the status, make sure it matches the status' secret.ResourceVersion. if existingStatusSecret != nil { if existingStatusSecret.ResourceVersion != acceptor.SecretRef().ResourceVersion { return true, false, nil @@ -270,9 +269,9 @@ func (r *PeeringAcceptorController) getExistingSecret(ctx context.Context, name return existingSecret, nil } -// createK8sSecret creates a secret and uses the controller's K8s client to apply the secret. It checks if +// createOrUpdateK8sSecret creates a secret and uses the controller's K8s client to apply the secret. It checks if // there's an existing secret with the same name and makes sure to update the existing secret if so. -func (r *PeeringAcceptorController) createK8sSecret(ctx context.Context, acceptor *consulv1alpha1.PeeringAcceptor, resp *api.PeeringGenerateTokenResponse) (string, error) { +func (r *PeeringAcceptorController) createOrUpdateK8sSecret(ctx context.Context, acceptor *consulv1alpha1.PeeringAcceptor, resp *api.PeeringGenerateTokenResponse) (string, error) { secretName := acceptor.Secret().Name secretNamespace := acceptor.Namespace secret := createSecret(secretName, secretNamespace, acceptor.Secret().Key, resp.PeeringToken) @@ -345,16 +344,13 @@ func (r *PeeringAcceptorController) deletePeering(ctx context.Context, peerName return nil } -// createSecret is a helper function that creates a corev1.SecretRef when provided inputs. +// createSecret is a helper function that creates a corev1.Secret when provided inputs. func createSecret(name, namespace, key, value string) *corev1.Secret { secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: name, Namespace: namespace, }, - StringData: map[string]string{ - key: value, - }, Data: map[string][]byte{ key: []byte(value), }, diff --git a/control-plane/connect-inject/peering_acceptor_controller_test.go b/control-plane/connect-inject/peering_acceptor_controller_test.go index b872d1cfd7..65ccfba0ad 100644 --- a/control-plane/connect-inject/peering_acceptor_controller_test.go +++ b/control-plane/connect-inject/peering_acceptor_controller_test.go @@ -11,6 +11,7 @@ import ( "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/sdk/testutil" + "github.com/hashicorp/consul/sdk/testutil/retry" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" @@ -402,7 +403,7 @@ func TestReconcileCreateUpdatePeeringAcceptor(t *testing.T) { // exists". Leaving this here documents that the entire contents of an existing secret should // be replaced. require.Equal(t, "", createdSecret.StringData["some-old-key"]) - decodedTokenData, err := base64.StdEncoding.DecodeString(createdSecret.StringData["data"]) + decodedTokenData, err := base64.StdEncoding.DecodeString(string(createdSecret.Data["data"])) require.NoError(t, err) require.Contains(t, string(decodedTokenData), "\"CA\":null") @@ -437,101 +438,83 @@ func TestReconcileCreateUpdatePeeringAcceptor(t *testing.T) { // TestReconcileDeletePeeringAcceptor reconciles a PeeringAcceptor resource that is no longer in Kubernetes, but still // exists in Consul. func TestReconcileDeletePeeringAcceptor(t *testing.T) { - t.Parallel() - nodeName := "test-node" - cases := []struct { - name string - initialConsulPeerName string - expErr string - }{ - { - name: "PeeringAcceptor ", - initialConsulPeerName: "acceptor-deleted", + // Add the default namespace. + ns := corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "default"}} + acceptor := &v1alpha1.PeeringAcceptor{ + ObjectMeta: metav1.ObjectMeta{ + Name: "acceptor-deleted", + Namespace: "default", + DeletionTimestamp: &metav1.Time{Time: time.Now()}, + Finalizers: []string{FinalizerName}, }, - } - for _, tt := range cases { - t.Run(tt.name, func(t *testing.T) { - // Add the default namespace. - ns := corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "default"}} - - acceptor := &v1alpha1.PeeringAcceptor{ - ObjectMeta: metav1.ObjectMeta{ - Name: "acceptor-deleted", - Namespace: "default", - DeletionTimestamp: &metav1.Time{Time: time.Now()}, - Finalizers: []string{FinalizerName}, - }, - Spec: v1alpha1.PeeringAcceptorSpec{ - Peer: &v1alpha1.Peer{ - Secret: &v1alpha1.Secret{ - Name: "acceptor-deleted-secret", - Key: "data", - Backend: "kubernetes", - }, - }, + Spec: v1alpha1.PeeringAcceptorSpec{ + Peer: &v1alpha1.Peer{ + Secret: &v1alpha1.Secret{ + Name: "acceptor-deleted-secret", + Key: "data", + Backend: "kubernetes", }, - } - k8sObjects := []runtime.Object{&ns, acceptor} - - // Add peering types to the scheme. - s := scheme.Scheme - s.AddKnownTypes(v1alpha1.GroupVersion, &v1alpha1.PeeringAcceptor{}, &v1alpha1.PeeringAcceptorList{}) - fakeClient := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(k8sObjects...).Build() - - // Create test consul server. - consul, err := testutil.NewTestServerConfigT(t, func(c *testutil.TestServerConfig) { - c.NodeName = nodeName - }) - require.NoError(t, err) - defer consul.Stop() - consul.WaitForServiceIntentions(t) - - cfg := &api.Config{ - Address: consul.HTTPAddr, - } - consulClient, err := api.NewClient(cfg) - require.NoError(t, err) - - // Add the initial peerings into Consul by calling the Generate token endpoint. - _, _, err = consulClient.Peerings().GenerateToken(context.Background(), api.PeeringGenerateTokenRequest{PeerName: tt.initialConsulPeerName}, nil) - require.NoError(t, err) - - // Create the peering acceptor controller. - controller := &PeeringAcceptorController{ - Client: fakeClient, - Log: logrtest.TestLogger{T: t}, - ConsulClient: consulClient, - Scheme: s, - } - namespacedName := types.NamespacedName{ - Name: "acceptor-deleted", - Namespace: "default", - } - - // Reconcile a resource that is not in K8s, but is still in Consul. - resp, err := controller.Reconcile(context.Background(), ctrl.Request{ - NamespacedName: namespacedName, - }) - if tt.expErr != "" { - require.EqualError(t, err, tt.expErr) - } else { - require.NoError(t, err) - } - require.False(t, resp.Requeue) - - // After reconciliation, Consul should not have the peering. - peering, _, err := consulClient.Peerings().Read(context.Background(), "acceptor-deleted", nil) - require.Nil(t, peering) - require.NoError(t, err) - - err = fakeClient.Get(context.Background(), namespacedName, acceptor) - require.EqualError(t, err, `peeringacceptors.consul.hashicorp.com "acceptor-deleted" not found`) - - oldSecret := &corev1.Secret{} - err = fakeClient.Get(context.Background(), namespacedName, oldSecret) - require.EqualError(t, err, `secrets "acceptor-deleted" not found`) - }) + }, + }, } + k8sObjects := []runtime.Object{&ns, acceptor} + + // Add peering types to the scheme. + s := scheme.Scheme + s.AddKnownTypes(v1alpha1.GroupVersion, &v1alpha1.PeeringAcceptor{}, &v1alpha1.PeeringAcceptorList{}) + fakeClient := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(k8sObjects...).Build() + + // Create test consul server. + consul, err := testutil.NewTestServerConfigT(t, func(c *testutil.TestServerConfig) { + c.NodeName = "test-node" + }) + require.NoError(t, err) + defer consul.Stop() + consul.WaitForServiceIntentions(t) + + cfg := &api.Config{ + Address: consul.HTTPAddr, + } + consulClient, err := api.NewClient(cfg) + require.NoError(t, err) + + // Add the initial peerings into Consul by calling the Generate token endpoint. + _, _, err = consulClient.Peerings().GenerateToken(context.Background(), api.PeeringGenerateTokenRequest{PeerName: "acceptor-deleted"}, nil) + require.NoError(t, err) + + // Create the peering acceptor controller. + controller := &PeeringAcceptorController{ + Client: fakeClient, + Log: logrtest.TestLogger{T: t}, + ConsulClient: consulClient, + Scheme: s, + } + namespacedName := types.NamespacedName{ + Name: "acceptor-deleted", + Namespace: "default", + } + + // Reconcile a resource that is not in K8s, but is still in Consul. + resp, err := controller.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: namespacedName, + }) + require.NoError(t, err) + require.False(t, resp.Requeue) + + // After reconciliation, Consul should not have the peering. + timer := &retry.Timer{Timeout: 5 * time.Second, Wait: 500 * time.Millisecond} + retry.RunWith(timer, t, func(r *retry.R) { + peering, _, err := consulClient.Peerings().Read(context.Background(), "acceptor-deleted", nil) + require.Nil(r, peering) + require.NoError(r, err) + }) + + err = fakeClient.Get(context.Background(), namespacedName, acceptor) + require.EqualError(t, err, `peeringacceptors.consul.hashicorp.com "acceptor-deleted" not found`) + + oldSecret := &corev1.Secret{} + err = fakeClient.Get(context.Background(), namespacedName, oldSecret) + require.EqualError(t, err, `secrets "acceptor-deleted" not found`) } func TestShouldGenerateToken(t *testing.T) { diff --git a/control-plane/connect-inject/peering_dialer_controller.go b/control-plane/connect-inject/peering_dialer_controller.go index 8ab18ab146..5ca50c8442 100644 --- a/control-plane/connect-inject/peering_dialer_controller.go +++ b/control-plane/connect-inject/peering_dialer_controller.go @@ -72,15 +72,6 @@ func (r *PeeringDialerController) Reconcile(ctx context.Context, req ctrl.Reques } } - // Get the status secret and the spec secret. - // Cases need to handle secretRefSet, statusSecret, secretSet, specSecret. - // no secretSet --> error bc spec needs to be set. - // no specSecret --> error bc waiting for spec secret to exist. - // no secretRefSet, yes secretSet, no statusSecret, yes specSecret --> initiate peering. - // yes secretRefSet, yes secretSet, no statusSecret, yes specSecret --> initiate peering. - // yes secretRefSet, yes secretSet, yes statusSecret, yes specSecret --> compare contents, if - // different initiate peering. - // TODO(peering): remove this once CRD validation exists. secretSet := false if dialer.Secret() != nil { @@ -162,7 +153,7 @@ func (r *PeeringDialerController) Reconcile(ctx context.Context, req ctrl.Reques } } - // Or, if the peering in Consul does exist, compare it to the contents of the spec's secret. If there's any + // Or, if the peering in Consul does exist, compare it to the spec's secret. If there's any // differences, initiate peering. if r.specStatusSecretsDifferent(dialer, specSecret) { r.Log.Info("the secret in status.secretRef exists and is different from spec.peer.secret; establishing peering with the existing spec.peer.secret", "secret-name", dialer.Secret().Name, "secret-namespace", dialer.Namespace) diff --git a/control-plane/connect-inject/peering_dialer_controller_test.go b/control-plane/connect-inject/peering_dialer_controller_test.go index 069c35cc32..4b453fce94 100644 --- a/control-plane/connect-inject/peering_dialer_controller_test.go +++ b/control-plane/connect-inject/peering_dialer_controller_test.go @@ -14,6 +14,7 @@ import ( "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/sdk/testutil" + "github.com/hashicorp/consul/sdk/testutil/retry" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -487,97 +488,78 @@ func TestSpecStatusSecretsDifferent(t *testing.T) { // TestReconcileDeletePeeringDialer reconciles a PeeringDialer resource that is no longer in Kubernetes, but still // exists in Consul. func TestReconcileDeletePeeringDialer(t *testing.T) { - t.Parallel() - nodeName := "test-node" - cases := []struct { - name string - initialConsulPeerNames []string - expErr string - }{ - { - name: "PeeringDialer no longer in K8s, still exists in Consul", - initialConsulPeerNames: []string{ - "dialer-deleted", + // Add the default namespace. + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "default"}} + + dialer := &v1alpha1.PeeringDialer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dialer-deleted", + Namespace: "default", + DeletionTimestamp: &metav1.Time{Time: time.Now()}, + Finalizers: []string{FinalizerName}, + }, + Spec: v1alpha1.PeeringDialerSpec{ + Peer: &v1alpha1.Peer{ + Secret: nil, }, }, } - for _, tt := range cases { - t.Run(tt.name, func(t *testing.T) { - // Add the default namespace. - ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "default"}} - - dialer := &v1alpha1.PeeringDialer{ - ObjectMeta: metav1.ObjectMeta{ - Name: "dialer-deleted", - Namespace: "default", - DeletionTimestamp: &metav1.Time{Time: time.Now()}, - Finalizers: []string{FinalizerName}, - }, - Spec: v1alpha1.PeeringDialerSpec{ - Peer: &v1alpha1.Peer{ - Secret: nil, - }, - }, - } - - // Create fake k8s client. - k8sObjects := []runtime.Object{ns, dialer} - - // Add peering types to the scheme. - s := scheme.Scheme - s.AddKnownTypes(v1alpha1.GroupVersion, &v1alpha1.PeeringDialer{}, &v1alpha1.PeeringDialerList{}) - fakeClient := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(k8sObjects...).Build() - - // Create test consul server. - consul, err := testutil.NewTestServerConfigT(t, func(c *testutil.TestServerConfig) { - c.NodeName = nodeName - }) - require.NoError(t, err) - defer consul.Stop() - consul.WaitForServiceIntentions(t) - - cfg := &api.Config{ - Address: consul.HTTPAddr, - } - consulClient, err := api.NewClient(cfg) - require.NoError(t, err) - - // Add the initial peerings into Consul by calling the Generate token endpoint. - _, _, err = consulClient.Peerings().GenerateToken(context.Background(), api.PeeringGenerateTokenRequest{PeerName: tt.initialConsulPeerNames[0]}, nil) - require.NoError(t, err) - // Create the peering dialer controller. - pdc := &PeeringDialerController{ - Client: fakeClient, - Log: logrtest.TestLogger{T: t}, - ConsulClient: consulClient, - Scheme: s, - } - namespacedName := types.NamespacedName{ - Name: "dialer-deleted", - Namespace: "default", - } + // Create fake k8s client. + k8sObjects := []runtime.Object{ns, dialer} - // Reconcile a resource that is not in K8s, but is still in Consul. - resp, err := pdc.Reconcile(context.Background(), ctrl.Request{ - NamespacedName: namespacedName, - }) - if tt.expErr != "" { - require.EqualError(t, err, tt.expErr) - } else { - require.NoError(t, err) - } - require.False(t, resp.Requeue) + // Add peering types to the scheme. + s := scheme.Scheme + s.AddKnownTypes(v1alpha1.GroupVersion, &v1alpha1.PeeringDialer{}, &v1alpha1.PeeringDialerList{}) + fakeClient := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(k8sObjects...).Build() - // After reconciliation, Consul should not have the peering. - peering, _, err := consulClient.Peerings().Read(context.Background(), "dialer-deleted", nil) - require.Nil(t, peering) - require.NoError(t, err) + // Create test consul server. + consul, err := testutil.NewTestServerConfigT(t, func(c *testutil.TestServerConfig) { + c.NodeName = "test-node" + }) + require.NoError(t, err) + defer consul.Stop() + consul.WaitForServiceIntentions(t) - err = fakeClient.Get(context.Background(), namespacedName, dialer) - require.EqualError(t, err, `peeringdialers.consul.hashicorp.com "dialer-deleted" not found`) - }) + cfg := &api.Config{ + Address: consul.HTTPAddr, } + consulClient, err := api.NewClient(cfg) + require.NoError(t, err) + + // Add the initial peerings into Consul by calling the Generate token endpoint. + _, _, err = consulClient.Peerings().GenerateToken(context.Background(), api.PeeringGenerateTokenRequest{PeerName: "dialer-deleted"}, nil) + require.NoError(t, err) + + // Create the peering dialer controller. + pdc := &PeeringDialerController{ + Client: fakeClient, + Log: logrtest.TestLogger{T: t}, + ConsulClient: consulClient, + Scheme: s, + } + namespacedName := types.NamespacedName{ + Name: "dialer-deleted", + Namespace: "default", + } + + // Reconcile a resource that is not in K8s, but is still in Consul. + resp, err := pdc.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: namespacedName, + }) + require.NoError(t, err) + require.False(t, resp.Requeue) + + // After reconciliation, Consul should not have the peering. + timer := &retry.Timer{Timeout: 5 * time.Second, Wait: 500 * time.Millisecond} + retry.RunWith(timer, t, func(r *retry.R) { + peering, _, err := consulClient.Peerings().Read(context.Background(), "dialer-deleted", nil) + require.Nil(r, peering) + require.NoError(r, err) + }) + + err = fakeClient.Get(context.Background(), namespacedName, dialer) + require.EqualError(t, err, `peeringdialers.consul.hashicorp.com "dialer-deleted" not found`) } func TestDialerUpdateStatus(t *testing.T) { diff --git a/control-plane/go.mod b/control-plane/go.mod index db7b2d9ec7..c4f5a31790 100644 --- a/control-plane/go.mod +++ b/control-plane/go.mod @@ -6,7 +6,7 @@ require ( github.com/go-logr/logr v0.4.0 github.com/google/go-cmp v0.5.7 github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 - github.com/hashicorp/consul/api v1.10.1-0.20220610161046-7001e1151cbe + github.com/hashicorp/consul/api v1.10.1-0.20220614213650-6453375ab228 github.com/hashicorp/consul/sdk v0.9.0 github.com/hashicorp/go-discover v0.0.0-20200812215701-c4b85f6ed31f github.com/hashicorp/go-hclog v0.16.1 @@ -131,6 +131,4 @@ require ( replace github.com/hashicorp/consul/sdk v0.9.0 => github.com/hashicorp/consul/sdk v0.4.1-0.20220531155537-364758ef2f50 -//replace github.com/hashicorp/consul/api v1.10.1-0.20220425143126-6d0162a58a94 => /Users/nitya/workspace/hashicorp/consul/api - go 1.17 diff --git a/control-plane/go.sum b/control-plane/go.sum index 8d26a90944..b67416a74a 100644 --- a/control-plane/go.sum +++ b/control-plane/go.sum @@ -297,8 +297,8 @@ github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0/go.mod h1:8NvIoxWQoOIhqOTXgf github.com/grpc-ecosystem/grpc-gateway v1.9.0/go.mod h1:vNeuVxBJEsws4ogUvrchl83t/GYV9WGTSLVdBhOQFDY= github.com/grpc-ecosystem/grpc-gateway v1.16.0/go.mod h1:BDjrQk3hbvj6Nolgz8mAMFbcEtjT1g+wF4CSlocrBnw= github.com/hashicorp/consul/api v1.1.0/go.mod h1:VmuI/Lkw1nC05EYQWNKwWGbkg+FbDBtguAZLlVdkD9Q= -github.com/hashicorp/consul/api v1.10.1-0.20220610161046-7001e1151cbe h1:YQSxqFG8IsG/qCQaPLnimycM8bpU6UYVJ5fURrJmDS4= -github.com/hashicorp/consul/api v1.10.1-0.20220610161046-7001e1151cbe/go.mod h1:ZlVrynguJKcYr54zGaDbaL3fOvKC9m72FhPvA8T35KQ= +github.com/hashicorp/consul/api v1.10.1-0.20220614213650-6453375ab228 h1:BqzKe5O+75uYcFfJI0mJz3rhCgdVztvEj3rEs4xpPr0= +github.com/hashicorp/consul/api v1.10.1-0.20220614213650-6453375ab228/go.mod h1:ZlVrynguJKcYr54zGaDbaL3fOvKC9m72FhPvA8T35KQ= github.com/hashicorp/consul/sdk v0.1.1/go.mod h1:VKf9jXwCTEY1QZP2MOLRhb5i/I/ssyNV1vwHyQBF0x8= github.com/hashicorp/consul/sdk v0.4.1-0.20220531155537-364758ef2f50 h1:GwbRRT+QxMRbYI608FGwTfcZ0iOVLX69B2ePjpQoyXw= github.com/hashicorp/consul/sdk v0.4.1-0.20220531155537-364758ef2f50/go.mod h1:yPkX5Q6CsxTFMjQQDJwzeNmUUF5NUGGbrDsv9wTb8cw= diff --git a/control-plane/main.go b/control-plane/main.go index f093901baf..7ec1340290 100644 --- a/control-plane/main.go +++ b/control-plane/main.go @@ -4,9 +4,8 @@ import ( "log" "os" - "github.com/mitchellh/cli" - "github.com/hashicorp/consul-k8s/control-plane/version" + "github.com/mitchellh/cli" ) func main() { diff --git a/control-plane/subcommand/inject-connect/command.go b/control-plane/subcommand/inject-connect/command.go index 32248f61c4..a39f8f44e9 100644 --- a/control-plane/subcommand/inject-connect/command.go +++ b/control-plane/subcommand/inject-connect/command.go @@ -462,7 +462,7 @@ func (c *Command) Run(args []string) int { mgr.GetWebhookServer().CertDir = c.flagCertDir mgr.GetWebhookServer().Register("/mutate", - &webhook.Admission{Handler: &connectinject.ConnectWebhook{ + &webhook.Admission{Handler: &connectinject.MeshWebhook{ Clientset: c.clientset, ConsulClient: c.consulClient, ImageConsul: c.flagConsulImage,