Add linting and vulnerability check, addressed code quality issues raised by...

Add linting and vulnerability check, addressed code quality issues raised by the linter, partially. A few things are mentioned in issue 27

.gitignore

@@ -18,3 +18,5 @@ gostructs.go
 *.log
 *.out
 gnmi-target
+
+gl-codeclimate.json

.gitlab-ci.yml

 stages:
   - build
+  - analyze
 
 variables:
   IMAGE_PATH: "${CI_REGISTRY_IMAGE}"
+  GOLANG_VERSION: "1.22"
 
 .build: &build
     stage: build
@@ -34,3 +36,25 @@ build-gnmi-target-debian:
         - IMAGE_NAME="$IMAGE_PATH/debian"
         - docker buildx build --provenance=false -t "$IMAGE_NAME:$TAG" -f examples/example01/target.Dockerfile --platform linux/amd64,linux/arm64 --target "debian" --push --build-arg "GITLAB_PROXY=${CI_DEPENDENCY_PROXY_GROUP_IMAGE_PREFIX}/" .
     <<: *build
+
+code-quality:
+    image: ${CI_DEPENDENCY_PROXY_GROUP_IMAGE_PREFIX}/golangci/golangci-lint:v1.59.0-alpine
+    stage: analyze
+    script:
+        # writes golangci-lint output to gl-code-quality-report.json
+        - apk add --update make
+        - make lint
+    artifacts:
+        reports:
+            codequality: gl-codeclimate.json
+        paths:
+            - gl-codeclimate.json
+    needs: []
+
+code-vulnerability:
+    image: ${CI_DEPENDENCY_PROXY_GROUP_IMAGE_PREFIX}/golang:$GOLANG_VERSION
+    stage: analyze
+    script:
+        - go install golang.org/x/vuln/cmd/govulncheck@latest
+        - govulncheck ./...
+    needs: []

.golangci.yml

+variables:
+    GOLANG_VERSION: "1.22"
+run:
+    go: $GOLANG_VERSION
+    concurrency: 8
+    timeout: 20m
+    issues-exit-code: 1
+    skip-dirs-default: true
+    modules-download-mode: readonly
+
+# output settings -> code-climate for GitLab
+output:
+    formats:
+      - format: code-climate
+        path: gl-codeclimate.json
+      - format: colored-line-number
+    print-issued-lines: true
+    print-linter-name: true
+    uniq-by-line: true
+    path-prefix: ""
+
+issues:
+    exclude-use-default: false
+    max-issues-per-linter: 0
+    max-same-issues: 0
+    #exclude-files:
+    #    - http.go
+    # directories to be ignored by linters
+    exclude-dirs:
+        - examples/models
+
+linters:
+    # enable the specific needed linters
+    # see here for full list: https://golangci-lint.run/usage/linters/
+    # linters to consider: gosimple, containedctx, contextcheck, depguard, errchkjson, exhaustive, exhaustruct, forbidigo,
+    # gochecknoinits, gocognit, goconst, gocritic, gofumpt, gomnd, gosec, importas, lll, nestif, nilerr, nlreturn, noctx, nolintlint,
+    # nosnakecase, paralleltest, prealloc, structcheck, testpackage, tparallel, unparam, wastedassign, wrapcheck, wsl
+    disable-all: true
+    enable:
+        - gofmt
+        - goimports
+        - gocyclo
+        - govet
+        - unused
+        - staticcheck
+        - typecheck
+        - revive
+        - whitespace
+        - errcheck
+        - ineffassign
+        - bidichk
+        - durationcheck
+        - errorlint
+        - exportloopref
+        - grouper
+        - makezero
+        - misspell
+        - nilnil
+        - predeclared
+        - godot
+        - errname
+
+# custom settings for linters
+linters-settings:
+    gocyclo:
+        min-complexity: 15
+    golint:
+        min-confidence: 0.8
+    errcheck:
+        # Report about not checking of errors in type assertions: `a := b.(MyStruct)`.
+        # Such cases aren't reported by default.
+        # Default: false
+        check-type-assertions: true
+    revive:
+        severity: warning
+        confidence: 0.8

Makefile

@@ -8,6 +8,8 @@ GOSDN_PRG := $(MAKEFILE_DIR)$(TOOLS_DIR)
 GOPATH := $(~/go)
 GOBIN := $(GOSDN_PRG)
 
+GOLANGCI_LINT_VERSION=v1.59.0
+
 GOCMD=go
 GOBUILD=$(GOCMD) build
 GOCLEAN=$(GOCMD) clean -cache -fuzzcache -testcache -modcache
@@ -22,6 +24,7 @@ install-tools:
 	@echo Install development tooling
 	mkdir -p $(GOSDN_PRG)
 	export GOBIN=$(GOSDN_PRG) && go install github.com/openconfig/ygot/generator@v0.28.3 &&\
+	go install github.com/golangci/golangci-lint/cmd/golangci-lint@$(GOLANGCI_LINT_VERSION) &&\
 	go install github.com/andresterba/go-ygot-generator-generator@v0.0.4
 	@echo Finished installing development tooling
 
@@ -42,6 +45,12 @@ container: build
 container-debug:
 	docker buildx build --rm -t gnmi-target-debug --load -f .examples/example01/Dockerfile.debug .
 
+lint: install-tools
+	./$(TOOLS_DIR)/golangci-lint run --config .golangci.yml
+
+lint-fix: install-tools
+	./$(TOOLS_DIR)/golangci-lint run --config .golangci.yml --fix
+
 self-certs:
 	mkdir -p ./artifacts/ssl/private
 	mkdir -p ./artifacts/ssl/certs

handler/handler.go

@@ -70,7 +70,7 @@ type HandlerJob struct {
 	Deletes []*gnmi.Path
 }
 
-// DefaultPathHandler should be embeded by all PathHandlers to provide a default
+// DefaultPathHandler should be embedded by all PathHandlers to provide a default
 // implementation of the PathHandler interface. Config and the PublishToSubs
 // function are provided through the Init() function of the PathHandler.
 // Init is called for each handler when the gnmitargets `Start()`

internal/gnmiserver/server.go

@@ -110,6 +110,7 @@ func (s *Server) callback(newConfig ygot.ValidatedGoStruct, existingConf ygot.Va
 	configDiff, err := ygot.DiffWithAtomic(existingConf, newConfig)
 	if err != nil {
 		//TODO error logging as we cannot do anything in this case
+		log.Errorf("callback DiffWithAtomic failed with %w", err)
 	}
 
 	// NOTE: This is a test implementation and not particularly sophisticated,
@@ -141,7 +142,7 @@ func (s *Server) callback(newConfig ygot.ValidatedGoStruct, existingConf ygot.Va
 	return configDiff, nil
 }
 
-// TODO: This will be moved
+// TODO: This will be moved.
 func (s *Server) PublishNotificationsToSubscribers(notifications []*gnmi.Notification) error {
 	for _, specificDiff := range notifications {
 		// First for gnmi Updates
@@ -197,7 +198,7 @@ func (s *Server) checkEncodingAndModel(encoding pb.Encoding, models []*pb.ModelD
 			}
 		}
 		if !isSupported {
-			return fmt.Errorf("unsupported model: %v", m)
+			return fmt.Errorf("unsupported model: %s", m)
 		}
 	}
 	return nil
@@ -208,7 +209,7 @@ func (s *Server) checkEncodingAndModel(encoding pb.Encoding, models []*pb.ModelD
 func (s *Server) doDelete(jsonTree map[string]interface{}, prefix, path *pb.Path) (*pb.UpdateResult, error) {
 	// Update json tree of the device config
 	var curNode interface{} = jsonTree
-	pathDeleted := false
+	//pathDeleted := false
 	fullPath := gnmiFullPath(prefix, path)
 	schema := s.model.schemaTreeRoot
 	for i, elem := range fullPath.Elem { // Delete sub-tree or leaf node.
@@ -221,10 +222,11 @@ func (s *Server) doDelete(jsonTree map[string]interface{}, prefix, path *pb.Path
 		if i == len(fullPath.Elem)-1 {
 			if elem.GetKey() == nil {
 				delete(node, elem.Name)
-				pathDeleted = true
+				//pathDeleted = true
 				break
 			}
-			pathDeleted = deleteKeyedListEntry(node, elem)
+			//pathDeleted =
+			deleteKeyedListEntry(node, elem)
 			break
 		}
 
@@ -239,20 +241,20 @@ func (s *Server) doDelete(jsonTree map[string]interface{}, prefix, path *pb.Path
 	}
 
 	//// Apply the validated operation to the config tree and device.
-	if pathDeleted {
-		//	newConfig, err := s.toGoStruct(jsonTree)
-		//	if err != nil {
-		//		return nil, status.Error(codes.Internal, err.Error())
-		//	}
-		//	if s.callback != nil {
-		//		if applyErr := s.callback(newConfig); applyErr != nil {
-		//			if rollbackErr := s.callback(s.config); rollbackErr != nil {
-		//				return nil, status.Errorf(codes.Internal, "error in rollback the failed operation (%v): %v", applyErr, rollbackErr)
-		//			}
-		//			return nil, status.Errorf(codes.Aborted, "error in applying operation to device: %v", applyErr)
-		//		}
-		//	}
-	}
+	//if pathDeleted {
+	//	newConfig, err := s.toGoStruct(jsonTree)
+	//	if err != nil {
+	//		return nil, status.Error(codes.Internal, err.Error())
+	//	}
+	//	if s.callback != nil {
+	//		if applyErr := s.callback(newConfig); applyErr != nil {
+	//			if rollbackErr := s.callback(s.config); rollbackErr != nil {
+	//				return nil, status.Errorf(codes.Internal, "error in rollback the failed operation (%v): %v", applyErr, rollbackErr)
+	//			}
+	//			return nil, status.Errorf(codes.Aborted, "error in applying operation to device: %v", applyErr)
+	//		}
+	//	}
+	//}
 	return &pb.UpdateResult{
 		Path: path,
 		Op:   pb.UpdateResult_DELETE,
@@ -262,6 +264,9 @@ func (s *Server) doDelete(jsonTree map[string]interface{}, prefix, path *pb.Path
 // doReplaceOrUpdate validates the replace or update operation to be applied to
 // the device, modifies the json tree of the config struct, then calls the
 // callback function to apply the operation to the device hardware.
+// TODO: cyclomatic complexity
+//
+// nolint:all
 func (s *Server) doReplaceOrUpdate(jsonTree map[string]interface{}, op pb.UpdateResult_Operation, prefix, path *pb.Path, val *pb.TypedValue) (*pb.UpdateResult, error) {
 	// Validate the operation.
 	fullPath := gnmiFullPath(prefix, path)
@@ -396,31 +401,34 @@ func (s *Server) doReplaceOrUpdate(jsonTree map[string]interface{}, op pb.Update
 func (s *Server) toGoStruct(jsonTree map[string]interface{}) (ygot.ValidatedGoStruct, error) {
 	jsonDump, err := json.Marshal(jsonTree)
 	if err != nil {
-		return nil, fmt.Errorf("error in marshaling IETF JSON tree to bytes: %v", err)
+		return nil, fmt.Errorf("error in marshaling IETF JSON tree to bytes: %w", err)
 	}
 	goStruct, err := s.model.NewConfigStruct(jsonDump)
 	if err != nil {
-		return nil, fmt.Errorf("error in creating config struct from IETF JSON data: %v", err)
+		return nil, fmt.Errorf("error in creating config struct from IETF JSON data: %w", err)
 	}
 	return goStruct, nil
 }
 
 // getGNMIServiceVersion returns a pointer to the gNMI service version string.
 // The method is non-trivial because of the way it is defined in the proto file.
+// TODO: Fix the deprecated path.GetElement
+//
+//nolint:all
 func getGNMIServiceVersion() (*string, error) {
 	gzB, _ := (&pb.Update{}).Descriptor()
 	r, err := gzip.NewReader(bytes.NewReader(gzB))
 	if err != nil {
-		return nil, fmt.Errorf("error in initializing gzip reader: %v", err)
+		return nil, fmt.Errorf("error in initializing gzip reader: %w", err)
 	}
-	defer r.Close()
+	defer r.Close() //nolint:errcheck
 	b, err := io.ReadAll(r)
 	if err != nil {
-		return nil, fmt.Errorf("error in reading gzip data: %v", err)
+		return nil, fmt.Errorf("error in reading gzip data: %w", err)
 	}
 	desc := &dpb.FileDescriptorProto{}
 	if err := proto.Unmarshal(b, desc); err != nil {
-		return nil, fmt.Errorf("error in unmarshaling proto: %v", err)
+		return nil, fmt.Errorf("error in unmarshaling proto: %w", err)
 	}
 	ver := proto.GetExtension(desc.Options, pb.E_GnmiService)
 	return ver.(*string), nil
@@ -472,6 +480,9 @@ func deleteKeyedListEntry(node map[string]interface{}, elem *pb.PathElem) bool {
 }
 
 // gnmiFullPath builds the full path from the prefix and path.
+// TODO: Fix the deprecated path.GetElement
+//
+//nolint:all
 func gnmiFullPath(prefix, path *pb.Path) *pb.Path {
 	fullPath := &pb.Path{Origin: path.Origin}
 	if path.GetElement() != nil {
@@ -483,6 +494,7 @@ func gnmiFullPath(prefix, path *pb.Path) *pb.Path {
 	return fullPath
 }
 
+/*
 // isNIl checks if an interface is nil or its value is nil.
 func isNil(i interface{}) bool {
 	if i == nil {
@@ -495,6 +507,7 @@ func isNil(i interface{}) bool {
 		return false
 	}
 }
+*/
 
 // setPathWithAttribute replaces or updates a child node of curNode in the IETF
 // JSON config tree, where the child node is indexed by pathElem with attribute.
@@ -564,6 +577,9 @@ func (s *Server) Capabilities(ctx context.Context, req *pb.CapabilityRequest) (*
 }
 
 // Get implements the Get RPC in gNMI spec.
+// TODO: Fix the deprecated fullPath.GetElement
+//
+//nolint:all
 func (s *Server) Get(ctx context.Context, req *pb.GetRequest) (*pb.GetResponse, error) {
 	if req.GetType() != pb.GetRequest_ALL {
 		return nil, status.Errorf(codes.Unimplemented, "unsupported request type: %s", pb.GetRequest_DataType_name[int32(req.GetType())])

internal/gnmiserver/subscribe.go

@@ -16,7 +16,7 @@ import (
 
 /* Subscribe: First cut of an implementation
  *
- * Subscribe is called as go-routine and does not need to spawn a new go routine
+ * Subscribe is called as go-routine and does not need to spawn a new go routine.
  */
 func (s *Server) Subscribe(stream gnmi.GNMI_SubscribeServer) error {
 	var subscribeMode gnmi.SubscriptionList_Mode
@@ -73,9 +73,7 @@ func (s *Server) Subscribe(stream gnmi.GNMI_SubscribeServer) error {
 				return status.Error(codes.Unimplemented, "SubscriptionMode_TARGET_DEFINED is not yet implemented.")
 			}
 		}
-
 	}
-
 	return status.Error(codes.Unimplemented, "Subscribe ONCE is not yet implemented.")
 }
 
@@ -100,6 +98,9 @@ func (s *Server) handleStreamOnChangeSubscription(stream gnmi.GNMI_SubscribeServ
 	return nil
 }
 
+// TODO: cyclomatic complexity
+//
+// nolint:all
 func (s *Server) streamOnChangeSubscriptionHandler(notificationSubscriber *notifications.Subscriber) {
 	notificationChannel := notificationSubscriber.ReturnListenChannel()
 

internal/notifications/notifications.go

@@ -116,16 +116,18 @@ func (b *Dispatcher) Unsubscribe(s *Subscriber, topic string) {
 	log.Printf("%s Unsubscribed for topic: %s", s.id, topic)
 }
 
-// Publish the message for given topic
+// Publish the message for given topic.
 func (b *Dispatcher) Publish(stringPath string, msg *gnmi.Notification) {
 	path, err := ygot.StringToStructuredPath(stringPath)
 	if err != nil {
-		//TODO: handle error
+		log.Errorf("Publish/ygot.StringToStructuredPath of %s failed with %v", stringPath, err)
+		return
 	}
 	topics, err := pathToStringList(path)
 	log.Debugf("topics: %v", topics)
 	if err != nil {
-		//TODO: handle error
+		log.Errorf("Publish/pathToStringList failed with %v", err)
+		return
 	}
 	for _, topic := range topics {
 		b.mu.RLock()
@@ -144,7 +146,7 @@ func (b *Dispatcher) Publish(stringPath string, msg *gnmi.Notification) {
 	}
 }
 
-// TODO: rename and add description
+// TODO: rename and add description.
 func pathToStringList(path *gnmi.Path) ([]string, error) {
 	paths := []string{"/"}
 	for p := path; len(p.Elem) != 0; p.Elem = p.Elem[:len(p.Elem)-1] {

target.go

@@ -57,7 +57,6 @@ func NewGnmiTarget(
 }
 
 func (gt *GnmiTarget) Start(bindAddress string, certFile string, keyFile string, caFile string, insecure bool) error {
-
 	//TODO: check if gt has been initialized correctly
 
 	gnmiModel := server.NewModel(gt.modeldata,