-
Notifications
You must be signed in to change notification settings - Fork 928
fix: cap max X11 forwarding ports and evict old #18561
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
Conversation
d6cba82
to
77f7937
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.
This is a good refactor, nice work 👍🏻. I'm however not entirely sold on the lru eviction policy since it's based on connection establishment time and not connection usage (i.e. we may end evicting a busy connection). (See related comment for additional context.)
77f7937
to
367d38c
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.
Raised two points inline and we should at least fix the nil check, but once that's fixed I think this is good to go, approved. 👍🏻
367d38c
to
4c219b3
Compare
Merge activity
|
partial for #18263
Caps the X11 forwarding sessions at a maximum port of 6200, and evicts the oldest session if we create new sessions while at the max.
Unit tests included higher in the stack.