ServiceResolver controller and webhook #325

lkysow · 2020-09-15T23:52:35Z

Changes proposed in this PR:

Implement support for service-resolver config entries:
- Copy its type definition over from Consul and implement toConsul() and matchesConsul()
- Create controller and webhook for it
- Run generators to create deepcopy and yaml files
Also make controller code generic between service-defaults and service-resolver by abstracting out common functionality into shared functions (which required introducing some interfaces)
- Introduced the concept of "config entry" as a generic config entry where common functionality between crd-specific objects will love
- New interface Reconciler is implemented by each CRD-specific reconciler
- New interface ConfigEntryCRD is implemented by each CRD object
- Abstracted all reconciler code into generic ConfigEntryReconciler
- Abstracted all webhook code into generic ValidateConfigEntry function
Update to controller-runtime 0.6.3 to fix spurious log error message.

How I've tested this PR: Acceptance tests from hashicorp/consul-helm#606

How I expect reviewers to test this PR:

Reviewing the acceptance tests from Support for ServiceResolver CRDs consul-helm#606 should show that the refactoring didn't break anything
I've built ghcr.io/lkysow/consul-k8s-dev:sep17-crd-controller-resolver that can be tested with
Analyze the decisions made for the refactor

Checklist:

Tests added

lkysow · 2020-09-17T15:47:41Z

api/v1alpha1/servicedefaults_types.go

 	metav1.TypeMeta   `json:",inline"`
 	metav1.ObjectMeta `json:"metadata,omitempty"`
+	Status            ConfigEntryStatus   `json:"status,omitempty"`
+	Spec              ServiceDefaultsSpec `json:"spec,omitempty"`


I couldn't pull metav1.TypeMeta, metav1.ObjectMeta and Status into a shared struct that is embedded into all config entries without breaking code generation so unfortunately we'll need to implement the ConfigEntryCRD interface for all config entries.

api/v1alpha1/servicedefaults_types.go

api/v1alpha1/serviceresolver_types.go

config/crd/bases/consul.hashicorp.com_servicedefaults.yaml

config/crd/kustomization.yaml

lkysow · 2020-09-17T15:50:55Z

controllers/configentry_controller.go

@@ -0,0 +1,299 @@
+package controllers


This is the generic controller that works for all config entries.

what do you think we should do with helper/controller.go?

I'd say that we should move it into sync-catalog but I think it may be used for health checks. We could rename it as something, maybe CacheController and move it into its own top-level package?

Yeah, we should consolidate it I think, although I don't think this is required for this PR.

controllers/configentry_webhook.go

controllers/serviceresolver_controller.go

thisisnotashwin

This is just a partial review. I feel a bit concerned about webhooks moving into the controllers package. Here is my rationale: validation and defaulting can be specific to the version of the CRD and hence it should live in the same package as the CRD it is performing validations/defaulting for (v1alpha1) in this case. Controllers on the other hand are CRD version agnostic (conversion webhooks should ensure they are converted to the version the controller supports).

I tried tinkering with its structure and ran into code-gen issues with the interface as well.

api/v1alpha1/configentry_status.go

api/v1alpha1/servicedefaults_types_test.go

controllers/configentry_controller.go

lkysow · 2020-09-17T20:24:24Z

Webhooks moved back to api/alphav1. Common webhooks code (and common interface) moved to api/common

thisisnotashwin

Havent yet run these but this looks great!! Will try running them now.

api/common/configentry.go

api/common/configentry_webhook.go

api/v1alpha1/serviceresolver_types.go

config/samples/consul_v1alpha1_serviceresolver.yaml

controllers/configentry_controller_test.go

controllers/servicedefaults_controller.go

controllers/serviceresolver_controller.go

subcommand/controller/command.go

thisisnotashwin

💥

lkysow · 2020-09-21T19:03:41Z

subcommand/webhook-cert-manager/command_test.go

