-
Notifications
You must be signed in to change notification settings - Fork 879
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
Assigning Address driver options #707
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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 was thinking of an
InterfaceIpamConf
structure so that it is easier to add more fields in future. But I cannot think of any other fields, besides the address and options.Therefore I think It is better your way, less boilerplate code.
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.
Yea that makes sense. I have no problem changing it to that if others feel that it is needed now.
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 seems that only one IP address can be passed. Would it be possible to have multiple per end point ?
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.
Thanks for the comment. Currently we do not support that, but I think only few changes would make it possible.
Just curious about the use case, is this for IP aliasing ?
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.
@aboch one of the use case is if you want to both have a VIP and a local address. This could be for anycast or implementing direct server return (https://www.nanog.org/meetings/nanog51/presentations/Monday/NANOG51.Talk45.nanog51-Schaumann.pdf). The second IP can be on the loopback or any interface.
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.
@jc-m we do envision the support for properly defined Services and yes, we will have Virtual IPs, Load balancers and all the whole 9 yards. It is just that we are not there yet.
But you did bring up an important point that if you have a custom IPAM driver, then this should be a case of the IPAM driver handling it & libnetwork should not worry about that restriction. But still, even if we make the current PR handle multiple-ip, it stops short at the IPAM layer. The endpoint isnt assigned to multiple IPs when it ends up in the network Driver.
Wont you feel better to have a full e2e support for multiple IPs from the User to IPAM driver and ends up finally in the endpoint as seen by the network driver ?
Again, as @aboch suggested, we are fully supportive of multiple IP assignment for the endpoints for all the use-cases that you suggested. Just that we dont have it covered e2e.
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.
@mavenugo I obviously care about the e2e support. I see how supporting multiple IPs per endpoints can be a problem with the current model. Another way would have been to pass the preferred IPs in libnetwork.EndpointOptionGeneric , and this would have provided more flexibility. Some drivers could simply ignore them, others could use them.
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.
@jc-m am glad that you brought up these points so that we can get it resolved soon. Will you mind opening an issue/proposal using which we can design it together to support multiple IPs e2e ?
My suggestion would be to let this PR work for a single IP which is supported e2e and work on a proposal to support multiple IPs end-to-end ?
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 agree with getting this one go with 1 IP for now. We will provide a proposal for the multiple IPs building on this PR.
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.
created #798