Move the CRDs to MetalLB, step 2 by fedepaol · Pull Request #1245 · metallb/metallb

fedepaol · 2022-03-03T16:45:50Z

This is based on #1237 , and is the second part related to what's being proposed in #942

Here we move from the version of CRDs shipped with the operator to the final design.
We still lack a CRD for the communities and a label selector for selecting the addresspool from the L2 and the BGPAdvertisements, which are gonna be added as a next step.

Also, the e2e tests are updated to reflect this new structure.

sabinaaledort · 2022-03-08T13:58:33Z

+
+// AddressPoolStatus defines the observed state of AddressPool.
+type AddressPoolStatus struct {
+	// INSERT ADDITIONAL STATUS FIELD - define observed state of cluster


is that a TODO?

It's generated by controller-gen, and it was the same in the operator.

sabinaaledort · 2022-03-08T13:59:37Z

+
+// BGPAdvertisementStatus defines the observed state of BGPAdvertisement.
+type BGPAdvertisementStatus struct {
+	// INSERT ADDITIONAL STATUS FIELD - define observed state of cluster


is that a TODO?

It's generated by controller-gen, and it was the same in the operator.

sabinaaledort · 2022-03-08T13:59:50Z

+}
+
+// BFDProfileStatus defines the observed state of BFDProfile.
+type BFDProfileStatus struct {


missing TODO comment?

It's generated by controller-gen, and it was the same in the operator.

sabinaaledort · 2022-03-08T15:46:25Z

-	flag.StringVar(&framework.TestContext.Provider, "provider", "", "The name of the Kubernetes provider (gce, gke, local, skeleton (the fallback if not set), etc.)")
-	framework.TestContext.KubeConfig = os.Getenv(clientcmd.RecommendedConfigPathEnvVar)
+	framework.RegisterClusterFlags(flag.CommandLine)
+	//flag.StringVar(&framework.TestContext.Provider, "provider", "", "The name of the Kubernetes provider (gce, gke, local, skeleton (the fallback if not set), etc.)")


why it's commented?

it's not needed any more as we are not importing the crds from the operator anymore, need to delete it.

sabinaaledort · 2022-03-09T07:11:43Z

+	DetectMultiplier *uint32 `json:"detectMultiplier,omitempty"`
+	// +kubebuilder:validation:Maximum:=60000
+	// +kubebuilder:validation:Minimum:=10
+	EchoInterval *uint32 `json:"echoInterval,omitempty"`


What about echoTransmitInterval? I see it in the design proposal

That was because the proposal was written when we were aiming at frr 8. Frr 7.5 only has echo interval.
That was the same in the operator, and in the configmap.

sabinaaledort · 2022-03-09T07:42:28Z

-				}
-				ad.Communities[v] = true
-			}
+func validateBGPAddPerPool(adv *BGPAdvertisement, pool *Pool) error {


validateBGPAddPerPool -> validateBGPAdvPerPool

sabinaaledort · 2022-03-10T07:06:17Z

+	case BGP:
+		ads, err := bgpAdvertisementsFromLegacyCR(p.Spec.BGPAdvertisements, cidrsPerAddresses, bgpCommunities)
+		if err != nil {
+			return nil, fmt.Errorf("parsing BGP communities: %s", err)


The err msg here is misleading. bgpAdvertisementsFromLegacyCR func parsing not just communities.

sabinaaledort · 2022-03-10T07:34:41Z

-			pool := c1.config.Pools[poolFor(c1.config.Pools, []net.IP{lbIP})]
-			response1 := c1.protocols[pool.Protocol].ShouldAnnounce(l, "test1", []net.IP{lbIP}, svc, test.eps[lbIP_s])
-			response2 := c2.protocols[pool.Protocol].ShouldAnnounce(l, "test1", []net.IP{lbIP}, svc, test.eps[lbIP_s])
+			// pool := c1.config.Pools[poolFor(c1.config.Pools, []net.IP{lbIP})]


should this commented line be removed?

sabinaaledort · 2022-03-10T07:36:25Z

@@ -198,11 +200,12 @@ type controllerConfig struct {

 	// For testing only, and will be removed in a future release.


i know it's unrelated to this PR, but do you know something about this comment from 4 years ago? the issue was closed.

liornoy · 2022-03-22T06:41:04Z

+		t.Fatal("two handlers, ip is not announced in bgp")
+	}
+	if !c.announced[config.Layer2]["testsvc"] {
+		t.Fatal("two handlers, ip is not announced in bgp")


liornoy · 2022-03-22T06:41:28Z

+		t.Fatal("one handler, ip is not announced in bgp")
+	}
+	if c.announced[config.Layer2]["testsvc"] {
+		t.Fatal("one handler, ip is not announced in bgp")


nit: ip is announced in l2

liornoy · 2022-03-22T06:43:36Z

+		t.Fatal("no handlers, l2 handler was called")
+	}
+	if bgpMockHandler.setBalancerCalled {
+		t.Fatal("no handlers, bgp handler was not called")


nit: was called

liornoy · 2022-03-22T06:45:31Z

+	if l2MockHandler.setBalancerCalled {
+		t.Fatal("no handlers, l2 handler was called")
+	}


it is a duplication of lines 144-146

liornoy · 2022-03-22T06:46:03Z

+		t.Fatal("no handlers, l2 handler was called")
+	}
+	if !bgpMockHandler.deleteBalancerCalled {
+		t.Fatal("no handlers, l2 delete handler was not called")


nit: bgp delete handler was not called

Done, thanks!

liornoy · 2022-03-22T06:46:36Z

+		t.Fatal("no handlers, ip is announced in bgp")
+	}
+	if c.announced[config.Layer2]["testsvc"] {
+		t.Fatal("no handlers, ip is announced in bgp")


Done, thanks!

This is the evolution of the CRDs defined in the operator, separating the IP allocation function handled by the address pool, the connection with bgp peers handled by the BGPPeer CRD, and the intent to advertise a prefix over L2 or BGP (or both) via the L2Advertisement and BGPAdvertisement CRDs. Instead of having a different addersspool version, we create a new CRD (IPPool) which will live together with the old address pool. This is required because it's not possible to have converting webhook from one CRD (addresspool) to two (addresspool, new version + BGP Advertisement or L2 advertisement). Also, it's not possible to have two different versions of the same CR stored in etcd. For this reason, it's safer to have a new CR and make metallb work with the old one. BGPPeer is then moved to v1beta2, because there it's possible to translate the node selector, and we'll add new fields in a compatible manner. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

With the separation of the addresspool from the protocol and the introduction of l2 and bgp advertisement, we change the internal structure so that the pool also contains the reference to its advertisements. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

AddressPool is the legacy CRD, and we need to handle it together with IPPools. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

In order to understand if an address (or a prefix) must be advertised, we now need to look at the list of advertisements the pool comes with. Also, now it's possible to have multiple protocols for the same addresspool, so the speaker internal structure changes to accomodate this. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

We change the tests to reflect the new structure, and the config updater to write directly the CRs instead of jumping in the intermediate configuration. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Cleaning up go.mod and go.sum after dependency changes. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Aligning the manifests to reflect the changes in the api. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Aligning the helm charts to reflect the changes in the api. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

We want to ensure that IPPool and AddressPools work together in order to offer compatibility to existing users. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

The configmap name is not needed anymore as there is no configmap, so removing the environment variable. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

The first parameter is of the wrong type, setting it to string. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

We removed the defaulting when parsing the config, so we want to ensure that the default value is being set on the cr. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

oribon · 2022-03-22T09:03:19Z

LGTM

Now that the integration code is a bit more complex, because it needs to take into account multiple protocols, we add a test that validates it. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

adampl · 2022-03-29T16:46:37Z

Is this going to resolve #277 ?

Nope, but it will pave the road for that. Feature wise, this is not introducing anything new, just a different (and cleaner) way to configure metallb. When this is in place, the interface would ideally be set on the l2 advertisement.

@fedepaol Thanks, at least it's good to know it will be possible in the future :)

fedepaol requested review from johananl, rata and russellb as code owners March 3, 2022 16:45

fedepaol force-pushed the newcrdsinmetallb branch 10 times, most recently from bd9404a to 7a9360b Compare March 8, 2022 11:25

sabinaaledort reviewed Mar 8, 2022

View reviewed changes

fedepaol force-pushed the newcrdsinmetallb branch from 7a9360b to c35f4b2 Compare March 8, 2022 16:07

sabinaaledort reviewed Mar 9, 2022

View reviewed changes

fedepaol force-pushed the newcrdsinmetallb branch 3 times, most recently from 573f797 to 4b363fc Compare March 9, 2022 14:30

sabinaaledort reviewed Mar 10, 2022

View reviewed changes

fedepaol force-pushed the newcrdsinmetallb branch 4 times, most recently from 91ce27b to 501e21a Compare March 11, 2022 13:25

fedepaol force-pushed the newcrdsinmetallb branch 3 times, most recently from 6907124 to c6a6704 Compare March 21, 2022 21:00

liornoy reviewed Mar 22, 2022

View reviewed changes

fedepaol added 12 commits March 22, 2022 09:58

Config: handle the legacy addresspool together with IPPools

23ad15e

AddressPool is the legacy CRD, and we need to handle it together with IPPools. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

E2e: use the new crd structure

ac2278b

We change the tests to reflect the new structure, and the config updater to write directly the CRs instead of jumping in the intermediate configuration. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Apply go mod tidy

e8e1287

Cleaning up go.mod and go.sum after dependency changes. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Manifests: add the new CRDS

2ee36ce

Aligning the manifests to reflect the changes in the api. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Helm: align the crds to the new version

b0c3049

Aligning the helm charts to reflect the changes in the api. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

E2E: add backward compatibility tests

94e3230

We want to ensure that IPPool and AddressPools work together in order to offer compatibility to existing users. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

CI: remove configmap name override when running helm

6ce8cac

The configmap name is not needed anymore as there is no configmap, so removing the environment variable. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Fix NeighborInfo function parameter

6a686f6

The first parameter is of the wrong type, setting it to string. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

E2E: check the default port of the bgppeer crd is set

ee57bba

We removed the defaulting when parsing the config, so we want to ensure that the default value is being set on the cr. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Speaker: add a unit test for the integration code

ea828a8

Now that the integration code is a bit more complex, because it needs to take into account multiple protocols, we add a test that validates it. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

fedepaol force-pushed the newcrdsinmetallb branch from c6a6704 to ea828a8 Compare March 22, 2022 09:08

Merge branch 'main' into newcrdsinmetallb

0973b4a

fedepaol merged commit 54e0c0f into metallb:main Mar 22, 2022

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

		@@ -198,11 +200,12 @@ type controllerConfig struct {

		// For testing only, and will be removed in a future release.

Conversation

fedepaol commented Mar 3, 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!

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!

oribon commented Mar 22, 2022

Uh oh!

adampl commented Mar 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants