Create remote site cache based on remote auth version #12130

rosstimothy · 2022-04-20T20:38:11Z

The cache policy used for a remote site is determined based on
the response from a version request. However the version response
was only returning the proxy version. If the remote site was not
running the same version for both auth and proxy, then the cache
policy chosen could be invalid.

The reverse tunnel agent now pings its auth server and reports
both the auth and proxy version in response to a version request.
To maintain backward compatibility the reverse tunnel server will
fallback to using the proxy version if the response does not
contain an auth version.

Fixes #12010

smallinsky

Left some comments.
@jakule Could you also take a look ?

lib/reversetunnel/agent.go

lib/reversetunnel/srv.go

jakule · 2022-04-21T16:18:50Z

@rosstimothy I was using this logic in my other PR, but this PR won't make a huge difference #12040.
But there is other thing. I assume that this PR will be backported to v9. The function checks if the cluster that you're connecting to is before v8. Technically we allow only one version different, so if this PR makes only to v9, then the only cluster that you can connect to older than v9 is v8. From what I understand this is not supported scenario in our case? Correct me if I'm wrong.

lib/reversetunnel/srv.go

lib/reversetunnel/agent.go

rosstimothy · 2022-04-22T13:32:45Z

@rosstimothy I was using this logic in my other PR, but this PR won't make a huge difference #12040. But there is other thing. I assume that this PR will be backported to v9. The function checks if the cluster that you're connecting to is before v8. Technically we allow only one version different, so if this PR makes only to v9, then the only cluster that you can connect to older than v9 is v8. From what I understand this is not supported scenario in our case? Correct me if I'm wrong.

This PR needs to be backported as far as possible. It was actually found by a customer upgrading from v6 -> v7. I don't think any of the changes introduced here really change the logic in which clusters you can connect to. It simply changes which service in the remote cluster is used to determine its version. Using the agent version is incorrect both for determining the cache policy, and for the new use case that you added in #12040. Since the cache is generated by connecting to the auth server, and the remote watchers are all created from the cache we need to know the auth version. Things only work as is if the auth version and agent version are the same.

jakule · 2022-04-22T17:52:55Z

@rosstimothy I was using this logic in my other PR, but this PR won't make a huge difference #12040. But there is other thing. I assume that this PR will be backported to v9. The function checks if the cluster that you're connecting to is before v8. Technically we allow only one version different, so if this PR makes only to v9, then the only cluster that you can connect to older than v9 is v8. From what I understand this is not supported scenario in our case? Correct me if I'm wrong.

This PR needs to be backported as far as possible. It was actually found by a customer upgrading from v6 -> v7. I don't think any of the changes introduced here really change the logic in which clusters you can connect to. It simply changes which service in the remote cluster is used to determine its version. Using the agent version is incorrect both for determining the cache policy, and for the new use case that you added in #12040. Since the cache is generated by connecting to the auth server, and the remote watchers are all created from the cache we need to know the auth version. Things only work as is if the auth version and agent version are the same.

In this case can we rename the function isPreV8Cluster() to something like isOldCachePolicy()? I was under the impression that this function is only needed in v9 cluster, but from what I understand is not true.

rosstimothy · 2022-04-22T17:59:32Z

@rosstimothy I was using this logic in my other PR, but this PR won't make a huge difference #12040. But there is other thing. I assume that this PR will be backported to v9. The function checks if the cluster that you're connecting to is before v8. Technically we allow only one version different, so if this PR makes only to v9, then the only cluster that you can connect to older than v9 is v8. From what I understand this is not supported scenario in our case? Correct me if I'm wrong.

This PR needs to be backported as far as possible. It was actually found by a customer upgrading from v6 -> v7. I don't think any of the changes introduced here really change the logic in which clusters you can connect to. It simply changes which service in the remote cluster is used to determine its version. Using the agent version is incorrect both for determining the cache policy, and for the new use case that you added in #12040. Since the cache is generated by connecting to the auth server, and the remote watchers are all created from the cache we need to know the auth version. Things only work as is if the auth version and agent version are the same.

In this case can we rename the function isPreV8Cluster() to something like isOldCachePolicy()? I was under the impression that this function is only needed in v9 cluster, but from what I understand is not true.

Yeah this could probably be named better. I believe it was initially added in v7 due to the need to support resource changes in the backend which introduced the two different remote cache policies. It looks like over the course of newer versions the name isPreXCluster has just kept incrementing the version number:

teleport/lib/reversetunnel/srv.go

Line 1069 in ec82941

ok, err := isPreV7Cluster(closeContext, sconn)

The cache policy used for a remote site is determined based on the response from a version request. However the version response was only returning the proxy version. If the remote site was not running the same version for both auth and proxy, then the cache policy chosen could be invalid. The reverse tunnel agent now pings its auth server and reports both the auth and proxy version in response to a version request. To maintain backward compatability the reverse tunnel server will fallback to using the proxy version if the response does not contain an auth version. Fixes #12010

… version

smallinsky · 2022-04-26T08:16:12Z

