Skip to content
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

Update Consul API support to 1.2.0 #643

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jhsolor
Copy link

@jhsolor jhsolor commented Oct 8, 2018

Consul 1.0.7 was the last version to support the string Script API, which had been long deprecated. 1.1.0 and later use []string Args. This PR updates Consul support to this new property, bumps the Consul version to 1.2, and the Alpine version to 3.8.

Various docker/ dependencies are updated as well. The fsouza/go-dockerclient dependency is pinned to 1.2.2 for this change to work.

@jhsolor
Copy link
Author

jhsolor commented Oct 8, 2018

Hi @josegonzalez, can you please take a look at this PR and merge it if you accept the changes? It fixes a couple open issues.

@jhsolor
Copy link
Author

jhsolor commented Oct 8, 2018

Also a nudge @dsouzajude @mattatcha @progrium to see if we can get this merged. Thanks in advance!

VERSION Outdated Show resolved Hide resolved
This reverts commit a6021cd.
@jhsolor
Copy link
Author

jhsolor commented Oct 9, 2018

Per @josegonzalez and @Dzevka8 I have reverted the version bump commit

@timiTao
Copy link

timiTao commented Oct 10, 2018

We meet with the same problem.

Guys :) Would be nice to resolve this MR :) @josegonzalez @dsouzajude @mattatcha @progrium

@josegonzalez
Copy link
Member

@timiTao We'll get to it when we get to it. There isn't any need to ping anyone to do that. Thanks.

@siulcode
Copy link

I think we are also experiencing the same issue with SERVICE_CHECK_SCRIPT. Hopefully this PR will help...

banuchka added a commit to banuchka/registrator that referenced this pull request Jan 24, 2019
@marcdeop
Copy link

marcdeop commented Feb 5, 2019

@timiTao We'll get to it when we get to it. There isn't any need to ping anyone to do that. Thanks.

We know there is no need to ping anyone but at this point, after almost six months, maybe it is time to ask for an update on the subject?

Are there any plans to support this?

If there aren't, we need to seriously consider moving away from using this project as we are currently blocked on our upgrade to consul in our infrastructure and we cannot have a mix setup forever.

Regardless, thanks for the opensource project that you created :-)

@krunalatkaplan
Copy link

@josegonzalez We are blocked by the same issue. It would be great if we can get an update on the issue.

@hampsterx
Copy link

yeah I gave up and wrote my own version. sigh

https://github.com/hampsterx/ecs-consul-reg

you probably don't want to use it but works for us (using aws ECS). pretty simple..

@sschr4mm
Copy link

sschr4mm commented Apr 9, 2019

@josegonzalez We're assuming you won't support this and have begun investigating an alternative solution that may take another 6-8 months to implement.

Some clarification from your side would help us plan such a long-term alternative. Thank you and appreciate everything you all have done.

@pmundt
Copy link
Contributor

pmundt commented Sep 22, 2019

I've come up with a similar fix in #669 before noticing this PR - would be good to get one of these merged, as the current tree simply fails to build for me otherwise.

@nicofuccella
Copy link
Member

+1

@ghost
Copy link

ghost commented Nov 19, 2019

Hi @josegonzalez

I have upgraded to consol 1.6.1 and started noticing errors in registrator to do with SERVICE_443_CHECK_SCRIPT like :

Unexpected response code: 400 (Invalid check: TTL must be > 0 for TTL checks)

My services are not being registered as expected.

I tried upgrading Registrator by deploying the latest docker image from master branch but am still seeing the above error.

after pulling down the branch for this PR and compiling the Registrator container and deploying it I can now see the services being successfully registered in consul and I'm able to browse the endpoints :

2019/11/19 04:34:51 added: xxxxxxxxxxxx xxxxxxxxxxx:xxxxxxxxx:443

Would you be able to review this PR and if you have no further objections approve and merge it into the master branch?

I think there are quite a few people experiencing this issue, so it would be nice to have this PR merged.

Registrator is a great application, let's keep the dream alive!

@josegonzalez
Copy link
Member

@nicofuccella this is your baby, have fun! :D

@mattatcha
Copy link
Member

@nicofuccella this is your baby, have fun! :D

I just merged in another PR without conflicts that handles this :)

@ghost
Copy link

ghost commented Nov 19, 2019

hi @mattatcha I pulled latest master Registrator container this morning and deployed, I'm seeing the following error in Consul in the Health Checks tab :

fork/exec curl --silent --fail http://xxxxxxxxx:xxxxxxxx/xxxxxxx/HealthCheckService: no such file or directory

I'm setting the healthcheck on our web server container via environment env :

SERVICE_4443_CHECK_SCRIPT=curl --silent --fail http://xxxxxxxxxx:xxxxxxx/xxxxxxxxx/HealthCheckService

If I revert the Registrator container back to the one compiled from this PR branch the health checks go green again and I'm able to browse the endpoint.

Did a quick google of the error and came across this :

hashicorp/consul#3726 (comment)

the Args array is executed directly without a shell, so it can't execute that command line as a single argument. You could do something like:

"Args": ["sh", "-c", "curl localhost --max-time 3 >/dev/null 2>&1"]

Is Consul expecting an array but getting the check via the environment variable as a string perhaps?

Just for laughs I tried to pass it through env as an array but got same error :

fork/exec ['sh', '-c', 'curl --silent --fail http://xxxxxxxx:xxxxxx/xxxxx/HealthCheckService']: no such file or directory

Is passsing through as an environment variable not supported anymore?
Do I need to pass the check args through consul.json configuration?

@mattatcha
Copy link
Member

@alaverty would you be able to create a PR with the correct changes to consul/consul.go?

@ghost
Copy link

ghost commented Nov 20, 2019

@mattatcha I have branched from master and made the required changes to consul/consul.go
i have raised PR GH-670

I've compiled the PR branch and deployed it and can confirm the healthchecks are working as expected and healthy in consul.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.