Use common HTTPClientConfig for marathon_sd configuration by plaflamme · Pull Request #4009 · prometheus/prometheus

plaflamme · 2018-03-25T13:34:39Z

This adds support for basic authentication which closes #3090

The support for specifying the client timeout was removed as discussed in prometheus/common#123. Marathon was the only sd mechanism doing this and configuring the timeout is done through Context.

@brian-brazil @fabxc this is another take on adding support for basic auth to marathon SD through the use of the standard HTTPClientConfig as mentioned in #3238

brian-brazil · 2018-03-25T17:13:50Z

discovery/marathon/marathon.go

I don't think it's a good idea to override the behaviour of the common HTTP client, bearer token should mean bearer token. If there's a bespoke auth method that some SD uses I think it should have different configuration field names to avoid confusion.

Sure, that's reasonable. The DC/OS documentation uses the term "authentication token". So Perhaps auth_token / auth_token_file?

How should backwards compatibility be handled (because this is currently supported through the standard property names)?

no backwards compatibility, users will have to rename the configuration properties

support both the old and new property names and log a warning

?

How should backwards compatibility be handled (because this is currently supported through the standard property names)?

Is basic auth supported by Marathon natively? I didn't see it in the docs, so the things to do might be to leave things as-is.

Marathon itself only supports basic authentication as per its documentation here. Marathon deployed as part of DC/OS only supports token-based authentication (as you noticed in the other documentation).

So arguably, it is the token-based authentication that should be dropped from marathon_sd. I wouldn't suggest doing that, since that would break existing functionality and leave no workaround.

Another complication is that the DC/OS documentation says they will eventually support the Bearer authorization header. This would also be supported with this PR, but it also means that we cannot easily be backwards compatible and future compatible with a new DC/OS version.

Perhaps this PR is the best way to go forward?

supports basic auth for marathon itself

supports token-based authentication for DC/OS

supports future Bearer authentication for DC/OS

Existing DC/OS users will have to rename their configuration properties from bearer_token to auth_token when deploying the new version. Not doing so will simply fail authenticating...

That does seem like the best approach overall, even if it does involve a breaking change.

Sounds good.

Is there anything else required to move this forward? Should this log a warning of some sort when bearer_token is set? Should I also make the changes to the documentation?

I don't see a need for a warning. Docs should be updated in the same PR.

I've added 2 commits:

one that updates the documentation and updates configuration validation accordingly

another that updates CHANGELOG.md to document the breaking change

I'm not 100% sure what the process is around CHANGELOG.md, I can drop that commit if you prefer.

This adds support for basic authentication which closes prometheus#3090 The support for specifying the client timeout was removed as discussed in prometheus/common#123. Marathon was the only sd mechanism doing this and configuring the timeout is done through `Context`.

…cation. DC/OS uses a custom `Authorization` header for authenticating. This adds 2 new configuration properties to reflect this. Existing configuration files that use the bearer token will no longer work. More work is required to make this backwards compatible.

plaflamme · 2018-03-25T20:55:32Z

@brian-brazil I've added a commit to address your concern. Marathon SD now uses custom configuration properties to configure the token-based authentication mechanism.

I also adjusted the original commit which was broken and failing tests.

Please let me know how backwards compatibility should be handled.

brian-brazil · 2018-03-30T12:25:06Z

CHANGELOG.md

@@ -1,3 +1,8 @@
+## next release
+
+* [CHANGE] `marathon_sd`: (**breaking**) use `auth_token` and `auth_token_file` for token-based authentication instead of `bearer_token` and `bearer_token_file` respectively.


CHANGES by definition are breaking, so no need to have the breaking.

brian-brazil · 2018-03-30T12:25:21Z

CHANGELOG.md

+## next release
+
+* [CHANGE] `marathon_sd`: (**breaking**) use `auth_token` and `auth_token_file` for token-based authentication instead of `bearer_token` and `bearer_token_file` respectively.
+* [ENHANCEMENT] `marathon_sd`: adds support for basic authentication.


basic and bearer, plus all the other option the common http client provides

brian-brazil · 2018-03-30T12:28:02Z

discovery/marathon/marathon.go

 	}
