-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Fixed issue with duplicate containerPorts in different address families #82374
Fixed issue with duplicate containerPorts in different address families #82374
Conversation
Welcome @xcelsion! |
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Hi @xcelsion. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
33e070e
to
b342b87
Compare
b342b87
to
acd9dfc
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.
@zouyee can you share more about how this relates to https://github.com/kubernetes/kubernetes/pull/80470/files?
Confirming that both are necessary and that we shouldn't combine both into the same pr?
@xcelsion please sign the CLA when you get a chance :) lmk if there's anything I can do to help with that. |
@mattjmcnaughton, I'll get that done soonish. I had another thought about this PR, the way its set-up currently, a mapping in conflict with another mapping through the use of one 'any' and one family-specific, would both pass through. Should we be worried about that, or does the deduplication only concern unique names for rkt? If not, we need to find a way to handle that situation as well. |
On Fri, Sep 06, 2019 at 12:09:59PM -0700, Niels van Oosterom wrote:
@mattjmcnaughton, I'll get that done soonish. I had another thought about this PR, the way its set-up currently, a mapping in conflict with another mapping through the use of one 'any' and one family-specific, would both pass through. Should we be worried about that, or does the deduplication only concern unique names for rkt? If not, we need to find a way to handle that situation as well.
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#82374 (comment)
Good question, but tbh I'm not sure the answer :) Hopefully someone else can
weigh in!
|
I signed it |
Given that you can already circumvent the duplicate filter by assigning a unique name to the offending mappings, I lean towards the 'only relevant in the context of unique names for rkt' answer. If this is not the case then we should probably decouple the duplicate check from the unique name check. |
/ok-to-test The comment is now out-of-date - we need a portmapping name for the dockershim CNI driver (and rkt has been removed anyways). Can you please update it? It may be that this code is no longer used outside of the dockershim, in which case we should move it. |
@xcelsion Considering this PR is open for a several days now and that we have the upcoming deadline today, can you try to finish the review and the cherry-pick today? If not, we can't guarantee that it will be merged for 1.16 and we usually recommend to move the PR for a next patch release if it isn't urgent. |
How would I go about getting the review finished? As far as I understand it, my hands are tied until the reviewers @mattjmcnaughton, @sjenning, and @vishh have signed off on this, correct? |
@xcelsion Hi, right now I saw this labeled as |
@xcelsion 1.16 Bug triage team here, can you confirm that this can wait for 1.16.1 or that otherwise that it needs critical-urgent status? Deadline is EOD for 1.16.0 cherry picks. |
Given that there is a workaround available, I'd say it can wait until v1.16.1. Perhaps we could add it to a known issues list or something? Edit: I wouldn't have the time to get the cherry-pick out on time either way. |
cc @khenidak |
eff68ef
to
2e5fbf5
Compare
…s different address families
2e5fbf5
to
ef39312
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
/assign @Random-Liu |
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 have to admit I don't know how to proceed with this, I'm new to contributing on GitHub. Everything is good to go as far as I'm concerned. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: xcelsion, yujuhong The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Allows duplicate containerPorts when hostIPs of different address families are supplied.
Which issue(s) this PR fixes:
Fixes #82373
Special notes for your reviewer:
NONE
Does this PR introduce a user-facing change?:
NONE
/sig network