Set up ClientStore when adding cluster in Connect#20263
Merged
Conversation
It keeps trying to Connect to proxy under port 3080 for some reason
gzdunek
reviewed
Jan 16, 2023
| cfg.WebProxyAddr = webProxyAddress | ||
| cfg.HomePath = s.Dir | ||
| cfg.KeysDir = s.Dir | ||
| cfg.ClientStore = client.NewFSClientStore(s.Dir) |
Contributor
There was a problem hiding this comment.
From what I see, client.NewClient(cfg) returns the configured ClientStore, so we could have
cfg.ClientStore = clusterClient.ClientStoreWouldn't it be better to use it?
Member
Author
There was a problem hiding this comment.
clusterClient doesn't exist at this point though. Ideally we wouldn't call SaveProfile on Config in the first place, I'm not sure why we're doing that – I left a comment to investigate this in the future.
Member
Author
There was a problem hiding this comment.
Ah, but I mean yeah, we could first create Config, then TeleportClient, then set the ClientStore on Config to that of TeleportClient.
It's a bit convoluted though IMHO. In case of ClientStore, the choice is between an in-memory implementation or an implementation that writes to disk. The whole reason we call SaveProfile is to save that to disk so that the subsequent calls to tshd see that cluster too. So I don't think it's bad that we're duplicating the logic a little bit here.
Contributor
There was a problem hiding this comment.
Yeah, I realized that it would create some sort of a circlar dependency between these functions :/
gzdunek
approved these changes
Jan 16, 2023
Member
Author
|
The integration test fails because the cluster set up in tests reports Since there's no port specified, we default to |
Member
Author
|
It might take me a while to get back to fixing the integration test, so for now I've removed everything but the immediate fix. If I don't manage to finish the integration test within an hour, I'll come back to it tomorrow or on Thursday, but I'd like to merge the fix in the meantime to fix broken master. |
3 tasks
Member
Author
|
@xacrimon @probakowski Ping, Connect is broken on master and I'd like to fix it with this one line change. |
xacrimon
approved these changes
Jan 17, 2023
probakowski
approved these changes
Jan 17, 2023
ravicious
added a commit
that referenced
this pull request
Jan 18, 2023
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
added a commit
that referenced
this pull request
Jan 18, 2023
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
added a commit
that referenced
this pull request
Jan 20, 2023
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
github-actions Bot
pushed a commit
that referenced
this pull request
Jan 20, 2023
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
github-actions Bot
pushed a commit
that referenced
this pull request
Jan 20, 2023
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
added a commit
that referenced
this pull request
Jan 20, 2023
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
added a commit
that referenced
this pull request
Jan 23, 2023
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
It looks like #19420 broke adding a new cluster in Connect:
Stacktrace
This PR fixes this by making sure that
ClientStoreis set up before callingSaveProfile.The existing integration tests didn't catch this issue because they largely skip the usual login procedure in favor of using test helpers to generate valid certs. The test user is created with a random password which we can't access.
I added another integration test which actually catches the issue with missingAdding the test turned out to be harder than expected, I created a separate issue for that (#20273) to tackle that in the coming days and clean up this part of the codebase as well.ClientStore.Unlike in tsh, logging in to a new cluster is Connect is broken into two steps. First you provide the URL of a proxy. Connect then pings the proxy and saves the profile to the home dir (
clusters.Storage.addCluster). At this point, the cluster is added to the list of clusters in the app but the user is not logged in yet – the actual login process happens in the next step.This is a remnant of times when logging out of a cluster and removing the cluster profile were two separate steps.