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

Bugfix/consul service reg fails tls #166

Merged
merged 17 commits into from
Oct 2, 2018

Conversation

adidanes
Copy link
Contributor

@adidanes adidanes commented Sep 18, 2018

Pull Request description

Fix Issue #153

Description of the change

Update Consul to last available version (1.2.3) to take advantage of this Consul bug fix.

Replace TCP checks by HTTP checks for Yorc service

What I did

  • Updated Consul version to 1.2.3
  • Created a Health endpoint in the Yorc REST API
  • Replaced the TCP check by a HTTP check that calls the Health endpoint
  • Updated Yorc documentation

How to verify it

  • Set-up a secured Yorc cluster (secured Consul cluster and secured Yorc instances). If you configure Yorc with ssl_verify set to true, you need to set enable_agent_tls_for_checks to true in the Consul client configuration :
{
  "domain": "yorc",
  "data_dir": "/tmp/consul",
  "client_addr": "0.0.0.0",
  "advertise_addr": "10.197.138.xxx",
  "retry_join": [ "10.197.138.yyy" ],
  "ports": {
     "https": 8543
  },
  "ca_file": "/etc/yorc/ca.pem",
  "key_file": "/etc/yorc/consul-client.key",
  "cert_file": "/etc/yorc/consul-client.pem",
  "enable_agent_tls_for_checks": true,
  "verify_outgoing": true,
  "verify_incoming_rpc": true,
  "ui": true
}
YORC http https ssl_verify
Consul http ok ok KO
Consul https ok ok ok if enable_agent_tls_for_checks

Description for the changelog

  1. Updated Consul version to 1.2.3
  2. Created a Health endpoint in the Yorc REST API
  3. Replaced the TCP check by a HTTP check that calls the Health endpoint
  4. Update Yorc Docker file to use updated version of Consul in the generated docker image
  5. Updated Yorc documentation

Applicable Issues

Fixes #153

Copy link
Member

@loicalbertin loicalbertin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also update Consul Go API in vendor dir by running

govendor fetch github.com/hashicorp/consul/...@v1.2.3

@adidanes adidanes requested review from loicalbertin and removed request for stefbenoist and laurentganne September 24, 2018 12:39
@codecov
Copy link

codecov bot commented Sep 24, 2018

Codecov Report

Merging #166 into develop will increase coverage by 0.68%.
The diff coverage is 25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop    #166      +/-   ##
==========================================
+ Coverage    42.42%   43.1%   +0.68%     
==========================================
  Files          134     135       +1     
  Lines        12685   12857     +172     
==========================================
+ Hits          5381    5542     +161     
- Misses        6476    6485       +9     
- Partials       828     830       +2
Impacted Files Coverage Δ
rest/structs.go 14.28% <ø> (ø) ⬆️
rest/health.go 0% <0%> (ø)
rest/http.go 58.26% <100%> (+0.33%) ⬆️
helper/stringutil/stringutil.go 84.37% <0%> (-15.63%) ⬇️
prov/scheduling/scheduler/scheduler.go 75.22% <0%> (ø) ⬆️
prov/monitoring/monitoring_mgr.go 64.07% <0%> (+1.11%) ⬆️
config/config.go 82.97% <0%> (+3.31%) ⬆️
commands/server.go 86.84% <0%> (+5.24%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f161e3...a5965da. Read the comment docs.

@adidanes
Copy link
Contributor Author

adidanes commented Sep 24, 2018

What's your feeling about using a specific constant of PubMaxRoutines only for tests: https://github.com/ystia/yorc/blob/bugfix/consul_service_reg_fails_tls/testutil/helper.go#L29

With 200 the tests are passing. I didn't test with a bigger value yet...

@loicalbertin
Copy link
Member

What's your feeling about using a specific constant of PubMaxRoutines only for tests: https://github.com/ystia/yorc/blob/bugfix/consul_service_reg_fails_tls/testutil/helper.go#L29

With 200 the tests are passing. I didn't test with a bigger value yet...

I feel not confortable by doing this as if it could fail in tests then it will eventually fail one day in production. so we ever have to reduce the default value even for the main program or investigate why we have such issues.

That makes me think that the issue may come from the way we get the client in tests we do not tweak its configuration as we do in the main prog. I have to check this.

Use the same method than in  a normal server by
tweaking Consul http client configuration
Copy link
Member

@loicalbertin loicalbertin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good but still have some interrogation on documentation

doc/secured.rst Show resolved Hide resolved
doc/secured.rst Show resolved Hide resolved
That will become errors in a future Docker releases
@stefbenoist
Copy link
Contributor

Consul Health checks work well in secured mode. I agree with Loic, we need our tests to stay closer to real-life.

@adidanes adidanes merged commit e0322b5 into develop Oct 2, 2018
@loicalbertin loicalbertin deleted the bugfix/consul_service_reg_fails_tls branch October 2, 2018 16:28
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.

Yorc consul service registration fails if using TLS
3 participants