From 20c81100341d27439e42f0790fc67aa00ed087ae Mon Sep 17 00:00:00 2001 From: Brandon Stoll Date: Thu, 9 Jul 2026 18:36:26 +0000 Subject: [PATCH] refactor(topo): address linter issues and style in topo nodes (4/5) This is part 4/5 of an overall cleanup effort to fix linter issues and format files across the repository. In this step: - Address linter issues and style formatting across topology node implementations (alpine, arista, cisco, drivenets, forward, juniper, nokia, openconfig, sonic). - Refactor error handling and test formatting in topo module. --- topo/node/alpine/alpine.go | 14 +++---- topo/node/alpine/alpine_test.go | 12 +++--- topo/node/arista/arista.go | 8 ++-- topo/node/arista/arista_test.go | 12 +++--- topo/node/cisco/cisco.go | 37 ++++++++++-------- topo/node/cisco/cisco_test.go | 15 ++++++-- topo/node/drivenets/drivenets.go | 6 +-- topo/node/forward/forward.go | 10 ++--- topo/node/juniper/juniper.go | 16 ++++---- topo/node/node.go | 31 ++++++++------- topo/node/node_test.go | 16 ++++---- topo/node/nokia/nokia.go | 4 +- topo/node/openconfig/openconfig.go | 10 ++--- topo/node/sonic/sonic.go | 16 +++++--- topo/node/sonic/sonic_test.go | 62 +++++++++++++++--------------- topo/topo.go | 23 ++++++----- topo/topo_test.go | 10 ++--- 17 files changed, 164 insertions(+), 138 deletions(-) diff --git a/topo/node/alpine/alpine.go b/topo/node/alpine/alpine.go index 43ee15b23..56f452062 100644 --- a/topo/node/alpine/alpine.go +++ b/topo/node/alpine/alpine.go @@ -22,7 +22,7 @@ import ( "github.com/openconfig/kne/topo/node" "google.golang.org/protobuf/proto" - "k8s.io/utils/pointer" + "k8s.io/utils/ptr" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -123,7 +123,7 @@ func (n *Node) CreatePod(ctx context.Context) error { Resources: node.ToResourceRequirements(pb.Constraints), ImagePullPolicy: "IfNotPresent", SecurityContext: &corev1.SecurityContext{ - Privileged: pointer.Bool(true), + Privileged: ptr.To(true), }, }} @@ -193,14 +193,14 @@ func (n *Node) CreatePod(ctx context.Context) error { Resources: node.ToResourceRequirements(pb.Constraints), ImagePullPolicy: "IfNotPresent", SecurityContext: &corev1.SecurityContext{ - Privileged: pointer.Bool(true), + Privileged: ptr.To(true), }, VolumeMounts: extraMounts, } alpineContainers = append(alpineContainers, containerSpec) default: // Only Dataplane container is supported as the custom container - return fmt.Errorf("Alpine supports only 1 custom container, %d provided.", numContainers) + return fmt.Errorf("alpine supports only 1 custom container, %d provided", numContainers) } } @@ -223,11 +223,11 @@ func (n *Node) CreatePod(ctx context.Context) error { }, ImagePullPolicy: "IfNotPresent", SecurityContext: &corev1.SecurityContext{ - Privileged: pointer.Bool(true), + Privileged: ptr.To(true), }, }}, Containers: alpineContainers, - TerminationGracePeriodSeconds: pointer.Int64(0), + TerminationGracePeriodSeconds: ptr.To(int64(0)), NodeSelector: map[string]string{}, Affinity: &corev1.Affinity{ PodAntiAffinity: &corev1.PodAntiAffinity{ @@ -261,7 +261,7 @@ func (n *Node) CreatePod(ctx context.Context) error { MountPath: pb.Config.ConfigPath + "/" + pb.Config.ConfigFile, ReadOnly: true, } - if vol.VolumeSource.ConfigMap != nil { + if vol.ConfigMap != nil { vm.SubPath = pb.Config.ConfigFile } for i, c := range pod.Spec.Containers { diff --git a/topo/node/alpine/alpine_test.go b/topo/node/alpine/alpine_test.go index 56a447816..9356a1adc 100644 --- a/topo/node/alpine/alpine_test.go +++ b/topo/node/alpine/alpine_test.go @@ -29,7 +29,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" kfake "k8s.io/client-go/kubernetes/fake" - "k8s.io/utils/pointer" + "k8s.io/utils/ptr" ) func TestNew(t *testing.T) { @@ -200,7 +200,7 @@ func TestCreatePod(t *testing.T) { Requests: corev1.ResourceList{}}, ImagePullPolicy: "IfNotPresent", SecurityContext: &corev1.SecurityContext{ - Privileged: pointer.Bool(true), + Privileged: ptr.To(true), }, }, wantDpCtr: corev1.Container{ @@ -212,7 +212,7 @@ func TestCreatePod(t *testing.T) { Requests: corev1.ResourceList{}}, ImagePullPolicy: "IfNotPresent", SecurityContext: &corev1.SecurityContext{ - Privileged: pointer.Bool(true), + Privileged: ptr.To(true), }, VolumeMounts: []corev1.VolumeMount{{Name: "files", MountPath: "/files"}}, }, @@ -246,7 +246,7 @@ func TestCreatePod(t *testing.T) { Requests: corev1.ResourceList{}}, ImagePullPolicy: "IfNotPresent", SecurityContext: &corev1.SecurityContext{ - Privileged: pointer.Bool(true), + Privileged: ptr.To(true), }, VolumeMounts: []corev1.VolumeMount{{ Name: "startup-config-volume", @@ -264,7 +264,7 @@ func TestCreatePod(t *testing.T) { Requests: corev1.ResourceList{}}, ImagePullPolicy: "IfNotPresent", SecurityContext: &corev1.SecurityContext{ - Privileged: pointer.Bool(true), + Privileged: ptr.To(true), }, VolumeMounts: []corev1.VolumeMount{{ Name: "files", @@ -297,7 +297,7 @@ func TestCreatePod(t *testing.T) { Requests: corev1.ResourceList{}}, ImagePullPolicy: "IfNotPresent", SecurityContext: &corev1.SecurityContext{ - Privileged: pointer.Bool(true), + Privileged: ptr.To(true), }, }, }} diff --git a/topo/node/arista/arista.go b/topo/node/arista/arista.go index e3c70a551..6eb5a9146 100644 --- a/topo/node/arista/arista.go +++ b/topo/node/arista/arista.go @@ -259,7 +259,7 @@ func (n *Node) CreateCRD(ctx context.Context) error { }, } for label, v := range proto.GetLabels() { - device.ObjectMeta.Labels[label] = v + device.Labels[label] = v } for _, service := range proto.GetServices() { insidePort := service.Inside @@ -396,7 +396,7 @@ func (n *Node) ConfigPush(ctx context.Context, r io.Reader) error { } if resp.Failed == nil { - log.Infof("%s - finished config push", n.Impl.Proto.Name) + log.Infof("%s - finished config push", n.Proto.Name) } return resp.Failed @@ -422,7 +422,7 @@ func (n *Node) ResetCfg(ctx context.Context) error { } if resp.Failed == nil { - log.Infof("%s - finshed resetting config", n.Name()) + log.Infof("%s - finished resetting config", n.Name()) } return resp.Failed @@ -504,7 +504,7 @@ func (n *Node) FixInterfaces() error { for k, v := range n.Proto.Interfaces { switch { default: - return fmt.Errorf("Unrecognized interface name: %s", v.Name) + return fmt.Errorf("unrecognized interface name: %s", v.Name) case !strings.HasPrefix(k, "eth"), ethIntfRe.MatchString(v.Name), mgmtIntfRe.MatchString(v.Name): case v.Name == "": n.Proto.Interfaces[k].Name = fmt.Sprintf("Ethernet%s", strings.TrimPrefix(k, "eth")) diff --git a/topo/node/arista/arista_test.go b/topo/node/arista/arista_test.go index 9a2ede06e..754d1bd98 100644 --- a/topo/node/arista/arista_test.go +++ b/topo/node/arista/arista_test.go @@ -87,7 +87,7 @@ func TestNew(t *testing.T) { }, }, }, - wantErr: "Unrecognized interface name: Ethernet1/2/3/4", + wantErr: "unrecognized interface name: Ethernet1/2/3/4", }, { desc: "invalid eth intfs 2", nImpl: &node.Impl{ @@ -97,7 +97,7 @@ func TestNew(t *testing.T) { }, }, }, - wantErr: "Unrecognized interface name: Ethernet", + wantErr: "unrecognized interface name: Ethernet", }, { desc: "invalid management intfs 1", nImpl: &node.Impl{ @@ -109,7 +109,7 @@ func TestNew(t *testing.T) { }, }, }, - wantErr: "Unrecognized interface name: Management1/2/3", + wantErr: "unrecognized interface name: Management1/2/3", }, { desc: "invalid management intfs 2", nImpl: &node.Impl{ @@ -119,7 +119,7 @@ func TestNew(t *testing.T) { }, }, }, - wantErr: "Unrecognized interface name: Management", + wantErr: "unrecognized interface name: Management", }, { desc: "default check with empty topo proto", nImpl: &node.Impl{ @@ -457,7 +457,7 @@ func TestCRD(t *testing.T) { Proto: tt.proto, }, } - node.Impl.Proto.Name = name + node.Proto.Name = name err := node.CreateCRD(ctx) if s := errdiff.Check(err, tt.wantErr); s != "" { t.Errorf("New() unexpected err: %s", s) @@ -623,7 +623,7 @@ func TestStatus(t *testing.T) { Proto: &topopb.Node{}, }, } - node.Impl.Proto.Name = name + node.Proto.Name = name status, err := node.Status(ctx) if s := errdiff.Check(err, tt.cantWatch); s != "" { t.Errorf("Status() unexpected err: %s", s) diff --git a/topo/node/cisco/cisco.go b/topo/node/cisco/cisco.go index 88bd38a5b..76298deca 100644 --- a/topo/node/cisco/cisco.go +++ b/topo/node/cisco/cisco.go @@ -37,7 +37,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/rest" log "k8s.io/klog/v2" - "k8s.io/utils/pointer" + "k8s.io/utils/ptr" ) const ( @@ -161,7 +161,7 @@ func (n *Node) Create(ctx context.Context) error { initContainerImage = node.DefaultInitContainerImage } secContext := &corev1.SecurityContext{ - Privileged: pointer.Bool(true), + Privileged: ptr.To(true), } tty := false stdin := false @@ -169,8 +169,8 @@ func (n *Node) Create(ctx context.Context) error { // terminal. This is not required for 8000e nodes. if pb.Model == ModelXRD { secContext = &corev1.SecurityContext{ - Privileged: pointer.Bool(true), - RunAsUser: pointer.Int64(0), + Privileged: ptr.To(true), + RunAsUser: ptr.To(int64(0)), Capabilities: &corev1.Capabilities{ Add: []corev1.Capability{"SYS_ADMIN"}, }, @@ -221,7 +221,7 @@ func (n *Node) Create(ctx context.Context) error { }, }, }}, - TerminationGracePeriodSeconds: pointer.Int64(0), + TerminationGracePeriodSeconds: ptr.To(int64(0)), NodeSelector: map[string]string{}, Affinity: &corev1.Affinity{ PodAntiAffinity: &corev1.PodAntiAffinity{ @@ -243,7 +243,7 @@ func (n *Node) Create(ctx context.Context) error { }, } for label, v := range n.GetProto().GetLabels() { - pod.ObjectMeta.Labels[label] = v + pod.Labels[label] = v } if pb.Config.ConfigData != nil { vol, err := n.CreateConfig(ctx) @@ -256,7 +256,7 @@ func (n *Node) Create(ctx context.Context) error { MountPath: pb.Config.ConfigPath + "/" + pb.Config.ConfigFile, ReadOnly: true, } - if vol.VolumeSource.ConfigMap != nil { + if vol.ConfigMap != nil { vm.SubPath = pb.Config.ConfigFile } for i, c := range pod.Spec.Containers { @@ -277,10 +277,10 @@ func (n *Node) Create(ctx context.Context) error { } // DefaultNodeConstraints returns default node constraints for CISCO. -// If the model for 8000e is specificied correctly it returns defaults for 8000e. +// If the model for 8000e is specified correctly it returns defaults for 8000e. // Otherwise, it returns defaults for XRD by default. func (n *Node) DefaultNodeConstraints() node.Constraints { - if n.Impl == nil || n.Impl.Proto == nil { + if n.Impl == nil || n.Proto == nil { return defaultXRDConstraints } switch n.GetProto().Model { @@ -292,7 +292,7 @@ func (n *Node) DefaultNodeConstraints() node.Constraints { return defaultXRDConstraints } -// validateHostConstraints - Validates host contraints through the default node's implementation. It skips the validation optionally +// validateHostConstraints - Validates host constraints through the default node's implementation. It skips the validation optionally // based on skipValidation flag which is useful for unit tests func validateHostConstraints(n *Node, skipValidation bool) error { if skipValidation { @@ -409,7 +409,10 @@ func getCiscoInterfaceID(pb *tpb.Node, eth string) (string, error) { return pb.Interfaces[eth].Name, nil } // ethWithIDRegx.MatchString(eth) was successful, so no need to do extra check here - ethID, _ := strconv.Atoi(ethRegx.Split(eth, -1)[1]) + ethID, err := strconv.Atoi(ethRegx.Split(eth, -1)[1]) + if err != nil { + return "", fmt.Errorf("failed to parse interface ID from %q: %w", eth, err) + } eid := ethID - 1 switch pb.Model { case "8201": @@ -657,8 +660,8 @@ func endTelnet(d *scraplinetwork.Driver) error { // sending ctrl + ] (^]) to end telnet session gracefully. Otherwise, the next connection can be blocked. endTelnet := string(byte(29)) + " quit\n" log.Infof("Closing the connection by sending ctrl+] quit \n") - d.SendCommand(endTelnet) - return nil + _, err := d.SendCommand(endTelnet) + return err } func (n *Node) ResetCfg(ctx context.Context) error { @@ -671,9 +674,9 @@ func (n *Node) ResetCfg(ctx context.Context) error { var cmd string if n.Proto.Model == ModelXRD { - // Copy the snooped management interface config from a know location and the startup config from + // Copy the snooped management interface config from a known location and the startup config from // the mounted location so it can be applied. This is required to preserve the snooped management - // IP addres and since the "copy" xr_cli command can only access files on disk 0/1. + // IP address and since the "copy" xr_cli command can only access files on disk 0/1. startup_config := n.Proto.Config.Env["XR_EVERY_BOOT_CONFIG"] if startup_config == "" { return status.Errorf(codes.InvalidArgument, "XR_EVERY_BOOT_CONFIG is not set") @@ -748,7 +751,7 @@ func (n *Node) ConfigPush(ctx context.Context, r io.Reader) error { return err } if resp.Failed == nil { - log.Infof("%s - finished config push", n.Impl.Proto.Name) + log.Infof("%s - finished config push", n.Proto.Name) } return resp.Failed @@ -757,7 +760,7 @@ func (n *Node) ConfigPush(ctx context.Context, r io.Reader) error { func (n *Node) GenerateSelfSigned(context.Context) error { // IOS XR automatically generates a self-signed certificate when gRPC is first enabled. // If the startup configuration contains a gRPC configuration, or if the user configures - // gRPC after bootup, the self-signed cert will automatically be created and used. + // gRPC after boot up, the self-signed cert will automatically be created and used. return status.Errorf(codes.Unimplemented, "certificate generation is not supported") } diff --git a/topo/node/cisco/cisco_test.go b/topo/node/cisco/cisco_test.go index 7950127b0..c0d1eec61 100644 --- a/topo/node/cisco/cisco_test.go +++ b/topo/node/cisco/cisco_test.go @@ -41,7 +41,10 @@ func init() { } func defaultNode(pb *tpb.Node) *tpb.Node { - node, _ := defaults(pb) + node, err := defaults(pb) + if err != nil { + panic(err) + } return node } @@ -945,8 +948,14 @@ func TestNodeStatus(t *testing.T) { }() podIsUpRegex = regexp.MustCompile("fake log") // this is the expected log from a fake pod } - nImpl, _ := New(tt.ni) - n, _ := nImpl.(*Node) + nImpl, err := New(tt.ni) + if err != nil { + t.Fatalf("New() failed: %v", err) + } + n, ok := nImpl.(*Node) + if !ok { + t.Fatalf("nImpl is not a *Node") + } status, err := n.Status(ctx) if err != nil { t.Errorf("Error is not expected for Node Status") diff --git a/topo/node/drivenets/drivenets.go b/topo/node/drivenets/drivenets.go index 56fb895a4..85be438f9 100644 --- a/topo/node/drivenets/drivenets.go +++ b/topo/node/drivenets/drivenets.go @@ -121,7 +121,7 @@ var clientFn = func(c *rest.Config) (clientset.Interface, error) { } func (n *Node) Create(ctx context.Context) error { - if n.Impl.Proto.Model != modelCdnos { + if n.Proto.Model != modelCdnos { return fmt.Errorf("cannot create an instance of an unknown model") } return n.cdnosCreate(ctx) @@ -200,7 +200,7 @@ func (n *Node) cdnosCreate(ctx context.Context) error { } func (n *Node) Status(ctx context.Context) (node.Status, error) { - if n.Impl.Proto.Model != modelCdnos { + if n.Proto.Model != modelCdnos { return node.StatusUnknown, fmt.Errorf("invalid model specified") } return n.cdnosStatus(ctx) @@ -228,7 +228,7 @@ func (n *Node) cdnosStatus(ctx context.Context) (node.Status, error) { } func (n *Node) Delete(ctx context.Context) error { - if n.Impl.Proto.Model != modelCdnos { + if n.Proto.Model != modelCdnos { return fmt.Errorf("unknown model") } return n.cdnosDelete(ctx) diff --git a/topo/node/forward/forward.go b/topo/node/forward/forward.go index 5d4a63870..cb1c12f2b 100644 --- a/topo/node/forward/forward.go +++ b/topo/node/forward/forward.go @@ -26,7 +26,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" log "k8s.io/klog/v2" - "k8s.io/utils/pointer" + "k8s.io/utils/ptr" ) const ( @@ -168,10 +168,10 @@ func (n *Node) CreatePod(ctx context.Context) error { Resources: node.ToResourceRequirements(pb.Constraints), ImagePullPolicy: "IfNotPresent", SecurityContext: &corev1.SecurityContext{ - Privileged: pointer.Bool(true), + Privileged: ptr.To(true), }, }}, - TerminationGracePeriodSeconds: pointer.Int64(0), + TerminationGracePeriodSeconds: ptr.To(int64(0)), NodeSelector: map[string]string{}, Affinity: &corev1.Affinity{ PodAntiAffinity: &corev1.PodAntiAffinity{ @@ -193,7 +193,7 @@ func (n *Node) CreatePod(ctx context.Context) error { }, } for label, v := range n.GetProto().GetLabels() { - pod.ObjectMeta.Labels[label] = v + pod.Labels[label] = v } if pb.Config.ConfigData != nil { vol, err := n.CreateConfig(ctx) @@ -206,7 +206,7 @@ func (n *Node) CreatePod(ctx context.Context) error { MountPath: pb.Config.ConfigPath + "/" + pb.Config.ConfigFile, ReadOnly: true, } - if vol.VolumeSource.ConfigMap != nil { + if vol.ConfigMap != nil { vm.SubPath = pb.Config.ConfigFile } for i, c := range pod.Spec.Containers { diff --git a/topo/node/juniper/juniper.go b/topo/node/juniper/juniper.go index a1697a610..dfe19c2f3 100644 --- a/topo/node/juniper/juniper.go +++ b/topo/node/juniper/juniper.go @@ -23,7 +23,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/fields" log "k8s.io/klog/v2" - "k8s.io/utils/pointer" + "k8s.io/utils/ptr" ) // ErrIncompatibleCliConn raised when an invalid scrapligo cli transport type is found. @@ -171,10 +171,10 @@ func (n *Node) SpawnCLIConn() error { } // DefaultNodeConstraints returns default node constraints for Juniper. -// If the model for cptx is specificied correctly it returns defaults for cptx. +// If the model for cptx is specified correctly it returns defaults for cptx. // Otherwise, it returns defaults for ncptx by default. func (n *Node) DefaultNodeConstraints() node.Constraints { - if n.Impl == nil || n.Impl.Proto == nil { + if n.Impl == nil || n.Proto == nil { return defaultNCPTXConstraints } switch n.GetProto().Model { @@ -496,8 +496,8 @@ func (n *Node) Create(ctx context.Context) error { Resources: node.ToResourceRequirements(pb.Constraints), ImagePullPolicy: "IfNotPresent", SecurityContext: &corev1.SecurityContext{ - Privileged: pointer.Bool(true), - RunAsUser: pointer.Int64(0), + Privileged: ptr.To(true), + RunAsUser: ptr.To(int64(0)), Capabilities: &corev1.Capabilities{ Add: []corev1.Capability{"SYS_ADMIN", "NET_ADMIN"}, }, @@ -564,7 +564,7 @@ func (n *Node) Create(ctx context.Context) error { }, }, }, - TerminationGracePeriodSeconds: pointer.Int64(0), + TerminationGracePeriodSeconds: ptr.To(int64(0)), NodeSelector: map[string]string{}, Affinity: &corev1.Affinity{ PodAntiAffinity: &corev1.PodAntiAffinity{ @@ -586,7 +586,7 @@ func (n *Node) Create(ctx context.Context) error { }, } for label, v := range n.GetProto().GetLabels() { - pod.ObjectMeta.Labels[label] = v + pod.Labels[label] = v } if pb.Config.ConfigData != nil { vol, err := n.CreateConfig(ctx) @@ -599,7 +599,7 @@ func (n *Node) Create(ctx context.Context) error { MountPath: pb.Config.ConfigPath + "/" + pb.Config.ConfigFile, ReadOnly: true, } - if vol.VolumeSource.ConfigMap != nil { + if vol.ConfigMap != nil { vm.SubPath = pb.Config.ConfigFile } for i, c := range pod.Spec.Containers { diff --git a/topo/node/node.go b/topo/node/node.go index 3184d9051..a216ee9ac 100644 --- a/topo/node/node.go +++ b/topo/node/node.go @@ -29,7 +29,7 @@ import ( "k8s.io/client-go/rest" "k8s.io/client-go/tools/remotecommand" log "k8s.io/klog/v2" - "k8s.io/utils/pointer" + "k8s.io/utils/ptr" ) type Interface interface { @@ -286,7 +286,7 @@ func validateBoundedInteger(nodeConstraint *tpb.BoundedInteger, hostCons int) er if nodeConstraint.MinValue > nodeConstraint.MaxValue { return fmt.Errorf("invalid bounds. Max value %d is less than min value %d", nodeConstraint.MaxValue, nodeConstraint.MinValue) } - if !(nodeConstraint.MinValue <= int64(hostCons) && int64(hostCons) <= nodeConstraint.MaxValue) { + if int64(hostCons) < nodeConstraint.MinValue || int64(hostCons) > nodeConstraint.MaxValue { return fmt.Errorf("invalid bounded integer constraint. min: %d max %d constraint data %d", nodeConstraint.MinValue, nodeConstraint.MaxValue, hostCons) } @@ -429,10 +429,10 @@ func (n *Impl) CreatePod(ctx context.Context) error { Resources: ToResourceRequirements(pb.Constraints), ImagePullPolicy: "IfNotPresent", SecurityContext: &corev1.SecurityContext{ - Privileged: pointer.Bool(true), + Privileged: ptr.To(true), }, }}, - TerminationGracePeriodSeconds: pointer.Int64(0), + TerminationGracePeriodSeconds: ptr.To(int64(0)), NodeSelector: map[string]string{}, Affinity: &corev1.Affinity{ PodAntiAffinity: &corev1.PodAntiAffinity{ @@ -454,7 +454,7 @@ func (n *Impl) CreatePod(ctx context.Context) error { }, } for label, v := range n.GetProto().GetLabels() { - pod.ObjectMeta.Labels[label] = v + pod.Labels[label] = v } if pb.Config.ConfigData != nil { vol, err := n.CreateConfig(ctx) @@ -467,7 +467,7 @@ func (n *Impl) CreatePod(ctx context.Context) error { MountPath: pb.Config.ConfigPath + "/" + pb.Config.ConfigFile, ReadOnly: true, } - if vol.VolumeSource.ConfigMap != nil { + if vol.ConfigMap != nil { vm.SubPath = pb.Config.ConfigFile } for i, c := range pod.Spec.Containers { @@ -531,7 +531,7 @@ func (n *Impl) CreateService(ctx context.Context) error { // Large topologies may try to allocate more NodePorts than are // supported in default clusters. // https://kubernetes.io/docs/concepts/services-networking/service/#load-balancer-nodeport-allocation - AllocateLoadBalancerNodePorts: pointer.Bool(false), + AllocateLoadBalancerNodePorts: ptr.To(false), }, } sS, err := n.KubeClient.CoreV1().Services(n.Namespace).Create(ctx, s, metav1.CreateOptions{}) @@ -573,7 +573,7 @@ func (n *Impl) DeleteConfig(ctx context.Context) error { } log.V(1).Infof("Deleted config file %s", path) case vs.ConfigMap != nil: - name := vs.ConfigMap.LocalObjectReference.Name + name := vs.ConfigMap.Name if err := n.KubeClient.CoreV1().ConfigMaps(n.Namespace).Delete(ctx, name, metav1.DeleteOptions{}); err != nil { return err } @@ -589,7 +589,7 @@ func (n *Impl) DeleteService(ctx context.Context) error { TypeMeta: metav1.TypeMeta{ APIVersion: "v1", }, - GracePeriodSeconds: pointer.Int64(0), + GracePeriodSeconds: ptr.To(int64(0)), }) } @@ -627,6 +627,7 @@ func (n *Impl) Exec(ctx context.Context, cmd []string, stdin io.Reader, stdout i return err } log.Infof("Execing %s on %s", cmd, n.Name()) + //nolint:staticcheck return exec.Stream(remotecommand.StreamOptions{ Stdin: stdin, Stdout: stdout, @@ -725,9 +726,13 @@ func (n *Impl) PatchCLIConnOpen(bin string, cliCmd []string, opts []scrapliutil. // for a given platform. Retries indefinitely till success and returns a scrapligo network driver instance. func (n *Impl) GetCLIConn(platform string, opts []scrapliutil.Option) (*scraplinetwork.Driver, error) { if log.V(1).Enabled() { - li, _ := scraplilogging.NewInstance(scraplilogging.WithLevel("debug"), + li, err := scraplilogging.NewInstance(scraplilogging.WithLevel("debug"), scraplilogging.WithLogger(log.Info)) - opts = append(opts, scrapliopts.WithLogger(li)) + if err != nil { + log.Warningf("Failed to create scrapli logging instance: %v", err) + } else { + opts = append(opts, scrapliopts.WithLogger(li)) + } } for { @@ -773,10 +778,10 @@ func GetNodeLinks(n *tpb.Node) ([]topologyv1.Link, error) { continue } if ifc.PeerIntName == "" { - return nil, fmt.Errorf("interface %q PeerIntName canot be empty", ifcName) + return nil, fmt.Errorf("interface %q PeerIntName cannot be empty", ifcName) } if ifc.PeerName == "" { - return nil, fmt.Errorf("interface %q PeerName canot be empty", ifcName) + return nil, fmt.Errorf("interface %q PeerName cannot be empty", ifcName) } links = append(links, topologyv1.Link{ UID: int(ifc.Uid), diff --git a/topo/node/node_test.go b/topo/node/node_test.go index 3119861c7..0f08a30f6 100644 --- a/topo/node/node_test.go +++ b/topo/node/node_test.go @@ -15,7 +15,7 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" kfake "k8s.io/client-go/kubernetes/fake" "k8s.io/client-go/rest" - "k8s.io/utils/pointer" + "k8s.io/utils/ptr" ) func NewNR(impl *Impl) (Node, error) { @@ -160,18 +160,18 @@ func TestCreateConfig(t *testing.T) { }, }, }, { - desc: "config file dne", + desc: "config file does not exist", node: &topopb.Node{ Name: "dev1", Vendor: topopb.Vendor(1001), Config: &topopb.Config{ ConfigFile: "test.cfg", ConfigData: &topopb.Config_File{ - File: "testdata/dne.cfg", + File: "testdata/nonexistent.cfg", }, }, }, - wantErr: "open testdata/dne.cfg: no such file", + wantErr: "open testdata/nonexistent.cfg: no such file", }} for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { @@ -199,7 +199,7 @@ func TestCreateConfig(t *testing.T) { t.Errorf("CreateConfig() did not create the expected file: %v", err) } case vs.ConfigMap != nil: - gotCM, err := n.KubeClient.CoreV1().ConfigMaps(n.Namespace).Get(ctx, vs.ConfigMap.LocalObjectReference.Name, metav1.GetOptions{}) + gotCM, err := n.KubeClient.CoreV1().ConfigMaps(n.Namespace).Get(ctx, vs.ConfigMap.Name, metav1.GetOptions{}) if err != nil { t.Errorf("CreateConfig() did not create the expected configmap: %v", err) } @@ -255,7 +255,7 @@ func TestService(t *testing.T) { }}, Selector: map[string]string{"app": "dev1"}, Type: "LoadBalancer", - AllocateLoadBalancerNodePorts: pointer.Bool(false), + AllocateLoadBalancerNodePorts: ptr.To(false), }, }}, }, { @@ -301,7 +301,7 @@ func TestService(t *testing.T) { }}, Selector: map[string]string{"app": "dev2"}, Type: "LoadBalancer", - AllocateLoadBalancerNodePorts: pointer.Bool(false), + AllocateLoadBalancerNodePorts: ptr.To(false), }, }}, }, { @@ -365,7 +365,7 @@ func TestValidateConstraints(t *testing.T) { constraintValues map[string]int }{ { - desc: "Invalid case - contraint value is greater than upper bound", + desc: "Invalid case - constraint value is greater than upper bound", node: &topopb.Node{ Name: "node1", HostConstraints: []*topopb.HostConstraint{ diff --git a/topo/node/nokia/nokia.go b/topo/node/nokia/nokia.go index 6769f3c21..0c122961f 100644 --- a/topo/node/nokia/nokia.go +++ b/topo/node/nokia/nokia.go @@ -211,7 +211,7 @@ func (n *Node) ConfigPush(ctx context.Context, r io.Reader) error { } if resp.Failed != nil { - log.Infof("%s - failed saving config to file", n.Impl.Proto.Name) + log.Infof("%s - failed saving config to file", n.Proto.Name) return resp.Failed } @@ -228,7 +228,7 @@ func (n *Node) ConfigPush(ctx context.Context, r io.Reader) error { } if mresp.Failed != nil { - log.Infof("%s - failed config push", n.Impl.Proto.Name) + log.Infof("%s - failed config push", n.Proto.Name) return resp.Failed } diff --git a/topo/node/openconfig/openconfig.go b/topo/node/openconfig/openconfig.go index 2f9cf5d84..7c6326453 100644 --- a/topo/node/openconfig/openconfig.go +++ b/topo/node/openconfig/openconfig.go @@ -160,7 +160,7 @@ var clientFn = func(c *rest.Config) (clientset.Interface, error) { } func (n *Node) Create(ctx context.Context) error { - switch n.Impl.Proto.Model { + switch n.Proto.Model { case modelLemming: return n.lemmingCreate(ctx) case modelMagna: @@ -236,19 +236,19 @@ func (n *Node) lemmingCreate(ctx context.Context) error { } func (n *Node) Status(ctx context.Context) (node.Status, error) { - switch n.Impl.Proto.Model { + switch n.Proto.Model { case modelMagna: // magna's status uses the standard underlying node implementation. return n.Impl.Status(ctx) case modelLemming: return n.lemmingStatus(ctx) default: - return node.StatusUnknown, fmt.Errorf("invalid model specified.") + return node.StatusUnknown, fmt.Errorf("invalid model specified") } } func (n *Node) DefaultNodeConstraints() node.Constraints { - switch n.Impl.Proto.Model { + switch n.Proto.Model { case modelLemming: return defaultLemmingConstraints default: @@ -278,7 +278,7 @@ func (n *Node) lemmingStatus(ctx context.Context) (node.Status, error) { } func (n *Node) Delete(ctx context.Context) error { - switch n.Impl.Proto.Model { + switch n.Proto.Model { case modelMagna: // magna's implementation uses the standard underlying node implementation. return n.Impl.Delete(ctx) diff --git a/topo/node/sonic/sonic.go b/topo/node/sonic/sonic.go index 4841c100b..86f0bd5da 100644 --- a/topo/node/sonic/sonic.go +++ b/topo/node/sonic/sonic.go @@ -11,6 +11,8 @@ // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. // See the License for the specific language governing permissions and // limitations under the License. + +// Package sonic implements a SONIC node in the topology. package sonic import ( @@ -20,7 +22,7 @@ import ( "github.com/openconfig/kne/topo/node" "google.golang.org/protobuf/proto" - "k8s.io/utils/pointer" + "k8s.io/utils/ptr" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -52,6 +54,7 @@ var ( } ) +// New returns a new SONIC node. func New(nodeImpl *node.Impl) (node.Node, error) { if nodeImpl == nil { return nil, fmt.Errorf("nodeImpl cannot be nil") @@ -83,10 +86,12 @@ func renameInterfaces(in map[string]*tpb.Interface) map[string]*tpb.Interface { return intf } +// Node is a SONIC node. type Node struct { *node.Impl } +// Create creates the SONIC node. func (n *Node) Create(ctx context.Context) error { if err := n.ValidateConstraints(); err != nil { return fmt.Errorf("node %s failed to validate node with errors: %s", n.Name(), err) @@ -120,7 +125,7 @@ func (n *Node) CreatePod(ctx context.Context) error { Resources: node.ToResourceRequirements(pb.Constraints), ImagePullPolicy: "IfNotPresent", SecurityContext: &corev1.SecurityContext{ - Privileged: pointer.Bool(true), + Privileged: ptr.To(true), }, }} @@ -143,11 +148,11 @@ func (n *Node) CreatePod(ctx context.Context) error { }, ImagePullPolicy: "IfNotPresent", SecurityContext: &corev1.SecurityContext{ - Privileged: pointer.Bool(true), + Privileged: ptr.To(true), }, }}, Containers: sonicContainers, - TerminationGracePeriodSeconds: pointer.Int64(0), + TerminationGracePeriodSeconds: ptr.To(int64(0)), NodeSelector: map[string]string{}, Affinity: &corev1.Affinity{ PodAntiAffinity: &corev1.PodAntiAffinity{ @@ -180,7 +185,7 @@ func (n *Node) CreatePod(ctx context.Context) error { MountPath: pb.Config.ConfigPath + "/" + pb.Config.ConfigFile, ReadOnly: true, } - if vol.VolumeSource.ConfigMap != nil { + if vol.ConfigMap != nil { vm.SubPath = pb.Config.ConfigFile } for i, c := range pod.Spec.Containers { @@ -221,6 +226,7 @@ func defaults(pb *tpb.Node) *tpb.Node { return pb } +// DefaultNodeConstraints returns the default node constraints. func (n *Node) DefaultNodeConstraints() node.Constraints { return defaultConstraints } diff --git a/topo/node/sonic/sonic_test.go b/topo/node/sonic/sonic_test.go index db6dd1e90..3ddfa056d 100644 --- a/topo/node/sonic/sonic_test.go +++ b/topo/node/sonic/sonic_test.go @@ -27,7 +27,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" kfake "k8s.io/client-go/kubernetes/fake" - "k8s.io/utils/pointer" + "k8s.io/utils/ptr" ) func TestNew(t *testing.T) { @@ -152,11 +152,11 @@ func TestNew(t *testing.T) { func TestCreatePod(t *testing.T) { tests := []struct { - desc string - nImpl *node.Impl - wantInitCtr corev1.Container - wantSonicCtr corev1.Container - wantErr string + desc string + nImpl *node.Impl + wantInitContainer corev1.Container + wantSonicContainer corev1.Container + wantErr string }{{ desc: "simple sonic container", nImpl: &node.Impl{ @@ -170,16 +170,16 @@ func TestCreatePod(t *testing.T) { }, }, }, - wantInitCtr: corev1.Container{ - Name: "init-sonic-node", - Image: node.DefaultInitContainerImage, - Args: []string{"1", "10", "1"}, + wantInitContainer: corev1.Container{ + Name: "init-sonic-node", + Image: node.DefaultInitContainerImage, + Args: []string{"1", "10", "1"}, ImagePullPolicy: "IfNotPresent", SecurityContext: &corev1.SecurityContext{ - Privileged: pointer.Bool(true), + Privileged: ptr.To(true), }, }, - wantSonicCtr: corev1.Container{ + wantSonicContainer: corev1.Container{ Name: "sonic-node", Image: "sonicImage", Command: []string{"sonicCommand"}, @@ -189,7 +189,7 @@ func TestCreatePod(t *testing.T) { }, ImagePullPolicy: "IfNotPresent", SecurityContext: &corev1.SecurityContext{ - Privileged: pointer.Bool(true), + Privileged: ptr.To(true), }, }, }, { @@ -214,16 +214,16 @@ func TestCreatePod(t *testing.T) { }, }, }, - wantInitCtr: corev1.Container{ - Name: "init-sonic-node", - Image: "customInitImage", - Args: []string{"3", "5", "1"}, + wantInitContainer: corev1.Container{ + Name: "init-sonic-node", + Image: "customInitImage", + Args: []string{"3", "5", "1"}, ImagePullPolicy: "IfNotPresent", SecurityContext: &corev1.SecurityContext{ - Privileged: pointer.Bool(true), + Privileged: ptr.To(true), }, }, - wantSonicCtr: corev1.Container{ + wantSonicContainer: corev1.Container{ Name: "sonic-node", Image: "sonicImage", Command: []string{"sonicCommand"}, @@ -233,7 +233,7 @@ func TestCreatePod(t *testing.T) { }, ImagePullPolicy: "IfNotPresent", SecurityContext: &corev1.SecurityContext{ - Privileged: pointer.Bool(true), + Privileged: ptr.To(true), }, }, }, { @@ -252,16 +252,16 @@ func TestCreatePod(t *testing.T) { }, }, }, - wantInitCtr: corev1.Container{ - Name: "init-sonic-node", - Image: node.DefaultInitContainerImage, - Args: []string{"1", "10", "1"}, + wantInitContainer: corev1.Container{ + Name: "init-sonic-node", + Image: node.DefaultInitContainerImage, + Args: []string{"1", "10", "1"}, ImagePullPolicy: "IfNotPresent", SecurityContext: &corev1.SecurityContext{ - Privileged: pointer.Bool(true), + Privileged: ptr.To(true), }, }, - wantSonicCtr: corev1.Container{ + wantSonicContainer: corev1.Container{ Name: "sonic-node", Image: "sonicImage", Command: []string{"sonicCommand"}, @@ -271,7 +271,7 @@ func TestCreatePod(t *testing.T) { }, ImagePullPolicy: "IfNotPresent", SecurityContext: &corev1.SecurityContext{ - Privileged: pointer.Bool(true), + Privileged: ptr.To(true), }, VolumeMounts: []corev1.VolumeMount{{ Name: "startup-config-volume", @@ -305,16 +305,16 @@ func TestCreatePod(t *testing.T) { if len(initContainers) != 1 { t.Fatalf("Num init containers mismatch: want: 1 got: %v", len(initContainers)) } - if s := cmp.Diff(tt.wantInitCtr, initContainers[0]); s != "" { - t.Fatalf("Init Container mismatch: %s,\n got:\n%v \n want:\n%v\n", s, initContainers[0], tt.wantInitCtr) + if s := cmp.Diff(tt.wantInitContainer, initContainers[0]); s != "" { + t.Fatalf("Init Container mismatch: %s,\n got:\n%v \n want:\n%v\n", s, initContainers[0], tt.wantInitContainer) } containers := pod.Spec.Containers if len(containers) != 1 { t.Fatalf("Num containers mismatch: want: 1 got: %v", len(containers)) } - if s := cmp.Diff(tt.wantSonicCtr, containers[0]); s != "" { - t.Fatalf("Sonic Container mismatch: %s,\n got:\n%v \n want:\n%v\n", s, containers[0], tt.wantSonicCtr) + if s := cmp.Diff(tt.wantSonicContainer, containers[0]); s != "" { + t.Fatalf("Sonic Container mismatch: %s,\n got:\n%v \n want:\n%v\n", s, containers[0], tt.wantSonicContainer) } }) } diff --git a/topo/topo.go b/topo/topo.go index 2e131194b..2ea4f8c1d 100644 --- a/topo/topo.go +++ b/topo/topo.go @@ -267,7 +267,7 @@ func (m *Manager) Create(ctx context.Context, timeout time.Duration) (rerr error } } ctx, cancel := context.WithCancel(ctx) - // Watch the containter status of the pods so we can fail if a container fails to start running. + // Watch the container status of the pods so we can fail if a container fails to start running. if w, err := pods.NewWatcher(ctx, m.kClient, cancel); err != nil { log.Warningf("Failed to start pod watcher: %v", err) } else { @@ -409,7 +409,10 @@ func (m *Manager) Show(ctx context.Context) (*cpb.ShowTopologyResponse, error) { } stateMap := &stateMap{} for _, n := range m.nodes { - phase, _ := n.Status(ctx) + phase, err := n.Status(ctx) + if err != nil { + return nil, err + } stateMap.setNodeState(n.Name(), phase) } return &cpb.ShowTopologyResponse{ @@ -530,8 +533,8 @@ func setLinkPeer(nodeName string, podName string, link *topologyv1.Link, peerSpe for _, peerSpec := range peerSpecs { for _, peerLink := range peerSpec.Spec.Links { // make sure self ifc and peer ifc belong to same link (and hence UID) but are not the same interfaces - if peerLink.UID == link.UID && !(nodeName == link.PeerPod && peerLink.LocalIntf == link.LocalIntf) { - link.PeerPod = peerSpec.ObjectMeta.Name + if peerLink.UID == link.UID && (nodeName != link.PeerPod || peerLink.LocalIntf != link.LocalIntf) { + link.PeerPod = peerSpec.Name link.PeerIntf = peerLink.LocalIntf return nil } @@ -568,7 +571,7 @@ func (m *Manager) topologySpecs(ctx context.Context) ([]*topologyv1.Topology, er return nil, fmt.Errorf("specs do not exist for node %s", link.PeerPod) } - if err := setLinkPeer(nodeName, spec.ObjectMeta.Name, link, peerSpecs); err != nil { + if err := setLinkPeer(nodeName, spec.Name, link, peerSpecs); err != nil { return nil, err } } @@ -677,10 +680,10 @@ func (m *Manager) createMeshnetTopologies(ctx context.Context) error { } log.V(2).Infof("Got topology specs for namespace %s: %+v", m.topo.Name, topologies) for _, t := range topologies { - log.Infof("Creating topology for meshnet node %s", t.ObjectMeta.Name) + log.Infof("Creating topology for meshnet node %s", t.Name) sT, err := m.tClient.Topology(m.topo.Name).Create(ctx, t, metav1.CreateOptions{}) if err != nil { - return fmt.Errorf("could not create topology for meshnet node %s: %v", t.ObjectMeta.Name, err) + return fmt.Errorf("could not create topology for meshnet node %s: %v", t.Name, err) } log.V(1).Infof("Meshnet Node:\n%+v\n", sT) } @@ -695,8 +698,8 @@ func (m *Manager) deleteMeshnetTopologies(ctx context.Context) error { } var errs errlist.List for _, n := range nodes { - if err := m.tClient.Topology(m.topo.Name).Delete(ctx, n.ObjectMeta.Name, metav1.DeleteOptions{}); err != nil { - errs.Add(fmt.Errorf("failed to delete meshnet node %q: %w", n.ObjectMeta.Name, err)) + if err := m.tClient.Topology(m.topo.Name).Delete(ctx, n.Name, metav1.DeleteOptions{}); err != nil { + errs.Add(fmt.Errorf("failed to delete meshnet node %q: %w", n.Name, err)) } } return errs.Err() @@ -718,7 +721,7 @@ func (m *Manager) checkNodeStatus(ctx context.Context, timeout time.Duration) er phase, err := n.Status(ctx) if err != nil || phase == node.StatusFailed { - return fmt.Errorf("Node %s: Status %s Reason %v", n, phase, err) + return fmt.Errorf("node %s: status %s reason %v", n, phase, err) } if phase == node.StatusRunning { log.Infof("Node %s: Status %s", n, phase) diff --git a/topo/topo_test.go b/topo/topo_test.go index a958a7211..e871ad60a 100644 --- a/topo/topo_test.go +++ b/topo/topo_test.go @@ -555,7 +555,7 @@ func TestCreate(t *testing.T) { }, }, }, - wantErr: `Node "bad" (vendor: "1002", model: ""): Status FAILED`, + wantErr: `node "bad" (vendor: "1002", model: ""): status FAILED`, }, { desc: "failed to report metrics, create still passes", opts: []Option{WithUsageReporting(true, "", "")}, @@ -678,7 +678,7 @@ func TestDelete(t *testing.T) { Type: watch.Deleted, Object: &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "dne", + Name: "nonexistent", }, }, }, @@ -1796,7 +1796,7 @@ func TestConfigPush(t *testing.T) { wantErr: "does not implement ConfigPusher interface", }, { desc: "node not found", - name: "dne", + name: "nonexistent", wantErr: "not found", }} for _, tt := range tests { @@ -1834,7 +1834,7 @@ func TestResetCfg(t *testing.T) { wantErr: "does not implement Resetter interface", }, { desc: "node not found", - name: "dne", + name: "nonexistent", wantErr: "not found", }} for _, tt := range tests { @@ -1901,7 +1901,7 @@ func TestGenerateSelfSigned(t *testing.T) { name: "no_info", }, { desc: "node not found", - name: "dne", + name: "nonexistent", wantErr: "not found", }} for _, tt := range tests {