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.

Sign up for GitHub

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

Jump to bottom

udn, allocator: Add masquerade IPs #4458

Merged
merged 1 commit into from
Jul 5, 2024
Merged

udn, allocator: Add masquerade IPs #4458

merged 1 commit into from
Jul 5, 2024

Conversation

qinqon
Copy link
Contributor

@qinqon qinqon commented Jun 20, 2024 •
edited
Loading

What this PR does and why is it needed

Add a function to calculate the
masquerade IPs for shared and local gw for user defined networks.

Depends-on:

How to verify it

There are some unit test tesing masquerade IPs generation

Details to documentation updates

NONE

Description for the changelog

Add user defined network masquerade IPs generator

Does this PR introduce a user-facing change?

NONE

Add user defined network masquerade IPs generator

@qinqon qinqon changed the title allocator: Add fib table id allocator for vrf allocator: Add user defined networks allocators Jun 20, 2024
@qinqon qinqon force-pushed the net-segmentation-id-allocator branch from de5936b to af14b5b Compare June 20, 2024 08:52
@coveralls
Copy link

Coverage Status

Changes unknown
when pulling af14b5b on qinqon:net-segmentation-id-allocator
into ** on ovn-org:master**.

@qinqon qinqon force-pushed the net-segmentation-id-allocator branch 5 times, most recently from 50ad17e to 7b64fd7 Compare June 24, 2024 06:48
@github-actions github-actions bot added the area/unit-testing Issues related to adding/updating unit tests label Jun 24, 2024
@qinqon qinqon force-pushed the net-segmentation-id-allocator branch 2 times, most recently from 2afbcdc to e554a75 Compare June 24, 2024 07:08
@tssurya tssurya added kind/feature All issues/PRs that are new features feature/multi-networks Issues related to secondary networks, L3, L2, localnet feature/pods All issues related to the PodAPI feature/user-defined-network-segmentation All PRs related to User defined network segmentation labels Jun 24, 2024
@qinqon qinqon force-pushed the net-segmentation-id-allocator branch from e554a75 to 597edb5 Compare June 24, 2024 09:54
@qinqon qinqon marked this pull request as ready for review June 24, 2024 09:59
@qinqon qinqon requested a review from a team as a code owner June 24, 2024 09:59
@qinqon qinqon requested a review from jcaamano June 24, 2024 09:59
@qinqon qinqon force-pushed the net-segmentation-id-allocator branch from 597edb5 to 0c99b2c Compare June 24, 2024 10:05
@tssurya tssurya added the installation Issues related to installing ovn-kubernetes CNI label Jun 24, 2024
tssurya
tssurya reviewed Jun 24, 2024
View reviewed changes
Copy link
Member

@tssurya tssurya left a comment

Choose a reason for hiding this comment

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

I think first 4 commits are missing UTs + we should add docs for these https://ovn-kubernetes.io/getting-started/configuration/#gateway-config once we finalize them
reviewed half way till commit6 thanks for the split makes it easy.

go-controller/pkg/config/config.go Outdated Show resolved Hide resolved
go-controller/pkg/config/config.go Outdated Show resolved Hide resolved
go-controller/pkg/config/config.go Outdated Show resolved Hide resolved
go-controller/pkg/config/config.go Outdated Show resolved Hide resolved
go-controller/pkg/libovsdb/util/router.go Outdated Show resolved Hide resolved
go-controller/pkg/allocator/udn/allocator.go Outdated Show resolved Hide resolved
@qinqon qinqon force-pushed the net-segmentation-id-allocator branch 5 times, most recently from 98bf1c0 to f8495e9 Compare June 24, 2024 14:07
@qinqon qinqon requested a review from tssurya June 24, 2024 14:07
@qinqon
Copy link
Contributor Author

qinqon commented Jun 24, 2024

@tssurya adding events here is worth it ?

