Repository: metallb/metallb
Pull request #1608 (2022-09-13 11:43:57)

Layer2: send event when specified interface doesn't exist
Layer2: send event to service when specified interface doesn't exist

fedepaol: A couple of small comments, plus: would it make sense to have a test for this? We already have the code infrastructure for fetching the events, should be straightforward.
qingwusunny: e2etest has been added @fedepaol 
liornoy: No comments from me. looks good.
fedepaol: Thanks! One nit about the test and it's good to go!
fedepaol: LGTM, thanks!

Patch:
diff --git a/internal/layer2/announcer.go b/internal/layer2/announcer.go
index b6fa3b7f..89971ba5 100644
--- a/internal/layer2/announcer.go
+++ b/internal/layer2/announcer.go
@@ -19,10 +19,11 @@ type Announce struct {
 	logger log.Logger
 
 	sync.RWMutex
-	arps     map[int]*arpResponder
-	ndps     map[int]*ndpResponder
-	ips      map[string][]IPAdvertisement // svcName -> IPAdvertisements
-	ipRefcnt map[string]int               // ip.String() -> number of uses
+	nodeInterfaces []string // current local interfaces' name list
+	arps           map[int]*arpResponder
+	ndps           map[int]*ndpResponder
+	ips            map[string][]IPAdvertisement // svcName -> IPAdvertisements
+	ipRefcnt       map[string]int               // ip.String() -> number of uses
 
 	// This channel can block - do not write to it while holding the mutex
 	// to avoid deadlocking.
@@ -32,12 +33,13 @@ type Announce struct {
 // New returns an initialized Announce.
 func New(l log.Logger) (*Announce, error) {
 	ret := &Announce{
-		logger:   l,
-		arps:     map[int]*arpResponder{},
-		ndps:     map[int]*ndpResponder{},
-		ips:      map[string][]IPAdvertisement{},
-		ipRefcnt: map[string]int{},
-		spamCh:   make(chan IPAdvertisement, 1024),
+		logger:         l,
+		nodeInterfaces: []string{},
+		arps:           map[int]*arpResponder{},
+		ndps:           map[int]*ndpResponder{},
+		ips:            map[string][]IPAdvertisement{},
+		ipRefcnt:       map[string]int{},
+		spamCh:         make(chan IPAdvertisement, 1024),
 	}
 	go ret.interfaceScan()
 	go ret.spamLoop()
@@ -63,8 +65,10 @@ func (a *Announce) updateInterfaces() {
 	defer a.Unlock()
 
 	keepARP, keepNDP := map[int]bool{}, map[int]bool{}
+	curIfs := make([]string, 0, len(ifs))
 	for _, intf := range ifs {
 		ifi := intf
+		curIfs = append(curIfs, ifi.Name)
 		l := log.With(a.logger, "interface", ifi.Name)
 		addrs, err := ifi.Addrs()
 		if err != nil {
@@ -122,6 +126,8 @@ func (a *Announce) updateInterfaces() {
 		}
 	}
 
+	a.nodeInterfaces = curIfs
+
 	for i, client := range a.arps {
 		if !keepARP[i] {
 			client.Close()
@@ -306,6 +312,16 @@ func (a *Announce) AnnounceName(name string) bool {
 	return ok
 }
 
+// GetInterfaces returns current interfaces list.
+func (a *Announce) GetInterfaces() []string {
+	a.Lock()
+	defer a.Unlock()
+
+	localInterfaces := make([]string, len(a.nodeInterfaces))
+	copy(localInterfaces, a.nodeInterfaces)
+	return localInterfaces
+}
+
 // dropReason is the reason why a layer2 protocol packet was not
 // responded to.
 type dropReason int
diff --git a/internal/layer2/ip_advertisement.go b/internal/layer2/ip_advertisement.go
index 90895f31..d08db996 100644
--- a/internal/layer2/ip_advertisement.go
+++ b/internal/layer2/ip_advertisement.go
@@ -42,6 +42,15 @@ func (i1 *IPAdvertisement) Equal(i2 *IPAdvertisement) bool {
 	return i1.interfaces.Equal(i2.interfaces)
 }
 
+func (i *IPAdvertisement) MatchInterfaces(intfs ...string) bool {
+	for _, intf := range intfs {
+		if i.matchInterface(intf) {
+			return true
+		}
+	}
+	return false
+}
+
 func (i *IPAdvertisement) matchInterface(intf string) bool {
 	if i == nil {
 		return false
diff --git a/speaker/bgp_controller.go b/speaker/bgp_controller.go
index f933822f..5147e669 100644
--- a/speaker/bgp_controller.go
+++ b/speaker/bgp_controller.go
@@ -247,7 +247,7 @@ func (c *bgpController) syncBFDProfiles(profiles map[string]*config.BFDProfile)
 	return c.sessionManager.SyncBFDProfiles(profiles)
 }
 
-func (c *bgpController) SetBalancer(l log.Logger, name string, lbIPs []net.IP, pool *config.Pool) error {
+func (c *bgpController) SetBalancer(l log.Logger, name string, lbIPs []net.IP, pool *config.Pool, _ service, _ *v1.Service) error {
 	c.svcAds[name] = nil
 	for _, lbIP := range lbIPs {
 		for _, adCfg := range pool.BGPAdvertisements {
diff --git a/speaker/layer2_controller.go b/speaker/layer2_controller.go
index c07d2bee..2448f0e3 100644
--- a/speaker/layer2_controller.go
+++ b/speaker/layer2_controller.go
@@ -134,9 +134,16 @@ func (c *layer2Controller) ShouldAnnounce(l log.Logger, name string, toAnnounce
 	return "notOwner"
 }
 
-func (c *layer2Controller) SetBalancer(l log.Logger, name string, lbIPs []net.IP, pool *config.Pool) error {
+func (c *layer2Controller) SetBalancer(l log.Logger, name string, lbIPs []net.IP, pool *config.Pool, client service, svc *v1.Service) error {
+	ifs := c.announcer.GetInterfaces()
 	for _, lbIP := range lbIPs {
 		ipAdv := ipAdvertisementFor(lbIP, c.myNode, pool.L2Advertisements)
+		if !ipAdv.MatchInterfaces(ifs...) {
+			level.Warn(l).Log("op", "SetBalancer", "protocol", "layer2", "service", name, "IPAdvertisement", ipAdv,
+				"localIfs", ifs, "msg", "the specified interfaces used to announce LB IP don't exist")
+			client.Errorf(svc, "announceFailed", "the interfaces specified by LB IP %q doesn't exist in assigned node %q with protocol %q", lbIP.String(), c.myNode, config.Layer2)
+			continue
+		}
 		c.announcer.SetBalancer(name, ipAdv)
 	}
 	return nil
diff --git a/speaker/main.go b/speaker/main.go
index 63b79c66..d840f45b 100644
--- a/speaker/main.go
+++ b/speaker/main.go
@@ -323,7 +323,7 @@ func (c *controller) handleService(l log.Logger,
 		return c.deleteBalancerProtocol(l, protocol, name, deleteReason)
 	}
 
-	if err := handler.SetBalancer(l, name, lbIPs, pool); err != nil {
+	if err := handler.SetBalancer(l, name, lbIPs, pool, c.client, svc); err != nil {
 		level.Error(l).Log("op", "setBalancer", "error", err, "msg", "failed to announce service")
 		return controllers.SyncStateError
 	}
@@ -472,7 +472,7 @@ func (c *controller) SetNode(l log.Logger, node *v1.Node) controllers.SyncState
 type Protocol interface {
 	SetConfig(log.Logger, *config.Config) error
 	ShouldAnnounce(log.Logger, string, []net.IP, *config.Pool, *v1.Service, epslices.EpsOrSlices) string
-	SetBalancer(log.Logger, string, []net.IP, *config.Pool) error
+	SetBalancer(log.Logger, string, []net.IP, *config.Pool, service, *v1.Service) error
 	DeleteBalancer(log.Logger, string, string) error
 	SetNode(log.Logger, *v1.Node) error
 }


Test patch:
diff --git a/e2etest/l2tests/interface_selector.go b/e2etest/l2tests/interface_selector.go
index 1aba4e85..868de727 100644
--- a/e2etest/l2tests/interface_selector.go
+++ b/e2etest/l2tests/interface_selector.go
@@ -4,6 +4,7 @@ package l2tests
 
 import (
 	"context"
+	"fmt"
 	"time"
 
 	"github.com/onsi/ginkgo"
@@ -173,5 +174,54 @@ var _ = ginkgo.Describe("L2-interface selector", func() {
 				}, 10*time.Second, 1*time.Second).Should(gomega.Not(gomega.HaveOccurred()))
 			}
 		})
+
+		ginkgo.It("Specify not existing interfaces", func() {
+			resources := internalconfig.ClusterResources{
+				L2Advs: []metallbv1beta1.L2Advertisement{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "with-interfaces",
+						},
+						Spec: metallbv1beta1.L2AdvertisementSpec{
+							Interfaces: []string{"foo"},
+						},
+					},
+				},
+			}
+			err := ConfigUpdater.Update(resources)
+			framework.ExpectNoError(err)
+
+			svc, _ := service.CreateWithBackend(cs, f.Namespace.Name, "lb-service")
+			defer func() {
+				err := cs.CoreV1().Services(svc.Namespace).Delete(context.TODO(), svc.Name, metav1.DeleteOptions{})
+				framework.ExpectNoError(err)
+			}()
+
+			ingressIP := e2eservice.GetIngressPoint(&svc.Status.LoadBalancer.Ingress[0])
+			err = mac.FlushIPNeigh(ingressIP, executor.Host)
+			framework.ExpectNoError(err)
+
+			// check arp respond
+			for i := range LocalNics {
+				gomega.Consistently(func() error {
+					return mac.RequestAddressResolutionFromIface(ingressIP, LocalNics[i], executor.Host)
+				}, 10*time.Second, 1*time.Second).Should(gomega.HaveOccurred())
+			}
+
+			// check announceFailed event
+			gomega.Eventually(func() error {
+				events, err := cs.CoreV1().Events(svc.Namespace).List(context.Background(), metav1.ListOptions{FieldSelector: "reason=announceFailed"})
+				if err != nil {
+					return err
+				}
+
+				for _, e := range events.Items {
+					if e.InvolvedObject.Name == svc.Name {
+						return nil
+					}
+				}
+				return fmt.Errorf("service hasn't receive the \"announceFailed\" event")
+			}, 1*time.Minute, 1*time.Second).Should(gomega.BeNil())
+		})
 	})
 })
diff --git a/speaker/speaker_test.go b/speaker/speaker_test.go
index efae2ff5..85a36341 100644
--- a/speaker/speaker_test.go
+++ b/speaker/speaker_test.go
@@ -179,7 +179,7 @@ func (m *MockProtocol) ShouldAnnounce(_ log.Logger, _ string, _ []net.IP, _ *con
 	return "no announce"
 }
 
-func (m *MockProtocol) SetBalancer(_ log.Logger, _ string, _ []net.IP, _ *config.Pool) error {
+func (m *MockProtocol) SetBalancer(_ log.Logger, _ string, _ []net.IP, _ *config.Pool, _ service, _ *v1.Service) error {
 	m.setBalancerCalled = true
 	return nil
 }