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.
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
fix: Add used port check for publish flag #2190
base: main
Are you sure you want to change the base?
fix: Add used port check for publish flag #2190
Changes from all commits
afd1cee
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
may be adding a case for
SCTP
to controller support/behavior ofSCTP
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.
Added.
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.
It appears that the SCTP protocol is currently unsupported, therefore no further processing is required.
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.
I could be wrong but from this line https://github.com/containerd/nerdctl/blob/main/pkg/portutil/portutil.go#L61, it seems like users could use
sctp
. There is also a test to check the parsing ofsctp
https://github.com/containerd/nerdctl/blob/main/pkg/portutil/portutil_test.go#L348.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.
I only checkd in https://github.com/containerd/nerdctl/blob/main/pkg/portutil/procnet/procnet_linux.go#L38 , Let me check it.
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.
I carefully checked the code and did not find an implementation that supports the SCTP protocol. The line just supports parsing command line arguments.
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.
Additionally there a unit test that checks the parsing of "sctp" https://github.com/containerd/nerdctl/blob/main/pkg/portutil/portutil_test.go#L348, so without the protocol check below, the unit test fails.
As a side note, if sctp is not supported then why do we support parsing it in the ParseFlagP ?
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.
Yep..., let it works
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.
I quickly checked, it looks like
SCTP
is not supported but it is but it is not blocking either. IMHO, we have to add a comment here to highlight this point ( It's misleading without a comment)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.
Updated.