Add support for clustersetIP #1231

vthapar · 2024-09-24T09:16:16Z

Add ClustersetIP flags to deploy-broker and join.
Add ClustersetIP CIDRs to config CRs.
Show clustersetupCIDR in show networks output.
Add use-clusterset-ip flag to service export

submariner-bot · 2024-09-24T09:16:19Z

🤖 Created branch: z_pr1231/vthapar/clustersetip
🚀 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-24T13:35:09Z

cmd/subctl/deploybroker.go

@@ -89,6 +90,10 @@ func addDeployBrokerFlags(flags *pflag.FlagSet) {

 	flags.StringVar(&deployflags.BrokerURL, "broker-url", "",
 		"broker API endpoint URL (stored in the broker information file, defaults to the context URL)")
+	flags.BoolVar(&deployflags.BrokerSpec.ClustersetIPEnabled, "enable-clusterset-ip", false,
+		"set default support for ClustersetIP in connecting clusters (default disabled)")


Suggested change

"set default support for ClustersetIP in connecting clusters (default disabled)")

"set default support for use of clusterset IP for exported services in connecting clusters (default disabled)")

tpantelis · 2024-09-24T13:36:04Z

cmd/subctl/deploybroker.go

+	flags.BoolVar(&deployflags.BrokerSpec.ClustersetIPEnabled, "enable-clusterset-ip", false,
+		"set default support for ClustersetIP in connecting clusters (default disabled)")
+	flags.StringVar(&deployflags.BrokerSpec.ClustersetIPCIDRRange, "clusterset-ip-cidr-range",
+		clustersetip.DefaultCIDR, "ClustersetIP CIDR supernet range for allocating ClustersetIP CIDRs to each cluster")


Suggested change

clustersetip.DefaultCIDR, "ClustersetIP CIDR supernet range for allocating ClustersetIP CIDRs to each cluster")

clustersetip.DefaultCIDR, "Clusterset IP CIDR supernet range for allocating clusterset IP CIDRs to each cluster")

tpantelis · 2024-09-24T13:36:17Z

cmd/subctl/export.go

+
+func addExportServiceFlags(cmd *cobra.Command) {
+	exportRestConfigProducer.SetupFlags(exportServiceCmd.Flags())
+	cmd.PersistentFlags().BoolVar(&useClustersetIP, "use-clusterset-ip", false, "use clusterset ip for this service")


Suggested change

cmd.PersistentFlags().BoolVar(&useClustersetIP, "use-clusterset-ip", false, "use clusterset ip for this service")

cmd.PersistentFlags().BoolVar(&useClustersetIP, "use-clusterset-ip", false, "use clusterset IP for this service")

tpantelis · 2024-09-24T13:37:18Z

cmd/subctl/join.go

@@ -147,6 +147,10 @@ func addJoinFlags(cmd *cobra.Command) {
 		"check the broker certificate (disable this to allow \"insecure\" connections)")
 	cmd.Flags().StringVar(&joinFlags.BrokerURL, "broker-url", "",
 		"URL of the broker API endpoint (overrides the URL stored in the broker information file)")
+	cmd.Flags().BoolVar(&joinFlags.EnableClustersetIP, "enable-clusterset-ip", false,
+		"set default support for ClustersetIP in the cluster (default disabled)")


Suggested change

"set default support for ClustersetIP in the cluster (default disabled)")

"set default support for use of clusterset IP for exported services in the cluster (default disabled)")

tpantelis · 2024-09-24T13:38:12Z

cmd/subctl/join.go

+	cmd.Flags().BoolVar(&joinFlags.EnableClustersetIP, "enable-clusterset-ip", false,
+		"set default support for ClustersetIP in the cluster (default disabled)")
+	cmd.Flags().StringVar(&joinFlags.ClustersetIPCIDR, "clusterset-ip-cidr", "",
+		"ClustersetIP CIDR to be allocated to the cluster")


Suggested change

"ClustersetIP CIDR to be allocated to the cluster")

"Clusterset IP CIDR to be allocated to the cluster")

tpantelis · 2024-09-24T13:48:39Z

pkg/deploy/submariner.go

