Skip to content

Integ tests: Use address of web UI as Proxy.PublicAddrs#20328

Merged
ravicious merged 5 commits intomasterfrom
ravicious/correct-public-addr
Jan 20, 2023
Merged

Integ tests: Use address of web UI as Proxy.PublicAddrs#20328
ravicious merged 5 commits intomasterfrom
ravicious/correct-public-addr

Conversation

@ravicious
Copy link
Copy Markdown
Member

@ravicious ravicious commented Jan 18, 2023

When pinging the proxy, TeleportClient takes the response and updates a couple of values based on that response. If proxySettings.SSH.PublicAddr is not empty, it tries to parse the address and then set it as tc.WebProxyAddr.

If it cannot parse the port number, it uses the default (3080).

What is getting returned as proxySettings.SSH.PublicAddr? That's determined by setProxyPublicAddressesSettings in lib/service/proxy_settings.go. It uses the first element from Proxy.PublicAddrs.

settings.SSH.PublicAddr = p.cfg.Proxy.PublicAddrs[0].String()

Now, the integration test helpers set up the Teleport instance in such way that the first Proxy.PublicAddrs is set to i.Hostname which resolves to just "localhost" in tests. So if any test pings the proxy first and then tries to make another request with the same TeleportClient, the subsequent request will try to reach out to localhost:3080. This happened to me when trying to add a new test (#20263).

This PR fixes this by making sure that the first element of Proxy.PublicAddrs actually points at the address of the web UI. i.Web is also used as tconf.Proxy.WebAddr.Addr a couple of lines below.

See also the message from Marek about backwards compatibility of SSHProxyHostPort.

i.Web is also used as tconf.Proxy.WebAddr.Addr a couple of lines below.

When pinging the proxy, TeleportClient takes the response and updates a
couple of values based on that response. If proxySettings.SSH.PublicAddr
is not empty, it tries to parse the address and then set it as
tc.WebProxyAddr. [1]

If it cannot parse the port number, it uses the default (3080).

What is getting returned as `proxySettings.SSH.PublicAddr`? That's determined
by setProxyPublicAddressesSettings in lib/service/proxy_settings.go. [2]
It uses the first element from Proxy.PublicAddrs.

Now, the integration test helpers set up the Teleport instance in such
way that the first Proxy.PublicAddrs is set to i.Hostname which resolves
to just "localhost" in tests. So if any test pings the proxy first and then
tries to make another request with the same TeleportClient, the subsequent
request will try to reach out to localhost:3080. This happened to me when
trying to add a new test. [3]

This PR fixes this by making sure that the first element of Proxy.PublicAddrs
actually points at the address of the web UI.

See also the message from Marek about backwards compatibility of
SSHProxyHostPort. [4] [5]

[1] https://github.com/gravitational/teleport/blob/885d7397ab3746154342712bec86bcdb3ea93eab/lib/client/api.go#L3666-L3673
[2] https://github.com/gravitational/teleport/blob/806a568ada7c640a64eb60f718e7d56be34049ad/lib/service/proxy_settings.go#L112
[3] #20263
[4] https://gravitational.slack.com/archives/C0DF0TPMY/p1673895327794379?thread_ts=1673891288.249809&cid=C0DF0TPMY
[5] https://github.com/gravitational/teleport/blob/db7fdff8097bcc883af8d9dde6a271d07e418550/api/client/webclient/webclient.go#L490
@ravicious
Copy link
Copy Markdown
Member Author

@espadolini I'd appreciate if you could take a look at this, it's a one line change in integration test setup which doesn't seem to break the tests. It would allow me to push a PR which adds a test that depends on this change.

@ravicious
Copy link
Copy Markdown
Member Author

Thanks!

@ravicious ravicious enabled auto-merge (squash) January 19, 2023 14:38
@github-actions
Copy link
Copy Markdown
Contributor

@ravicious See the table below for backport results.

Branch Result
branch/v11 Create PR
branch/v12 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants