Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(665)

Issue 97230043: cmd/juju: check use-proxy before resolving address

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 4 months ago by axw
Modified:
6 years, 4 months ago
Reviewers:
mp+218911, fwereade
Visibility:
Public.

Description

cmd/juju: check use-proxy before resolving address We were translating machin/unit IDs to internal addresses even if use-proxy=false. This manifested most prominently when attempting to juju ssh to a machine or unit in a 1.18 environment. Fixes lp:1313785 https://code.launchpad.net/~axwalk/juju-core/lp1313785-ssh-useproxy/+merge/218911 (do not edit description out of merge proposal)

Patch Set 1 #

Patch Set 2 : cmd/juju: check use-proxy before resolving address #

Total comments: 6

Patch Set 3 : cmd/juju: check use-proxy before resolving address #

Unified diffs Side-by-side diffs Delta from patch set Stats (+83 lines, -68 lines) Patch
A [revision details] View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/juju/debughooks_test.go View 1 2 4 chunks +32 lines, -24 lines 0 comments Download
M cmd/juju/scp.go View 1 2 1 chunk +5 lines, -10 lines 0 comments Download
M cmd/juju/scp_test.go View 1 2 chunks +10 lines, -2 lines 0 comments Download
M cmd/juju/ssh.go View 1 2 2 chunks +22 lines, -12 lines 0 comments Download
M cmd/juju/ssh_test.go View 1 2 4 chunks +12 lines, -20 lines 0 comments Download

Messages

Total messages: 4
axw
Please take a look.
6 years, 4 months ago (2014-05-09 05:22:47 UTC) #1
axw
Please take a look.
6 years, 4 months ago (2014-05-09 06:31:53 UTC) #2
fwereade
LGTM with minors https://codereview.appspot.com/97230043/diff/20001/cmd/juju/debughooks_test.go File cmd/juju/debughooks_test.go (right): https://codereview.appspot.com/97230043/diff/20001/cmd/juju/debughooks_test.go#newcode35 cmd/juju/debughooks_test.go:35: result: regexp.QuoteMeta(debugHooksArgs + "ubuntu@dummyenv-2.internal sudo /bin/bash ...
6 years, 4 months ago (2014-05-09 06:47:58 UTC) #3
axw
6 years, 4 months ago (2014-05-09 07:47:39 UTC) #4
Please take a look.

https://codereview.appspot.com/97230043/diff/20001/cmd/juju/debughooks_test.go
File cmd/juju/debughooks_test.go (right):

https://codereview.appspot.com/97230043/diff/20001/cmd/juju/debughooks_test.g...
cmd/juju/debughooks_test.go:35: result: regexp.QuoteMeta(debugHooksArgs +
"ubuntu@dummyenv-2.internal sudo /bin/bash -c 'F=$(mktemp); echo
IyEvYmluL2Jhc2gKKAojIExvY2sgdGhlIGp1anUtPHVuaXQ+LWRlYnVnIGxvY2tmaWxlLgpmbG9jayAtbiA4IHx8IChlY2hvICJGYWlsZWQgdG8gYWNxdWlyZSAvdG1wL2p1anUtdW5pdC1tb25nb2RiLTEtZGVidWctaG9va3M6IHVuaXQgaXMgYWxyZWFkeSBiZWluZyBkZWJ1Z2dlZCIgMj4mMTsgZXhpdCAxKQooCiMgQ2xvc2UgdGhlIGluaGVyaXRlZCBsb2NrIEZELCBvciB0bXV4IHdpbGwga2VlcCBpdCBvcGVuLgpleGVjIDg+Ji0KCiMgV3JpdGUgb3V0IHRoZSBkZWJ1Zy1ob29rcyBhcmdzLgplY2hvICJlMzBLIiB8IGJhc2U2NCAtZCA+IC90bXAvanVqdS11bml0LW1vbmdvZGItMS1kZWJ1Zy1ob29rcwoKIyBMb2NrIHRoZSBqdWp1LTx1bml0Pi1kZWJ1Zy1leGl0IGxvY2tmaWxlLgpmbG9jayAtbiA5IHx8IGV4aXQgMQoKIyBXYWl0IGZvciB0bXV4IHRvIGJlIGluc3RhbGxlZC4Kd2hpbGUgWyAhIC1mIC91c3IvYmluL3RtdXggXTsgZG8KICAgIHNsZWVwIDEKZG9uZQoKaWYgWyAhIC1mIH4vLnRtdXguY29uZiBdOyB0aGVuCiAgICAgICAgaWYgWyAtZiAvdXNyL3NoYXJlL2J5b2J1L3Byb2ZpbGVzL3RtdXggXTsgdGhlbgogICAgICAgICAgICAgICAgIyBVc2UgYnlvYnUvdG11eCBwcm9maWxlIGZvciBmYW1pbGlhciBrZXliaW5kaW5ncyBhbmQgYnJhbmRpbmcKICAgICAgICAgICAgICAgIGVjaG8gInNvdXJjZS1maWxlIC91c3Ivc2hhcmUvYnlvYnUvcHJvZmlsZXMvdG11eCIgPiB+Ly50bXV4LmNvbmYKICAgICAgICBlbHNlCiAgICAgICAgICAgICAgICAjIE90aGVyd2lzZSwgdXNlIHRoZSBsZWdhY3kganVqdS90bXV4IGNvbmZpZ3VyYXRpb24KICAgICAgICAgICAgICAgIGNhdCA+IH4vLnRtdXguY29uZiA8PEVORAogICAgICAgICAgICAgICAgCiMgU3RhdHVzIGJhcgpzZXQtb3B0aW9uIC1nIHN0YXR1cy1iZyBibGFjawpzZXQtb3B0aW9uIC1nIHN0YXR1cy1mZyB3aGl0ZQoKc2V0LXdpbmRvdy1vcHRpb24gLWcgd2luZG93LXN0YXR1cy1jdXJyZW50LWJnIHJlZApzZXQtd2luZG93LW9wdGlvbiAtZyB3aW5kb3ctc3RhdHVzLWN1cnJlbnQtYXR0ciBicmlnaHQKCnNldC1vcHRpb24gLWcgc3RhdHVzLXJpZ2h0ICcnCgojIFBhbmVzCnNldC1vcHRpb24gLWcgcGFuZS1ib3JkZXItZmcgd2hpdGUKc2V0LW9wdGlvbiAtZyBwYW5lLWFjdGl2ZS1ib3JkZXItZmcgd2hpdGUKCiMgTW9uaXRvciBhY3Rpdml0eSBvbiB3aW5kb3dzCnNldC13aW5kb3ctb3B0aW9uIC1nIG1vbml0b3ItYWN0aXZpdHkgb24KCiMgU2NyZWVuIGJpbmRpbmdzLCBzaW5jZSBwZW9wbGUgYXJlIG1vcmUgZmFtaWxpYXIgd2l0aCB0aGF0LgpzZXQtb3B0aW9uIC1nIHByZWZpeCBDLWEKYmluZCBDLWEgbGFzdC13aW5kb3cKYmluZCBhIHNlbmQta2V5IEMtYQoKYmluZCB8IHNwbGl0LXdpbmRvdyAtaApiaW5kIC0gc3BsaXQtd2luZG93IC12CgojIEZpeCBDVFJMLVBHVVAvUEdET1dOIGZvciB2aW0Kc2V0LXdpbmRvdy1vcHRpb24gLWcgeHRlcm0ta2V5cyBvbgoKIyBQcmV2ZW50IEVTQyBrZXkgZnJvbSBhZGRpbmcgZGVsYXkgYW5kIGJyZWFraW5nIFZpbSdzIEVTQyA+IGFycm93IGtleQpzZXQtb3B0aW9uIC1zIGVzY2FwZS10aW1lIDAKCkVORAogICAgICAgIGZpCmZpCgooCiAgICAjIENsb3NlIHRoZSBpbmhlcml0ZWQgbG9jayBGRCwgb3IgdG11eCB3aWxsIGtlZXAgaXQgb3Blbi4KICAgIGV4ZWMgOT4mLQogICAgZXhlYyB0bXV4IG5ldy1zZXNzaW9uIC1zIG1vbmdvZGIvMQopCikgOT4vdG1wL2p1anUtdW5pdC1tb25nb2RiLTEtZGVidWctaG9va3MtZXhpdAopIDg+L3RtcC9qdWp1LXVuaXQtbW9uZ29kYi0xLWRlYnVnLWhvb2tzCmV4aXQgJD8K
| base64 -d > $F; . $F'\n"),
On 2014/05/09 06:47:58, fwereade wrote:
> I kinda feel that one of these ought to still be using .dns -- wouldn't these
> carry on passing if we changed to always-use-internal?

Reverted these, and added a new test that explicitly tests --proxy=true.

https://codereview.appspot.com/97230043/diff/20001/cmd/juju/ssh.go
File cmd/juju/ssh.go (right):

https://codereview.appspot.com/97230043/diff/20001/cmd/juju/ssh.go#newcode138
cmd/juju/ssh.go:138: }
On 2014/05/09 06:47:58, fwereade wrote:
> can we maybe wrap this block up into a shared method and reuse it? I know it's
> not the *same* as in scp.go, but it's very similar -- a method taking (pty
bool)
> perhaps?

Done.

https://codereview.appspot.com/97230043/diff/20001/cmd/juju/ssh_test.go
File cmd/juju/ssh_test.go (right):

https://codereview.appspot.com/97230043/diff/20001/cmd/juju/ssh_test.go#newco...
cmd/juju/ssh_test.go:98: 
On 2014/05/09 06:47:58, fwereade wrote:
> ?

Deleted, remnant of deciphering test behaviour.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b