lib/reversetunnel/agent.go

 				if err != nil {
-					log.Debugf("Failed to reply to %v request: %v.", r.Type, err)
+					a.log.WithError(err).Debugf("Failed to ping auth server.")


Can we increase severity level to INFO ?
In case where the ping call failed it hole addRemoteCluster operation fails so it would be nice to have insight about this.

smallinsky · 2022-04-26T08:17:29Z

lib/reversetunnel/agent.go

-					log.Debugf("Failed to reply to %v request: %v.", r.Type, err)
+					a.log.WithError(err).Debugf("Failed to ping auth server.")
+					if err := r.Reply(false, []byte("Failed to retrieve auth version")); err != nil {
+						a.log.Debugf("Failed to reply to %version request: %v.", err)


Suggested change

a.log.Debugf("Failed to reply to %version request: %v.", err)

a.log.Debugf("Failed to reply to %v request: %v.", r.Type, err)

* Create remote site cache based on remote auth version The cache policy used for a remote site is determined based on the response from a version request. However the version response was only returning the proxy version. If the remote site was not running the same version for both auth and proxy, then the cache policy chosen could be invalid. The reverse tunnel agent now pings its auth server and reports both the auth version in response to a version request. Fixes #12010 (cherry picked from commit 4f2ad1f) # Conflicts: # lib/reversetunnel/srv.go

rosstimothy · 2022-04-26T19:52:00Z

💚 All backports created successfully

Status	Branch	Result
✅	branch/v7
✅	branch/v8
✅	branch/v9

Questions ?

Please refer to the Backport tool documentation

* Create remote site cache based on remote auth version The cache policy used for a remote site is determined based on the response from a version request. However the version response was only returning the proxy version. If the remote site was not running the same version for both auth and proxy, then the cache policy chosen could be invalid. The reverse tunnel agent now pings its auth server and reports both the auth version in response to a version request. Fixes #12010 (cherry picked from commit 4f2ad1f)

* Create remote site cache based on remote auth version The cache policy used for a remote site is determined based on the response from a version request. However the version response was only returning the proxy version. If the remote site was not running the same version for both auth and proxy, then the cache policy chosen could be invalid. The reverse tunnel agent now pings its auth server and reports both the auth version in response to a version request. Fixes #12010 (cherry picked from commit 4f2ad1f) # Conflicts: # lib/reversetunnel/srv.go

* Create remote site cache based on remote auth version The cache policy used for a remote site is determined based on the response from a version request. However the version response was only returning the proxy version. If the remote site was not running the same version for both auth and proxy, then the cache policy chosen could be invalid. The reverse tunnel agent now pings its auth server and reports both the auth version in response to a version request. Fixes #12010 (cherry picked from commit 4f2ad1f)

github-actions bot requested review from marcoandredinis and smallinsky April 20, 2022 20:38

smallinsky reviewed Apr 21, 2022

View reviewed changes

lib/reversetunnel/agent.go Outdated Show resolved Hide resolved

lib/reversetunnel/srv.go Outdated Show resolved Hide resolved

smallinsky requested a review from jakule April 21, 2022 09:02

jakule reviewed Apr 21, 2022

View reviewed changes

lib/reversetunnel/srv.go Outdated Show resolved Hide resolved

lib/reversetunnel/agent.go Outdated Show resolved Hide resolved

rosstimothy force-pushed the tross/remote_version branch from 134451a to fbd3278 Compare April 25, 2022 13:53

rosstimothy requested review from jakule and smallinsky April 25, 2022 13:53

jakule approved these changes Apr 25, 2022

View reviewed changes

rosstimothy added 3 commits April 25, 2022 13:49

remove versionResponse in favor of returning either the agent or auth…

d12095f

… version

refactor remote access point creation

ac35e46

rosstimothy force-pushed the tross/remote_version branch from ff7d0cb to ac35e46 Compare April 25, 2022 17:49

smallinsky approved these changes Apr 26, 2022

View reviewed changes

rosstimothy and others added 2 commits April 26, 2022 08:29

bump failed ping message and make it more informative

c5bcb40

Merge branch 'master' into tross/remote_version

78c56fa

rosstimothy merged commit 4f2ad1f into master Apr 26, 2022

rosstimothy deleted the tross/remote_version branch April 26, 2022 14:39

rosstimothy mentioned this pull request Apr 26, 2022

[branch/v7] Create remote site cache based on remote auth version (#12130) #12249

Merged

rosstimothy mentioned this pull request Apr 26, 2022

[branch/v8] Create remote site cache based on remote auth version (#12130) #12250

Merged

rosstimothy mentioned this pull request Apr 26, 2022

[branch/v9] Create remote site cache based on remote auth version (#12130) #12251

Merged

webvictim mentioned this pull request Jun 8, 2022

Opened in error #13285

Closed

alexatcanva mentioned this pull request Oct 13, 2022

BUGFIX | Fix Teleport ALPN Proxy not being HTTP CONNECT Proxy Aware alexatcanva/teleport#30

Open

marcoandredinis removed their request for review March 24, 2023 09:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create remote site cache based on remote auth version #12130

Create remote site cache based on remote auth version #12130

rosstimothy commented Apr 20, 2022

smallinsky left a comment

jakule commented Apr 21, 2022

rosstimothy commented Apr 22, 2022

jakule commented Apr 22, 2022

rosstimothy commented Apr 22, 2022

smallinsky Apr 26, 2022

smallinsky Apr 26, 2022

rosstimothy commented Apr 26, 2022

	a.log.Debugf("Failed to reply to %version request: %v.", err)
	a.log.Debugf("Failed to reply to %v request: %v.", r.Type, err)

Create remote site cache based on remote auth version #12130

Create remote site cache based on remote auth version #12130

Conversation

rosstimothy commented Apr 20, 2022

smallinsky left a comment

Choose a reason for hiding this comment

jakule commented Apr 21, 2022

rosstimothy commented Apr 22, 2022

jakule commented Apr 22, 2022

rosstimothy commented Apr 22, 2022

smallinsky Apr 26, 2022

Choose a reason for hiding this comment

smallinsky Apr 26, 2022

Choose a reason for hiding this comment

rosstimothy commented Apr 26, 2022

💚 All backports created successfully

Questions ?