diff --git a/.gitignore b/.gitignore index a43df50290c76cbc16c2bdc4989eabec412bf62b..e0b14fb242dbbd1a79b41fac043fb24e3874d335 100644 --- a/.gitignore +++ b/.gitignore @@ -18,3 +18,5 @@ gostructs.go *.log *.out gnmi-target + +gl-codeclimate.json diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 1e2c51b8ca0a3b800172a86967ecc287a73c69c0..481db67f92bef035187c71fd0f7d894be7557d20 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -1,8 +1,10 @@ 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: [] diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 0000000000000000000000000000000000000000..8d4b9395b803b09544a5d37f217d48dc5f8467dc --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,76 @@ +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 diff --git a/Makefile b/Makefile index 773a5a55942515937e20e583fdfa03d8f03a0a86..68ad35f5e3bd9acd98d194ddf1ba1fc2ed91854a 100644 --- a/Makefile +++ b/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 diff --git a/handler/handler.go b/handler/handler.go index 7ed5fdd6935a81416dac652989ebef4ac394140f..4484b6124ac596b113fc18be3cbc4c1414194417 100644 --- a/handler/handler.go +++ b/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()` diff --git a/internal/gnmiserver/server.go b/internal/gnmiserver/server.go index e74732f9a4eb9cf3d796e51c3df72101e1d705b9..1ad9553a74bcc114874353ee79fbda24ce4635e0 100644 --- a/internal/gnmiserver/server.go +++ b/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())]) diff --git a/internal/gnmiserver/subscribe.go b/internal/gnmiserver/subscribe.go index 9a15c9a70edcbb5ced10aaa478daa418fe9be960..f43738005a062c778d8ca2bd68d2df0704722370 100644 --- a/internal/gnmiserver/subscribe.go +++ b/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() diff --git a/internal/notifications/notifications.go b/internal/notifications/notifications.go index 0d30b7aea7c1f028a6ad758e5db368f45e5c9248..bcd56912dd7ff8964ffb35fa4e6a70792b588b20 100644 --- a/internal/notifications/notifications.go +++ b/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] { diff --git a/target.go b/target.go index 02230ecb6bf697abe9145ad9c476f635e317180a..0cee19c0282e9bba4634005c07f9bb64f476b503 100644 --- a/target.go +++ b/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,