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

fix ping request in healthCheckNode #283

Merged
merged 3 commits into from
May 19, 2017

Conversation

m3co-code
Copy link
Contributor

I was manually testing marathon member failover and stumbled over a problem with the current implementation of healthCheckNode. It builds a request like http://127.0.0.1:57142//ping. Testing with a Marathon cluster of version 1.3.10 I think this is a real problem.

I checked the test cases and saw there are actually tests. The problem why this is not detected by the tests is that go cleans up such paths by default. See StackOverflow for some explanation. Also due to this sanitisation I was not able to come up with a good regression test :/

@timoreimann
Copy link
Collaborator

@marco-jantke since we can't easily write a regression test, how about doing one or more of the following changes to buildMarathonRequest:

  1. Add a GoDoc explaining that uri must not have a leading slash.
  2. Have the function strip leading slashes off of the uri parameter.
  3. Have the function panic if uri contains leading slashes.

Personally, I'm in favor of option 3 since we seem to be in control of what we pass to the function, so having the leading slash would be a programming error of ours. Thus, the panic would serve as an invariant validation.

Besides, how about renaming uri to path? It's not really a URI, after all.

WDYT?

@m3co-code
Copy link
Contributor Author

I did now steps 1 and 3, I also think it makes sense to directly panic as the invocation is completely in the hand of the library. I also checked all other invocations to my best knowledge and nothing else was using a leading slash for the path. Tests and running examples push my confidence in this.

Also I renamed uri to path in the codebase, where it made sense. I started only at the buildMarathonRequest, but saw that the usage of uri was ambiguous throughout the codebase. What I did not change is the methods.yml and of course no public interface was changed. I made it in an extra commit, so just let me know if I went too far :D

Copy link
Collaborator

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

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

Excellent, looking good to me. 👍

Thanks for making go-marathon a better place. :-)

@timoreimann timoreimann merged commit 15ea23e into gambol99:master May 19, 2017
ldez pushed a commit to timoreimann/traefik that referenced this pull request May 22, 2017
Our vendored copy contains a bug that causes unavailable Marathon nodes
to never be marked as available again due to a misconstruction in the
URL to the Marathon health check / ping endpoint used by go-marathon
internally.

A fix[1] has been published.

[1]gambol99/go-marathon#283
timoreimann added a commit to traefik/traefik that referenced this pull request May 22, 2017
Our vendored copy contains a bug that causes unavailable Marathon nodes
to never be marked as available again due to a misconstruction in the
URL to the Marathon health check / ping endpoint used by go-marathon
internally.

A fix[1] has been published.

[1]gambol99/go-marathon#283
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.

2 participants