@qinqon qinqon force-pushed the net-segmentation-id-allocator branch from f8495e9 to 43c57c4 Compare June 24, 2024 14:17
@qinqon qinqon force-pushed the net-segmentation-id-allocator branch 2 times, most recently from 10e25f0 to 472f5b8 Compare July 4, 2024 12:49
@tssurya tssurya added this to the v1.1.0 milestone Jul 4, 2024
dceara
dceara reviewed Jul 5, 2024
View reviewed changes
go-controller/pkg/allocator/udn/ips.go Outdated Show resolved Hide resolved
go-controller/pkg/allocator/udn/ips.go Outdated Show resolved Hide resolved
go-controller/pkg/allocator/udn/ips_test.go Outdated Show resolved Hide resolved
go-controller/pkg/allocator/udn/ips.go Outdated
Comment on lines 67 to 69
if !subnetCIDR.Contains(masqueradeIP) {
return nil, fmt.Errorf("failed calculating user defined network %s masquerade IPs: IP %s is not within subnet %s", idName, masqueradeIP, subnetCIDR)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'd move this outside the loop and call it twice. Like that it would allow us to specify which of the two IPs (Shared or Local) fallse outside the subnet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this is call per Network we will not hurt performance too much, I will do that.

@qinqon qinqon force-pushed the net-segmentation-id-allocator branch from 472f5b8 to 4ecc3c9 Compare July 5, 2024 07:34
@qinqon qinqon requested a review from dceara July 5, 2024 07:35
tssurya
tssurya requested changes Jul 5, 2024
View reviewed changes
go-controller/pkg/allocator/udn/ips.go Outdated Show resolved Hide resolved
go-controller/pkg/allocator/udn/ips.go Outdated Show resolved Hide resolved
go-controller/pkg/allocator/udn/ips.go Outdated Show resolved Hide resolved
go-controller/pkg/types/const.go Outdated Show resolved Hide resolved
go-controller/pkg/allocator/udn/ips.go Outdated
// added to the subnet (169.254.169.0) with AddIPOffset to convert the offset to the
// masquerade IPs 169.254.169.13 and 169.254.169.14
numberOfIPs := 2
previousNetworkID := networkID - 1
Copy link
Member

Choose a reason for hiding this comment

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

and just to be sure the networkIDs are serially getting allocated correct? can there be a gap value or something where a number never got allocated? meaning networkID is valid given we will pass it from netInfo, using networkID-1 as the previous network's ID is that a guaranteed scenario always?

Copy link
Contributor Author

@qinqon qinqon Jul 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

and just to be sure the networkIDs are serially getting allocated correct?

We don't care if they are serial or not, we just care about the stuff starting from our network-id.

can there be a gap value or something where a number never got allocated?

Even if there is a gap it does not affect the fact that we are going to start from the networkID

meaning networkID is valid given we will pass it from netInfo, using networkID-1 as the previous network's ID is that a guaranteed scenario always?

We know for sure that our networks start from 1 so the lower limit of network-1 is going to be 0 so all is good.

go-controller/pkg/allocator/udn/ips.go Outdated Show resolved Hide resolved
@qinqon qinqon force-pushed the net-segmentation-id-allocator branch from 4ecc3c9 to 9484d61 Compare July 5, 2024 08:45
@qinqon qinqon requested a review from tssurya July 5, 2024 08:45
@qinqon qinqon force-pushed the net-segmentation-id-allocator branch from 9484d61 to 43b1487 Compare July 5, 2024 10:08
tssurya
tssurya reviewed Jul 5, 2024
View reviewed changes
go-controller/pkg/generator/udn/ips.go Outdated
// Let's illustrate the expected IPs for networkID 1 and 2
// with userDefinedNetworkMasqueradeIPBase=10 and subnet=169.254.169.0/16
// networkID=1
// Shared: 169.254.169.11
Copy link
Member

Choose a reason for hiding this comment

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

nit: this sticked as shared/local

go-controller/pkg/generator/udn/ips.go Outdated
firstIPOffset := secondIPOffset - 1

masqueradeIPs := &MasqueradeIPs{}
masqueradeIPs.GatewayRouter, err = ipGenerator.GenerateIP(firstIPOffset)
Copy link
Member

Choose a reason for hiding this comment

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

looks really nice now!

@qinqon qinqon force-pushed the net-segmentation-id-allocator branch from 43b1487 to fb7df6c Compare July 5, 2024 10:13
@qinqon qinqon requested a review from tssurya July 5, 2024 10:13
@qinqon qinqon force-pushed the net-segmentation-id-allocator branch 2 times, most recently from f370c4c to e165b0e Compare July 5, 2024 10:17
tssurya
tssurya previously approved these changes Jul 5, 2024
View reviewed changes
Copy link
Member

@tssurya tssurya left a comment

Choose a reason for hiding this comment

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

This lgtm
Waiting for LGTM from @dceara and/or @pperiyasamy

@coveralls
Copy link

Coverage Status

coverage: 52.635% (-0.04%) from 52.676%
when pulling e165b0e on qinqon:net-segmentation-id-allocator
into f4c015b on ovn-org:master.

@pperiyasamy
Copy link
Contributor

Thanks @qinqon for a nice API, LGTM from me.

dceara
dceara reviewed Jul 5, 2024
View reviewed changes
go-controller/pkg/generator/udn/masquerade_ips.go Outdated

// For networkID=2 and base=10 this will be 14
secondIPOffset := userDefinedNetworkMasqueradeIPBase + networkID*numberOfIPs

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: numberOfIPs seems a bit superfluous. We assume anyway that it's always 2. Why not just call:

ipGenerator.GenerateIP(userDefinedNetworkMasqueradeIPBase + networkID * numberOfIPs -1)

and:

ipGenerator.GenerateIP(userDefinedNetworkMasqueradeIPBase + networkID * numberOfIPs)

@qinqon qinqon dismissed tssurya’s stale review July 5, 2024 12:55

The merge-base changed after approval.

@tssurya tssurya force-pushed the net-segmentation-id-allocator branch from e165b0e to 8364358 Compare July 5, 2024 13:01
@qinqon
udn, allocator: Add masquerade IPs
c8e2e27 
Add a function to calculate the
masquerade IPs for shared and local gw for user defined networks.

Signed-off-by: Enrique Llorente <[email protected]>
@qinqon qinqon force-pushed the net-segmentation-id-allocator branch from 8364358 to c8e2e27 Compare July 5, 2024 13:49
@qinqon qinqon requested review from dceara and tssurya July 5, 2024 13:57
dceara
dceara approved these changes Jul 5, 2024
View reviewed changes
Copy link
Contributor

@dceara dceara 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!

@tssurya tssurya dismissed trozet’s stale review July 5, 2024 16:01

code has changed since tim's review

@tssurya tssurya merged commit 67735f1 into ovn-org:master Jul 5, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tssurya tssurya tssurya approved these changes

@dceara dceara dceara approved these changes

@martinkennelly martinkennelly martinkennelly left review comments

@jcaamano jcaamano Awaiting requested review from jcaamano jcaamano is a code owner automatically assigned from ovn-org/ovn-kubernetes-members

@arghosh93 arghosh93 Awaiting requested review from arghosh93

@trozet trozet Awaiting requested review from trozet

@pperiyasamy pperiyasamy Awaiting requested review from pperiyasamy

Assignees
No one assigned
Labels
area/unit-testing Issues related to adding/updating unit tests feature/multi-networks Issues related to secondary networks, L3, L2, localnet feature/pods All issues related to the PodAPI feature/user-defined-network-segmentation All PRs related to User defined network segmentation installation Issues related to installing ovn-kubernetes CNI kind/feature All issues/PRs that are new features
Projects
OVN Kubernetes Project
Archived in project
Milestone
v1.1.0
Development

Successfully merging this pull request may close these issues.
8 participants
@qinqon @coveralls @pperiyasamy @tssurya @trozet @dceara @martinkennelly @arghosh93