V2 MeshConfig Controller and TrafficPermissions CRD #2967

thisisnotashwin · 2023-09-15T19:48:57Z

Changes proposed in this PR:

Add Generic V2 MeshConfig Controller
Add CRDs for V2 TrafficPermissions (fka Intentions)

Things that need to be done in a future PR:

ENT Tests
Add the migration annotation to assume control of resources already in Consul from K8s
Some refactoring in the meshConfigController tests to keep them generic
Update Contributing guides for the V2 code

How I've tested this PR:

Unit/Integration tests
Manual testing: I spun up the latest Consul CE version with these changes and I was able to create a test TrafficPermission in Consul then delete it.

How I expect reviewers to test this PR: ☕ ☕ ☕ ☕ ☕ ☕ 👓

Checklist:

Tests added
~~CHANGELOG entry added~~

DanStough

Looking awesome, @thisisnotashwin ! Left some comments as I was reading through it.

control-plane/api/v2alpha1/groupversion_info.go

control-plane/api/v2alpha1/traffic_permissions_types.go

control-plane/config-entries/controllersv2/configentry_controller.go

DanStough · 2023-09-22T02:46:32Z

control-plane/api/common/common.go

+// ConsulTenancyConfig manages settings related to Consul namespaces and partitions.
+type ConsulTenancyConfig struct {
+	// EnableConsulPartitions indicates that a user is running Consul Enterprise.
+	EnableConsulPartitions bool
+	// ConsulPartition is the Consul Partition to which this controller belongs.
+	ConsulPartition string
+	// EnableConsulNamespaces indicates that a user is running Consul Enterprise.
+	EnableConsulNamespaces bool
+	// ConsulDestinationNamespace is the name of the Consul namespace to create
+	// all resources in. If EnableNSMirroring is true this is ignored.
+	ConsulDestinationNamespace string
+	// EnableNSMirroring causes Consul namespaces to be created to match the
+	// k8s namespace of any config entry custom resource. Resources will
+	// be created in the matching Consul namespace.
+	EnableNSMirroring bool
+	// NSMirroringPrefix is an optional prefix that can be added to the Consul
+	// namespaces created while mirroring. For example, if it is set to "k8s-",
+	// then the k8s `default` namespace will be mirrored in Consul's
+	// `k8s-default` namespace.
+	NSMirroringPrefix string
+}


Refactored here since this is shared by both the connect and config controllers.

control-plane/config-entries/controllersv2/meshconfig_controller.go

DanStough · 2023-09-22T03:34:50Z

control-plane/config-entries/controllersv2/meshconfig_controller_test.go

