Endpoints controller with tests #455

ndhanushkodi · 2021-03-15T16:24:40Z

The endpoints controller and test code is ready for review.

The only things expected to change at this point are code and tests in handler (to remove always setting the service-name annotation) and inject-connect (to get the command unit tests to pass since the manager takes a kubeconfig that's hard to fake out).

Changes in this PR:

supports creating, updating and deleting endpoints
- with different consul service name (using the service-name pod annotation)
updating endpoints supports adding and removing addresses from the endpoints object.

Todos:

testing with new connect-init command
testing end to end

Future PR:

delete default annotation for service name (to not cause rebase issues with webhook refactor PR lined up after this one)
inject-connect tests-- the webhook PR has a fix for one of these tests, and the other test is signal handling that would likely be refactored out. There's a TODO for this in the code.
add metrics config to proxy registration (this is going to a future PR so it won't conflict with the upcoming webhook refactor PR)
deleting old service registration

Co-authored-by: Iryna Shustava iryna@hashicorp.com
Co-authored-by: Kyle Schochenmaier kyle.schochenmaier@hashicorp.com

How I've tested this PR:

Unit tests for endpoints controller

How I expect reviewers to test this PR:

Checklist:

Tests added
CHANGELOG entry added (HashiCorp engineers only, community PRs should not add a changelog entry)

thisisnotashwin

This looks really good so far Nitya!! I do want to give it one more pass cuz it is complicated (not the code, mostly just the number of things it has to manage)!!

subcommand/inject-connect/command.go

connect-inject/endpoints_controller.go

ndhanushkodi · 2021-03-21T07:41:54Z

connect-inject/endpoints_controller.go

+		return ctrl.Result{}, err
+	}
+
+	r.Log.Info("retrieved service from kube", "serviceEndpoints", serviceEndpoints)


The whole serviceEndpoints object shouldn't be logged here, just the name, same for other logs. Make sure to decide what should be Debug vs Info. Should info logs log every reconcile or only debug?

ndhanushkodi · 2021-03-22T02:29:09Z

connect-inject/endpoints_controller.go

+		return ctrl.Result{}, err
+	}
+
+	r.Log.Info("retrieved service from kube", "serviceEndpoints", serviceEndpoints)


I changed it to "retrieved Kubernetes Endpoints" since it's the endpoints object

* supports using serviceName annotation (different k8s service name) * supports using servicePort annotation * supports creating endpoints (with serviceName annotation) * supports updating endpoints (adding and removing addresses) * supports deleting endpoints (with different k8s service name) todo: * tags, meta, and upstreams from annotations + tests * cleaning up and refactoring code to run the endpoints controller with manager * deleting old service registration * testing with new connect-init command * testing end to end Co-authored-by: Iryna Shustava <iryna@hashicorp.com> Co-authored-by: Kyle Schochenmaier <kyle.schochenmaier@hashicorp.com>

deregistration to use catalog api rather than agent api

* they needed the auth plugin import: _ "k8s.io/client-go/plugin/pkg/client/auth" * kubernetes/client-go#242

Todos left: * remove service name default annotation * make inject-connect unit tests pass without a kubeconfig * end to end testing

method. This way, additional edge case logic can be tested separately, rather than in TestReconcileCreateEndpoint.

This will be a part of a future PR that enables endpoints-controller and removes the old method of service registration.

ishustava

Looking great!! Awesome work thinking through all the test case scenarios!

I've left some comments and questions, let me know what you think!

ishustava · 2021-03-19T21:04:46Z

connect-inject/endpoints_controller.go

+// "k8s-service-name". So, we query Consul services by "k8s-service-name" metadata, which is only exposed on the agent
+// API. Therefore, we need to query all agents who have services matching that metadata, and deregister each service
+// instance. When querying by the k8s service name and namespace, the request will return service instances and
+// associated proxy service instances.


great job on this func and the comments!! It was very easy to read!

connect-inject/endpoints_controller.go

connect-inject/endpoints_controller_test.go

thisisnotashwin

Ok i gave this a thorough code review!! left a bunch of suggestions. Most of them are random tidy ups and some of them are questions.
I haven't actually run this but im guessing that will be within the scope of the glue PR.
Im ok with this getting merged when the suggestions and open comments have been resolved!!

This is excellent work @ndhanushkodi and the code is really easy to read for the nature of the complexity it is trying to solve!! Love the comments as well!! Amazing job overall!!

connect-inject/endpoints_controller.go

thisisnotashwin · 2021-03-23T21:52:16Z

connect-inject/endpoints_controller.go

+// "k8s-service-name". So, we query Consul services by "k8s-service-name" metadata, which is only exposed on the agent
+// API. Therefore, we need to query all agents who have services matching that metadata, and deregister each service
+// instance. When querying by the k8s service name and namespace, the request will return service instances and
+// associated proxy service instances.


connect-inject/endpoints_controller.go

thisisnotashwin · 2021-03-23T21:56:30Z

connect-inject/endpoints_controller.go

