Handle the broker in the operator #1085

skitt · 2021-02-12T16:01:26Z

Fixes: #203
Signed-off-by: Stephen Kitt [email protected]

skitt · 2021-02-17T09:03:00Z

This PR updates subctl so that deploy-broker goes through the operator, but it doesn’t remove all the non-operator-based code — I’m wondering whether we should add a flag to allow broker deployment without the operator, or if we should just forget that and only support the operator.

mangelajo · 2021-02-17T09:41:14Z

This PR updates subctl so that deploy-broker goes through the operator, but it doesn’t remove all the non-operator-based code — I’m wondering whether we should add a flag to allow broker deployment without the operator, or if we should just forget that and only support the operator.

+1 for operator only for now...

mangelajo · 2021-02-17T09:41:37Z

apis/submariner/v1alpha1/submariner_types.go

+	// INSERT ADDITIONAL SPEC FIELDS - desired state of cluster
+	// Important: Run "make" to regenerate code after modifying this file
+
+	Components                  []string `json:"components,omitempty"`


Thanks for updating last moment, I just realized you were into this too :/

mangelajo · 2021-02-17T09:47:46Z

apis/submariner/v1alpha1/submariner_types.go

+	DefaultCustomDomains        []string `json:"defaultCustomDomains,omitempty"`
+	GlobalnetCidrRange          string   `json:"globalnetCidrRange,omitempty"`
+	DefaultGlobalnetClusterSize uint     `json:"defaultGlobalnetClusterSize"`
+	GlobalnetEnabled            bool     `json:"globalnetEnabled"`


Can we make GlobalNetEnabled an optional/omitempty argument?, same for DefaultGlobalnetClusterSize?, is it possible -an alternative- to provide default values and allow omission?

Is it really necessary?, could it be that "GlobalnetCidrRange == GlobalnetEnabled?" (not sure about it though).

How about making Globalnet a component? I’ll try that in a separate PR... Also, Globalnet can be enabled without a CIDR range, can’t it?

Hmm, that'd be rather neat I think @skitt

Without a CIDR range I'm not sure, I guess we need something at least for auto-assignment of clusters. But if you don't want auto-assignment I guess you don't need it @sridhargaddam ^

Correct, the globalCIDR is required for auto-assignment to clusters while the cluster is joining the Broker. We provide flexibility to users to specify custom globalCIDR, but IMO we should continue to have a default CIDR to support clusters that do not explicitly specify globalCIDR while joining.

The fields are now all optional.

#1102 tracks the componentisation of Globalnet.

mangelajo

Some comments

mangelajo · 2021-02-17T09:52:10Z

config/samples/submariner_v1alpha1_broker.yaml

+#  globalnetCidrRange:
+  defaultGlobalnetClusterSize: 8192


Is there a way to allow omission of 'defaultGlobanetClusterSize', and also, provide a default value? I guess that can be done in code.

This is just an example, intended for documentation in the operator when shown in the catalog.

mangelajo · 2021-02-17T09:55:05Z

pkg/subctl/cmd/deploybroker.go

-		err = broker.Ensure(config)
+
+		status.Start("Setting up broker RBAC")
+		err = broker.Ensure(config, false)


I was expecting that the operator would create the RBAC details too, I guess it's not very secure though, but thinking of usability and the concept of doing it on the operator... it's weird that we would need to setup RBAC manually if not doing it with subctl.

We discussed this in the past, and decided to forgo dealing with RBAC in the operator, because it requires granting too many privileges to the operator.

I know, I remember, but I start to see issues with our thinking as we move the broker here too, which is different to just "provide RBAC to submariner services", ... now we need to provide RBAC / Accounts to signed up clusters...

We will need to document the RBAC steps unless we change that.

Can we remove the rbac.go in favour of working with embedded yaml files?
The yaml files could be tracked in config/rbac. This way I can use them in the operator bundle.

@SteveMattar this is tracked in #1105.

pkg/subctl/operator/brokercr/ensure.go

pkg/subctl/operator/submarinerop/crds/ensure.go

mangelajo · 2021-02-17T10:22:24Z

Sorry, the second batch of comments came separate.

skitt · 2021-02-17T17:48:46Z

This needs #1096 first.

sridhargaddam · 2021-02-17T17:57:42Z

apis/submariner/v1alpha1/submariner_types.go

+	DefaultCustomDomains        []string `json:"defaultCustomDomains,omitempty"`
+	GlobalnetCidrRange          string   `json:"globalnetCidrRange,omitempty"`
+	DefaultGlobalnetClusterSize uint     `json:"defaultGlobalnetClusterSize,omitempty"`
+	GlobalnetEnabled            bool     `json:"globalnetEnabled,omitempty"`


We might require changes to subctl to prompt the user to provide the GlobalnetCIDR/GlobalnetClusterSize while joining a new Cluster, if there are no defaults configured when Broker is deployed.

This is already the case, I’ll do that in a separate PR. #1103 tracks this.

I suggest we do something like:

https://github.com/submariner-io/submariner-operator/blob/devel/apis/submariner/v1alpha1/submariner_types.go#L131

https://github.com/submariner-io/submariner-operator/blob/devel/apis/submariner/v1alpha1/servicediscovery_types.go#L84

Well, or may be not, not sure, if they are going through the process of using the operator directly.... may be it's better to let them be explicit

I prefer #1104 ;-)

pkg/subctl/operator/brokercr/ensure.go

mangelajo · 2021-02-22T09:48:50Z

apis/submariner/v1alpha1/submariner_types.go

+	DefaultCustomDomains        []string `json:"defaultCustomDomains,omitempty"`
+	GlobalnetCidrRange          string   `json:"globalnetCidrRange,omitempty"`
+	DefaultGlobalnetClusterSize uint     `json:"defaultGlobalnetClusterSize,omitempty"`
+	GlobalnetEnabled            bool     `json:"globalnetEnabled,omitempty"`


I suggest we do something like:

https://github.com/submariner-io/submariner-operator/blob/devel/apis/submariner/v1alpha1/submariner_types.go#L131

https://github.com/submariner-io/submariner-operator/blob/devel/apis/submariner/v1alpha1/servicediscovery_types.go#L84

SteveMattar

What about the broker RBAC? Currently the only way it works is by using subctl.
Should we export these roles to config/rbac?

SteveMattar · 2021-02-22T10:46:46Z

config/crd/kustomization.yaml

@@ -5,19 +5,22 @@
 resources:
 #  - bases/submariner.io_servicediscoveries.yaml
  - bases/submariner.io_submariners.yaml
+#  - bases/submariner.io_brokers.yaml


Do we want to hide this CRD? how do we install the broker when working with the bundle only?

Ah, right, we do want to show the CRD!

SteveMattar · 2021-02-22T10:57:44Z

main.go

+	if err = (&submariner.BrokerReconciler{
+		Client: mgr.GetClient(),
+		Config: mgr.GetConfig(),
+		Log:    ctrl.Log.WithName("controllers").WithName("Broker"),
+		Scheme: mgr.GetScheme(),
+	}).SetupWithManager(mgr); err != nil {
+		log.Error(err, "unable to create controller", "controller", "Broker")
+		os.Exit(1)
+	}


I think you can add this function to controllers.AddToManager this is how we usually start a new Reconciler

This is what the operator SDK generated for me...

SteveMattar · 2021-02-22T11:07:44Z

Sorry @skitt just saw the previous comments

skitt · 2021-02-22T11:22:56Z

What about the broker RBAC? Currently the only way it works is by using subctl.
Should we export these roles to config/rbac?

This is what I’d asked you about a couple of weeks ago; I couldn’t get the SDK to generate the appropriate role.yaml file, or rather, I could, but it overwrote all the existing RBAC setup that’s in there.

SteveMattar · 2021-02-22T11:30:30Z

What about the broker RBAC? Currently the only way it works is by using subctl.
Should we export these roles to config/rbac?

This is what I’d asked you about a couple of weeks ago; I couldn’t get the SDK to generate the appropriate role.yaml file, or rather, I could, but it overwrote all the existing RBAC setup that’s in there.

Sorry you might have missed my message on slack... the thread is confusing sometimes :)
I'll just drop my comment here:
"I found the problem... when controller-gen generates the rbac manifests the default output is config/rbac/role.yaml but the +kubebuilder:rbac markers for submariner and servicediscovery never worked... The reason is because there is a missing empty line between those markers and the class definition. That means the role.yaml file was never updated."

skitt · 2021-02-22T13:53:51Z

What about the broker RBAC? Currently the only way it works is by using subctl.
Should we export these roles to config/rbac?

This is what I’d asked you about a couple of weeks ago; I couldn’t get the SDK to generate the appropriate role.yaml file, or rather, I could, but it overwrote all the existing RBAC setup that’s in there.

Sorry you might have missed my message on slack... the thread is confusing sometimes :)
I'll just drop my comment here:
"I found the problem... when controller-gen generates the rbac manifests the default output is config/rbac/role.yaml but the +kubebuilder:rbac markers for submariner and servicediscovery never worked... The reason is because there is a missing empty line between those markers and the class definition. That means the role.yaml file was never updated."

Right, that’s what I found out too (which is how I disabled the Broker RBAC handling, see the TODO there). I hesitated to re-enable the full RBAC handling because it produces ClusterRoles, not Roles as we currently have...

tpantelis · 2021-02-22T14:17:34Z

apis/submariner/v1alpha1/submariner_types.go

+
+	Components                  []string `json:"components,omitempty"`
+	DefaultCustomDomains        []string `json:"defaultCustomDomains,omitempty"`
+	GlobalnetCidrRange          string   `json:"globalnetCidrRange,omitempty"`


We've been trending from Cidr -> CIDR elsewhere...

Suggested change

GlobalnetCidrRange string `json:"globalnetCidrRange,omitempty"`

GlobalnetCIDRRange string `json:"globalnetCIDRRange,omitempty"`

Ah yes, fixed.

tpantelis · 2021-02-22T14:36:38Z

pkg/subctl/operator/brokercr/ensure.go

+	return nil
+}
+
+func createBroker(clientSet submarinerclientset.Interface, namespace string, brokerCR *submarinerv1a1.Broker) (bool, error) {


Do we need to return bool here instead of just err?

tpantelis · 2021-02-22T14:42:52Z

pkg/subctl/operator/brokercr/ensure.go

+	_, err := clientSet.SubmarinerV1alpha1().Brokers(namespace).Create(brokerCR)
+	if err == nil {
+		return true, nil
+	} else if errors.IsAlreadyExists(err) {
+		retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error {
+			// We can’t always handle existing resources, and we want to overwrite them anyway, so delete them
+			err := clientSet.SubmarinerV1alpha1().Brokers(namespace).Delete(brokerCR.Name, &metav1.DeleteOptions{})
+			if err != nil {
+				return fmt.Errorf("failed to delete pre-existing cfg %s : %s", brokerCR.Name, err)
+			}
+			_, err = clientSet.SubmarinerV1alpha1().Brokers(namespace).Create(brokerCR)
+			if err != nil {
+				return fmt.Errorf("failed to create cfg  %s : %s", brokerCR.Name, err)
+			}
+			return nil
+		})
+		return false, retryErr
+	}
+	return false, err


We really don't need RetryOnConflict as it applies to Update. We could refactor to a loop as per below but not a big deal either way. I can add a CreateAnew function in admiral that does the DeleteIfExistsThenCreate pattern.

Suggested change

_, err := clientSet.SubmarinerV1alpha1().Brokers(namespace).Create(brokerCR)

if err == nil {

return true, nil

} else if errors.IsAlreadyExists(err) {

retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error {

// We can’t always handle existing resources, and we want to overwrite them anyway, so delete them

err := clientSet.SubmarinerV1alpha1().Brokers(namespace).Delete(brokerCR.Name, &metav1.DeleteOptions{})

if err != nil {

return fmt.Errorf("failed to delete pre-existing cfg %s : %s", brokerCR.Name, err)

}

_, err = clientSet.SubmarinerV1alpha1().Brokers(namespace).Create(brokerCR)

if err != nil {

return fmt.Errorf("failed to create cfg %s : %s", brokerCR.Name, err)

}

return nil

})

return false, retryErr

}

return false, err

for {

_, err := clientSet.SubmarinerV1alpha1().Brokers(namespace).Create(brokerCR)

if err == nil {

return nil

}

if errors.IsAlreadyExists(err) {

err := clientSet.SubmarinerV1alpha1().Brokers(namespace).Delete(brokerCR.Name, &metav1.DeleteOptions{})

if err != nil && !errors.IsNotFound(err) {

return fmt.Errorf("failed to delete pre-existing Broker instance %q : %v", brokerCR.Name, err)

}

continue

}

return err

}

return nil

Ah yes, a creation can never return an error which RetryOnConflict will recognise as a conflict. We’d really have to loop ourselves, with a backoff...

(I’ll submit an appropriate function for Admiral tomorrow.)

I'm actually working on that now 😄

submariner-io/admiral#185

mangelajo · 2021-02-23T10:06:08Z

pkg/broker/ensure.go

@@ -34,7 +34,7 @@ import (
 	"k8s.io/client-go/rest"
 )

-func Ensure(config *rest.Config) error {


Why is the crds bool added, where is it used? :-?

Oops, something got lost in a rebase it seems...

Now restored.

Fixes: submariner-io#203 Signed-off-by: Stephen Kitt <[email protected]>

skitt requested review from mangelajo, mkolesnik, Oats87 and tpantelis as code owners February 12, 2021 16:01

skitt requested a review from SteveMattar February 12, 2021 16:23

skitt force-pushed the broker-operator branch 2 times, most recently from 62db248 to d9b907d Compare February 17, 2021 09:01

skitt force-pushed the broker-operator branch from d9b907d to fb970db Compare February 17, 2021 09:14

mangelajo reviewed Feb 17, 2021

View reviewed changes

mangelajo suggested changes Feb 17, 2021

View reviewed changes

skitt force-pushed the broker-operator branch from fb970db to ed1f5d2 Compare February 17, 2021 17:05

skitt requested review from mangelajo and sridhargaddam February 17, 2021 17:06

skitt force-pushed the broker-operator branch from ed1f5d2 to 298d9c5 Compare February 17, 2021 17:26

skitt marked this pull request as draft February 17, 2021 17:48

skitt force-pushed the broker-operator branch from 298d9c5 to a2fb7c8 Compare February 17, 2021 17:50

sridhargaddam reviewed Feb 17, 2021

View reviewed changes

skitt force-pushed the broker-operator branch from a2fb7c8 to 26907ed Compare February 22, 2021 08:19

skitt marked this pull request as ready for review February 22, 2021 08:19

skitt mentioned this pull request Feb 22, 2021

subctl should prompt for Globalnet settings #1103

Closed

mangelajo suggested changes Feb 22, 2021

View reviewed changes

mangelajo approved these changes Feb 22, 2021

View reviewed changes

SteveMattar reviewed Feb 22, 2021

View reviewed changes

tpantelis reviewed Feb 22, 2021

View reviewed changes

skitt force-pushed the broker-operator branch 7 times, most recently from 92e551b to f33cf2d Compare February 23, 2021 09:58

mangelajo reviewed Feb 23, 2021

View reviewed changes

Handle the broker in the operator

f29f66b

Fixes: submariner-io#203 Signed-off-by: Stephen Kitt <[email protected]>

skitt force-pushed the broker-operator branch from f33cf2d to f29f66b Compare February 23, 2021 10:37

skitt requested review from SteveMattar, tpantelis and sridhargaddam February 23, 2021 12:16

SteveMattar approved these changes Feb 23, 2021

View reviewed changes

sridhargaddam approved these changes Feb 23, 2021

View reviewed changes

tpantelis approved these changes Feb 23, 2021

View reviewed changes

tpantelis merged commit 8cbb4c5 into submariner-io:devel Feb 23, 2021

manosnoam mentioned this pull request Apr 20, 2021

[Doc] for Submariner 0.9+ operator image should be accessible before broker deploy submariner-io/submariner-website#483

Closed

	GlobalnetCidrRange string `json:"globalnetCidrRange,omitempty"`
	GlobalnetCIDRRange string `json:"globalnetCIDRRange,omitempty"`

Handle the broker in the operator #1085

Handle the broker in the operator #1085

Conversation

skitt commented Feb 12, 2021

skitt commented Feb 17, 2021

mangelajo commented Feb 17, 2021

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

mangelajo 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

mangelajo commented Feb 17, 2021

skitt commented Feb 17, 2021

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

SteveMattar 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

SteveMattar commented Feb 22, 2021

skitt commented Feb 22, 2021

SteveMattar commented Feb 22, 2021

skitt commented Feb 22, 2021

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