Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

logcheck: contextual logging + enhanced checks #297

Merged
merged 11 commits into from Mar 21, 2022
Merged

logcheck: contextual logging + enhanced checks #297

merged 11 commits into from Mar 21, 2022

Commits on Mar 21, 2022

  1. logcheck: add test cases for parameter check 

    The feature was implemented without adding those. We need tests to avoid
regressions.
    @pohly
    pohly committed Mar 21, 2022
    Copy the full SHA
    93980ca View commit details
    Browse the repository at this point in the history

  2. logcheck: move into package 

    The motivation is that at some point in the future this may allow linking the
linter into golangci.
    @pohly
    pohly committed Mar 21, 2022
    Copy the full SHA
    0c36cf7 View commit details
    Browse the repository at this point in the history

  3. logcheck: also check for format specifier in calls like klog.Info 

    Not all of the structured logging calls in klog handle format strings.
This check finds errors in Kubernetes like this one:

cmd/kubeadm/app/util/users/users_linux.go:152:3: logging function "Info" should not use format specifier "%v" (logcheck)
		klog.V(1).Info("Could not open %q, using default system limits: %v", pathLoginDef, err)
    @pohly
    pohly committed Mar 21, 2022
    Copy the full SHA
    3f08024 View commit details
    Browse the repository at this point in the history

  4. logcheck: support running as golangci-lint plugin 

    Running the linter as part of golangci-lint has several advantages:
- faster checking because finding files and parsing is shared
  with other linters
- allows Kubernetes to get rid of the complex and buggy
  hack/verify-structured-logging.sh (kubernetes/kubernetes#106746)
- support for // nolint:logcheck

For this to work, some changes are necessary:
- support env variables for configuration (plugins cannot be configured via
  .golangci-lint.yaml)
- per-file overrides for the overall "allow unstructured" flag

The plugin code itself is trivial.
    @pohly
    pohly committed Mar 21, 2022
    Copy the full SHA
    df531eb View commit details
    Browse the repository at this point in the history

  5. logcheck: support import renaming and improve klog.V(2) handling 

    Importing klog under a different name was not supported because the name of the
identifier was checked instead of the actual package:

   import glog "k8s.io/klog/v2"
   glog.Info("is unstructured")

The hack for peeling the V(2) from a selector that has a CallExpr as expression
was fragile. Checking the type against klog.Verbose is better and also covers
this code fragment:

  if klogV := klog.V(2); klogV.Enabled) {
     klogV.Info("is unstructured")
  }
    @pohly
    pohly committed Mar 21, 2022
    Copy the full SHA
    c4c35ff View commit details
    Browse the repository at this point in the history

  6. logcheck: support logr.Logger 

    Key/value pairs for Logger.WithValues, Logger.Info and Logger.Error must pass
the same sanity checks as the corresponding klog calls.
    @pohly
    pohly committed Mar 21, 2022
    Copy the full SHA
    567c037 View commit details
    Browse the repository at this point in the history

  7. logcheck: warn about functions with both Logger and Context 

    That leads to ambiguities when calling the function because it must be
considered carefully whether the context must contain the logger.
    @pohly
    pohly committed Mar 21, 2022
    Copy the full SHA
    5b4c326 View commit details
    Browse the repository at this point in the history

  8. logcheck: detect if V().Enabled() 

    Besides being slightly inefficient, not storing the result of V()
in a variable is often also an indication for not using the right call inside
the if block:

  if klog.V(2).Enabled() {
      klog.InfoS("some log message")
  }

This records the message with v=0.

Correct is:

 if klogV := klog.V(2); klogV.Enabled() {
     klogV.InfoS("some log message")
 }

Detecting if clauses that do use this condition with a local variable but then
incorrectly use different logger inside the if block is a separate problem and
not solved yet.
    @pohly
    pohly committed Mar 21, 2022
    Copy the full SHA
    41995aa View commit details
    Browse the repository at this point in the history

  9. logcheck: optionally warn about using direct klog logging 

    This is even stricter than the check for unstructured klog functions: only
contextual logging functions are allowed.

merge: logcheck: optionally warn about using legacy klog logging
    @pohly
    pohly committed Mar 21, 2022
    Copy the full SHA
    317fe52 View commit details
    Browse the repository at this point in the history

  10. logcheck: add check for With* and NewContext 

    The corresponding helper functions in klog should be used instead. This is
needed for the feature gate in Kubernetes.
    @pohly
    pohly committed Mar 21, 2022
    Copy the full SHA
    216a02e View commit details
    Browse the repository at this point in the history

  11. logcheck: harmonize report messages 

    If the message consists of full sentences, it should use valid spelling (upper
capital at the beginning, full stop at the end). Shorter messages should be
consistent with messages from other tools where lower case seems to be more
common (https://grep.app/search?q=pass.Report).
    @pohly
    pohly committed Mar 21, 2022
    Copy the full SHA
    1876dee View commit details
    Browse the repository at this point in the history