@@ -332,6 +332,7 @@ func TestRun_SecretUpdates(t *testing.T) {
 // the certs or the webhook config, it retries the update every second until
 // it succeeds.
 func TestCertWatcher(t *testing.T) {
+	t.SkipNow()


This test is being fixed in #326 but marking skipped now so CI is green. Won't merge with this.

ishustava

❤️ Love the refactor!! I've left some comments, ideas, questions, and suggestions. I haven't run it yet, but thought it'd be worth leaving the comments I have so far.

api/common/configentry.go

api/v1alpha1/serviceresolver_types_test.go

config/crd/patches/webhook_in_servicedefaults.yaml

ishustava · 2020-09-22T00:39:32Z

controllers/configentry_controller.go

@@ -0,0 +1,299 @@
+package controllers


what do you think we should do with helper/controller.go?

controllers/configentry_controller.go

api/common/configentry_webhook.go

ishustava

Looks great!! A couple of minor comments, nothing blocking though.

api/common/configentry_webhook_test.go

* Also make controller and webhook code generic * Update to controller-runtime 0.6.3 to fix spurious log error message.

Enterprise License Job PodSecurityPolicy

lkysow force-pushed the crd-controller-resolver branch 7 times, most recently from 0f88eb7 to 070a667 Compare September 16, 2020 20:35

lkysow commented Sep 17, 2020

View reviewed changes

lkysow requested a review from thisisnotashwin September 17, 2020 15:52

lkysow marked this pull request as ready for review September 17, 2020 15:52

lkysow mentioned this pull request Sep 17, 2020

Support for ServiceResolver CRDs hashicorp/consul-helm#606

Merged

thisisnotashwin reviewed Sep 17, 2020

View reviewed changes

api/v1alpha1/configentry_status.go Outdated Show resolved Hide resolved

api/v1alpha1/servicedefaults_types_test.go Show resolved Hide resolved

controllers/configentry_controller.go Outdated Show resolved Hide resolved

controllers/configentry_controller.go Outdated Show resolved Hide resolved

thisisnotashwin reviewed Sep 18, 2020

View reviewed changes

thisisnotashwin approved these changes Sep 21, 2020

View reviewed changes

lkysow requested review from a team and ishustava and removed request for a team September 21, 2020 18:59

lkysow commented Sep 21, 2020

View reviewed changes

ishustava reviewed Sep 22, 2020

View reviewed changes

lkysow requested a review from ishustava September 22, 2020 20:46

ishustava approved these changes Sep 22, 2020

View reviewed changes

api/common/configentry_webhook_test.go Outdated Show resolved Hide resolved

api/common/configentry_webhook_test.go Show resolved Hide resolved

api/common/configentry_webhook_test.go Outdated Show resolved Hide resolved

ServiceResolver controller and webhook

d62b005

* Also make controller and webhook code generic * Update to controller-runtime 0.6.3 to fix spurious log error message.

lkysow force-pushed the crd-controller-resolver branch from 245b858 to d62b005 Compare September 22, 2020 22:04

lkysow merged commit b333e0b into crd-controller-base Sep 22, 2020

lkysow deleted the crd-controller-resolver branch September 22, 2020 22:05

ndhanushkodi pushed a commit to ndhanushkodi/consul-k8s that referenced this pull request Jul 9, 2021

Merge pull request hashicorp#325 from hashicorp/psps

1e9f22f

Enterprise License Job PodSecurityPolicy

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ServiceResolver controller and webhook #325

ServiceResolver controller and webhook #325

lkysow commented Sep 15, 2020 •

edited

Loading

lkysow Sep 17, 2020

lkysow Sep 17, 2020

ishustava Sep 22, 2020

lkysow Sep 22, 2020

ishustava Sep 22, 2020

thisisnotashwin left a comment •

edited

Loading

lkysow commented Sep 17, 2020

thisisnotashwin left a comment

thisisnotashwin left a comment

lkysow Sep 21, 2020

ishustava left a comment

ishustava Sep 22, 2020

ishustava left a comment

ServiceResolver controller and webhook #325

ServiceResolver controller and webhook #325

Conversation

lkysow commented Sep 15, 2020 • edited Loading

lkysow Sep 17, 2020

Choose a reason for hiding this comment

lkysow Sep 17, 2020

Choose a reason for hiding this comment

ishustava Sep 22, 2020

Choose a reason for hiding this comment

lkysow Sep 22, 2020

Choose a reason for hiding this comment

ishustava Sep 22, 2020

Choose a reason for hiding this comment

thisisnotashwin left a comment • edited Loading

Choose a reason for hiding this comment

lkysow commented Sep 17, 2020

thisisnotashwin left a comment

Choose a reason for hiding this comment

thisisnotashwin left a comment

Choose a reason for hiding this comment

lkysow Sep 21, 2020

Choose a reason for hiding this comment

ishustava left a comment

Choose a reason for hiding this comment

ishustava Sep 22, 2020

Choose a reason for hiding this comment

ishustava left a comment

Choose a reason for hiding this comment

lkysow commented Sep 15, 2020 •

edited

Loading

thisisnotashwin left a comment •

edited

Loading