Add clustersetip module to discovery #3230

vthapar · 2024-09-18T05:48:11Z

...to allow subctl to allocate clustersetIP cidrs during join

submariner-bot · 2024-09-18T05:48:14Z

🤖 Created branch: z_pr3230/vthapar/clusterset_cidr
🚀 Full E2E won't run until the "ready-to-test" label is applied. I will add it automatically once the PR has 2 approvals, or you can add it manually.

tpantelis · 2024-09-18T15:16:10Z

pkg/cidr/cidr.go

@@ -272,3 +272,38 @@ func uintToIP(ip uint32) net.IP {

 	return netIP
 }
+
+func GetValidClusterSize(cidrRange string, clusterSize uint) (uint, error) {


I think the term "ClusterSize" is a bit confusing. Elsewhere in this code, I referred to it as "AllocationSize".

Suggested change

func GetValidClusterSize(cidrRange string, clusterSize uint) (uint, error) {

func GetValidAllocationSize(cidrRange string, allocationSize uint) (uint, error) {

Agree. I wasn't too happy with it, so left it similar to globalnet to discuss during review.

tpantelis · 2024-09-18T15:19:01Z

pkg/discovery/clustersetip/clustersetip.go

+func isCIDRPreConfigured(clusterID string, clustersetIPNetworks map[string]*cidr.ClusterInfo) bool {
+	// ClustersetIPCIDR is not pre-configured
+	if clustersetIPNetworks[clusterID] == nil || clustersetIPNetworks[clusterID].CIDRs == nil ||
+		len(clustersetIPNetworks[clusterID].CIDRs) == 0 {
+		return false
+	}
+
+	// ClustersetIPCIDR is pre-configured
+	return true
+}


I think we could move this to the cidr package (I think I had intended to).

I missed this one.

tpantelis · 2024-09-18T15:31:05Z

pkg/discovery/clustersetip/config_map.go

+	DefaultClustersetIPCIDR        = "243.0.0.0/8"
+	DefaultClustersetIPClusterSize = 4096 // i.e., x.x.x.x/20 subnet mask


Suggested change

DefaultClustersetIPCIDR = "243.0.0.0/8"

DefaultClustersetIPClusterSize = 4096 // i.e., x.x.x.x/20 subnet mask

DefaultCIDR = "243.0.0.0/8"

DefaultAllocationSize = 4096 // i.e., x.x.x.x/20 subnet mask

tpantelis · 2024-09-18T15:32:02Z

pkg/discovery/clustersetip/clustersetip.go

+type Config struct {
+	ClusterID        string
+	ClustersetIPCIDR string
+	ClusterSize      uint


Suggested change

ClusterSize uint

AllocationSize uint

tpantelis · 2024-09-18T15:44:42Z

pkg/discovery/clustersetip/config_map.go

+	data := map[string]string{
+		clustersetIPEnabledKey:  "false",
+		clustersetIPCidrRange:   string(cidrRange),
+		clustersetIPClusterSize: fmt.Sprint(defaultClustersetIPClusterSize),
+	}
+
+	if clustersetIPEnabled {
+		data[clustersetIPEnabledKey] = "true"
+	}


Suggested change

data := map[string]string{

clustersetIPEnabledKey: "false",

clustersetIPCidrRange: string(cidrRange),

clustersetIPClusterSize: fmt.Sprint(defaultClustersetIPClusterSize),

}

if clustersetIPEnabled {

data[clustersetIPEnabledKey] = "true"

}

data := map[string]string{

clustersetIPEnabledKey: strconv.FormatBool(clustersetIPEnabled),

clustersetIPCidrRange: string(cidrRange),

clustersetIPClusterSize: fmt.Sprint(defaultClustersetIPClusterSize),

}

tpantelis · 2024-09-18T15:48:42Z

pkg/discovery/clustersetip/config_map.go

+	defaultClustersetIPClusterSize uint, namespace string,
+) (*corev1.ConfigMap, error) {
+	labels := map[string]string{
+		"component": "submariner-clustersetIP",


I don't think we need this label. Globalnet code sets it to "submariner-globalnet" which is a larger component, though not sure why it needs the label at all. But "submariner-clustersetIP" isn't a larger component.

tpantelis · 2024-09-18T15:51:16Z

pkg/discovery/clustersetip/clustersetip.go

+	return nil
+}
+
+func AllocateAndUpdateClustersetIPCIDRConfigMap(ctx context.Context, brokerAdminClient controllerClient.Client, brokerNamespace string,


This is quite a mouthful 😄 How about:

Suggested change

func AllocateAndUpdateClustersetIPCIDRConfigMap(ctx context.Context, brokerAdminClient controllerClient.Client, brokerNamespace string,

func AllocateCIDRFromConfigMap(ctx context.Context, brokerAdminClient controllerClient.Client, brokerNamespace string,

Thanks, done.

tpantelis · 2024-09-18T15:57:10Z

pkg/discovery/clustersetip/clustersetip.go

+	return &clustersetIPInfo, configMap, err //nolint:wrapcheck // No need to wrap
+}
+
+func AssignClustersetIPs(clustersetIPInfo *Info, netconfig Config, status reporter.Interface) (string, error) {


This doesn't need to be public.

Suggested change

func AssignClustersetIPs(clustersetIPInfo *Info, netconfig Config, status reporter.Interface) (string, error) {

func assignClustersetIPs(clustersetIPInfo *Info, netconfig Config, status reporter.Interface) (string, error) {

tpantelis · 2024-09-18T15:59:45Z

pkg/discovery/clustersetip/clustersetip.go

+	return true
+}
+
+func ValidateClustersetIPConfiguration(clustersetIPInfo *Info, netconfig Config, status reporter.Interface) (string, error) {


This doesn't need to be public.

Suggested change

func ValidateClustersetIPConfiguration(clustersetIPInfo *Info, netconfig Config, status reporter.Interface) (string, error) {

func validateClustersetIPConfiguration(clustersetIPInfo *Info, netconfig Config, status reporter.Interface) (string, error) {

This gets called from test, so need public.

...to allow subctl to allocate clustersetIP cidrs during join Signed-off-by: Vishal Thapar <[email protected]>

submariner-bot · 2024-09-20T13:06:10Z

🤖 Closed branches: [z_pr3230/vthapar/clusterset_cidr]

vthapar requested review from Oats87, skitt, sridhargaddam and tpantelis as code owners September 18, 2024 05:48

tpantelis reviewed Sep 18, 2024

View reviewed changes

vthapar force-pushed the clusterset_cidr branch from 13d26e4 to d424aa5 Compare September 19, 2024 14:10

vthapar requested a review from tpantelis September 19, 2024 14:11

Add clustersetip module to discovery

a97c228

...to allow subctl to allocate clustersetIP cidrs during join Signed-off-by: Vishal Thapar <[email protected]>

vthapar force-pushed the clusterset_cidr branch from d424aa5 to a97c228 Compare September 19, 2024 14:33

vthapar requested a review from aswinsuryan September 19, 2024 17:25

tpantelis approved these changes Sep 19, 2024

View reviewed changes

aswinsuryan approved these changes Sep 20, 2024

View reviewed changes

submariner-bot added the ready-to-test When a PR is ready for full E2E testing label Sep 20, 2024

tpantelis merged commit e2de28e into submariner-io:devel Sep 20, 2024
40 checks passed

tpantelis mentioned this pull request Sep 11, 2024

Add support for clusterset IP for MCS Compliance submariner-io/enhancements#229

Closed

23 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add clustersetip module to discovery #3230

Add clustersetip module to discovery #3230

vthapar commented Sep 18, 2024

submariner-bot commented Sep 18, 2024

tpantelis Sep 18, 2024

vthapar Sep 19, 2024

tpantelis Sep 18, 2024

vthapar Sep 19, 2024

tpantelis Sep 18, 2024

tpantelis Sep 18, 2024

vthapar Sep 19, 2024

tpantelis Sep 18, 2024

vthapar Sep 19, 2024

tpantelis Sep 18, 2024

tpantelis Sep 18, 2024

vthapar Sep 19, 2024

tpantelis Sep 18, 2024

vthapar Sep 19, 2024

tpantelis Sep 18, 2024

vthapar Sep 19, 2024

submariner-bot commented Sep 20, 2024

	func GetValidClusterSize(cidrRange string, clusterSize uint) (uint, error) {
	func GetValidAllocationSize(cidrRange string, allocationSize uint) (uint, error) {

		DefaultClustersetIPCIDR = "243.0.0.0/8"
		DefaultClustersetIPClusterSize = 4096 // i.e., x.x.x.x/20 subnet mask

	func AllocateAndUpdateClustersetIPCIDRConfigMap(ctx context.Context, brokerAdminClient controllerClient.Client, brokerNamespace string,
	func AllocateCIDRFromConfigMap(ctx context.Context, brokerAdminClient controllerClient.Client, brokerNamespace string,

	func AssignClustersetIPs(clustersetIPInfo *Info, netconfig Config, status reporter.Interface) (string, error) {
	func assignClustersetIPs(clustersetIPInfo *Info, netconfig Config, status reporter.Interface) (string, error) {

	func ValidateClustersetIPConfiguration(clustersetIPInfo *Info, netconfig Config, status reporter.Interface) (string, error) {
	func validateClustersetIPConfiguration(clustersetIPInfo *Info, netconfig Config, status reporter.Interface) (string, error) {

Add clustersetip module to discovery #3230

Add clustersetip module to discovery #3230

Conversation

vthapar commented Sep 18, 2024

submariner-bot commented Sep 18, 2024

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

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

submariner-bot commented Sep 20, 2024