Allow user to specify URLs on the command line, and use that to allow per-target tokens. #1901

tomwilkie · 2016-09-30T19:29:02Z

tomwilkie · 2016-09-30T19:35:14Z

So currently you need to use this like this:

./scope launch http://user:<token>@frontend.dev.weave.works:80 https://user:<token>@cloud.weave.works:443

I feel like we should:

default to port 80/443 is http(s) is specified, and default to 4040 is just a hostname is used (to preserve existing behaviour)?
potentially treat the username as the token, so one can do https://<token>@cloud.weave.works?
make it so badly formed URLs cause scope launch to fail.

@2opremio WDYT?

2opremio · 2016-10-04T09:13:52Z

WDYT?

When were users introduced as part of the authentication? I thought that the probe only required a token.

Also, we should review, update and redeploy the Scope documentation/guides (and maybe blogposts) before merging this since it changes how we launch Scope.

default to port 80/443 is http(s) is specified, and default to 4040 is just a hostname is used (to preserve existing behaviour)?

Uhm, that's a tricky one. I would only default 80/443 if omitted when providing the scheme. Defaulting 4040 when you are providing the scheme feels too magical to me.

If you want to preserve the current behaviour then I would propose to omit the scheme, default to 4040 if the port is omitted and force using 80/443(https) explicitly. This is what I believe we do right now.

potentially treat the username as the token, so one can do https://@cloud.weave.works?

I still don't fully understand the role of users when pushing reports from probes. Users have their own token now?

tomwilkie · 2016-10-04T16:42:21Z

When were users introduced as part of the authentication?

They are not, the user part of the URL is ignored.

changes how we launch Scope.

This is backwards compatible - if you don't specify a password in the URL, the one from --service-token is used.

I would only default 80/443 if omitted when providing the scheme.

Sorry if it wasn't clear - that was the suggestion. Looks like we agree!

I still don't fully understand the role of users when pushing reports from probes. Users have their own token now?

As an aside, there are user tokens now, but this is not what I'm using here (and I don't propose we do, as it makes it harder)

2opremio · 2016-10-04T18:58:41Z

This is backwards compatible - if you don't specify a password in the URL, the one from --service-token is used.

Not when using explicit targets, at least if we don't default to 4040 when not providing the scheme (which would be a bit odd).

They are not, the user part of the URL is ignored.

Then let's use the user part of the URL as a token instead of the password.

Sorry if it wasn't clear - that was the suggestion. Looks like we agree!

Great :)

tomwilkie · 2016-10-04T19:05:01Z

Not when using explicit targets, at least if we don't default to 4040 when not providing the scheme (which would be a bit odd).

I'm confused - how about if we:

default to port 4040 when no scheme AND no port is specified
if a port is specified always use that
if a scheme is specified AND no port is specified, use 80/443 as appropriate

That should be backwards compatible, right? Can you give an example where it isn't?

2opremio · 2016-10-04T22:23:52Z

It would, but defaulting to different ports based on whether the scheme is
provided or not seems a bit unintuitive to me.

However, I don't see a better solution, so go ahead.

On Oct 4, 2016 21:07, "Tom Wilkie" notifications@github.com wrote:

Not when using explicit targets, at least if we don't default to 4040 when
not providing the scheme (which would be a bit odd).

I'm confused - how about if we:

default to port 4040 when no scheme AND no port is specified

if a port is specified always use that

if a scheme is specified AND no port is specified, use 80/443 as
appropriate

That should be backwards compatible, right? Can you give an example where
it isn't?

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#1901 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACQOJPcukcvS_nfFSIwjmnqagQwM5Nzhks5qwqNfgaJpZM4KLbpF
.

tomwilkie · 2016-10-04T23:20:24Z

Okay with latest changes user can now do:

./scope launch http://<token>@frontend.dev.weave.works https://<token>@cloud.weave.works

Which is much cleaner. Should be good to merge now?

2opremio · 2016-10-05T17:12:23Z

LGTM, please squash.

app/controls_test.go

@@ -43,7 +44,7 @@ func TestControl(t *testing.T) {
 			Value: "foo",
 		}
 	})
-	client, err := appclient.NewAppClient(probeConfig, ip+":"+port, ip+":"+port, controlHandler)
+	client, err := appclient.NewAppClient(probeConfig, ip+":"+port, url.URL{Scheme: "http", Host: ip + ":" + port}, controlHandler)


app/pipes_internal_test.go

@@ -83,7 +84,7 @@ func TestPipeClose(t *testing.T) {
 	probeConfig := appclient.ProbeConfig{
 		ProbeID: "foo",
 	}
-	client, err := appclient.NewAppClient(probeConfig, ip+":"+port, ip+":"+port, nil)
+	client, err := appclient.NewAppClient(probeConfig, ip+":"+port, url.URL{Scheme: "http", Host: ip + ":" + port}, nil)


… per-target tokens. Also: - Parse targets on startup and catch badly formed ones before Scope can start. - If no port is specified, use default port for scheme; if no scheme is specificed, use 4040. - Use username as probe token

tomwilkie assigned 2opremio Sep 30, 2016

tomwilkie changed the title ~~Allow user to specify URLs on the command line, and use that to allow per-target tokens.~~ [WIP] Allow user to specify URLs on the command line, and use that to allow per-target tokens. Sep 30, 2016

tomwilkie changed the title ~~[WIP] Allow user to specify URLs on the command line, and use that to allow per-target tokens.~~ Allow user to specify URLs on the command line, and use that to allow per-target tokens. Oct 3, 2016

2opremio reviewed Oct 5, 2016

View reviewed changes

tomwilkie force-pushed the 1696-per-target-token branch from de5890a to 2ea2c4a Compare October 5, 2016 17:33

tomwilkie merged commit 2a00fd2 into master Oct 5, 2016

tomwilkie deleted the 1696-per-target-token branch October 5, 2016 17:59

2opremio mentioned this pull request Mar 21, 2017

Elide probe tokens from urls in logs #2365

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow user to specify URLs on the command line, and use that to allow per-target tokens. #1901

Allow user to specify URLs on the command line, and use that to allow per-target tokens. #1901

tomwilkie commented Sep 30, 2016

tomwilkie commented Sep 30, 2016 •

edited

Loading

2opremio commented Oct 4, 2016

tomwilkie commented Oct 4, 2016

2opremio commented Oct 4, 2016

tomwilkie commented Oct 4, 2016

2opremio commented Oct 4, 2016

tomwilkie commented Oct 4, 2016

2opremio commented Oct 5, 2016

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

Allow user to specify URLs on the command line, and use that to allow per-target tokens. #1901

Allow user to specify URLs on the command line, and use that to allow per-target tokens. #1901

Conversation

tomwilkie commented Sep 30, 2016

tomwilkie commented Sep 30, 2016 • edited Loading

2opremio commented Oct 4, 2016

tomwilkie commented Oct 4, 2016

2opremio commented Oct 4, 2016

tomwilkie commented Oct 4, 2016

2opremio commented Oct 4, 2016

tomwilkie commented Oct 4, 2016

2opremio commented Oct 5, 2016

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

tomwilkie commented Sep 30, 2016 •

edited

Loading