Skip to content
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

[ADDED] LeafNode min_version new option #3013

Merged
merged 3 commits into from
Apr 7, 2022
Merged

[ADDED] LeafNode min_version new option #3013

merged 3 commits into from
Apr 7, 2022

Conversation

kozlovic
Copy link
Member

@kozlovic kozlovic commented Apr 7, 2022

If set, a server configured to accept leafnode connections will
reject a remote server whose version is below that value. Note
that servers prior to v2.8.0 are not sending their version
in the CONNECT protocol, which means that anything below 2.8.0
would be rejected.

Configuration example:

leafnodes {
    port: 7422
    min_version: 2.8.0
}

The option is a string and can have the "v" prefix:

min_version: "v2.9.1"

Note that although suffix such as -beta would be accepted,
only the major, minor and update are used for the version comparison.

Signed-off-by: Ivan Kozlovic ivan@synadia.com

If set, a server configured to accept leafnode connections will
reject a remote server whose version is below that value. Note
that servers prior to v2.8.0 are not sending their version
in the CONNECT protocol, which means that anything below 2.8.0
would be rejected.

Configuration example:
```
leafnodes {
    port: 7422
    min_version: 2.8.0
}
```
The option is a string and can have the "v" prefix:
```
min_version: "v2.9.1"
```
Note that although suffix such as `-beta` would be accepted,
only the major, minor and update are used for the version comparison.

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Thanks.

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
@kozlovic
Copy link
Member Author

kozlovic commented Apr 7, 2022

@matthiashanel @philpennock @davidkemper Do you guys want to do a review or is Derek's LGTM enough and I can merge?

Copy link
Contributor

@matthiashanel matthiashanel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments. ok with logic in general

server/client.go Show resolved Hide resolved
server/leafnode.go Outdated Show resolved Hide resolved
server/opts.go Outdated Show resolved Hide resolved
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
@kozlovic
Copy link
Member Author

kozlovic commented Apr 7, 2022

@matthiashanel Implemented the requested changes. Have a second look when you have time.

Copy link
Contributor

@matthiashanel matthiashanel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kozlovic kozlovic merged commit 47776bd into main Apr 7, 2022
@kozlovic kozlovic deleted the ln_min_version branch April 7, 2022 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants