DualStack Proposal #237

Ayush21298 · 2022-06-08T14:47:19Z

Proposal

Brief Design Proposal for IPv4/IPv6 DualStack support.

Checklist

I wasn't sure about the proposal format being used for the project, so if anything is missing please add it in the checklist :)

Related Issue

Dual Stack Support

Signed-off-by: Ayush Patel <[email protected]>

dougbtv · 2022-06-09T13:35:05Z

doc/proposals/dualstack.md

+      "mode": "bridge",
+      "ipam": {
+        "type": "whereabouts",
+-       "range": "192.168.2.225/28",


I think we both want to keep this for backwards compatibility.

Ooops, I see this is one example, I see that other keeps this.

dougbtv · 2022-06-09T13:55:32Z

doc/proposals/dualstack.md

+      Type                string            `json:"type"`
+      Routes              []*cnitypes.Route `json:"routes"`
+      Datastore           string            `json:"datastore"`
+     IPs                 []IPConfig        `json:"ips"`


Name ideas:

IPRanges

IPConfigs

IPAliases

IPCIDRs

CIDRs

I would go for IPRanges . And name the attribute type RangeConfiguration or range config.

RangeConfiguration ++

maiqueb

Thanks for this proposal.

I'd like to actually congratulate you on the focus on the changes from the current API, and how easy you made it for us to understand them. Kudos.

I am OK with pretty much everything you wrote, but still pointed a couple of nits.

doc/proposals/dualstack.md

maiqueb · 2022-06-13T09:19:46Z

doc/proposals/dualstack.md

+
+<hr>
+
+## Introduction <a name="introduction"></a>


Isn't this <a name="introduction"></a> redundant in markdown ? AFAIU, there's an implicit anchor after the lowercase, dash separated, name of the heading. In this case - introduction.

maiqueb · 2022-06-13T09:20:53Z

doc/proposals/dualstack.md

+This is a design proposal document for introducing IPv4/IPv6 DualStack support in whereabouts.
+
+
+### Goal <a name="goal"></a>


same; goal can be used directly without specifying the anchor name.

maiqueb · 2022-06-13T09:21:01Z

doc/proposals/dualstack.md

+
+<hr>
+
+## Design <a name="design"></a>


maiqueb · 2022-06-13T09:21:33Z

doc/proposals/dualstack.md

+
+Basically, rather than having IP related configurations directly as key members of `IPAMConfig` object, they be included as element of an object type, let's say `IPConfig` and `IPAMConfig` should have an array of `IPConfig` as it's element.
+
+### Changes in IPAM Config <a name="configChanges"></a>


here it makes sense - assuming you don't want to use the implicit changes-in-ipam-config anchor name.

maiqueb · 2022-06-13T09:29:07Z

doc/proposals/dualstack.md

+      "mode": "bridge",
+      "ipam": {
+        "type": "whereabouts",
+        "ips": [


ditto. I would name this ranges instead.

maiqueb · 2022-06-13T09:29:34Z

doc/proposals/dualstack.md

+
+```diff
+
+type IPConfig struct {


RangeConfiguration ?

maiqueb · 2022-06-13T09:30:15Z

doc/proposals/dualstack.md

+      Type                string            `json:"type"`
+      Routes              []*cnitypes.Route `json:"routes"`
+      Datastore           string            `json:"datastore"`
+     IPs                 []IPConfig        `json:"ips"`


I would go for IPRanges . And name the attribute type RangeConfiguration or range config.

maiqueb · 2022-06-13T09:35:04Z

doc/proposals/dualstack.md

+        "exclude": [
+           "192.168.2.229/30",
+           "192.168.2.236/32",
+          "2001::0/124"


how would you map the entries on the exclusion list to the correct range ? Here it actually seems doable since the ranges are from different IP families, but, what if you want to use this feature to configure multiple IPs in the same interface ?

We can take separate unions of IPv4 and IPv6 excluded ranges and just skip the IPs that belongs to that union set/list :)
I don't think we need to map it to the exact range. ^^

maiqueb · 2022-06-13T09:36:32Z

doc/proposals/dualstack.md

+First approach requires addition on a new _type_ for encapsulating all the IP related information on `IPAMConfig` and requires significant amount of change in the current code. But with this additional effort, it will have additional benefit of allowing us to allocate as many as we required IP addresses of any IP family without any constraints. _(We need to support the existing fields too for backward compatibility)_
+
+Second approach is somewhat simpler. It just adds some additional fields for _secondary_ IP address related configuration (which might be IPv4 or IPv6 depending on stack policy). The downside is that we will limit ourselves to adding at max 2 IP address for a pod. _(TBH, it also seems okay for now, but I prefer the first approach)_


I agree with the analysis presented here: first approach is preferable - simply because the second one does not scale.

Furthermore, the IP is somewhat clunky IMO.

I vote for option 1 as well.

Please add a paragraph to the summary indicating what is the chosen option and why.

coveralls · 2022-06-13T23:41:09Z

Pull Request Test Coverage Report for Build 2462334462

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 54.252%

Totals
Change from base Build 2353203143:	0.0%
Covered Lines:	874
Relevant Lines:	1611

💛 - Coveralls

- Updated attribute and struct name as per discussion - Minor formatting in proposal document

maiqueb

Nearly there.

Are you ready to propose a test plan for the feature, or would you prefer to do so later on ?

I suggest you never the less familiarize yourself with the e2e tests we have; imo, you would add a new test suite there.

maiqueb · 2022-06-24T07:55:07Z

doc/proposals/dualstack.md


-Basically, rather than having IP related configurations directly as key members of `IPAMConfig` object, they be included as element of an object type, let's say `IPConfig` and `IPAMConfig` should have an array of `IPConfig` as it's element.
+Rather than having IP related configurations directly as key members of `IPAMConfig` object, the IP configuration would be wrapped in a new struct - let's say `IPConfig`.


Suggested change

Rather than having IP related configurations directly as key members of `IPAMConfig` object, the IP configuration would be wrapped in a new struct - let's say `IPConfig`.

Rather than having IP related configurations directly as key members of `IPAMConfig` object, the IP configuration would be wrapped in a new struct - let's say `RangeConfiguration`.

maiqueb · 2022-06-24T07:57:17Z

doc/proposals/dualstack.md

+First approach requires addition on a new _type_ for encapsulating all the IP related information on `IPAMConfig` and requires significant amount of change in the current code. But with this additional effort, it will have additional benefit of allowing us to allocate as many as we required IP addresses of any IP family without any constraints. _(We need to support the existing fields too for backward compatibility)_
+
+Second approach is somewhat simpler. It just adds some additional fields for _secondary_ IP address related configuration (which might be IPv4 or IPv6 depending on stack policy). The downside is that we will limit ourselves to adding at max 2 IP address for a pod. _(TBH, it also seems okay for now, but I prefer the first approach)_


Please add a paragraph to the summary indicating what is the chosen option and why.

Ayush21298 · 2022-06-25T13:39:34Z

Thank you for your comments.
I made the changes as suggested :)

Are you ready to propose a test plan for the feature, or would you prefer to do so later on ?

I'm planning to do it latter along with the development :)

I suggest you never the less familiarize yourself with the e2e tests we have; imo, you would add a new test suite there.

Thank you for you guidance :)
I had a look at the unit test suites as well as e2e test suite and will update them as I go on along with development :)

Ayush21298 · 2022-07-13T15:11:37Z

Hello @dougbtv @maiqueb ~

Can you please review this 😅

maiqueb

I have a question about the proposed alternative: how do the new RangesConfiguration slice play along w/ the old Range attribute ? Is the behavior additive (i.e. 1 range + 2 entries in the RangeConfiguration results in 3 configured interfaces) ?

I would like to see a sub-section about this, explaining (w/ a couple of examples) how it would works.

Some alternatives for this I see:

additive behavior (described above)
only use the new range configuration array / slice (the old Range would be just used for reading - i.e. copy the first range from the array there)
if Range is provided, disallow multiple ranges

Ayush21298 · 2022-07-23T02:11:04Z

I have a question about the proposed alternative: how do the new RangesConfiguration slice play along w/ the old Range attribute ? Is the behavior additive (i.e. 1 range + 2 entries in the RangeConfiguration results in 3 configured interfaces) ?

I would like to see a sub-section about this, explaining (w/ a couple of examples) how it would works.

Some alternatives for this I see:
* additive behavior (described above)

* only use the new range configuration array / slice (the _old_ `Range` would be just used for reading - i.e. copy the first range from the array there)

* if `Range` is provided, disallow multiple ranges

@maiqueb As you described, when both old configuration and IPRanges are present, the behavior will be additive.

I have added a section for the same along with examples in the proposal. :)

maiqueb

Thank you for providing the examples - your proposal is now - imo - crystal clear.

I'll let this soak for a few weeks to give the community the time to also provide feedback / @dougbtv to back it up, but, I think you can start implementing this as is.

Good work.

Ayush21298 · 2022-08-04T16:33:10Z

Thank you for providing the examples - your proposal is now - imo - crystal clear.

I'll let this soak for a few weeks to give the community the time to also provide feedback / @dougbtv to back it up, but, I think you can start implementing this as is.

Good work.

Thanks @maiqueb :)
Yes I started working on the same line in PR #250. I'll try to finish it up ASAP :)

maiqueb · 2022-08-05T07:27:43Z

Thank you for providing the examples - your proposal is now - imo - crystal clear.
I'll let this soak for a few weeks to give the community the time to also provide feedback / @dougbtv to back it up, but, I think you can start implementing this as is.
Good work.

Thanks @maiqueb :) Yes I started working on the same line in PR #250. I'll try to finish it up ASAP :)

Take your time, no rush.

Feel free to reach out if you need help with the e2e tests.

JanScheurich · 2022-08-08T09:36:56Z

We fully support this initiative to let whereabouts assign IP addresses from multiple IP ranges (IPv4 and/or v6) to a secondary pod interface and agree with the proposed implementation.

Ayush21298 · 2022-08-18T11:54:26Z

@dougbtv Hi, can we merge this proposal ?

dougbtv

LGTM. Thanks Ayush for the proposal, looking forward to the functionality.

ryanlyy · 2022-11-30T05:50:31Z

all,

which release will support this feature? seems now only master has these code support.

Thanks

dougbtv · 2022-11-30T15:38:12Z

Thanks @ryanlyy -- we have published https://github.com/k8snetworkplumbingwg/whereabouts/releases/tag/v0.6 -- including this feature!

ryanlyy · 2022-12-01T01:36:30Z

@dougbtv thank you!

lubronzhan · 2023-08-31T01:10:10Z

Hi @Ayush21298 does this only support macvlan mode? looking at the test, only macvlan is tested.
Do you worried that with ipvlan, different pod might get same slaac ip through dhcp?
Thank you

DualStack Proposal

e1ccd37

Signed-off-by: Ayush Patel <[email protected]>

Ayush21298 requested a review from dougbtv as a code owner June 8, 2022 14:47

dougbtv reviewed Jun 9, 2022

View reviewed changes

maiqueb self-requested a review June 9, 2022 14:11

maiqueb requested changes Jun 13, 2022

View reviewed changes

This was referenced Jun 23, 2022

feat: add support multi-ranges #60

Closed

[WiP] add dual stack support #117

Closed

Update dualstack proposal document

6d83159

- Updated attribute and struct name as per discussion - Minor formatting in proposal document

dougbtv mentioned this pull request Jun 23, 2022

feat: Static ip allocations only #177

Closed

Ayush21298 requested review from dougbtv and maiqueb June 23, 2022 13:56

maiqueb requested changes Jun 24, 2022

View reviewed changes

Added a section for discussion and decisions in dualstack proposal

829f3fe

Ayush21298 requested a review from maiqueb June 25, 2022 13:33

Ayush21298 mentioned this pull request Jul 17, 2022

Add support for dualstack / multiple IP ranges #250

Merged

14 tasks

maiqueb requested changes Jul 18, 2022

View reviewed changes

Added explaination for backward compatibility in dualstack proposal

58cac5f

Ayush21298 requested a review from maiqueb July 23, 2022 02:11

maiqueb mentioned this pull request Aug 4, 2022

Enhancement: Allow re-use of ranges per namespace #50

Closed

maiqueb approved these changes Aug 4, 2022

View reviewed changes

dougbtv approved these changes Aug 18, 2022

View reviewed changes

dougbtv merged commit 6b202cb into k8snetworkplumbingwg:master Aug 18, 2022

		This is a design proposal document for introducing IPv4/IPv6 DualStack support in whereabouts.


		### Goal <a name="goal"></a>


		Basically, rather than having IP related configurations directly as key members of `IPAMConfig` object, they be included as element of an object type, let's say `IPConfig` and `IPAMConfig` should have an array of `IPConfig` as it's element.

		### Changes in IPAM Config <a name="configChanges"></a>

		First approach requires addition on a new _type_ for encapsulating all the IP related information on `IPAMConfig` and requires significant amount of change in the current code. But with this additional effort, it will have additional benefit of allowing us to allocate as many as we required IP addresses of any IP family without any constraints. _(We need to support the existing fields too for backward compatibility)_

		Second approach is somewhat simpler. It just adds some additional fields for _secondary_ IP address related configuration (which might be IPv4 or IPv6 depending on stack policy). The downside is that we will limit ourselves to adding at max 2 IP address for a pod. _(TBH, it also seems okay for now, but I prefer the first approach)_


		Basically, rather than having IP related configurations directly as key members of `IPAMConfig` object, they be included as element of an object type, let's say `IPConfig` and `IPAMConfig` should have an array of `IPConfig` as it's element.
		Rather than having IP related configurations directly as key members of `IPAMConfig` object, the IP configuration would be wrapped in a new struct - let's say `IPConfig`.

DualStack Proposal #237

DualStack Proposal #237

Conversation

Ayush21298 commented Jun 8, 2022 • edited by nicklesimba Loading

Proposal

Checklist

Related Issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maiqueb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 13, 2022 • edited Loading

Pull Request Test Coverage Report for Build 2462334462

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

maiqueb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ayush21298 commented Jun 25, 2022

Ayush21298 commented Jul 13, 2022

maiqueb left a comment

Choose a reason for hiding this comment

Ayush21298 commented Jul 23, 2022

maiqueb left a comment

Choose a reason for hiding this comment

Ayush21298 commented Aug 4, 2022

maiqueb commented Aug 5, 2022

JanScheurich commented Aug 8, 2022

Ayush21298 commented Aug 18, 2022

dougbtv left a comment

Choose a reason for hiding this comment

ryanlyy commented Nov 30, 2022

dougbtv commented Nov 30, 2022

ryanlyy commented Dec 1, 2022

lubronzhan commented Aug 31, 2023

Ayush21298 commented Jun 8, 2022 •

edited by nicklesimba

Loading

coveralls commented Jun 13, 2022 •

edited

Loading