From 17e3c555c5115f8c9176bae10ba45baa04d23a7b Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Tue, 25 Jan 2022 11:02:09 +0100 Subject: [PATCH] hack: integrate logcheck into golangci-lint Running logcheck as part of golangci-lint has several advantages: - faster checking because finding files and parsing is shared with other linters - gets rid of the complex and buggy hack/verify-structured-logging.sh (https://github.com/kubernetes/kubernetes/issues/106746) - support for // nolint:logcheck - works with Go 1.18 --- .golangci.yaml | 7 +++ hack/.structured_logging | 15 ----- hack/logcheck.conf | 22 +++++++ hack/make-rules/verify.sh | 1 - hack/tools/go.mod | 2 +- hack/tools/go.sum | 4 +- hack/verify-golangci-lint.sh | 15 ++++- hack/verify-structured-logging.sh | 97 ------------------------------- 8 files changed, 45 insertions(+), 118 deletions(-) delete mode 100644 hack/.structured_logging create mode 100644 hack/logcheck.conf delete mode 100755 hack/verify-structured-logging.sh diff --git a/.golangci.yaml b/.golangci.yaml index b47dc9da4f2..9aad4598eb5 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -21,10 +21,17 @@ linters: # - structcheck # - varcheck - ineffassign + - logcheck - staticcheck - unused linters-settings: # please keep this alphabetized + custom: + logcheck: + # Installed there by hack/verify-golangci-lint.sh. + path: _output/local/bin/logcheck.so + description: structured logging checker + original-url: k8s.io/klog/hack/tools staticcheck: go: "1.17" checks: [ diff --git a/hack/.structured_logging b/hack/.structured_logging deleted file mode 100644 index 044786157a8..00000000000 --- a/hack/.structured_logging +++ /dev/null @@ -1,15 +0,0 @@ -cmd/kube-proxy/... -cmd/kube-scheduler/app/config/... -cmd/kube-scheduler/app/testing/... -cmd/kubelet/... -pkg/kubelet/... -pkg/proxy/... -pkg/scheduler/apis/... -pkg/scheduler/framework/... -pkg/scheduler/internal/cache/fake/... -pkg/scheduler/internal/heap/... -pkg/scheduler/internal/queue/... -pkg/scheduler/metrics/... -pkg/scheduler/profile/... -pkg/scheduler/testing/... -pkg/scheduler/util/... diff --git a/hack/logcheck.conf b/hack/logcheck.conf new file mode 100644 index 00000000000..ced35224675 --- /dev/null +++ b/hack/logcheck.conf @@ -0,0 +1,22 @@ +# This file contains regular expressions that are matched against /, +# for example k8s.io/cmd/kube-scheduler/app/config/config.go. +# +# By default, structured logging call parameters are checked, but usage of +# those calls is not required. That is changed on a per-file basis. +# +# Remember to clean the golangci-lint cache when changing the configuration and +# running the verify-golangci-lint.sh script multiple times, otherwise +# golangci-lint will report stale results: +# _output/local/bin/golangci-lint cache clean + +# At this point we don't enforce the usage structured logging calls except in +# those packages that were migrated. This disables the check for other files. +-structured .* + +# Now enable it again for migrated packages. +structured k8s.io/kubernetes/cmd/kube-proxy/.* +structured k8s.io/kubernetes/cmd/kube-scheduler/.* +structured k8s.io/kubernetes/cmd/kubelet/.* +structured k8s.io/kubernetes/pkg/kubelet/.* +structured k8s.io/kubernetes/pkg/proxy/.* +structured k8s.io/kubernetes/pkg/scheduler/.* diff --git a/hack/make-rules/verify.sh b/hack/make-rules/verify.sh index d8c79fbf8f3..d3cec85c8c6 100755 --- a/hack/make-rules/verify.sh +++ b/hack/make-rules/verify.sh @@ -35,7 +35,6 @@ EXCLUDED_PATTERNS=( "verify-linkcheck.sh" # runs in separate Jenkins job once per day due to high network usage "verify-*-dockerized.sh" # Don't run any scripts that intended to be run dockerized "verify-govet-levee.sh" # Do not run levee analysis by default while KEP-1933 implementation is in alpha. - "verify-structured-logging.sh" # TODO(dims) Need to get this running with golang 1.18 ) # Exclude generated-files-remake in certain cases, if they're running in a separate job. diff --git a/hack/tools/go.mod b/hack/tools/go.mod index fef7ea199d4..8e60bc1ebc9 100644 --- a/hack/tools/go.mod +++ b/hack/tools/go.mod @@ -11,6 +11,6 @@ require ( github.com/google/go-flow-levee v0.1.5 gotest.tools/gotestsum v1.6.4 honnef.co/go/tools v0.2.2 - k8s.io/klog/hack/tools v0.0.0-20210917071902-331d2323a192 + k8s.io/klog/hack/tools v0.0.0-20220321210246-c697110cd8ac sigs.k8s.io/zeitgeist v0.2.0 ) diff --git a/hack/tools/go.sum b/hack/tools/go.sum index 789b77e09f3..226fb535125 100644 --- a/hack/tools/go.sum +++ b/hack/tools/go.sum @@ -1521,8 +1521,8 @@ honnef.co/go/tools v0.0.1-2020.1.3/go.mod h1:X/FiERA/W4tHapMX5mGpAtMSVEeEUOyHaw9 honnef.co/go/tools v0.0.1-2020.1.4/go.mod h1:X/FiERA/W4tHapMX5mGpAtMSVEeEUOyHaw9vFzvIQ3k= honnef.co/go/tools v0.2.2 h1:MNh1AVMyVX23VUHE2O27jm6lNj3vjO5DexS4A1xvnzk= honnef.co/go/tools v0.2.2/go.mod h1:lPVVZ2BS5TfnjLyizF7o7hv7j9/L+8cZY2hLyjP9cGY= -k8s.io/klog/hack/tools v0.0.0-20210917071902-331d2323a192 h1:u27Xm1of9MTDM1CZW3hg0Vv04ohywEG152G8mpr9n8Y= -k8s.io/klog/hack/tools v0.0.0-20210917071902-331d2323a192/go.mod h1:DXW3Mv8xqJvjXWiBSBHrK2O4mq5LMD0clqkv3b1g9HA= +k8s.io/klog/hack/tools v0.0.0-20220321210246-c697110cd8ac h1:c2NQfDLtcwrPdD9pnUcRPlMJ719zgRzdlufHULIW7mU= +k8s.io/klog/hack/tools v0.0.0-20220321210246-c697110cd8ac/go.mod h1:DXW3Mv8xqJvjXWiBSBHrK2O4mq5LMD0clqkv3b1g9HA= k8s.io/klog/v2 v2.0.0/go.mod h1:PBfzABfn139FHAV07az/IF9Wp1bkk3vpT2XSJ76fSDE= k8s.io/utils v0.0.0-20210802155522-efc7438f0176/go.mod h1:jPW/WVKK9YHAvNhRxK0md/EJ228hCsBRufyofKtW8HA= mvdan.cc/gofumpt v0.3.0 h1:kTojdZo9AcEYbQYhGuLf/zszYthRdhDNDUi2JKTxas4= diff --git a/hack/verify-golangci-lint.sh b/hack/verify-golangci-lint.sh index 19fc66d2603..cfd0c1365fc 100755 --- a/hack/verify-golangci-lint.sh +++ b/hack/verify-golangci-lint.sh @@ -36,14 +36,25 @@ PATH="${GOBIN}:${PATH}" export GO111MODULE=on # Install golangci-lint -echo 'installing golangci-lint ' +echo "installing golangci-lint and logcheck plugin from hack/tools into ${GOBIN}" pushd "${KUBE_ROOT}/hack/tools" >/dev/null go install github.com/golangci/golangci-lint/cmd/golangci-lint + go build -o "${GOBIN}/logcheck.so" -buildmode=plugin k8s.io/klog/hack/tools/logcheck/plugin popd >/dev/null cd "${KUBE_ROOT}" -# The config is in ${KUBE_ROOT}/.golangci.yaml +# The config is in ${KUBE_ROOT}/.golangci.yaml where it will be found +# even when golangci-lint is invoked in a sub-directory. +# +# The logcheck plugin currently has to be configured via env variables +# (https://github.com/golangci/golangci-lint/issues/1512). +# +# Remember to clean the golangci-lint cache when changing +# the configuration and running this script multiple times, +# otherwise golangci-lint will report stale results: +# _output/local/bin/golangci-lint cache clean +export LOGCHECK_CONFIG="${KUBE_ROOT}/hack/logcheck.conf" echo 'running golangci-lint ' >&2 res=0 if [[ "$#" -gt 0 ]]; then diff --git a/hack/verify-structured-logging.sh b/hack/verify-structured-logging.sh deleted file mode 100755 index db05ce60e55..00000000000 --- a/hack/verify-structured-logging.sh +++ /dev/null @@ -1,97 +0,0 @@ -#!/usr/bin/env bash - -# Copyright 2021 The Kubernetes Authors. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# 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. - -# This script is used to avoid regressions after a package is migrated -# to structured logging. once a package is completely migrated add -# it .structured_logging file to avoid any future regressions. - -set -o errexit -set -o nounset -set -o pipefail - -KUBE_ROOT=$(dirname "${BASH_SOURCE[0]}")/.. -source "${KUBE_ROOT}/hack/lib/init.sh" -source "${KUBE_ROOT}/hack/lib/util.sh" - -kube::golang::verify_go_version - -# Ensure that we find the binaries we build before anything else. -export GOBIN="${KUBE_OUTPUT_BINPATH}" -PATH="${GOBIN}:${PATH}" - -# Install logcheck -pushd "${KUBE_ROOT}/hack/tools" >/dev/null - GO111MODULE=on go install k8s.io/klog/hack/tools/logcheck -popd >/dev/null - -# We store packages that are migrated in .structured_logging -migrated_packages_file="${KUBE_ROOT}/hack/.structured_logging" - -# Ensure that file is sorted. -kube::util::check-file-in-alphabetical-order "${migrated_packages_file}" - -migrated_packages=() -while IFS='' read -r line; do - migrated_packages+=("$KUBE_ROOT/$line") -done < <(cat "${migrated_packages_file}") - -echo "Running structured logging static check on migrated packages" -ret=0 -GOOS=linux logcheck "${migrated_packages[@]}" || ret=$? -GOOS=windows logcheck "${migrated_packages[@]}" || ret=$? - -if [ $ret -eq 0 ]; then - echo "Structured logging static check passed on migrated packages :)" -else - echo "Please fix above failures. You can locally test via:" - echo "hack/verify-structured-logging.sh" -fi - -# Ignore migrated packages as they are already tested -# Trim the trailing /... from given packages -ignore_packages=$(grep -oE '^[a-zA-Z0-9/]+[^/\.]' < "${KUBE_ROOT}"/hack/.structured_logging) - -all_packages=() -# shellcheck disable=SC2010 -for i in $(ls -d ./*/ | grep -v 'staging' | grep -v 'vendor' | grep -v 'hack') -do - all_packages+=("$(go list ./"$i"/... 2> /dev/null | sed 's/k8s.io\/kubernetes\///g')") - all_packages+=(" ") -done -# We exclude vendor/ except vendor/k8s.io -# This is because vendor/k8s.io is symlinked to staging -# and needs to be checked -all_packages+=("vendor/k8s.io") - -# Packages to test = all_packages - ignored_packages -packages=() -while IFS='' read -r line; do - if [ -z "$line" ]; then continue; fi - packages+=("$KUBE_ROOT/$line/...") -done < <(echo "${all_packages[@]}" | tr " " "\n" | grep -v "$ignore_packages") - -echo -e "\nRunning structured logging static check on all other packages" -GOOS=linux logcheck -allow-unstructured "${packages[@]}" || ret=$? -GOOS=windows logcheck -allow-unstructured "${packages[@]}" || ret=$? - -if [ $ret -eq 0 ]; then - echo "Structured logging static check passed on all packages :)" -else - echo "Please fix above failures. You can locally test via:" - echo "hack/verify-structured-logging.sh" -fi - -exit $ret