From 9dfcc8787f66211a749933a29d97060b27dec0f5 Mon Sep 17 00:00:00 2001 From: Manuel Buil Date: Fri, 18 Feb 2022 16:38:43 +0100 Subject: [PATCH] Add golangci-lint github action Signed-off-by: Manuel Buil --- .github/workflows/golangci-lint.yaml | 14 ++++++ backend/awsvpc/awsvpc.go | 4 +- backend/extension/extension.go | 11 ++++- backend/extension/extension_network.go | 13 ++--- backend/gce/api.go | 7 +-- backend/ipsec/handle_charon.go | 16 +++--- backend/ipsec/ipsec_network.go | 14 +++--- backend/route_network.go | 14 +++--- backend/udp/cproxy_amd64.go | 5 +- backend/udp/udp_network_amd64.go | 15 +++--- backend/vxlan/vxlan_network.go | 12 ++--- backend/wireguard/device.go | 16 +++--- backend/wireguard/wireguard_network.go | 12 ++--- main.go | 47 ++++++++++++------ network/iptables.go | 22 +++++++-- network/iptables_test.go | 68 ++++++++++++++++++++------ pkg/ip/iface.go | 4 +- pkg/ip/iface_test.go | 3 ++ pkg/ip/ip6net.go | 5 +- pkg/mac/mac_test.go | 2 +- subnet/etcdv2/local_manager.go | 16 ------ subnet/etcdv2/mock_etcd.go | 13 +++-- subnet/etcdv2/mock_etcd_test.go | 2 +- subnet/etcdv2/registry.go | 19 ++----- subnet/etcdv2/registry_test.go | 17 +++++-- subnet/etcdv2/subnet_test.go | 32 +++++------- subnet/kube/kube.go | 8 +-- subnet/subnet_test.go | 20 ++++---- 28 files changed, 249 insertions(+), 182 deletions(-) create mode 100644 .github/workflows/golangci-lint.yaml diff --git a/.github/workflows/golangci-lint.yaml b/.github/workflows/golangci-lint.yaml new file mode 100644 index 0000000000..bbb0ac06d8 --- /dev/null +++ b/.github/workflows/golangci-lint.yaml @@ -0,0 +1,14 @@ +name: build flannel + +on: [push, pull_request] + +jobs: + golangci: + name: lint + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - name: golangci-lint + uses: golangci/golangci-lint-action@v2.5.2 + with: + args: "--out-${NO_FUTURE}format colored-line-number --skip-dirs='backend/udp' --timeout=5m" diff --git a/backend/awsvpc/awsvpc.go b/backend/awsvpc/awsvpc.go index 6c37f83714..621b7ea406 100644 --- a/backend/awsvpc/awsvpc.go +++ b/backend/awsvpc/awsvpc.go @@ -201,7 +201,7 @@ func (be *AwsVpcBackend) cleanupBlackholeRoutes(routeTableID string, network ip. if *route.State == "blackhole" && route.DestinationCidrBlock != nil { _, subnet, err := net.ParseCIDR(*route.DestinationCidrBlock) if err == nil && network.Contains(ip.FromIP(subnet.IP)) { - log.Infof("Removing blackhole route: ", *route.DestinationCidrBlock) + log.Infof("Removing blackhole route: %v", *route.DestinationCidrBlock) deleteRouteInput := &ec2.DeleteRouteInput{RouteTableId: &routeTableID, DestinationCidrBlock: route.DestinationCidrBlock} if _, err := ec2c.DeleteRoute(deleteRouteInput); err != nil { if ec2err, ok := err.(awserr.Error); !ok || ec2err.Code() != "InvalidRoute.NotFound" { @@ -299,7 +299,7 @@ func (be *AwsVpcBackend) detectRouteTableID(eni *ec2.InstanceNetworkInterface, e res, err = ec2c.DescribeRouteTables(routeTablesInput) if err != nil { - log.Infof("error describing route tables: ", err) + log.Infof("error describing route tables: %v", err) } if len(res.RouteTables) == 0 { diff --git a/backend/extension/extension.go b/backend/extension/extension.go index 32d51f1fce..1b5bd8f645 100644 --- a/backend/extension/extension.go +++ b/backend/extension/extension.go @@ -141,8 +141,15 @@ func runCmd(env []string, stdin string, name string, arg ...string) (string, err return "", err } - io.WriteString(stdinpipe, stdin) - io.WriteString(stdinpipe, "\n") + _, err = io.WriteString(stdinpipe, stdin) + if err != nil { + return "", err + } + + _, err = io.WriteString(stdinpipe, "\n") + if err != nil { + return "", err + } stdinpipe.Close() output, err := cmd.CombinedOutput() diff --git a/backend/extension/extension_network.go b/backend/extension/extension_network.go index 7d75cae47f..b3db769db2 100644 --- a/backend/extension/extension_network.go +++ b/backend/extension/extension_network.go @@ -28,7 +28,6 @@ import ( ) type network struct { - name string extIface *backend.ExternalInterface lease *subnet.Lease sm subnet.Manager @@ -60,14 +59,12 @@ func (n *network) Run(ctx context.Context) { defer wg.Wait() for { - select { - case evtBatch, ok := <-evts: - if !ok { - log.Infof("evts chan closed") - return - } - n.handleSubnetEvents(evtBatch) + evtBatch, ok := <-evts + if !ok { + log.Infof("evts chan closed") + return } + n.handleSubnetEvents(evtBatch) } } diff --git a/backend/gce/api.go b/backend/gce/api.go index 71950297ab..4532366460 100644 --- a/backend/gce/api.go +++ b/backend/gce/api.go @@ -17,12 +17,13 @@ package gce import ( + "context" "fmt" "time" - "golang.org/x/oauth2" "golang.org/x/oauth2/google" "google.golang.org/api/compute/v1" + "google.golang.org/api/option" log "k8s.io/klog" ) @@ -34,12 +35,12 @@ type gceAPI struct { } func newAPI() (*gceAPI, error) { - client, err := google.DefaultClient(oauth2.NoContext) + client, err := google.DefaultClient(context.Background()) if err != nil { return nil, fmt.Errorf("error creating client: %v", err) } - cs, err := compute.New(client) + cs, err := compute.NewService(context.TODO(), option.WithHTTPClient(client)) if err != nil { return nil, fmt.Errorf("error creating compute service: %v", err) } diff --git a/backend/ipsec/handle_charon.go b/backend/ipsec/handle_charon.go index 7cc8fce41a..816e3ca97f 100644 --- a/backend/ipsec/handle_charon.go +++ b/backend/ipsec/handle_charon.go @@ -63,13 +63,17 @@ func NewCharonIKEDaemon(ctx context.Context, wg *sync.WaitGroup, espProposal str } wg.Add(1) go func() { - select { - case <-ctx.Done(): - cmd.Process.Signal(syscall.SIGTERM) - cmd.Wait() - log.Infof("Stopped charon daemon") - wg.Done() + <-ctx.Done() + err := cmd.Process.Signal(syscall.SIGTERM) + if err != nil { + log.Errorf("Error while processing the signal: %v", err) + } + err = cmd.Wait() + if err != nil { + log.Errorf("Error while waiting for process to exit: %v", err) } + log.Infof("Stopped charon daemon") + wg.Done() }() return charon, nil } diff --git a/backend/ipsec/ipsec_network.go b/backend/ipsec/ipsec_network.go index 35c45d761a..2f619fd5d9 100644 --- a/backend/ipsec/ipsec_network.go +++ b/backend/ipsec/ipsec_network.go @@ -94,15 +94,13 @@ func (n *network) Run(ctx context.Context) { }() for { - select { - case evtsBatch, ok := <-evts: - if !ok { - log.Infof("evts chan closed") - return - } - log.Info("Handling event") - n.handleSubnetEvents(evtsBatch) + evtsBatch, ok := <-evts + if !ok { + log.Infof("evts chan closed") + return } + log.Info("Handling event") + n.handleSubnetEvents(evtsBatch) } } diff --git a/backend/route_network.go b/backend/route_network.go index 000af1184a..d1eeabe798 100644 --- a/backend/route_network.go +++ b/backend/route_network.go @@ -70,14 +70,12 @@ func (n *RouteNetwork) Run(ctx context.Context) { defer wg.Wait() for { - select { - case evtBatch, ok := <-evts: - if !ok { - log.Infof("evts chan closed") - return - } - n.handleSubnetEvents(evtBatch) + evtBatch, ok := <-evts + if !ok { + log.Infof("evts chan closed") + return } + n.handleSubnetEvents(evtBatch) } } @@ -169,7 +167,7 @@ func routeAdd(route *netlink.Route, ipFamily int, addToRouteList, removeFromRout log.Errorf("Error adding route to %v", route) return } - routeList, err = netlink.RouteListFiltered(ipFamily, &netlink.Route{Dst: route.Dst}, netlink.RT_FILTER_DST) + _, err = netlink.RouteListFiltered(ipFamily, &netlink.Route{Dst: route.Dst}, netlink.RT_FILTER_DST) if err != nil { log.Warningf("Unable to list routes: %v", err) } diff --git a/backend/udp/cproxy_amd64.go b/backend/udp/cproxy_amd64.go index 7d9b72a337..50b890d184 100644 --- a/backend/udp/cproxy_amd64.go +++ b/backend/udp/cproxy_amd64.go @@ -60,7 +60,10 @@ func writeCommand(f *os.File, cmd *C.command) { } buf := *(*[]byte)(unsafe.Pointer(&hdr)) - f.Write(buf) + _, err := f.Write(buf) + if err != nil { + log.Errorf("Error while writing the command %v. Error: %v", cmd, err) + } } func setRoute(ctl *os.File, dst ip.IP4Net, nextHopIP ip.IP4, nextHopPort int) { diff --git a/backend/udp/udp_network_amd64.go b/backend/udp/udp_network_amd64.go index 193a692901..f72bdf51fa 100644 --- a/backend/udp/udp_network_amd64.go +++ b/backend/udp/udp_network_amd64.go @@ -38,7 +38,6 @@ const ( type network struct { backend.SimpleNetwork - name string port int ctl *os.File ctl2 *os.File @@ -107,15 +106,13 @@ func (n *network) Run(ctx context.Context) { }() for { - select { - case evtBatch, ok := <-evts: - if !ok { - log.Infof("evts chan closed") - stopProxy(n.ctl) - return - } - n.processSubnetEvents(evtBatch) + evtBatch, ok := <-evts + if !ok { + log.Infof("evts chan closed") + stopProxy(n.ctl) + return } + n.processSubnetEvents(evtBatch) } } diff --git a/backend/vxlan/vxlan_network.go b/backend/vxlan/vxlan_network.go index 7cffb6b549..cb8df2f461 100644 --- a/backend/vxlan/vxlan_network.go +++ b/backend/vxlan/vxlan_network.go @@ -70,14 +70,12 @@ func (nw *network) Run(ctx context.Context) { defer wg.Wait() for { - select { - case evtBatch, ok := <-events: - if !ok { - log.Infof("evts chan closed") - return - } - nw.handleSubnetEvents(evtBatch) + evtBatch, ok := <-events + if !ok { + log.Infof("evts chan closed") + return } + nw.handleSubnetEvents(evtBatch) } } diff --git a/backend/wireguard/device.go b/backend/wireguard/device.go index d38dfdcdba..ac2997ab89 100644 --- a/backend/wireguard/device.go +++ b/backend/wireguard/device.go @@ -162,12 +162,13 @@ func newWGDevice(devAttrs *wgDeviceAttrs, ctx context.Context, wg *sync.WaitGrou // We remove the device to undo any change we did to the system. wg.Add(1) go func() { - select { - case <-ctx.Done(): - dev.remove() - log.Infof("Removed wireguard device %s", dev.attrs.name) - wg.Done() + <-ctx.Done() + err := dev.remove() + if err != nil { + log.Errorf("Error while removing device: %v", err) } + log.Infof("Removed wireguard device %s", dev.attrs.name) + wg.Done() }() return &dev, nil @@ -298,7 +299,10 @@ func (dev *wgDevice) addPeer(publicEndpoint string, peerPublicKeyRaw string, pee } // Remove peers from this endpoint with different public keys - dev.cleanupEndpointPeers(udpEndpoint, peerPublicKeyRaw) + err = dev.cleanupEndpointPeers(udpEndpoint, peerPublicKeyRaw) + if err != nil { + return fmt.Errorf("failed to clean up endpoint peers %w", err) + } return nil } diff --git a/backend/wireguard/wireguard_network.go b/backend/wireguard/wireguard_network.go index fb23a045e0..29d74c0913 100644 --- a/backend/wireguard/wireguard_network.go +++ b/backend/wireguard/wireguard_network.go @@ -112,7 +112,7 @@ func (n *network) handleSubnetEvents(batch []subnet.Event) { if len(event.Lease.Attrs.BackendData) > 0 { if err := json.Unmarshal(event.Lease.Attrs.BackendData, &wireguardAttrs); err != nil { - log.Errorf("failed to unmarshal BackendData: %w", err) + log.Errorf("failed to unmarshal BackendData: %v", err) continue } } @@ -131,7 +131,7 @@ func (n *network) handleSubnetEvents(batch []subnet.Event) { if len(event.Lease.Attrs.BackendV6Data) > 0 { if err := json.Unmarshal(event.Lease.Attrs.BackendV6Data, &wireguardAttrs); err != nil { - log.Errorf("failed to unmarshal BackendData: %w", err) + log.Errorf("failed to unmarshal BackendData: %v", err) continue } } @@ -141,7 +141,7 @@ func (n *network) handleSubnetEvents(batch []subnet.Event) { publicEndpoint, wireguardAttrs.PublicKey, event.Lease.IPv6Subnet.ToIPNet()); err != nil { - log.Errorf("failed to setup ipv6 peer (%s): %w", wireguardAttrs.PublicKey, err) + log.Errorf("failed to setup ipv6 peer (%s): %v", wireguardAttrs.PublicKey, err) } } @@ -157,7 +157,7 @@ func (n *network) handleSubnetEvents(batch []subnet.Event) { log.Info("Subnet removed: ", event.Lease.Subnet) if len(event.Lease.Attrs.BackendData) > 0 { if err := json.Unmarshal(event.Lease.Attrs.BackendData, &wireguardAttrs); err != nil { - log.Errorf("failed to unmarshal BackendData: %w", err) + log.Errorf("failed to unmarshal BackendData: %v", err) continue } } @@ -173,7 +173,7 @@ func (n *network) handleSubnetEvents(batch []subnet.Event) { log.Info("Subnet removed: ", event.Lease.IPv6Subnet) if len(event.Lease.Attrs.BackendV6Data) > 0 { if err := json.Unmarshal(event.Lease.Attrs.BackendV6Data, &wireguardAttrs); err != nil { - log.Errorf("failed to unmarshal BackendData: %w", err) + log.Errorf("failed to unmarshal BackendData: %v", err) continue } } @@ -181,7 +181,7 @@ func (n *network) handleSubnetEvents(batch []subnet.Event) { if err := n.v6Dev.removePeer( wireguardAttrs.PublicKey, ); err != nil { - log.Errorf("failed to remove ipv6 peer (%s): %w", wireguardAttrs.PublicKey, err) + log.Errorf("failed to remove ipv6 peer (%s): %v", wireguardAttrs.PublicKey, err) } } diff --git a/main.go b/main.go index cd5752bd3f..2e559458b3 100644 --- a/main.go +++ b/main.go @@ -78,10 +78,7 @@ type CmdLineOpts struct { etcdCAFile string etcdUsername string etcdPassword string - help bool version bool - autoDetectIPv4 bool - autoDetectIPv6 bool kubeSubnetMgr bool kubeApiUrl string kubeAnnotationPrefix string @@ -90,14 +87,11 @@ type CmdLineOpts struct { ifaceRegex flagSlice ipMasq bool subnetFile string - subnetDir string publicIP string publicIPv6 string subnetLeaseRenewMargin int healthzIP string healthzPort int - charonExecutablePath string - charonViciUri string iptablesResyncSeconds int iptablesForwardRules bool netConfPath string @@ -149,7 +143,12 @@ func init() { // klog will log to tmp files by default. override so all entries // can flow into journald (if running under systemd) - flag.Set("logtostderr", "true") + err := flag.Set("logtostderr", "true") + if err != nil { + log.Error("Can't set the logtostderr flag", err) + os.Exit(1) + } + // Only copy the non file logging options from klog copyFlag("v") @@ -160,7 +159,12 @@ func init() { flannelFlags.Usage = usage // now parse command line args - flannelFlags.Parse(os.Args[1:]) + err = flannelFlags.Parse(os.Args[1:]) + if err != nil { + log.Error("Can't parse flannel flags", err) + os.Exit(1) + } + } func copyFlag(name string) { @@ -212,7 +216,10 @@ func main() { os.Exit(0) } - flagutil.SetFlagsFromEnv(flannelFlags, "FLANNELD") + err := flagutil.SetFlagsFromEnv(flannelFlags, "FLANNELD") + if err != nil { + log.Error("Failed to set flag FLANNELD from env", err) + } // Log the config set via CLI flags log.Infof("CLI flags config: %+v", opts) @@ -382,7 +389,10 @@ func main() { wg.Done() }() - daemon.SdNotify(false, "READY=1") + _, err = daemon.SdNotify(false, "READY=1") + if err != nil { + log.Errorf("Failed to notify systemd the message READY=1 %v", err) + } // Kube subnet mgr doesn't lease the subnet for this node - it just uses the podCidr that's already assigned. if !opts.kubeSubnetMgr { @@ -480,7 +490,7 @@ func MonitorLease(ctx context.Context, sm subnet.Manager, bn backend.Network, wg }() renewMargin := time.Duration(opts.subnetLeaseRenewMargin) * time.Minute - dur := bn.Lease().Expiration.Sub(time.Now()) - renewMargin + dur := time.Until(bn.Lease().Expiration) - renewMargin for { select { @@ -493,7 +503,7 @@ func MonitorLease(ctx context.Context, sm subnet.Manager, bn backend.Network, wg } log.Info("Lease renewed, new expiration: ", bn.Lease().Expiration) - dur = bn.Lease().Expiration.Sub(time.Now()) - renewMargin + dur = time.Until(bn.Lease().Expiration) - renewMargin case e, ok := <-evts: if !ok { @@ -503,7 +513,7 @@ func MonitorLease(ctx context.Context, sm subnet.Manager, bn backend.Network, wg switch e.Type { case subnet.EventAdded: bn.Lease().Expiration = e.Lease.Expiration - dur = bn.Lease().Expiration.Sub(time.Now()) - renewMargin + dur = time.Until(bn.Lease().Expiration) - renewMargin log.Infof("Waiting for %s to renew lease", dur) case subnet.EventRemoved: @@ -747,7 +757,10 @@ func LookupExtIface(ifname string, ifregexS string, ipStack int) (*backend.Exter func WriteSubnetFile(path string, config *subnet.Config, ipMasq bool, bn backend.Network) error { dir, name := filepath.Split(path) - os.MkdirAll(dir, 0755) + err := os.MkdirAll(dir, 0755) + if err != nil { + return err + } tempFile := filepath.Join(dir, "."+name) f, err := os.Create(tempFile) if err != nil { @@ -789,7 +802,11 @@ func mustRunHealthz(stopChan <-chan struct{}, wg *sync.WaitGroup) { http.HandleFunc("/healthz", func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) - w.Write([]byte("flanneld is running")) + _, err := w.Write([]byte("flanneld is running")) + if err != nil { + log.Errorf("Handling /healthz error. %v", err) + panic(err) + } }) server := &http.Server{Addr: address} diff --git a/network/iptables.go b/network/iptables.go index 98d046564f..8cb9fddeda 100644 --- a/network/iptables.go +++ b/network/iptables.go @@ -144,7 +144,10 @@ func SetupAndEnsureIPTables(rules []IPTablesRule, resyncPeriod int) { } defer func() { - teardownIPTables(ipt, rules) + err := teardownIPTables(ipt, rules) + if err != nil { + log.Errorf("Failed to tear down IPTables: %v", err) + } }() for { @@ -166,7 +169,10 @@ func SetupAndEnsureIP6Tables(rules []IPTablesRule, resyncPeriod int) { } defer func() { - teardownIPTables(ipt, rules) + err := teardownIPTables(ipt, rules) + if err != nil { + log.Errorf("Failed to tear down IPTables: %v", err) + } }() for { @@ -187,7 +193,11 @@ func DeleteIPTables(rules []IPTablesRule) error { log.Errorf("Failed to setup IPTables. iptables binary was not found: %v", err) return err } - teardownIPTables(ipt, rules) + err = teardownIPTables(ipt, rules) + if err != nil { + log.Errorf("Failed to tear down IPTables: %v", err) + return err + } return nil } @@ -199,7 +209,11 @@ func DeleteIP6Tables(rules []IPTablesRule) error { log.Errorf("Failed to setup IP6Tables. iptables binary was not found: %v", err) return err } - teardownIPTables(ipt, rules) + err = teardownIPTables(ipt, rules) + if err != nil { + log.Errorf("Failed to tear down IPTables: %v", err) + return err + } return nil } diff --git a/network/iptables_test.go b/network/iptables_test.go index b2897a55f9..d759200a8e 100644 --- a/network/iptables_test.go +++ b/network/iptables_test.go @@ -106,11 +106,17 @@ func (mock *MockIPTables) AppendUnique(table string, chain string, rulespec ...s func TestDeleteRules(t *testing.T) { ipt := &MockIPTables{t: t} - setupIPTables(ipt, MasqRules(ip.IP4Net{}, lease())) + err := setupIPTables(ipt, MasqRules(ip.IP4Net{}, lease())) + if err != nil { + t.Error("Error setting up iptables") + } if len(ipt.rules) != 4 { t.Errorf("Should be 4 masqRules, there are actually %d: %#v", len(ipt.rules), ipt.rules) } - teardownIPTables(ipt, MasqRules(ip.IP4Net{}, lease())) + err = teardownIPTables(ipt, MasqRules(ip.IP4Net{}, lease())) + if err != nil { + t.Error("Error tearing down iptables") + } if len(ipt.rules) != 0 { t.Errorf("Should be 0 masqRules, there are actually %d: %#v", len(ipt.rules), ipt.rules) } @@ -118,11 +124,17 @@ func TestDeleteRules(t *testing.T) { func TestDeleteIP6Rules(t *testing.T) { ipt := &MockIPTables{t: t} - setupIPTables(ipt, MasqIP6Rules(ip.IP6Net{}, lease())) + err := setupIPTables(ipt, MasqIP6Rules(ip.IP6Net{}, lease())) + if err != nil { + t.Error("Error setting up iptables") + } if len(ipt.rules) != 4 { t.Errorf("Should be 4 masqRules, there are actually %d: %#v", len(ipt.rules), ipt.rules) } - teardownIPTables(ipt, MasqIP6Rules(ip.IP6Net{}, lease())) + err = teardownIPTables(ipt, MasqIP6Rules(ip.IP6Net{}, lease())) + if err != nil { + t.Error("Error tearing down iptables") + } if len(ipt.rules) != 0 { t.Errorf("Should be 0 masqRules, there are actually %d: %#v", len(ipt.rules), ipt.rules) } @@ -132,15 +144,21 @@ func TestEnsureRulesError(t *testing.T) { // If an error prevents a rule from being deleted, ensureIPTables should leave the rules as is // rather than potentially re-appending rules in an incorrect order ipt_correct := &MockIPTables{t: t} - setupIPTables(ipt_correct, MasqRules(ip.IP4Net{}, lease())) + err := setupIPTables(ipt_correct, MasqRules(ip.IP4Net{}, lease())) + if err != nil { + t.Error("Error setting up iptables") + } // setup a mock instance where we delete some masqRules and run `ensureIPTables` ipt_recreate := &MockIPTables{t: t} - setupIPTables(ipt_recreate, MasqRules(ip.IP4Net{}, lease())) + err = setupIPTables(ipt_recreate, MasqRules(ip.IP4Net{}, lease())) + if err != nil { + t.Error("Error setting up iptables") + } ipt_recreate.rules = ipt_recreate.rules[0:2] rule := ipt_recreate.rules[1] ipt_recreate.failDelete(rule.table, rule.chain, rule.rulespec, false) - err := ensureIPTables(ipt_recreate, MasqRules(ip.IP4Net{}, lease())) + err = ensureIPTables(ipt_recreate, MasqRules(ip.IP4Net{}, lease())) if err == nil { t.Errorf("ensureIPTables should have failed but did not.") } @@ -154,15 +172,21 @@ func TestEnsureIP6RulesError(t *testing.T) { // If an error prevents a rule from being deleted, ensureIPTables should leave the rules as is // rather than potentially re-appending rules in an incorrect order ipt_correct := &MockIPTables{t: t} - setupIPTables(ipt_correct, MasqIP6Rules(ip.IP6Net{}, lease())) + err := setupIPTables(ipt_correct, MasqIP6Rules(ip.IP6Net{}, lease())) + if err != nil { + t.Error("Error setting up iptables") + } // setup a mock instance where we delete some masqRules and run `ensureIPTables` ipt_recreate := &MockIPTables{t: t} - setupIPTables(ipt_recreate, MasqIP6Rules(ip.IP6Net{}, lease())) + err = setupIPTables(ipt_recreate, MasqIP6Rules(ip.IP6Net{}, lease())) + if err != nil { + t.Error("Error setting up iptables") + } ipt_recreate.rules = ipt_recreate.rules[0:2] rule := ipt_recreate.rules[1] ipt_recreate.failDelete(rule.table, rule.chain, rule.rulespec, false) - err := ensureIPTables(ipt_recreate, MasqIP6Rules(ip.IP6Net{}, lease())) + err = ensureIPTables(ipt_recreate, MasqIP6Rules(ip.IP6Net{}, lease())) if err == nil { t.Errorf("ensureIPTables should have failed but did not.") } @@ -175,15 +199,21 @@ func TestEnsureIP6RulesError(t *testing.T) { func TestEnsureRules(t *testing.T) { // If any masqRules are missing, they should be all deleted and recreated in the correct order ipt_correct := &MockIPTables{t: t} - setupIPTables(ipt_correct, MasqRules(ip.IP4Net{}, lease())) + err := setupIPTables(ipt_correct, MasqRules(ip.IP4Net{}, lease())) + if err != nil { + t.Error("Error setting up iptables") + } // setup a mock instance where we delete some masqRules and run `ensureIPTables` ipt_recreate := &MockIPTables{t: t} - setupIPTables(ipt_recreate, MasqRules(ip.IP4Net{}, lease())) + err = setupIPTables(ipt_recreate, MasqRules(ip.IP4Net{}, lease())) + if err != nil { + t.Error("Error setting up iptables") + } ipt_recreate.rules = ipt_recreate.rules[0:2] // set up a normal error that iptables returns when deleting a rule that is already gone deletedRule := ipt_correct.rules[3] ipt_recreate.failDelete(deletedRule.table, deletedRule.chain, deletedRule.rulespec, true) - err := ensureIPTables(ipt_recreate, MasqRules(ip.IP4Net{}, lease())) + err = ensureIPTables(ipt_recreate, MasqRules(ip.IP4Net{}, lease())) if err != nil { t.Errorf("ensureIPTables should have completed without errors") } @@ -195,15 +225,21 @@ func TestEnsureRules(t *testing.T) { func TestEnsureIP6Rules(t *testing.T) { // If any masqRules are missing, they should be all deleted and recreated in the correct order ipt_correct := &MockIPTables{t: t} - setupIPTables(ipt_correct, MasqIP6Rules(ip.IP6Net{}, lease())) + err := setupIPTables(ipt_correct, MasqIP6Rules(ip.IP6Net{}, lease())) + if err != nil { + t.Error("Error setting up iptables") + } // setup a mock instance where we delete some masqRules and run `ensureIPTables` ipt_recreate := &MockIPTables{t: t} - setupIPTables(ipt_recreate, MasqIP6Rules(ip.IP6Net{}, lease())) + err = setupIPTables(ipt_recreate, MasqIP6Rules(ip.IP6Net{}, lease())) + if err != nil { + t.Error("Error setting up iptables") + } ipt_recreate.rules = ipt_recreate.rules[0:2] // set up a normal error that iptables returns when deleting a rule that is already gone deletedRule := ipt_correct.rules[3] ipt_recreate.failDelete(deletedRule.table, deletedRule.chain, deletedRule.rulespec, true) - err := ensureIPTables(ipt_recreate, MasqIP6Rules(ip.IP6Net{}, lease())) + err = ensureIPTables(ipt_recreate, MasqIP6Rules(ip.IP6Net{}, lease())) if err != nil { t.Errorf("ensureIPTables should have completed without errors") } diff --git a/pkg/ip/iface.go b/pkg/ip/iface.go index aa778957f9..2479f21431 100644 --- a/pkg/ip/iface.go +++ b/pkg/ip/iface.go @@ -29,7 +29,7 @@ import ( func getIfaceAddrs(iface *net.Interface) ([]netlink.Addr, error) { link := &netlink.Device{ - netlink.LinkAttrs{ + LinkAttrs: netlink.LinkAttrs{ Index: iface.Index, }, } @@ -39,7 +39,7 @@ func getIfaceAddrs(iface *net.Interface) ([]netlink.Addr, error) { func getIfaceV6Addrs(iface *net.Interface) ([]netlink.Addr, error) { link := &netlink.Device{ - netlink.LinkAttrs{ + LinkAttrs: netlink.LinkAttrs{ Index: iface.Index, }, } diff --git a/pkg/ip/iface_test.go b/pkg/ip/iface_test.go index 7f7ba38558..804e117d04 100644 --- a/pkg/ip/iface_test.go +++ b/pkg/ip/iface_test.go @@ -92,6 +92,9 @@ func TestEnsureV6AddressOnLink(t *testing.T) { t.Fatal(err) } addrs, err = netlink.AddrList(lo, netlink.FAMILY_V6) + if err != nil { + t.Fatal(err) + } if len(addrs) != 2 { t.Fatalf("two addresses expected, addrs: %v", addrs) } diff --git a/pkg/ip/ip6net.go b/pkg/ip/ip6net.go index 7bfaf95f1d..8ed1b701d2 100644 --- a/pkg/ip/ip6net.go +++ b/pkg/ip/ip6net.go @@ -67,10 +67,7 @@ func GetIPv6SubnetMax(networkIP *IP6, subnetSize *big.Int) *IP6 { } func CheckIPv6Subnet(subnetIP *IP6, mask *big.Int) bool { - if (*big.Int)(subnetIP).Cmp(big.NewInt(0).And((*big.Int)(subnetIP), mask)) != 0 { - return false - } - return true + return (*big.Int)(subnetIP).Cmp(big.NewInt(0).And((*big.Int)(subnetIP), mask)) == 0 } func MustParseIP6(s string) *IP6 { diff --git a/pkg/mac/mac_test.go b/pkg/mac/mac_test.go index 719e082bf5..389c2b84fe 100644 --- a/pkg/mac/mac_test.go +++ b/pkg/mac/mac_test.go @@ -23,6 +23,6 @@ func TestNewHardwareAddr(t *testing.T) { // But an error should never be returned. _, err := NewHardwareAddr() if err != nil { - t.Fatalf(err) + t.Fatalf("err: %v", err) } } diff --git a/subnet/etcdv2/local_manager.go b/subnet/etcdv2/local_manager.go index 205ac5c62e..478e262403 100644 --- a/subnet/etcdv2/local_manager.go +++ b/subnet/etcdv2/local_manager.go @@ -42,14 +42,6 @@ type watchCursor struct { index uint64 } -func isErrEtcdTestFailed(e error) bool { - if e == nil { - return false - } - etcdErr, ok := e.(etcd.Error) - return ok && etcdErr.Code == etcd.ErrorCodeTestFailed -} - func isErrEtcdNodeExist(e error) bool { if e == nil { return false @@ -58,14 +50,6 @@ func isErrEtcdNodeExist(e error) bool { return ok || etcdErr.Code == etcd.ErrorCodeNodeExist } -func isErrEtcdKeyNotFound(e error) bool { - if e == nil { - return false - } - etcdErr, ok := e.(etcd.Error) - return ok || etcdErr.Code == etcd.ErrorCodeKeyNotFound -} - func (c watchCursor) String() string { return strconv.FormatUint(c.index, 10) } diff --git a/subnet/etcdv2/mock_etcd.go b/subnet/etcdv2/mock_etcd.go index f0f17a6af1..6cc0a3adbb 100644 --- a/subnet/etcdv2/mock_etcd.go +++ b/subnet/etcdv2/mock_etcd.go @@ -115,7 +115,7 @@ func (me *mockEtcd) findNode(key string) (*etcd.Node, *etcd.Node, error) { } // intermediates must be directories - if i < len(path)-1 && node.Dir != true { + if i < len(path)-1 && !node.Dir { return nil, nil, me.newError(etcd.ErrorCodeNotDir, "Intermediate node %s not a directory", part) } } @@ -243,7 +243,7 @@ func (me *mockEtcd) set(ctx context.Context, key, value string, opts *etcd.SetOp } if opts.Dir != node.Dir { - if opts.Dir == true { + if opts.Dir { return nil, me.newError(etcd.ErrorCodeNotDir, "Key %s is not a directory", key) } else { return nil, me.newError(etcd.ErrorCodeNotFile, "Key %s is not a file", key) @@ -300,12 +300,15 @@ func (me *mockEtcd) Set(ctx context.Context, key, value string, opts *etcd.SetOp func (me *mockEtcd) deleteNode(node *etcd.Node, parent *etcd.Node, recursive bool) (*etcd.Response, error) { for _, child := range me.nodes { if isChild, directChild := isChild(node, child); isChild { - if recursive == false { + if !recursive { return nil, me.newError(etcd.ErrorCodeDirNotEmpty, "Key %s not empty", node.Key) } if directChild { - me.deleteNode(child, node, true) + _, err := me.deleteNode(child, node, true) + if err != nil { + return nil, err + } me.index += 1 node.ModifiedIndex = me.index } @@ -350,7 +353,7 @@ func (me *mockEtcd) Delete(ctx context.Context, key string, opts *etcd.DeleteOpt } if opts.Dir != node.Dir { - if opts.Dir == true { + if opts.Dir { return nil, me.newError(etcd.ErrorCodeNotDir, "Key %s is not a directory", key) } else { return nil, me.newError(etcd.ErrorCodeNotFile, "Key %s is not a file", key) diff --git a/subnet/etcdv2/mock_etcd_test.go b/subnet/etcdv2/mock_etcd_test.go index ed63667c22..27cfb745bc 100644 --- a/subnet/etcdv2/mock_etcd_test.go +++ b/subnet/etcdv2/mock_etcd_test.go @@ -208,7 +208,7 @@ func TestMockEtcd(t *testing.T) { // Delete a key dopts := &etcd.DeleteOptions{Recursive: true, Dir: false} - r, err = m.Delete(ctx, "/coreos.com/network/foobar", dopts) + _, err = m.Delete(ctx, "/coreos.com/network/foobar", dopts) if err == nil { t.Fatalf("Unexpected success deleting a directory") } diff --git a/subnet/etcdv2/registry.go b/subnet/etcdv2/registry.go index ee20091086..0ae7a8c584 100644 --- a/subnet/etcdv2/registry.go +++ b/subnet/etcdv2/registry.go @@ -249,17 +249,6 @@ func (esr *etcdSubnetRegistry) client() etcd.KeysAPI { return esr.cli } -func (esr *etcdSubnetRegistry) resetClient() { - esr.mux.Lock() - defer esr.mux.Unlock() - - var err error - esr.cli, err = newEtcdClient(esr.etcdCfg) - if err != nil { - panic(fmt.Errorf("resetClient: error recreating etcd client: %v", err)) - } -} - func parseSubnetWatchResponse(resp *etcd.Response) (Event, error) { sn, tsn6 := ParseSubnetKey(resp.Node.Key) if sn == nil { @@ -274,8 +263,8 @@ func parseSubnetWatchResponse(resp *etcd.Response) (Event, error) { switch resp.Action { case "delete", "expire": return Event{ - EventRemoved, - Lease{ + Type: EventRemoved, + Lease: Lease{ EnableIPv4: true, Subnet: *sn, EnableIPv6: !sn6.Empty(), @@ -296,8 +285,8 @@ func parseSubnetWatchResponse(resp *etcd.Response) (Event, error) { } evt := Event{ - EventAdded, - Lease{ + Type: EventAdded, + Lease: Lease{ EnableIPv4: true, Subnet: *sn, EnableIPv6: !sn6.Empty(), diff --git a/subnet/etcdv2/registry_test.go b/subnet/etcdv2/registry_test.go index d41739a15d..27d7b8a7f9 100644 --- a/subnet/etcdv2/registry_test.go +++ b/subnet/etcdv2/registry_test.go @@ -94,7 +94,7 @@ func TestEtcdRegistry(t *testing.T) { ctx, _ := context.WithCancel(context.Background()) - config, err := r.getNetworkConfig(ctx) + _, err := r.getNetworkConfig(ctx) if err == nil { t.Fatal("Should hit error getting config") } @@ -102,9 +102,12 @@ func TestEtcdRegistry(t *testing.T) { // Populate etcd with a network netKey := "/coreos.com/network/config" netValue := "{ \"Network\": \"10.1.0.0/16\", \"Backend\": { \"Type\": \"host-gw\" } }" - m.Create(ctx, netKey, netValue) + _, err = m.Create(ctx, netKey, netValue) + if err != nil { + t.Fatal("Failed to create new entry", err) + } - config, err = r.getNetworkConfig(ctx) + config, err := r.getNetworkConfig(ctx) if err != nil { t.Fatal("Failed to get network config", err) } @@ -154,6 +157,9 @@ func TestEtcdRegistry(t *testing.T) { } leases, _, err := r.getSubnets(ctx) + if err != nil { + t.Fatal("Failed to get Subnets") + } if len(leases) != 1 { t.Fatalf("Unexpected number of leases %d (expected 1)", len(leases)) } @@ -165,6 +171,9 @@ func TestEtcdRegistry(t *testing.T) { if lease == nil { t.Fatal("Missing subnet lease") } + if err != nil { + t.Fatal("Failed to get Subnet") + } err = r.deleteSubnet(ctx, sn, ip.IP6Net{}) if err != nil { @@ -172,7 +181,7 @@ func TestEtcdRegistry(t *testing.T) { } // Make sure the lease got deleted - resp, err = m.Get(ctx, "/coreos.com/network/subnets/10.1.5.0-24", nil) + _, err = m.Get(ctx, "/coreos.com/network/subnets/10.1.5.0-24", nil) if err == nil { t.Fatal("Unexpected success getting deleted subnet") } diff --git a/subnet/etcdv2/subnet_test.go b/subnet/etcdv2/subnet_test.go index a6239398a6..52c57708b2 100644 --- a/subnet/etcdv2/subnet_test.go +++ b/subnet/etcdv2/subnet_test.go @@ -35,13 +35,13 @@ func newDummyRegistry() *MockSubnetRegistry { subnets := []Lease{ // leases within SubnetMin-SubnetMax range - {true, false, ip.IP4Net{ip.MustParseIP4("10.3.1.0"), 24}, ip.IP6Net{}, attrs, exp, 10}, - {true, false, ip.IP4Net{ip.MustParseIP4("10.3.2.0"), 24}, ip.IP6Net{}, attrs, exp, 11}, - {true, false, ip.IP4Net{ip.MustParseIP4("10.3.4.0"), 24}, ip.IP6Net{}, attrs, exp, 12}, - {true, false, ip.IP4Net{ip.MustParseIP4("10.3.5.0"), 24}, ip.IP6Net{}, attrs, exp, 13}, + {EnableIPv4: true, EnableIPv6: false, Subnet: ip.IP4Net{IP: ip.MustParseIP4("10.3.1.0"), PrefixLen: 24}, IPv6Subnet: ip.IP6Net{}, Attrs: attrs, Expiration: exp, Asof: 10}, + {EnableIPv4: true, EnableIPv6: false, Subnet: ip.IP4Net{IP: ip.MustParseIP4("10.3.2.0"), PrefixLen: 24}, IPv6Subnet: ip.IP6Net{}, Attrs: attrs, Expiration: exp, Asof: 11}, + {EnableIPv4: true, EnableIPv6: false, Subnet: ip.IP4Net{IP: ip.MustParseIP4("10.3.4.0"), PrefixLen: 24}, IPv6Subnet: ip.IP6Net{}, Attrs: attrs, Expiration: exp, Asof: 12}, + {EnableIPv4: true, EnableIPv6: false, Subnet: ip.IP4Net{IP: ip.MustParseIP4("10.3.5.0"), PrefixLen: 24}, IPv6Subnet: ip.IP6Net{}, Attrs: attrs, Expiration: exp, Asof: 13}, // hand created lease outside the range of subnetMin-SubnetMax for testing removal - {true, false, ip.IP4Net{ip.MustParseIP4("10.3.31.0"), 24}, ip.IP6Net{}, attrs, exp, 13}, + {EnableIPv4: true, EnableIPv6: false, Subnet: ip.IP4Net{IP: ip.MustParseIP4("10.3.31.0"), PrefixLen: 24}, IPv6Subnet: ip.IP6Net{}, Attrs: attrs, Expiration: exp, Asof: 13}, } config := `{ "Network": "10.3.0.0/16", "SubnetMin": "10.3.1.0", "SubnetMax": "10.3.25.0" }` @@ -78,7 +78,7 @@ func TestAcquireLease(t *testing.T) { // Test if a previous subnet will be used msr2 := newDummyRegistry() - prevSubnet := ip.IP4Net{ip.MustParseIP4("10.3.6.0"), 24} + prevSubnet := ip.IP4Net{IP: ip.MustParseIP4("10.3.6.0"), PrefixLen: 24} sm2 := NewMockManagerWithSubnet(msr2, prevSubnet, ip.IP6Net{}) prev, err := sm2.AcquireLease(context.Background(), &attrs) if err != nil { @@ -90,7 +90,7 @@ func TestAcquireLease(t *testing.T) { // Test that a previous subnet will not be used if it does not match the registry config msr3 := newDummyRegistry() - invalidSubnet := ip.IP4Net{ip.MustParseIP4("10.4.1.0"), 24} + invalidSubnet := ip.IP4Net{IP: ip.MustParseIP4("10.4.1.0"), PrefixLen: 24} sm3 := NewMockManagerWithSubnet(msr3, invalidSubnet, ip.IP6Net{}) l3, err := sm3.AcquireLease(context.Background(), &attrs) if err != nil { @@ -121,7 +121,10 @@ func TestConfigChanged(t *testing.T) { // Change config config := `{ "Network": "10.4.0.0/16" }` - msr.setConfig(config) + err = msr.setConfig(config) + if err != nil { + t.Fatal("Failed to set the Config", err) + } // Acquire again, should not reuse if l, err = sm.AcquireLease(context.Background(), &attrs); err != nil { @@ -133,17 +136,6 @@ func TestConfigChanged(t *testing.T) { } } -func newIP4Net(ipaddr string, prefix uint) ip.IP4Net { - a, err := ip.ParseIP4(ipaddr) - if err != nil { - panic("failed to parse ipaddr") - } - return ip.IP4Net{ - IP: a, - PrefixLen: prefix, - } -} - func acquireLease(ctx context.Context, t *testing.T, sm Manager) *Lease { extIaddr, _ := ip.ParseIP4("1.2.3.4") attrs := LeaseAttrs{ @@ -233,7 +225,7 @@ func TestWatchLeaseRemoved(t *testing.T) { } } - expected := ip.IP4Net{ip.MustParseIP4("10.3.31.0"), 24} + expected := ip.IP4Net{IP: ip.MustParseIP4("10.3.31.0"), PrefixLen: 24} // Sanity check to make sure acquired lease is not this. // It shouldn't be as SubnetMin/SubnetMax in config is [10.3.1.0/24 to 10.3.25.0/24] if l.Subnet.Equal(expected) { diff --git a/subnet/kube/kube.go b/subnet/kube/kube.go index 6a19fafcdb..eeda093bc6 100644 --- a/subnet/kube/kube.go +++ b/subnet/kube/kube.go @@ -159,7 +159,7 @@ func newKubeSubnetManager(ctx context.Context, c clientset.Interface, sc *subnet }, UpdateFunc: ksm.handleUpdateLeaseEvent, DeleteFunc: func(obj interface{}) { - node, isNode := obj.(*v1.Node) + _, isNode := obj.(*v1.Node) // We can get DeletedFinalStateUnknown instead of *api.Node here and we need to handle that correctly. if !isNode { deletedState, ok := obj.(cache.DeletedFinalStateUnknown) @@ -167,7 +167,7 @@ func newKubeSubnetManager(ctx context.Context, c clientset.Interface, sc *subnet log.Infof("Error received unexpected object: %v", obj) return } - node, ok = deletedState.Obj.(*v1.Node) + node, ok := deletedState.Obj.(*v1.Node) if !ok { log.Infof("Error deletedFinalStateUnknown contained non-Node object: %v", deletedState.Obj) return @@ -195,7 +195,7 @@ func (ksm *kubeSubnetManager) handleAddLeaseEvent(et subnet.EventType, obj inter log.Infof("Error turning node %q to lease: %v", n.ObjectMeta.Name, err) return } - ksm.events <- subnet.Event{et, l} + ksm.events <- subnet.Event{Type: et, Lease: l} } func (ksm *kubeSubnetManager) handleUpdateLeaseEvent(oldObj, newObj interface{}) { @@ -226,7 +226,7 @@ func (ksm *kubeSubnetManager) handleUpdateLeaseEvent(oldObj, newObj interface{}) log.Infof("Error turning node %q to lease: %v", n.ObjectMeta.Name, err) return } - ksm.events <- subnet.Event{subnet.EventAdded, l} + ksm.events <- subnet.Event{Type: subnet.EventAdded, Lease: l} } func (ksm *kubeSubnetManager) GetNetworkConfig(ctx context.Context) (*subnet.Config, error) { diff --git a/subnet/subnet_test.go b/subnet/subnet_test.go index f0110476ca..c59451fbaf 100644 --- a/subnet/subnet_test.go +++ b/subnet/subnet_test.go @@ -19,21 +19,18 @@ import ( "testing" ) -func mkIP4Net(s string, plen uint) ip.IP4Net { - ip4, err := ip.ParseIP4(s) - if err != nil { - panic(err) - } - - return ip.IP4Net{ip4, plen} -} - func TestSubnetNodev4(t *testing.T) { key := "10.12.13.0-24" sn, sn6 := ParseSubnetKey(key) if sn == nil { t.Errorf("Failed to parse ipv4 address") + return + } + + if sn.ToIPNet() == nil { + t.Errorf("Failed to transform sn into IPNet") + return } if sn.ToIPNet().String() != "10.12.13.0/24" { @@ -58,6 +55,11 @@ func TestSubnetNodev6(t *testing.T) { return } + if sn.ToIPNet() == nil { + t.Errorf("Failed to transform sn into IPNet") + return + } + if sn.ToIPNet().String() != "10.12.13.0/24" { t.Errorf("Unexpected ipv4 network") }