+	cases := []struct {
+		name       string
+		meshConfig common.MeshConfig
+		expected   *pbauth.TrafficPermissions


I realize these tests are suppose to be generic, but in the interest of time, I just hardcoded the TrafficPermissions structs in several places. I suspect they will get refactored shortly at the expense of some boilerplate to convert to a pbresource.Resource.

ndhanushkodi

Had a little time this evening and started reviewing, so I thought I'd leave my half review-- I got through the _types.go and _types_test.go and all the generated files and renames and the refactor, but didn't make it to the controller itself for a thorough review in case someone is reviewing in the east coast AM!

ndhanushkodi · 2023-09-22T04:25:01Z

charts/consul/templates/crd-exportedservices.yaml

@@ -1,21 +1,18 @@
 {{- if .Values.connectInject.enabled }}
-# Copyright (c) HashiCorp, Inc.
-# SPDX-License-Identifier: MPL-2.0
-


Do we want to bring back the license headers? Probably the generation script blew them away.

Non-blocking

ndhanushkodi · 2023-09-22T04:29:27Z

control-plane/PROJECT

+  group: auth
+  kind: TrafficPermissions
+  path: github.com/hashicorp/consul-k8s/control-plane/api/v2alpha1
+  version: v2alpha1


nit: looks like Iryna updated versions in core to v2beta1 rather than alpha so we should match

ndhanushkodi · 2023-09-22T04:41:53Z

control-plane/api/v2alpha1/traffic_permissions_types_test.go

+
+		TheirName            string
+		TheirConsulNamespace string
+		TheirConsulPartition string


Wow, thanks for adding ent here, I don't think the other config entries had ent tests like this. So thorough!!

ndhanushkodi · 2023-09-22T04:50:43Z

control-plane/config/crd/patches/cainjection_in_trafficpermissions.yaml

@@ -0,0 +1,11 @@
+# Copyright (c) HashiCorp, Inc.


We usually delete the patches folder in config/crd

ndhanushkodi · 2023-09-22T04:51:42Z

control-plane/config/rbac/trafficpermissions_editor_role.yaml

@@ -0,0 +1,27 @@
+# Copyright (c) HashiCorp, Inc.


We usually delete this since it's a sample

ndhanushkodi · 2023-09-22T04:51:55Z

control-plane/config/rbac/trafficpermissions_viewer_role.yaml

@@ -0,0 +1,23 @@
+# Copyright (c) HashiCorp, Inc.
+# SPDX-License-Identifier: MPL-2.0
+


We usually delete since its a sample

ndhanushkodi · 2023-09-22T04:52:14Z

control-plane/config/samples/consul_v2alpha1_trafficpermissions.yaml

@@ -0,0 +1,14 @@
+# Copyright (c) HashiCorp, Inc.
+# SPDX-License-Identifier: MPL-2.0
+


usually delete this since it's a sample

ndhanushkodi · 2023-09-22T04:53:42Z

control-plane/connect-inject/controllers/endpointsv2/endpoints_controller_test.go

@@ -113,7 +114,7 @@ func TestReconcile_CreateService(t *testing.T) {
 		//		}
 		//		return []runtime.Object{endpoints, service}
 		//	},
-		//	expectedResource: &pbresource.Resource{
+		//	expectedResource: &pbresource.MeshConfig{


I think this is not meant to be MeshConfig here since I see a Service below?

ishustava · 2023-09-22T14:29:54Z

charts/consul/templates/connect-inject-mutatingwebhookconfiguration.yaml

+    - apiGroups:
+        - auth.consul.hashicorp.com
+      apiVersions:
+        - v2alpha1


🚗 this should be v2beta1 once hashicorp/consul#18930 is merged

yup!! i have incorporated those changes into this PR to ease the eventual rebase

thisisnotashwin

APPROVED!!

thisisnotashwin · 2023-09-22T13:45:15Z

charts/consul/templates/connect-inject-mutatingwebhookconfiguration.yaml

+    - v1
+  clientConfig:
+    service:
+      name: {{ template "consul.fullname" . }}-connect-injector


nit: maybe we can call this mesh-injector? im OK moving away from the injector verbiage altogether but WDYT?

thisisnotashwin · 2023-09-22T15:14:07Z

charts/consul/templates/connect-inject-mutatingwebhookconfiguration.yaml

+    - apiGroups:
+        - auth.consul.hashicorp.com
+      apiVersions:
+        - v2alpha1


yup!! i have incorporated those changes into this PR to ease the eventual rebase

thisisnotashwin requested a review from DanStough September 15, 2023 19:48

thisisnotashwin force-pushed the ashwin/NET-5317-v2-config-controller branch 2 times, most recently from 3c604a4 to a1de453 Compare September 15, 2023 19:53

DanStough reviewed Sep 19, 2023

View reviewed changes

thisisnotashwin force-pushed the ashwin/NET-5317-v2-config-controller branch 2 times, most recently from cb319c7 to a8bf220 Compare September 20, 2023 14:26

DanStough force-pushed the ashwin/NET-5317-v2-config-controller branch from f8eb943 to bc68ecb Compare September 22, 2023 02:42

DanStough changed the title ~~WIP: add controller for traffic permissions~~ V2 MeshConfig Controller and TrafficPermissions CRD Sep 22, 2023

DanStough marked this pull request as ready for review September 22, 2023 02:43

DanStough added pr/no-changelog PR does not need a corresponding .changelog entry pr/no-backport signals that a PR will not contain a backport label labels Sep 22, 2023

DanStough reviewed Sep 22, 2023

View reviewed changes

control-plane/config-entries/controllersv2/meshconfig_controller.go Outdated Show resolved Hide resolved

DanStough reviewed Sep 22, 2023

View reviewed changes

DanStough requested a review from zalimeni September 22, 2023 03:41

ndhanushkodi reviewed Sep 22, 2023

View reviewed changes

thisisnotashwin force-pushed the ashwin/NET-5317-v2-config-controller branch from 43632ab to 72e3d0f Compare September 22, 2023 13:43

ishustava reviewed Sep 22, 2023

View reviewed changes

thisisnotashwin commented Sep 22, 2023

View reviewed changes

thisisnotashwin and others added 9 commits September 22, 2023 13:00

WIP: add controller for traffic permissions

06c691e

WIP: bugfix, refactor and add webhook

8a2f62e

WIP: add tests for webhook and tp types

cfff207

WIP: finish tests, add webhook to yaml

113909f

WIP: bugfix chart

be1cd75

WIP: remove unnecessary comments

ffe91ea

WIP: remove unnecessary header

df7cc76

Add finalizers and complete manual tests

21b38e2

Make changes based on feedback

37309df

thisisnotashwin force-pushed the ashwin/NET-5317-v2-config-controller branch from 6e6d74f to 37309df Compare September 22, 2023 17:00

thisisnotashwin enabled auto-merge (squash) September 22, 2023 17:02

zalimeni approved these changes Sep 22, 2023

View reviewed changes

thisisnotashwin merged commit e57edf5 into main Sep 22, 2023

thisisnotashwin deleted the ashwin/NET-5317-v2-config-controller branch September 22, 2023 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

V2 MeshConfig Controller and TrafficPermissions CRD #2967

V2 MeshConfig Controller and TrafficPermissions CRD #2967

thisisnotashwin commented Sep 15, 2023 •

edited by DanStough

Loading

DanStough left a comment

DanStough Sep 22, 2023

DanStough Sep 22, 2023

ndhanushkodi left a comment

ndhanushkodi Sep 22, 2023

ndhanushkodi Sep 22, 2023

ndhanushkodi Sep 22, 2023

ndhanushkodi Sep 22, 2023

ndhanushkodi Sep 22, 2023

ndhanushkodi Sep 22, 2023

ndhanushkodi Sep 22, 2023

ndhanushkodi Sep 22, 2023

ndhanushkodi Sep 22, 2023

ishustava Sep 22, 2023

thisisnotashwin Sep 22, 2023

thisisnotashwin left a comment

thisisnotashwin Sep 22, 2023

thisisnotashwin Sep 22, 2023

		@@ -0,0 +1,23 @@
		# Copyright (c) HashiCorp, Inc.
		# SPDX-License-Identifier: MPL-2.0

		@@ -0,0 +1,14 @@
		# Copyright (c) HashiCorp, Inc.
		# SPDX-License-Identifier: MPL-2.0

V2 MeshConfig Controller and TrafficPermissions CRD #2967

V2 MeshConfig Controller and TrafficPermissions CRD #2967

Conversation

thisisnotashwin commented Sep 15, 2023 • edited by DanStough Loading

DanStough left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndhanushkodi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thisisnotashwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thisisnotashwin commented Sep 15, 2023 •

edited by DanStough

Loading