+				if _, ok := endpointsAddressesMap[serviceRegistration.Address]; !ok {
+					// If the service address is not in the Endpoints addresses, deregister it.
+					if err = client.Agent().ServiceDeregister(svcID); err != nil {
+						r.Log.Error(err, "failed to deregister service instance", "consul-service-id", svcID)


Can we create a constant for "consul-service-id"?

It seems a little strange to me to make it a const since it's just a key in the log. Basically I am not sure what I would call the const to have it be more readable than the string "consul-service-id".

connect-inject/endpoints_controller.go

subcommand/inject-connect/command.go

ishustava

Looks great!!

ishustava · 2021-03-24T17:02:10Z

connect-inject/endpoints_controller.go

+	// Get the tags from the deprecated tags annotation and combine.
+	if raw, ok := pod.Annotations[annotationConnectTags]; ok && raw != "" {
+		tags = append(tags, strings.Split(raw, ",")...)
+	}


It looks like the annotation was renamed in #141, and we've kept both names for backward compatibility. Since this is meant to be a refactor I think we can keep it. It seems relatively cheap to maintain.

Add basic endpoints controller with tests * reconciles k8s services with Consul * supports creating, updating, and deleting endpoints, both with matching Consul and k8s service names, and with different Consul and k8s service names Co-authored-by: Iryna Shustava <iryna@hashicorp.com> Co-authored-by: Kyle Schochenmaier <kyle.schochenmaier@hashicorp.com>

0.21.0 release

ndhanushkodi marked this pull request as draft March 15, 2021 16:24

ndhanushkodi changed the title ~~WIP Endpoints controller with tests~~ Endpoints controller with tests Mar 18, 2021

ndhanushkodi marked this pull request as ready for review March 18, 2021 17:12

ndhanushkodi requested review from a team, ishustava and thisisnotashwin and removed request for a team March 18, 2021 17:12

ndhanushkodi force-pushed the basic-endpoints-controller-and-tests branch from 9390339 to affaf94 Compare March 18, 2021 18:51

thisisnotashwin reviewed Mar 18, 2021

View reviewed changes

ndhanushkodi commented Mar 21, 2021

View reviewed changes

ndhanushkodi commented Mar 22, 2021

View reviewed changes

ndhanushkodi force-pushed the basic-endpoints-controller-and-tests branch from 10900d1 to 752eca4 Compare March 22, 2021 05:11

ndhanushkodi added the theme/tproxy Items related to transparent proxy label Mar 22, 2021

ndhanushkodi and others added 12 commits March 22, 2021 15:26

Add todos, use consul client (for linter), refactor service instance

406a46b

deregistration to use catalog api rather than agent api

Support meta and tags, cleanup, comments

8e4f1b1

inject-connect command tests pass locally

428d8aa

* they needed the auth plugin import: _ "k8s.io/client-go/plugin/pkg/client/auth" * kubernetes/client-go#242

Finished endpoints controller

431ad28

Todos left: * remove service name default annotation * make inject-connect unit tests pass without a kubeconfig * end to end testing

Add one more case to Create tests

5441857

Run tests in parallel

821d837

Skip tests and add comments for why

201be6b

Fix case where k8s deployment is deleted but not the k8s service.

83f5435

Extract creating the service and proxy instance registrations into a

288b8d4

method. This way, additional edge case logic can be tested separately, rather than in TestReconcileCreateEndpoint.

Cleanup and address review comments

0f69d7d

Remove running the endpoints controller in this PR.

715854a

This will be a part of a future PR that enables endpoints-controller and removes the old method of service registration.

ndhanushkodi force-pushed the basic-endpoints-controller-and-tests branch from 752eca4 to 715854a Compare March 22, 2021 22:27

hashicorp deleted a comment from thisisnotashwin Mar 22, 2021

ishustava reviewed Mar 23, 2021

View reviewed changes

ndhanushkodi requested a review from thisisnotashwin March 23, 2021 18:04

Address review comments

16ef1ca

ndhanushkodi requested a review from ishustava March 23, 2021 21:06

thisisnotashwin approved these changes Mar 23, 2021

View reviewed changes

Address review comments

dfca111

ishustava approved these changes Mar 24, 2021

View reviewed changes

ndhanushkodi merged commit 281552c into feature-tproxy Mar 24, 2021

ndhanushkodi deleted the basic-endpoints-controller-and-tests branch March 24, 2021 22:01

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

Merge pull request hashicorp#455 from hashicorp/release-0.21.0

b6648e7

0.21.0 release

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Endpoints controller with tests #455

Endpoints controller with tests #455

ndhanushkodi commented Mar 15, 2021 •

edited

thisisnotashwin left a comment

ndhanushkodi Mar 21, 2021

ndhanushkodi Mar 22, 2021

ishustava left a comment

ishustava Mar 19, 2021

thisisnotashwin Mar 23, 2021

thisisnotashwin left a comment

thisisnotashwin Mar 23, 2021

thisisnotashwin Mar 23, 2021

ndhanushkodi Mar 23, 2021

ishustava left a comment

ishustava Mar 24, 2021

Endpoints controller with tests #455

Endpoints controller with tests #455

Conversation

ndhanushkodi commented Mar 15, 2021 • edited

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

ishustava 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 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

ishustava left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndhanushkodi commented Mar 15, 2021 •

edited