From 61ad16dea5f19abe11f0f1f4f2cd5a0d234f6e80 Mon Sep 17 00:00:00 2001 From: leonnicolas Date: Thu, 30 Sep 2021 12:03:00 +0200 Subject: [PATCH] add suggestions from review Signed-off-by: leonnicolas --- cmd/kg/main.go | 2 +- cmd/kgctl/main.go | 3 ++- cmd/kgctl/showconf.go | 10 ++++++++-- pkg/k8s/backend.go | 13 +++++++++++-- pkg/mesh/mesh.go | 6 +++--- pkg/mesh/topology.go | 14 +++++++------- pkg/wireguard/conf.go | 11 +++++++++-- 7 files changed, 41 insertions(+), 18 deletions(-) diff --git a/cmd/kg/main.go b/cmd/kg/main.go index 19809c9..ee85fdb 100644 --- a/cmd/kg/main.go +++ b/cmd/kg/main.go @@ -234,7 +234,7 @@ func runRoot(_ *cobra.Command, _ []string) error { c := kubernetes.NewForConfigOrDie(config) kc := kiloclient.NewForConfigOrDie(config) ec := apiextensions.NewForConfigOrDie(config) - b = k8s.New(c, kc, ec, topologyLabel) + b = k8s.New(c, kc, ec, topologyLabel, log.With(logger, "component", "k8s backend")) default: return fmt.Errorf("backend %v unknown; possible values are: %s", backend, availableBackends) } diff --git a/cmd/kgctl/main.go b/cmd/kgctl/main.go index 3bf4565..89c0f8e 100644 --- a/cmd/kgctl/main.go +++ b/cmd/kgctl/main.go @@ -21,6 +21,7 @@ import ( "path/filepath" "strings" + "github.com/go-kit/kit/log" "github.com/spf13/cobra" apiextensions "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset" "k8s.io/client-go/kubernetes" @@ -88,7 +89,7 @@ func runRoot(_ *cobra.Command, _ []string) error { c := kubernetes.NewForConfigOrDie(config) kc := kiloclient.NewForConfigOrDie(config) ec := apiextensions.NewForConfigOrDie(config) - opts.backend = k8s.New(c, kc, ec, topologyLabel) + opts.backend = k8s.New(c, kc, ec, topologyLabel, log.NewNopLogger()) default: return fmt.Errorf("backend %v unknown; posible values are: %s", backend, availableBackends) } diff --git a/cmd/kgctl/showconf.go b/cmd/kgctl/showconf.go index 7ff2909..2670046 100644 --- a/cmd/kgctl/showconf.go +++ b/cmd/kgctl/showconf.go @@ -183,17 +183,23 @@ func runShowConfNode(_ *cobra.Command, args []string) error { fallthrough case outputFormatYAML: p := t.AsPeer() + if p == nil { + return errors.New("cannot generate config from nil peer") + } p.AllowedIPs = append(p.AllowedIPs, showConfOpts.allowedIPs...) p.DeduplicateIPs() - k8sp := translatePeer(&p) + k8sp := translatePeer(p) k8sp.Name = hostname return showConfOpts.serializer.Encode(k8sp, os.Stdout) case outputFormatWireGuard: p := t.AsPeer() + if p == nil { + return errors.New("cannot generate config from nil peer") + } p.AllowedIPs = append(p.AllowedIPs, showConfOpts.allowedIPs...) p.DeduplicateIPs() c, err := (&wireguard.Conf{ - Peers: []wireguard.Peer{p}, + Peers: []wireguard.Peer{*p}, }).Bytes() if err != nil { return fmt.Errorf("failed to generate configuration: %v", err) diff --git a/pkg/k8s/backend.go b/pkg/k8s/backend.go index f7d81fc..62a6575 100644 --- a/pkg/k8s/backend.go +++ b/pkg/k8s/backend.go @@ -25,6 +25,8 @@ import ( "strings" "time" + "github.com/go-kit/kit/log" + "github.com/go-kit/kit/log/level" "golang.zx2c4.com/wireguard/wgctrl/wgtypes" v1 "k8s.io/api/core/v1" apiextensions "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset" @@ -67,6 +69,8 @@ const ( jsonRemovePatch = `{"op": "remove", "path": "%s"}` ) +var logger = log.NewNopLogger() + type backend struct { nodes *nodeBackend peers *peerBackend @@ -99,10 +103,12 @@ type peerBackend struct { } // New creates a new instance of a mesh.Backend. -func New(c kubernetes.Interface, kc kiloclient.Interface, ec apiextensions.Interface, topologyLabel string) mesh.Backend { +func New(c kubernetes.Interface, kc kiloclient.Interface, ec apiextensions.Interface, topologyLabel string, l log.Logger) mesh.Backend { ni := v1informers.NewNodeInformer(c, 5*time.Minute, nil) pi := v1alpha1informers.NewPeerInformer(kc, 5*time.Minute, nil) + logger = l + return &backend{ &nodeBackend{ client: c, @@ -395,7 +401,10 @@ func translatePeer(peer *v1alpha1.Peer) *mesh.Peer { } } - key, _ := wgtypes.ParseKey(peer.Spec.PublicKey) + key, err := wgtypes.ParseKey(peer.Spec.PublicKey) + if err != nil { + level.Error(logger).Log("msg", "failed to parse public key", "peer", peer.Name, "err", err.Error()) + } var psk *wgtypes.Key if k, err := wgtypes.ParseKey(peer.Spec.PresharedKey); err != nil { // Set key to nil to avoid setting a key to the zero value wgtypes.Key{} diff --git a/pkg/mesh/mesh.go b/pkg/mesh/mesh.go index ce44c75..146a6e5 100644 --- a/pkg/mesh/mesh.go +++ b/pkg/mesh/mesh.go @@ -542,7 +542,7 @@ func (m *Mesh) applyTopology() { equal, diff := conf.Equal(wgDevice) if !equal { level.Info(m.logger).Log("msg", "WireGuard configurations are different", "diff", diff) - level.Debug(m.logger).Log("changing wg config", "config", conf.WGConfig()) + level.Debug(m.logger).Log("msg", "changing wg config", "config", conf.WGConfig()) if err := wgClient.ConfigureDevice(m.kiloIfaceName, conf.WGConfig()); err != nil { level.Error(m.logger).Log("error", err) m.errorCounter.WithLabelValues("apply").Inc() @@ -660,7 +660,7 @@ func nodesAreEqual(a, b *Node) bool { } // Check the DNS name first since this package // is doing the DNS resolution. - if a.Endpoint.StringOpt(false) != b.Endpoint.StringOpt(false) { + if a.Endpoint.StringOpt(true) != b.Endpoint.StringOpt(true) { return false } // Ignore LastSeen when comparing equality we want to check if the nodes are @@ -689,7 +689,7 @@ func peersAreEqual(a, b *Peer) bool { } // Check the DNS name first since this package // is doing the DNS resolution. - if a.Endpoint.StringOpt(false) != b.Endpoint.StringOpt(false) { + if a.Endpoint.StringOpt(true) != b.Endpoint.StringOpt(true) { return false } if len(a.AllowedIPs) != len(b.AllowedIPs) { diff --git a/pkg/mesh/topology.go b/pkg/mesh/topology.go index 63a1238..8998307 100644 --- a/pkg/mesh/topology.go +++ b/pkg/mesh/topology.go @@ -294,7 +294,7 @@ func (t *Topology) updateEndpoint(endpoint *wireguard.Endpoint, key wgtypes.Key, if ok { return wireguard.NewEndpointFromUDPAddr(e) } - return nil + return endpoint } // Conf generates a WireGuard configuration file for a given Topology. @@ -339,12 +339,12 @@ func (t *Topology) Conf() *wireguard.Conf { // AsPeer generates the WireGuard peer configuration for the local location of the given Topology. // This configuration can be used to configure this location as a peer of another WireGuard interface. -func (t *Topology) AsPeer() wireguard.Peer { +func (t *Topology) AsPeer() *wireguard.Peer { for _, s := range t.segments { if s.location != t.location { continue } - p := wireguard.Peer{ + p := &wireguard.Peer{ PeerConfig: wgtypes.PeerConfig{ AllowedIPs: s.allowedIPs, PublicKey: s.key, @@ -353,11 +353,11 @@ func (t *Topology) AsPeer() wireguard.Peer { } return p } - return wireguard.Peer{} + return nil } // PeerConf generates a WireGuard configuration file for a given peer in a Topology. -func (t *Topology) PeerConf(name string) wireguard.Conf { +func (t *Topology) PeerConf(name string) *wireguard.Conf { var pka *time.Duration var psk *wgtypes.Key for i := range t.peers { @@ -367,7 +367,7 @@ func (t *Topology) PeerConf(name string) wireguard.Conf { break } } - c := wireguard.Conf{} + c := &wireguard.Conf{} for _, s := range t.segments { peer := wireguard.Peer{ PeerConfig: wgtypes.PeerConfig{ @@ -417,7 +417,7 @@ func findLeader(nodes []*Node) int { leaders = append(leaders, i) } - if nodes[i].Endpoint != nil && isPublic(nodes[i].Endpoint.IP()) { + if nodes[i].Endpoint.IP() != nil && isPublic(nodes[i].Endpoint.IP()) { public = append(public, i) } } diff --git a/pkg/wireguard/conf.go b/pkg/wireguard/conf.go index d844b56..67ba589 100644 --- a/pkg/wireguard/conf.go +++ b/pkg/wireguard/conf.go @@ -50,7 +50,11 @@ type Conf struct { } // WGConfig returns a wgytpes.Config from a Conf. -func (c Conf) WGConfig() wgtypes.Config { +func (c *Conf) WGConfig() wgtypes.Config { + if c == nil { + // The empty Config will do nothing, when applied. + return wgtypes.Config{} + } r := c.Config wgPs := make([]wgtypes.PeerConfig, len(c.Peers)) for i, p := range c.Peers { @@ -246,7 +250,10 @@ func (p *Peer) DeduplicateIPs() { } // Bytes renders a WireGuard configuration to bytes. -func (c Conf) Bytes() ([]byte, error) { +func (c *Conf) Bytes() ([]byte, error) { + if c == nil { + return nil, nil + } var err error buf := bytes.NewBuffer(make([]byte, 0, 512)) if c.PrivateKey != nil {