-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don’t downgrade SSH port forwarding in roles for clients 17.1.0 and above #50645
Conversation
263ad72
to
319a1e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but I don't really work on product changes, and I don't have a good understanding of the implication of these changes. Not sure why GH assigned me. Approving to unblock.
} | ||
if supported, err := utils.MinVerWithoutPreRelease( | ||
clientVersion.String(), | ||
minSupportedSSHPortForwardingVersion.String()); supported || err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current PR description states:
Don’t downgrade SSH port forwarding in roles for v18.0+
However, the minSupportedSSHPortForwardingVersion is actually set to Major: 17, Minor: 1, meaning the change ensures SSH port forwarding is not downgraded starting from version 17.1.0 and upwards, not just for v18.0+.
To make the description clearer and more accurate, could you update it to:
Don’t downgrade SSH port forwarding in roles for clients 17.1.0 and above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, thanks :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGMT when https://github.com/gravitational/teleport/pull/50645/files#r1906976966 will be addressed.
That's why I requested @eriktate to review, as he wrote this part. Also, assuming that the original idea used to be allowing granular options in many major versions, starting from different minor number in each, I asked around whether there are plans to support granular options in v16, and Zac said no. That's why I decided that "version >= 17.1" is a good final choice. |
Previously, v18.0+ clients got the port forwarding options downgraded.