-	if len(c.BearerToken) > 0 && len(c.BearerTokenFile) > 0 {
-		return fmt.Errorf("at most one of bearer_token & bearer_token_file must be configured")
+	if len(c.AuthToken) > 0 && len(c.AuthTokenFile) > 0 {


We should have unittests for these, see where we test bad configs already.

brian-brazil · 2018-03-30T12:28:57Z

discovery/marathon/marathon.go

-		bf, err := ioutil.ReadFile(conf.BearerTokenFile)
+	authToken := conf.AuthToken
+	if len(authToken) == 0 && len(conf.AuthTokenFile) > 0 {
+		b, err := ioutil.ReadFile(conf.AuthTokenFile)


This needs to be read on every request.

For this part, I did the same as what the http client does for the bearer_token_file configuration property here which doesn't seem to read the file on every request.

Perhaps I am mis-reading that code somehow? Or perhaps that change is in the works? I can certainly give it a stab if you feel strongly about supporting this now.

That's not right, https://github.com/prometheus/prometheus/pull/3709/files was meant to do that.

Oh, ok, I looked at the wrong http client. Alrighty, I'll take a stab at implementing this.

You're looking at the right one, it needs fixing.

I may need a bit of guidance on this one. The approach I'm considering is to create a AuthTokenRoundTripper and then use that to wrap the RoundTripper that httputil creates. But to put that new rt on the client, I see 2 approaches:

create client using NewClientFromConfig and then override its RoundTripper member

create NewHTTPRoundTripperFromConfig in httputil (which is basically NewClientFromConfig minus the call to newClient)

Mutating the client seems like a bad idea to me, but it makes the change entirely local to marathon.go which perhaps is what we want in this case, I'm not sure.

Perhaps there's an alternative approach that you might suggest?

Ok, I took the second approach in 2a0e760 and 904902e. Please let me know if that's okay.

brian-brazil · 2018-03-30T12:29:20Z

discovery/marathon/marathon.go

-	// According to  https://dcos.io/docs/1.8/administration/id-and-access-mgt/managing-authentication
+	// According to https://docs.mesosphere.com/1.11/security/oss/managing-authentication/
 	// DC/OS wants with "token=" a different Authorization header than implemented in httputil/client.go
 	// so we set this implicitly here


brian-brazil · 2018-03-30T12:29:46Z

docs/configuration/configuration.md

+
+# Sets the `Authorization` header on every request with the
+# configured username and password.
+# This is mutually exclusive from other authentication mechanisms.


s/from/with/

brian-brazil · 2018-03-30T12:30:58Z

docs/configuration/configuration.md

+
+# Sets the `Authorization` header on every request with
+# the configured bearer token. It is mutually exclusive with `bearer_token_file` and other authentication mechanisms.
+# NOTE: At this time, DC/OS marathon does not support standard Bearer token authentication. Use `auth_token` instead.


Mention a version number so this doesn't go stale

Also be more explicit about what new features are brought in by using the common HTTP client.

This delegates the handling of the `Authorization` header to the `http.RoundTripper`. This allows reading the `auth_token_file` on every request as well as isolating authentication in the http client itself (no need to pass the authentication token around).

plaflamme · 2018-04-02T14:28:40Z

The travis-ci build failure looks like a flake in the k8s tests. Is there anything that I should do about that?

brian-brazil · 2018-04-03T15:28:30Z

discovery/marathon/marathon.go

+	rt        http.RoundTripper
+}
+
+// newAuthTokenRoundTripper adds the provided auth token to a request


Comments should end with a full stop.

brian-brazil · 2018-04-04T13:32:45Z

You've got conflicts

Conflicts: discovery/marathon/marathon.go

plaflamme · 2018-04-04T13:50:48Z

@brian-brazil resolved

brian-brazil · 2018-04-05T08:08:25Z

Thanks!

…#4009) This adds support for basic authentication which closes prometheus#3090 The support for specifying the client timeout was removed as discussed in prometheus/common#123. Marathon was the only sd mechanism doing this and configuring the timeout is done through `Context`. DC/OS uses a custom `Authorization` header for authenticating. This adds 2 new configuration properties to reflect this. Existing configuration files that use the bearer token will no longer work. More work is required to make this backwards compatible.

brian-brazil reviewed Mar 25, 2018

View reviewed changes

plaflamme force-pushed the marathon-http-client branch from cb5729d to 0538ff4 Compare March 25, 2018 20:35

plaflamme added 2 commits March 25, 2018 16:43

plaflamme force-pushed the marathon-http-client branch from 0538ff4 to b90e216 Compare March 25, 2018 20:43

plaflamme added 2 commits March 28, 2018 16:11

Update documentation and add configuration validation

6e2cadb

Adds CHANGELOG entry for marathon_sd breaking changes

8a95775

brian-brazil reviewed Mar 30, 2018

View reviewed changes

plaflamme added 5 commits March 30, 2018 09:42

Changes are breaking by definition

ca7621e

Also be more explicit about what new features are brought in by using the common HTTP client.

Tidy up marathon_sd documentation

85bbb89

Adds tests for bad marathon configs

0b575b8

Expose NewRoundTripperFromConfig so clients can extend behaviour

2a0e760

brian-brazil reviewed Apr 3, 2018

View reviewed changes

Review feedback: comments should end with a full stop.

6f1cd1f

Merge remote-tracking branch 'origin/master' into marathon-http-client

915da47

Conflicts: discovery/marathon/marathon.go

brian-brazil merged commit 2aba238 into prometheus:master Apr 5, 2018

brian-brazil mentioned this pull request Apr 10, 2018

support marathon basic auth (Bearer) #4074

Closed

		@@ -1,3 +1,8 @@
		## next release

		* [CHANGE] `marathon_sd`: (breaking) use `auth_token` and `auth_token_file` for token-based authentication instead of `bearer_token` and `bearer_token_file` respectively.

Conversation

plaflamme commented Mar 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plaflamme commented Mar 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plaflamme Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plaflamme commented Apr 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brian-brazil commented Apr 4, 2018

Uh oh!

plaflamme commented Apr 4, 2018

Uh oh!

brian-brazil commented Apr 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

plaflamme Mar 30, 2018 •

edited

Loading