@@ -46,6 +47,7 @@ type SubmarinerOptions struct {
 	LoadBalancerEnabled           bool
 	HealthCheckEnabled            bool
 	BrokerK8sInsecure             bool
+	ClustersetipEnabled           bool


Suggested change

ClustersetipEnabled bool

ClustersetIPEnabled bool

tpantelis · 2024-09-24T13:50:21Z

pkg/deploy/submariner.go

@@ -105,6 +108,7 @@ func populateSubmarinerSpec(options *SubmarinerOptions, brokerInfo *broker.Info,
 		BrokerK8sApiServer:       brokerURL,
 		BrokerK8sSecret:          brokerSecret.ObjectMeta.Name,
 		BrokerK8sInsecure:        options.BrokerK8sInsecure,
+		ClustersetIPEnabled:      options.ClustersetipEnabled,


Suggested change

ClustersetIPEnabled: options.ClustersetipEnabled,

ClustersetIPEnabled: options.ClustersetipEnabled,

ClustersetIPCIDR: clustersetConfig.ClustersetIPCIDR,

tpantelis · 2024-09-24T13:52:00Z

pkg/deploy/submariner.go

+	if clustersetConfig.ClustersetIPCIDR != "" {
+		submarinerSpec.ClustersetIPCIDR = clustersetConfig.ClustersetIPCIDR
+	}


We can just initialize above regardless if empty.

Suggested change

if clustersetConfig.ClustersetIPCIDR != "" {

submarinerSpec.ClustersetIPCIDR = clustersetConfig.ClustersetIPCIDR

}

Ah right, forgot that this is omitifempty.

tpantelis · 2024-09-24T13:52:59Z

pkg/join/join.go

+	err = clustersetip.AllocateCIDRFromConfigMap(ctx, brokerClientProducer.ForGeneral(), brokerNamespace,
+		&clustersetConfig, status)
+	if err != nil {
+		return errors.Wrap(err, "unable to determine clustersetIP CIDR")


Suggested change

return errors.Wrap(err, "unable to determine clustersetIP CIDR")

return errors.Wrap(err, "unable to determine the clusterset IP CIDR")

tpantelis · 2024-09-24T13:53:52Z

pkg/service/export.go

@@ -43,6 +44,10 @@ func Export(clientProducer client.Producer, serviceNamespace, svcName string, st
 		},
 	}

+	if useClustersetIP {
+		mcsServiceExport.SetAnnotations(map[string]string{lhconstants.UseClustersetIP: "true"})


Suggested change

mcsServiceExport.SetAnnotations(map[string]string{lhconstants.UseClustersetIP: "true"})

mcsServiceExport.SetAnnotations(map[string]string{lhconstants.UseClustersetIP: strconv.FormatBool(true)})

This is the tricky one. Way flags work, default is false. But I don't want to set anything if user hasn't given an explicit option. In this case, setting it to false is different from not setting it at all. Not setting it means it will use the global setting on cluster. Setting false means even if global is true, it will not use clustersetip for this service.

I realize the way I've done means user can never set false explicitly, but I went with my option for backwards compatibility where existing usage is to not have it set at all.

Any suggestions on how we can handle it better? This is a case where we want true, false, nil. Can we do that?

EDIT: How about make it a string?

yeah make it a string and verify it's either "true" or "false".

tpantelis · 2024-09-24T18:57:30Z

pkg/deploy/broker.go

+	if err = clustersetip.ValidateExistingClustersetIPNetworks(ctx, clientProducer.ForGeneral(), options.BrokerNamespace); err != nil {
+		return status.Error(err, "error validating existing clustersetIPCIDR configmap")
+	}
+
+	if err = clustersetip.CreateConfigMap(ctx, clientProducer.ForGeneral(), options.BrokerSpec.ClustersetIPEnabled,
+		options.BrokerSpec.ClustersetIPCIDRRange, 0, options.BrokerNamespace); err != nil {
+		return status.Error(err, "error creating clustersetIPCIDR configmap on Broker")
+	}


Shouldn't the broker controller handle this as it does for globalnet?

Shouldn't the broker controller handle this as it does for globalnet?

Yeah, it should but that is for addon use case where addon operator runs broker controller. There is no broker controller when usin subctl to deploy broker, that is why we do this for globlanet too in subctl.

subctl/pkg/deploy/broker.go

Line 80 in e6b9fe5

if options.BrokerSpec.GlobalnetEnabled {

Actually, the broker controller always runs in the operator pod - it's activated by creation of the Broker resource.

The behavior wrt the addon vs subctl is actually the opposite: the addon creates the globalnet ConfigMap and does not create the Broker resource while subctl does both tho I'm not sure why. It's fine if we do the same for clusterset IP here to mirror globalnet behavior but perhaps we should also do the same in the broker controller otherwise it renders the new fields in the Broker CRD meaningless. We could revisit the duplicate behavior later.

Also note that the helm charts does neither but it's moot b/c we expect the user to pass in the global CIDR when deploying the operator.

1. Add ClustersetIP flags to deploy-broker and join. 2. Add ClustersetIP CIDRs to config CRs. 3. Show clustersetupCIDR in `show networks` output. 4. Add use-clusterset-ip flag to `service export` Refer: submariner-io/enhancements#230 Signed-off-by: Vishal Thapar <[email protected]>

submariner-bot · 2024-09-25T21:06:20Z

🤖 Closed branches: [z_pr1231/vthapar/clustersetip]

vthapar requested review from aswinsuryan, dfarrell07, Jaanki, maayanf24, Oats87, skitt, sridhargaddam and tpantelis as code owners September 24, 2024 09:16

tpantelis requested changes Sep 24, 2024

View reviewed changes

tpantelis reviewed Sep 24, 2024

View reviewed changes

tpantelis mentioned this pull request Sep 24, 2024

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

Closed

23 tasks

vthapar force-pushed the clustersetip branch 3 times, most recently from 4e5d3c1 to 670cb45 Compare September 25, 2024 13:12

tpantelis approved these changes Sep 25, 2024

View reviewed changes

tpantelis requested a review from yboaron September 25, 2024 13:30

tpantelis force-pushed the clustersetip branch from 670cb45 to 3dd3407 Compare September 25, 2024 19:07

tpantelis added the ready-to-test When a PR is ready for full E2E testing label Sep 25, 2024

tpantelis merged commit 3c28b2e into submariner-io:devel Sep 25, 2024
33 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for clustersetIP #1231

Add support for clustersetIP #1231

vthapar commented Sep 24, 2024

submariner-bot commented Sep 24, 2024

tpantelis Sep 24, 2024

vthapar Sep 25, 2024

tpantelis Sep 24, 2024

vthapar Sep 25, 2024

tpantelis Sep 24, 2024

vthapar Sep 25, 2024

tpantelis Sep 24, 2024

vthapar Sep 25, 2024

tpantelis Sep 24, 2024

vthapar Sep 25, 2024

tpantelis Sep 24, 2024

vthapar Sep 25, 2024

tpantelis Sep 24, 2024

vthapar Sep 25, 2024

tpantelis Sep 24, 2024

vthapar Sep 25, 2024

tpantelis Sep 24, 2024

tpantelis Sep 24, 2024

vthapar Sep 25, 2024 •

edited

Loading

tpantelis Sep 25, 2024

tpantelis Sep 24, 2024

vthapar Sep 25, 2024

tpantelis Sep 25, 2024

submariner-bot commented Sep 25, 2024

	"set default support for ClustersetIP in connecting clusters (default disabled)")
	"set default support for use of clusterset IP for exported services in connecting clusters (default disabled)")

	clustersetip.DefaultCIDR, "ClustersetIP CIDR supernet range for allocating ClustersetIP CIDRs to each cluster")
	clustersetip.DefaultCIDR, "Clusterset IP CIDR supernet range for allocating clusterset IP CIDRs to each cluster")

	cmd.PersistentFlags().BoolVar(&useClustersetIP, "use-clusterset-ip", false, "use clusterset ip for this service")
	cmd.PersistentFlags().BoolVar(&useClustersetIP, "use-clusterset-ip", false, "use clusterset IP for this service")

	"ClustersetIP CIDR to be allocated to the cluster")
	"Clusterset IP CIDR to be allocated to the cluster")

	ClustersetIPEnabled: options.ClustersetipEnabled,
	ClustersetIPEnabled: options.ClustersetipEnabled,
	ClustersetIPCIDR: clustersetConfig.ClustersetIPCIDR,

	if clustersetConfig.ClustersetIPCIDR != "" {
	submarinerSpec.ClustersetIPCIDR = clustersetConfig.ClustersetIPCIDR
	}

	return errors.Wrap(err, "unable to determine clustersetIP CIDR")
	return errors.Wrap(err, "unable to determine the clusterset IP CIDR")

	mcsServiceExport.SetAnnotations(map[string]string{lhconstants.UseClustersetIP: "true"})
	mcsServiceExport.SetAnnotations(map[string]string{lhconstants.UseClustersetIP: strconv.FormatBool(true)})

Add support for clustersetIP #1231

Add support for clustersetIP #1231

Conversation

vthapar commented Sep 24, 2024

submariner-bot commented Sep 24, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vthapar Sep 25, 2024 • edited Loading

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 25, 2024

vthapar Sep 25, 2024 •

edited

Loading