-
Notifications
You must be signed in to change notification settings - Fork 326
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
Ignore services in endpoint controller using consul.hashicorp.com/service-ignore
#858
Conversation
consul.hashicorp.com/connect-ignore
consul.hashicorp.com/service-ignore
1745150
to
7a21d1f
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.
hey @t-eckert. I took a quick look and the approach looks sound. Left some comment inline.
2ae5ec2
to
970ff02
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 great @t-eckert, great work on this one.
I left a couple comments, 👍🏽
…oint to the same pod.
Co-authored-by: Kyle Schochenmaier <kschoche@gmail.com>
Co-authored-by: Kyle Schochenmaier <kschoche@gmail.com>
Co-authored-by: Kyle Schochenmaier <kschoche@gmail.com>
Co-authored-by: Kyle Schochenmaier <kschoche@gmail.com>
Co-authored-by: Iryna Shustava <ishustava@users.noreply.github.com>
Co-authored-by: Kyle Schochenmaier <kschoche@gmail.com>
8d55c60
to
d978d20
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 great Thomas!
@t-eckert is there a specific reason why this has to be set as a label? most/all helm charts support setting custom annotations through values but labels are sometimes not supported. Everything else in consul also uses annotations this seems to be the only case where a label is used instead. |
@@ -179,6 +179,12 @@ func (c *Command) Run(args []string) int { | |||
c.logger.Info("Check to ensure a Kubernetes service has been created for this application." + | |||
" If your pod is not starting also check the connect-inject deployment logs.") | |||
} | |||
if len(serviceList) > 2 { | |||
c.logger.Error("There are multiple Consul services registered for this pod when there must only be one." + |
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.
@t-eckert I am running multiple stateful sets in kubernets (kafka, zookeeper, elasticsearch, redis) and all of those have two services normal + headless but I dont get this error? shouldnt this fail the pod scheduling?
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.
Hmm, yes it should be failing scheduling if both the headless and non-headless services select the same pods. Maybe open up an issue about this.
Because we're reacting on changes to the Endpoint object which gets its label from the Service but it doesn't get annotations from the Service. Note that if you have ACLs enabled we're planning on automatically ignoring all Services that don't have the same name as the |
It that a good idea? If you run stateful sets the headless service is used by most client-to-server connections. Or is there a solution for this? Maybe you could first test that with the most common stateful sets like kafka, zookeeper, etc. What is recommendation, should stateful sets be used with the service mesh? I feel like the transparent proxy design does not consider stateful sets correctly, for example the issues mentioned in #932 |
Currently Consul only supports one service pointing at one set of pods. If you have two services that point at the same set of pods then none of the pods will start. In addition, when ACLs are enabled the RE statefulsets we definitely want to support them. We need to triage and get to your issue. |
ok thanks, that is definitely good to hear and of course this is not an easy problem to solve. The work you guys are doing is great and I really appreciate it |
* Pass either build URL or build number to terraform resources for GCP/AWS/Azure. On GKE, we can't pass build URL because / is not a valid character in labels * Remove scripting logic that makes tests fail early (we don't need it in nightly acceptance tests) * Fix metrics merging test on GKE. We are running tests there with enterprise license enabled, but since we're setting the consul image to OSS one, we should also unset enterprise license values so that we don't try to run the enterprise license commands against OSS binary, where they don't exist
Changes proposed in this PR:
consul.hashicorp.com/service-ignore: "true"
, it will not be registered by the endpoints controller. If is has been registered before, it will be deregistered.mapAddresses
function created as a refactor. I spun off reading this "subprocess" in theReconcile
method and got sidetracked. I think it makes sense to abstract over it. Feel free to disagree.How I've tested this PR:
How I expect reviewers to test this PR:
To test the behavior run the
install
script which will install Consul onto the cluster; create two services, one with theconnect-ignore
label; and create a pod which will come online.Here is a demo of the change, initially the services are deployed without the
connect-ignore
label on the "headless" service. The pod crashes asconsul-connect-inject-init
cannot register the service. When the label is added to the "headless" service, the pod comes up in a healthy state:Screen.Recording.2021-11-19.at.1.02.38.PM.mov
Checklist: