From 3bea049ce95e5f6be55e66482e9eff9b7130e182 Mon Sep 17 00:00:00 2001 From: Pablo Acevedo Montserrat Date: Fri, 5 Jul 2024 17:19:41 +0200 Subject: [PATCH 1/3] OCPBUGS-36587: filter ip family in router service controller --- pkg/loadbalancerservice/controller.go | 6 +++++- pkg/loadbalancerservice/router.go | 21 ++++++++++++++------- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/pkg/loadbalancerservice/controller.go b/pkg/loadbalancerservice/controller.go index f5462fee4f..fa29d17f93 100644 --- a/pkg/loadbalancerservice/controller.go +++ b/pkg/loadbalancerservice/controller.go @@ -32,6 +32,8 @@ type LoadbalancerServiceController struct { NICNames []string NodeIP string KubeConfig string + Ipv4 bool + Ipv6 bool client *kubernetes.Clientset indexer cache.Indexer queue workqueue.RateLimitingInterface @@ -55,6 +57,8 @@ func NewLoadbalancerServiceController(cfg *config.Config) *LoadbalancerServiceCo NICNames: nicNames, NodeIP: cfg.Node.NodeIP, KubeConfig: cfg.KubeConfigPath(config.KubeAdmin), + Ipv4: cfg.IsIPv4(), + Ipv6: cfg.IsIPv6(), } } @@ -123,7 +127,7 @@ func (c *LoadbalancerServiceController) Run(ctx context.Context, ready chan<- st go wait.Until(c.runWorker, time.Second, stopCh) - go defaultRouterWatch(c.IPAddresses, c.NICNames, c.updateDefaultRouterServiceStatus, stopCh) + go defaultRouterWatch(c.IPAddresses, c.NICNames, c.Ipv4, c.Ipv6, c.updateDefaultRouterServiceStatus, stopCh) close(ready) diff --git a/pkg/loadbalancerservice/router.go b/pkg/loadbalancerservice/router.go index 61ca921a3d..fd84b6d903 100644 --- a/pkg/loadbalancerservice/router.go +++ b/pkg/loadbalancerservice/router.go @@ -18,11 +18,11 @@ const ( type serviceUpdateFunction func([]string) error -func defaultRouterWatch(ipAddresses, nicNames []string, updateFunc serviceUpdateFunction, stopCh <-chan struct{}) { +func defaultRouterWatch(ipAddresses, nicNames []string, ipv4, ipv6 bool, updateFunc serviceUpdateFunction, stopCh <-chan struct{}) { updateChan := make(chan netlink.AddrUpdate) doneChan := make(chan struct{}) for { - ips, err := defaultRouterListenAddresses(ipAddresses, nicNames) + ips, err := defaultRouterListenAddresses(ipAddresses, nicNames, ipv4, ipv6) if err != nil { klog.ErrorS(err, "unable to determine default router listening addresses") continue @@ -41,7 +41,7 @@ func defaultRouterWatch(ipAddresses, nicNames []string, updateFunc serviceUpdate for { select { case <-updateChan: - ips, err := defaultRouterListenAddresses(ipAddresses, nicNames) + ips, err := defaultRouterListenAddresses(ipAddresses, nicNames, ipv4, ipv6) if err != nil { klog.Errorf("unable to determine default router listening addresses: %v", err) break @@ -67,7 +67,7 @@ func isDefaultRouterService(svc *corev1.Service) bool { svc.Namespace == defaultRouterServiceNamespace } -func defaultRouterListenAddresses(ipAddresses, nicNames []string) ([]string, error) { +func defaultRouterListenAddresses(ipAddresses, nicNames []string, ipv4, ipv6 bool) ([]string, error) { allowedAddresses, err := config.AllowedListeningIPAddresses() if err != nil { return nil, err @@ -98,7 +98,7 @@ func defaultRouterListenAddresses(ipAddresses, nicNames []string) ([]string, err klog.Warningf("NIC %v not found in the host. Skipping", nicName) continue } - nicAddresses, err := ipAddressesFromNIC(nicName) + nicAddresses, err := ipAddressesFromNIC(nicName, ipv4, ipv6) if err != nil { return nil, err } @@ -122,13 +122,20 @@ func defaultRouterListenAddresses(ipAddresses, nicNames []string) ([]string, err return ipList, nil } -func ipAddressesFromNIC(name string) ([]string, error) { +func ipAddressesFromNIC(name string, ipv4, ipv6 bool) ([]string, error) { link, err := netlink.LinkByName(name) if err != nil { return nil, err } - addrList, err := netlink.AddrList(link, netlink.FAMILY_ALL) + family := netlink.FAMILY_V4 + if ipv6 { + family = netlink.FAMILY_ALL + if !ipv4 { + family = netlink.FAMILY_V6 + } + } + addrList, err := netlink.AddrList(link, family) if err != nil { return nil, err } From 1c3160efca4933a46fb8e3f1f51038286366720e Mon Sep 17 00:00:00 2001 From: Pablo Acevedo Montserrat Date: Fri, 5 Jul 2024 23:27:09 +0200 Subject: [PATCH 2/3] OCPBUGS-36587: Add further validations to listenAddress and IP family --- pkg/config/config.go | 26 ++++++++++++++++++-------- pkg/config/config_test.go | 22 ++++++++++++++++++++++ pkg/loadbalancerservice/router.go | 2 +- 3 files changed, 41 insertions(+), 9 deletions(-) diff --git a/pkg/config/config.go b/pkg/config/config.go index a21e3d48a6..de540af62a 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -380,7 +380,7 @@ func (c *Config) validate() error { } if len(c.Ingress.ListenAddress) != 0 { - if err := validateRouterListenAddress(c.Ingress.ListenAddress, c.ApiServer.AdvertiseAddress, c.ApiServer.SkipInterface); err != nil { + if err := validateRouterListenAddress(c.Ingress.ListenAddress, c.ApiServer.AdvertiseAddress, c.ApiServer.SkipInterface, c.IsIPv4(), c.IsIPv6()); err != nil { return fmt.Errorf("error validating ingress.listenAddress: %w", err) } } @@ -464,8 +464,8 @@ func checkAdvertiseAddressConfigured(advertiseAddress string) error { return fmt.Errorf("Advertise address: %s not present in any interface", advertiseAddress) } -func validateRouterListenAddress(ingressListenAddresses []string, advertiseAddress string, skipInterface bool) error { - addresses, err := AllowedListeningIPAddresses() +func validateRouterListenAddress(ingressListenAddresses []string, advertiseAddress string, skipInterface, ipv4, ipv6 bool) error { + addresses, err := AllowedListeningIPAddresses(ipv4, ipv6) if err != nil { return err } @@ -474,10 +474,13 @@ func validateRouterListenAddress(ingressListenAddresses []string, advertiseAddre return err } for _, entry := range ingressListenAddresses { - if net.ParseIP(entry) != nil { + if ip := net.ParseIP(entry); ip != nil { if entry == advertiseAddress && !skipInterface { continue } + if (ip.To4() != nil && !ipv4) || (ip.To4() == nil && !ipv6) { + return fmt.Errorf("IP %v does not match family of service/cluster network", entry) + } if !slices.Contains(addresses, entry) { return fmt.Errorf("IP %v not present in any of the host's interfaces", entry) } @@ -502,7 +505,7 @@ func getForbiddenIPs() ([]*net.IPNet, error) { return banned, nil } -func getHostAddresses() ([]net.IP, error) { +func getHostAddresses(ipv4, ipv6 bool) ([]net.IP, error) { handle, err := netlink.NewHandle() if err != nil { return nil, err @@ -511,13 +514,20 @@ func getHostAddresses() ([]net.IP, error) { if err != nil { return nil, err } + family := netlink.FAMILY_V4 + if ipv6 { + family = netlink.FAMILY_ALL + if !ipv4 { + family = netlink.FAMILY_V6 + } + } addresses := make([]net.IP, 0, len(links)*2) for _, link := range links { // Filter out slave NICs. These include ovs/ovn created interfaces, in case of a restart. if link.Attrs().ParentIndex != 0 || link.Attrs().MasterIndex != 0 { continue } - addressList, err := handle.AddrList(link, netlink.FAMILY_ALL) + addressList, err := handle.AddrList(link, family) if err != nil { return nil, err } @@ -528,12 +538,12 @@ func getHostAddresses() ([]net.IP, error) { return addresses, nil } -func AllowedListeningIPAddresses() ([]string, error) { +func AllowedListeningIPAddresses(ipv4, ipv6 bool) ([]string, error) { bannedAddresses, err := getForbiddenIPs() if err != nil { return nil, err } - hostAddresses, err := getHostAddresses() + hostAddresses, err := getHostAddresses(ipv4, ipv6) if err != nil { return nil, err } diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 9f1618004b..ab5796dc09 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -507,6 +507,28 @@ func TestValidate(t *testing.T) { }(), expectErr: true, }, + { + name: "ingress-listen-address-bad-ip-family-1", + config: func() *Config { + c := mkDefaultConfig() + c.Ingress.ListenAddress = []string{"1.2.3.4"} + c.Network.ClusterNetwork = []string{"fd01::/48"} + c.Network.ServiceNetwork = []string{"fd02::/112"} + return c + }(), + expectErr: true, + }, + { + name: "ingress-listen-address-bad-ip-family-2", + config: func() *Config { + c := mkDefaultConfig() + c.Ingress.ListenAddress = []string{"fe80::1"} + c.Network.ClusterNetwork = []string{"10.42.0.0/16"} + c.Network.ServiceNetwork = []string{"10.43.0.0/16"} + return c + }(), + expectErr: true, + }, { name: "audit-log-flag-values-unexpected-values", config: func() *Config { diff --git a/pkg/loadbalancerservice/router.go b/pkg/loadbalancerservice/router.go index fd84b6d903..4b45859907 100644 --- a/pkg/loadbalancerservice/router.go +++ b/pkg/loadbalancerservice/router.go @@ -68,7 +68,7 @@ func isDefaultRouterService(svc *corev1.Service) bool { } func defaultRouterListenAddresses(ipAddresses, nicNames []string, ipv4, ipv6 bool) ([]string, error) { - allowedAddresses, err := config.AllowedListeningIPAddresses() + allowedAddresses, err := config.AllowedListeningIPAddresses(ipv4, ipv6) if err != nil { return nil, err } From 62dc711165a48ae3024e052ae26e32195c8014a5 Mon Sep 17 00:00:00 2001 From: Pablo Acevedo Montserrat Date: Mon, 8 Jul 2024 09:36:57 +0200 Subject: [PATCH 3/3] OCPBUGS-36587: Nits for verify-go --- pkg/config/config.go | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/pkg/config/config.go b/pkg/config/config.go index de540af62a..bf161aae85 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -474,21 +474,22 @@ func validateRouterListenAddress(ingressListenAddresses []string, advertiseAddre return err } for _, entry := range ingressListenAddresses { - if ip := net.ParseIP(entry); ip != nil { - if entry == advertiseAddress && !skipInterface { + if entry == advertiseAddress && !skipInterface { + continue + } + ip := net.ParseIP(entry) + if ip == nil { + if slices.Contains(nicNames, entry) { continue } - if (ip.To4() != nil && !ipv4) || (ip.To4() == nil && !ipv6) { - return fmt.Errorf("IP %v does not match family of service/cluster network", entry) - } - if !slices.Contains(addresses, entry) { - return fmt.Errorf("IP %v not present in any of the host's interfaces", entry) - } - } else if slices.Contains(nicNames, entry) { - continue - } else { return fmt.Errorf("interface %v not present in the host", entry) } + if (ip.To4() != nil && !ipv4) || (ip.To4() == nil && !ipv6) { + return fmt.Errorf("IP %v does not match family of service/cluster network", entry) + } + if !slices.Contains(addresses, entry) { + return fmt.Errorf("IP %v not present in any of the host's interfaces", entry) + } } return nil }