-
Notifications
You must be signed in to change notification settings - Fork 324
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support Consul ENT Namespaces in the endpoints controller #475
Conversation
7c3d03a
to
ce55bf9
Compare
2563125
to
076c8aa
Compare
@@ -178,12 +178,15 @@ consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ | |||
{{- if .NamespaceMirroringEnabled }} | |||
{{- /* If namespace mirroring is enabled, the auth method is | |||
defined in the default namespace */}} | |||
-namespace="default" | |||
-auth-method-namespace="default" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice change!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, the flag names are great and helped me follow that we only need to pass that flag when acls are enabled and the -service-namespace flag when acls are not enabled but namespaces are!
076c8aa
to
9edc827
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ashwin this looks extremely thorough and well thought out!! My biggest question was about logic on iterating through all the namespaces.
@@ -178,12 +178,15 @@ consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ | |||
{{- if .NamespaceMirroringEnabled }} | |||
{{- /* If namespace mirroring is enabled, the auth method is | |||
defined in the default namespace */}} | |||
-namespace="default" | |||
-auth-method-namespace="default" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, the flag names are great and helped me follow that we only need to pass that flag when acls are enabled and the -service-namespace flag when acls are not enabled but namespaces are!
9edc827
to
b4b2569
Compare
b4b2569
to
528cf97
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it, this looks great!
Tested it and it looks happy :)
One minor nit which is not blocking, maybe we could update the comments in the endpoints_controller_test_ent.go to have proper punctuation? I made a couple comments around it but there were quite a few and didn't want to be too nit picky. file it under "it would be nice"!
33d4f96
to
9536fe9
Compare
done 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking pretty good Ashwin!! Love all the endpoins controller tests you've added! I left some comments, the only one that's blocking I'd say is the command tests for the connect-init
command.
I've also created hashicorp/consul-helm#904 to run and enable acceptance tests and will wait for those to finish to make sure everything is passing!
// TestReconcileCreateEndpoint tests the logic to create service instances in Consul from the addresses in the Endpoints | ||
// object. The cases test an empty endpoints object, a basic endpoints object with one address, a basic endpoints object | ||
// with two addresses, and an endpoints object with every possible customization. | ||
// This test covers EndpointsController.createServiceRegistrations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we update this comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also we need to update comments for other tests 😀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol..tru!
LocalServiceAddress: "", | ||
LocalServicePort: 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a huge deal, but we can omit these since they are set to their default values.
subcommand/connect-init/command.go
Outdated
@@ -51,6 +53,8 @@ func (c *Command) init() { | |||
c.flagSet.StringVar(&c.flagACLAuthMethod, "acl-auth-method", "", "Name of the auth method to login to.") | |||
c.flagSet.StringVar(&c.flagPodName, "pod-name", "", "Name of the pod.") | |||
c.flagSet.StringVar(&c.flagPodNamespace, "pod-namespace", "", "Name of the pod namespace.") | |||
c.flagSet.StringVar(&c.flagAuthMethodNamespace, "auth-method-namespace", "", "Consul namespace the auth-method is defined in") | |||
c.flagSet.StringVar(&c.flagServiceNamespace, "service-namespace", "", "Consul destination namespace of the service.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now looking at it in combination with pod-namespace
I wonder if we need to disambiguate this and be explicit that this the Consul namespace? maybe consul-service-namespace
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pending Iryna's comments, it looks good to me. Great work on this!
9536fe9
to
077dd1b
Compare
- In order to support Consul namespaces, the controller now accepts values namespacesEnabled, consulDestinationNamespace, mirroringEnabled and mirroringPrefix which determine which Consul namespace a service and it's proxy should be registered in when created. This behavior is fairly straightforward when registering an endpoint but is a little tricker when de-registration is concerned. During de-registration, as we use a consul agent, we need to iterate through the list of all the namespaces in Consul that we register services against and create a Client that targets that namespace and agent to find services registered against the agent in a given namespace. - Additional changes here require the creating the ACL authmethod in the default namespace if namespace mirroring is configured and but in the destination namespace otherwise. This also requires us to explicitly specify the destination namespace of the service in order to query the agent for the service being available accurately. - Update the log format of the endpoints controller to be less verbose
077dd1b
to
8558eca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
I ran this code with acceptance tests too, and they all pass! Great work!
* Add Consul Enterprise Namespace support to endpoints controller - In order to support Consul namespaces, the controller now accepts values namespacesEnabled, consulDestinationNamespace, mirroringEnabled and mirroringPrefix which determine which Consul namespace a service and its proxy should be registered when created. This behavior is fairly straightforward when registering an endpoint but is a little trickier when de-registration is concerned. During de-registration, as we use a consul agent, we need to iterate through the list of all the namespaces in Consul that we register services against and create a Client that targets that namespace and agent to find services registered against the agent in a given namespace. - Additional changes here require creating the ACL auth-method in the default namespace if namespace mirroring is configured and but in the destination namespace otherwise. This also requires us to explicitly specify the destination namespace of the service in order to query the agent for the service being available accurately. - Update the log format of the endpoints controller to be less verbose
Changes proposed in this PR:
Register/Deregister services from the correct namespace.
Ensure we create the ACL for the service in the correct namespace when creating it and we query for the service in the namespace it gets created in during connect init.
Additionally, clean up logging in the endpoints controller to be less verbose
controller.endpoints-controller retrieved Kubernetes Endpoints {"endpoints": "consul-dns", "endpoints-namespace": "default"}
->controller.endpoints retrieved {"name": "consul-dns", "ns":"default"}
How I've tested this PR:
connect
acceptance tests against it and it went green.How I expect reviewers to test this PR:
Image for helm:
ashwinvenkatesh/consul-k8s@sha256:adfbb40eedb2943e3008d7f5bd57753784b94cf6a0d79c5fbe49dc843ebe1afd
Checklist: