Skip to content
This repository has been archived by the owner on Jun 20, 2024. It is now read-only.

add proxy-env and proxy-config which query weaveproxy for port #848

Merged
merged 1 commit into from
Jun 10, 2015

Conversation

paulbellamy
Copy link
Contributor

If the overall approach looks fine, I want to modify the tests to use this.

Closes #753

proxy-config)
proxy_addr "-H="
exit $?
;;

This comment was marked as abuse.

@rade
Copy link
Member

rade commented Jun 4, 2015

I said that ideally these commands would run w/o weavexec.

And why do we need to inspect the proxy container? We know what the port is; it's hard-coded in the script.

One reason I can see for both is the looming #805. So perhaps we should tackle that one first.

@paulbellamy
Copy link
Contributor Author

You later said that running it locally won't work due to #805.

I agree, that doing #805 first is a good plan.

@paulbellamy
Copy link
Contributor Author

The situation this doesn't handle, is if you have:

$ weave_on <remote_host> launch-proxy -L 127.0.0.1:12375
083896110ec5162b18dec9b833433812314a82a8be719642ff9effdb5d481425
$ weave proxy-env
export DOCKER_HOST=tcp://127.0.0.1:12375

Which won't work if you are not on the remote host. But aside from throwing an error, there isn't much we can do in that situation.

@paulbellamy paulbellamy force-pushed the proxy-env branch 2 times, most recently from ec69408 to 7a50aaf Compare June 10, 2015 12:09
@paulbellamy paulbellamy assigned rade and unassigned paulbellamy Jun 10, 2015

CMD="run -e WEAVE_CIDR=10.2.1.4/24 $BASE_IMAGE $CHECK_ETHWE_UP"
assert_raises "eval '$(weave_on $HOST1 proxy-env)' ; docker $CMD"
assert_raises "docker $(weave_on $HOST1 proxy-config) $CMD"

This comment was marked as abuse.

@rade
Copy link
Member

rade commented Jun 10, 2015

weave_on <remote_host> launch-proxy -L 127.0.0.1:12375

I don't understand this comment. What does -L have to do with anything? Did you mean -H?

Also, it seems to me that -H isn't handled at all, and that we are simply stashing and retrieving 12375 - in a very, very, complicated way!

@rade rade assigned paulbellamy and unassigned rade Jun 10, 2015
@paulbellamy
Copy link
Contributor Author

Yeah. Needs a bit of a rework now that #805 is done.

@rade
Copy link
Member

rade commented Jun 10, 2015

We could hold our noses and extract the listen address from the proxy's docker logs. Seems preferable to me to doing a lot of arg parsing and introducing bogus -p.

@@ -805,6 +811,18 @@ proxy_args() {
done
}

proxy_addr() {
if addr=$(docker logs weaveproxy | grep -oE "proxy listening on .*"); then

This comment was marked as abuse.

This comment was marked as abuse.

@@ -805,6 +811,18 @@ proxy_args() {
done
}

proxy_addr() {
if addr=$(docker logs weaveproxy | head -n3 | grep -oE "proxy listening on .*"); then

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@rade
Copy link
Member

rade commented Jun 10, 2015

docs need updating

globally adjust your `DOCKER_HOST` to point at the proxy, e.g.
All docker commands can be run via the proxy, so it is safe to use
`weave proxy-env` to globally adjust your `DOCKER_HOST` to point at
the proxy, e.g.

This comment was marked as abuse.

It's ugly, but beats arg-parsing, inspecting the container and piecing
the address together from exposed ports.

We could just use the host from DOCKER_HOST, but that will fail in the
case of a host with multiple IPs, where the proxy only listens on one.
@rade
Copy link
Member

rade commented Jun 10, 2015

done?

@paulbellamy paulbellamy removed their assignment Jun 10, 2015
@paulbellamy
Copy link
Contributor Author

Yes.

rade added a commit that referenced this pull request Jun 10, 2015
add proxy-env and proxy-config

Closes #753.
@rade rade merged commit afd0275 into weaveworks:master Jun 10, 2015
@rade rade modified the milestone: 1.0 Jun 10, 2015
@paulbellamy paulbellamy deleted the proxy-env branch June 10, 2015 14:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants