Move the CRDs to MetalLB #1237

fedepaol · 2022-02-22T14:27:36Z

This is the first step of what's described in #942, which is moving the CRDs as they are from the operator to metallb.

The PR is split in several commits for review convenience, but it was done afterwars so there might be some change leak in between them.

We replaced the low level informer / workqueue structure with controller-runtime, providing a higher level way to define controllers (and yaml generation for the CRDs).
We wired the existing callbacks to the new controllers, and added a channel based controller to replicate the corner case where a config change was causing all the services to be reprocessed.

Finally, the old "operator mode" is the only enabled one now, as the configmap is not there anymore.

msherif1234 · 2022-02-23T12:21:16Z

I don't think this using the latest changes from operator, can we have release for operator now and just base this PR based on it so we know exactly where we are

fedepaol · 2022-02-23T12:22:36Z

I don't think this using the latest changes from operator, can we have release for operator now and just base this PR based on it so we know exactly where we are

I'll check, but can you make an example? Are you talking about the webhooks? Or what?
The operator was released.

sabinaaledort · 2022-02-23T13:37:11Z

commit 90d4218 (Add new CR definitions as APIs) looks good

sabinaaledort · 2022-02-23T14:01:11Z

internal/config/config.go

-	}
-
-	err := validate(&raw)
+func For(crds CRSConfig, validate Validate) (*Config, error) {


Wondering about the name of the function, why For and not Parse?

Because no parsing is happening here, we are getting a Config for what we are passing.
Before we were parsing a yaml file, so the old name made sense.

I just think the name is unclear for what is done here. But maybe that's just me.
It is parsing objects. For example: parsed, err := bfdProfileFromCR(bfd)

It's not parsing. And from the calling sites, it will look config.For(crds) which I think is clear.

sabinaaledort · 2022-02-23T14:23:23Z

internal/config/config.go

-	EchoMode         bool    `yaml:"echo-mode"`
-	PassiveMode      bool    `yaml:"passive-mode"`
-	MinimumTTL       *uint32 `yaml:"minimum-ttl"`
+type CRSConfig struct {


Maybe "CRsConfig?

Yeah that name is pretty ugly, I knew it had to change while adding it :-)
K8sConfig or Resources or ClusterResources are valid alternatives, maybe the latter is the most convincing one (config.ClusterResources from outside the package)

sabinaaledort · 2022-02-24T11:24:12Z

internal/k8s/epslices/endpoint_slices.go

+func serviceNameForSlice(endpointSlice *discovery.EndpointSlice) (string, error) {
+	serviceName, ok := endpointSlice.Labels["kubernetes.io/service-name"]
+	if !ok || serviceName == "" {
+		return "", fmt.Errorf("endpointSlice missing %s label", "kubernetes.io/service-name")


I know you just copied it but should we fix the fmt.Errorf here? The %s is not needed

sabinaaledort · 2022-02-24T11:24:20Z

internal/k8s/epslices/endpoint_slices.go

+	Slices
+)
+
+const SlicesServiceIndexName = "ServiceName"


I think it's not used

we use it here https://github.com/fedepaol/metallb/blob/0f608cd2638ae4e45bd50cc580a1d236dfeb50f1/internal/k8s/k8s.go#L179 and here https://github.com/fedepaol/metallb/blob/fcc7c64bdb47ab702cfb94af6c924a3312302ad1/internal/k8s/controllers/endpoints.go#L21

Those are coming in a sequent commit, I split the commits afterwards so that could be the reason.

sabinaaledort · 2022-02-24T12:33:01Z

internal/k8s/controllers/config_controller.go

+		level.Error(r.Logger).Log("controller", "ConfigReconciler", "error", "failed to get addresspools", "error", err)
+		return ctrl.Result{RequeueAfter: RetryPeriod}, err
+	}
+	var bgpPeers metallbv1beta1.BGPPeerList


missing empty line for space?

pperiyasamy · 2022-02-25T14:25:08Z

manifests/metallb-frr.yaml

@@ -1,3 +1,411 @@
+apiVersion: apiextensions.k8s.io/v1


@fedepaol can you move CRDs definition and controller/speaker service account config into a separate file(s) ?
This way operator would just refer to these file(s) and deploy it.

I think that makes sense, but goes in the direction of changing the manifests to a more kustomize-friendly structure.
I'd defer that after we complete this PR and before we do a final release.

Not sure where to add this, we'd also need to cover this with CI - generate from api and check that the helm/raw manifests CRDs match.

My idea is, we switch to kustomize (where we can have a folder with CRDs only), and THEN we generate the -frr and vanilla manifest from there

sabinaaledort · 2022-02-27T17:51:13Z

commit 7b0623e (Replace the custom controller logic with controller-runtime) looks good

sabinaaledort · 2022-02-27T17:56:16Z

controller/main.go

-		kubeconfig      = flag.String("kubeconfig", "", "absolute path to the kubeconfig file (only needed when running outside of k8s)")
+		port      = flag.Int("port", 7472, "HTTP listening port for Prometheus metrics")
+		namespace = flag.String("namespace", os.Getenv("METALLB_NAMESPACE"), "config / memberlist secret namespace")
+		// kubeconfig      = flag.String("kubeconfig", "", "absolute path to the kubeconfig file (only needed when running outside of k8s)")


Why it's commented?

it's a result of my ramblings with controller-runtime, turns out the runtime looks for kubeconfig by itself and the parameter is no needed. I'll remove it.

sabinaaledort · 2022-02-27T18:04:10Z

Commit ab2941a (E2ETests: remove configmap mode) looks good

sabinaaledort · 2022-02-27T18:06:46Z

charts/metallb/templates/crds.yaml

+    kind: ""
+    plural: ""
+  conditions: []
+  storedVersions: []


missing new line

liornoy · 2022-02-27T18:38:21Z

api/v1alpha1/addresspool_types.go

+
+// AddressPoolSpec defines the desired state of AddressPool.
+type AddressPoolSpec struct {
+	// Protocol can be used to select how the announcement is done,


liornoy · 2022-02-27T18:38:54Z

api/v1beta1/addresspool_types.go

+
+// AddressPoolSpec defines the desired state of AddressPool.
+type AddressPoolSpec struct {
+	// Protocol can be used to select how the announcement is done,


liornoy · 2022-02-27T18:40:57Z

api/v1beta1/bgppeer_types.go

+	// +optional
+	Password string `json:"password,omitempty" yaml:"password,omitempty"`
+
+	BFDProfile string `json:"bfdProfile,omitempty" yaml:"bfdprofile,omitempty"`


shouldn't it be optional also?

maybe add the following comment: Name of the BFDProfile to attach to this peer

This version of the CRDs is transient, and must be the same as the current one in the operator, so we'll be able to write conversion webhook and to preserve backward compatibility for those users already consuming the CRDs from the operator. For this particular annotation, I think it's useless since we are already able to write BGPPeers without bfdprofile(s), but that applies to the whole definition.

liornoy · 2022-02-27T18:46:44Z

internal/config/config.go

 	port := uint16(179)
-	if p.Port != 0 {
-		port = p.Port
+	if p.Spec.Port != 0 {
+		port = p.Spec.Port


should we add port default value=179 to the CRD instead of this?

See the comment above, I will do in the new version of the API

sabinaaledort · 2022-02-28T10:42:05Z

I reviewed all the commits. Left few small comments. Overall looks good.

liornoy · 2022-03-02T12:25:21Z

internal/config/config.go

 	ret := &Pool{
-		Protocol:      p.Protocol,
-		AvoidBuggyIPs: p.AvoidBuggyIPs,
+		Protocol:      Proto(p.Spec.Protocol), // TODO CRDs: validate proto


If the addresspool is coming from a k8s custom resource,
that has the Kubebuilder validation for enum: layer2; bgp.
why another validation is required here?

that was a reminder for me because I don't like unchecked castings. I can remove the TODO, this version of CRDs / translation is transient and this part will go away in any case.

liornoy · 2022-03-02T12:25:54Z

internal/config/config.go

-	res.EchoInterval, err = bfdIntFromConfig(p.EchoInterval, 10, 60000)
+	res.EchoInterval, err = bfdIntFromConfig(p.Spec.EchoInterval, 10, 60000)
 	if err != nil {
 		return nil, errors.Wrap(err, "invalid minimum ttl value")


nit: "invalid echo interval value"

liornoy · 2022-03-02T12:26:26Z

internal/config/config.go

 }

-func parseBGPAdvertisements(ads []bgpAdvertisement, cidrsPerAddresses map[string][]*net.IPNet, communities map[string]uint32) ([]*BGPAdvertisement, error) {
+func bgpAdvertisementsFromCR(ads []metallbv1beta1.BgpAdvertisement, cidrsPerAddresses map[string][]*net.IPNet, communities map[string]uint32) ([]*BGPAdvertisement, error) {


Just an opinion: it would be nice to have a lint rule that limits
code lines to 120 chars. (this one is 173 chars long and runs off the screen)

Yeah I don't know if / what go guidelines are, but makes sense I guess

liornoy · 2022-03-02T12:27:08Z

internal/config/config.go

-		if rawAd.AggregationLength != nil {
-			ad.AggregationLength = *rawAd.AggregationLength
+		if crdAd.AggregationLength != nil {
+			ad.AggregationLength = int(*crdAd.AggregationLength) // TODO CRD cast


what does it mean CRD cast?

it's another reminder that this could be done in a more elegant way, but I'll remove it

Import the api definition as it is from the metallb operator. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

In order to preserve metallb's non k8s internal structure, we keep the configuration as it is today, but we build it starting from the collection of the CRs (in a single struct) as opposed to doing that using the raw configfile. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

This is required to avoid circular dependencies between the k8s package and the new added controllers package. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

We use controller-runtime to create a controllers listening for the newly added CRs. At the same time we add controllers for services and nodes. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Here we do the wiring between the added controllers and the k8s independent logic scattered across the internal folder. We preserve the handlers, and we change the way we feed them. A new set of controllers listen for config, nodes, services, and call the previous callbacks. No need to wait for the controllers to be synced, because now this is handled by the manager. The corner case where we ask for reprocessing all the services is handled by the ServiceReload controller, which listens to a channel for events. It's one way to avoid chainging the existing logic, because it assumes one controller in charge of the reload and other controllers able to signal it. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Update the packages moved to avoid circular dependencies. Also, since the manager handles the check for the caches to be synced before starting, there is no need anymore to do it manually. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Fixing the dependencies moved to avoid circular dependencies. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Now that the configmap is removed, we remove the configmap mode from the tests and we leave only CRs. We still have the updater, as we'll change the format of the CRs and we'll want to guarantee backward compatibility. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

We add the new CRDs and set the permissions, and we remove all the configmap related configurations. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

We remove the permissions on the configmaps and add the CRDs and the related permissions. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

We need to adjust the operator not to ship the CRs and only to deploy metallb and its CRDs. Until then, the operator lane must be disabled. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

We can now reach frr from different controllers (i.e. when the config changes, or when a service changes) so we need to protect it to avoid races. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

The core implementation assumes that the events are processed serially, because originally there was a single queue being fed by all the events. When switching to controller-runtime we have several controllers, each one backed up by a different goroutine. To preserve the seriality, we add a mutex to avoid concurrent processing of k8s events. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

liornoy · 2022-03-06T14:05:15Z

Should we delete the manifests/tutorial.yaml files (configmap) and supply CRs yaml examples instead?

fedepaol · 2022-03-07T08:40:57Z

Should we delete the manifests/tutorial.yaml files (configmap) and supply CRs yaml examples instead?

Yes, but I'd do that when everything settles down and we start working on the docs.

sabinaaledort · 2022-03-08T15:18:01Z

/lgtm

mingfang · 2022-07-06T15:17:35Z

bring back configmap support. CRDs are terrible in general.

fedepaol requested review from johananl, rata and russellb as code owners February 22, 2022 14:27

fedepaol changed the title ~~Move the CRDs in MetalLB~~ Move the CRDs to MetalLB Feb 22, 2022

fedepaol force-pushed the crdsinmetallb branch 2 times, most recently from 29fd9ce to a44d321 Compare February 22, 2022 16:32

fedepaol force-pushed the crdsinmetallb branch 4 times, most recently from 0656b47 to 900e23c Compare February 23, 2022 12:54

sabinaaledort reviewed Feb 23, 2022

View reviewed changes

fedepaol force-pushed the crdsinmetallb branch from 900e23c to 7535d7f Compare February 23, 2022 14:15

sabinaaledort reviewed Feb 23, 2022

View reviewed changes

pperiyasamy mentioned this pull request Feb 23, 2022

Remove CRDs and its validating webhooks metallb/metallb-operator#169

Merged

fedepaol force-pushed the crdsinmetallb branch from 7535d7f to f7f3489 Compare February 23, 2022 17:28

sabinaaledort reviewed Feb 24, 2022

View reviewed changes

fedepaol force-pushed the crdsinmetallb branch from f7f3489 to 3fdb522 Compare February 24, 2022 17:21

pperiyasamy reviewed Feb 25, 2022

View reviewed changes

sabinaaledort reviewed Feb 27, 2022

View reviewed changes

liornoy reviewed Feb 27, 2022

View reviewed changes

fedepaol force-pushed the crdsinmetallb branch from d65d744 to 61855dc Compare February 28, 2022 13:17

fedepaol force-pushed the crdsinmetallb branch 2 times, most recently from 96adbbf to bfdb1ec Compare March 2, 2022 11:45

liornoy reviewed Mar 2, 2022

View reviewed changes

fedepaol force-pushed the crdsinmetallb branch 2 times, most recently from b6ae19d to 9a1bd08 Compare March 2, 2022 16:26

fedepaol mentioned this pull request Mar 3, 2022

Move the CRDs to MetalLB, step 2 #1245

Merged

fedepaol added 13 commits March 3, 2022 17:46

Add new CR definitions as APIs

6bfddf4

Import the api definition as it is from the metallb operator. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Move the endpointslices structure to a different package

f9edeb9

This is required to avoid circular dependencies between the k8s package and the new added controllers package. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Add controllers for the CRs

fe5f742

We use controller-runtime to create a controllers listening for the newly added CRs. At the same time we add controllers for services and nodes. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Speaker: adjust moved dependencies

9ca9a48

Fixing the dependencies moved to avoid circular dependencies. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Helm: add CRDs and remove configmap

681f8db

We add the new CRDs and set the permissions, and we remove all the configmap related configurations. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Manifests: add CRDs

f7641aa

We remove the permissions on the configmaps and add the CRDs and the related permissions. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

CI: disable the operator lane

62aecc7

We need to adjust the operator not to ship the CRs and only to deploy metallb and its CRDs. Until then, the operator lane must be disabled. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

FRR: protect the session manager with a mutex

4a3a2d0

We can now reach frr from different controllers (i.e. when the config changes, or when a service changes) so we need to protect it to avoid races. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

fedepaol force-pushed the crdsinmetallb branch from 9a1bd08 to 323584a Compare March 3, 2022 16:47

Merge branch 'main' into crdsinmetallb

9f661ef

fedepaol merged commit 1b0a48f into metallb:main Mar 9, 2022

fedepaol mentioned this pull request Mar 12, 2022

HEADS UP: Breaking changes in the MetalLB main branch #1257

Closed

simu mentioned this pull request Oct 11, 2022

Implement CRD-based configuration projectsyn/component-metallb#39

Closed

cprivitere mentioned this pull request Oct 12, 2022

Add MetalLB 0.13 support kubernetes-sigs/cloud-provider-equinix-metal#322

Closed

eyenx mentioned this pull request Oct 19, 2022

chore(misc/metallb): update to 0.13.x adfinis/helm-charts#834

Merged

6 tasks

Move the CRDs to MetalLB #1237

Move the CRDs to MetalLB #1237

Uh oh!

Conversation

fedepaol commented Feb 22, 2022

Uh oh!

msherif1234 commented Feb 23, 2022

Uh oh!

fedepaol commented Feb 23, 2022

Uh oh!

sabinaaledort commented Feb 23, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sabinaaledort commented Feb 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sabinaaledort commented Feb 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fedepaol Feb 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sabinaaledort commented Feb 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

sabinaaledort commented Feb 27, 2022 •

edited

Loading

fedepaol Feb 28, 2022 •

edited

Loading