search

OpenPeeDeeP / depguard

Go linter that checks if package imports are in a list of acceptable packages.
GNU General Public License v3.0
137 stars 15 forks source link

Reusing configuration #56

Open ianlewis opened 1 year ago

ianlewis commented 1 year ago

Is there a way to reuse configuration from one rule in another? For example, I'd like to create a common rule for all code, and then sub-rules for test and non-test code. As it stands currently, I think I need to largely duplicate the rule for test and non-test code.

rules:
  main:
    files:
      - $all
      - "!$test"
    allow:
      # allowed packages
    deny:
      # denied packages
  test:
    files:
      - $test
    allow:
      # Copy every everything from main here.
      "github.com/google/go-cmp"
    deny:
      #

Also, if I need exceptions for a particular package I can't just specify the exception, I have to exclude the package files from the test and non-test rules, copy both the test and non-test rules and then update them with the exception. This becomes pretty hard to maintain pretty quickly.

For example, all of the following rules would be mostly the same except for some small differences.

rules:
  exception:
    # rules for exception
  exception-tests:
    # rules for tests for the exceptional package
  main:
     # rules for everything else
  test:
     # rules for tests for everything else
dixonwille commented 1 year ago

Don't see the need to copy configs. A file can match multiple rules https://github.com/OpenPeeDeeP/depguard/blob/6a315f2326b2084d60c0a056bcafa10d1e9f5bd7/settings_test.go#LL602C1-L606C4

rules:
  global:
    files:
      - $all
    allow:
      # allowed packages
    deny:
      # denied packages
  test:
    files:
      - $test
    allow:
      "github.com/google/go-cmp"
    deny:
      #

Removing the "!$test" will from main (I renamed main to global above) will allow all test files to match on both lists and should lint appropriately.

ianlewis commented 1 year ago

Hmm, I don't see it doing that. When I used a configuration like this all of my test files failed with errors saying that I wasn't allowed to import stdlib packages. So it seemed like all rules were run completely independently of each other (I'm using golangci-lint 1.53.2).

I may have been doing something wrong but I'll try to add a simple example when my hands free up.

dixonwille commented 1 year ago

I do see the flaw in the code. It does match multiple list, but each list is checked independently of each other, so file has to pass all rule sets they are in... I'll see if I can plan something up, will probably use the $variables I introduced in this version as that is the best backwards compatible way to do it. Brute force would be to combine the lists but I believe that is backwards breaking behavior...

dixonwille commented 1 year ago

https://github.com/OpenPeeDeeP/depguard/blob/6a315f2326b2084d60c0a056bcafa10d1e9f5bd7/depguard.go#LL72C3-L88C4

Relevant code to above comment... Notice that it checks if it is allowed in each list, and if it fails throws a lint error.

ianlewis commented 1 year ago

Apologies. I must have just done something very wrong. I was able to get it to work the way you described and I now can't recreate the config that was failing for me.

I think that what i was trying to do initially was to create a blanket deny in global but then allow it in test when what I should have done is leave it out of the global allow list and only add it to test. Another reason for my confusion is that there is a global allow rule for github.com/google org in my case.

ianlewis commented 1 year ago

Ok, If I have something like the following all the files under exception/ fail badly for me.

rules:                                                                                                                                                                                                                                                                                                                                                                     
  global:                                                                                                                                                                                                                                                                                                                                                                     
    files:                                                                                                                                                                                                                                                                                                                                                                 
      - $all                                                                                                                                                                                                                                                                                                                                                               
    allow:                                                                                                                                                                                                                                                                                                                                                                 
      - $gostd                                                                                                                                                                                                                                                                                                                                                             

  exception:                                                                                                                                                                                                                                                                                                                                                               
    files:                                                                                                                                                                                                                                                                                                                                                                 
      - "**/exception/**/*.go"                                                                                                                                                                                                                                                                                                                                               
    allow:                                                                                                                                                                                                                                                                                                                                                                 
      - "github.com/google/go-cmp"

So it seems that if the allow was empty in exception it will allow anything and so it effectively merges well with global.allow but when I specify a exception.allow then only that list is strictly allowed.

i.e. I can do this.

rules:                                                                                                                                                                                                                                                                                                                                                                     
  all:                                                                                                                                                                                                                                                                                                                                                                     
    files:                                                                                                                                                                                                                                                                                                                                                                 
      - $all                                                                                                                                                                                                                                                                                                                                                               
    allow:                                                                                                                                                                                                                                                                                                                                                                 
      - $gostd                                                                                                                                                                                                                                                                                                                                                             
      - "github.com/google/go-cmp"                                                                                                                                                                                                                                                                                                                                         

  main:                                                                                                                                                                                                                                                                                                                                                                    
    files:                                                                                                                                                                                                                                                                                                                                                                 
      - $all                                                                                                                                                                                                                                                                                                                                                               
      - "!**/exception/**/*.go"                                                                                                                                                                                                                                                                                                                                              
    deny:                                                                                                                                                                                                                                                                                                                                                                  
      - pkg: "github.com/google/go-cmp"                                                                                                                                                                                                                                                                                                                                    
        desc: Please don't use go-cmp

So If I want to have a global allow list and grant exceptions for individual directories it seems the best way to do that currently is to allow everything in the global rule and deny it in the main rule. This is kind of the reverse of how I wanted to express the exception.

ianlewis commented 1 year ago

For reference here is the config I'm currently messing with: https://github.com/slsa-framework/slsa-github-generator/pull/2248/files#diff-6179837f7df53a6f05c522b6b7bb566d484d5465d9894fb04910dd08bb40dcc9

advdv commented 1 year ago

I'm kinda running into the same issue. I think this linter has great potential but I just couldn't get the lists to "overlap" to prevent having to specify dependencies twice. I personally think the README needs a big section that explains the "mental model" a bit better. Maybe more examples, or maybe just different approaches to whitelisting/blacklisting in certain ways. Like, what happens if a package is in two lists? what if it is allowed in one and denied in the other? Is the order of rules important? etc.

If it helps, I'm trying it on this repo: https://github.com/crewlinker/clgo with a end configuration like this: 

  depguard:
    rules:
      tasks:
        files:
          - "**/magefiles/*.go"
        allow:
          - $gostd
          - github.com/magefile/mage
      main:
        files:
          - $all
          - "!$test"
          - "!**/magefiles/*.go"
          - "!**/clzap/logs_testutil.go" #special exception for ginkgo writer
        allow:
          - $gostd
          - github.com/aws/aws-lambda-go/lambda
          - github.com/crewlinker/clgo/
          - github.com/caarlos0/env/v6
          - github.com/aws/aws-sdk-go-v2
          - github.com/go-logr/zapr
          - github.com/jackc/pgx/v5
          - github.com/redis/go-redis
          - github.com/XSAM/otelsql
          - ariga.io/atlas/sql
      zaplog:
        files:
          - "**/clzap/logs_testutil.go"
        allow:
          - $gostd
          - github.com/onsi/ginkgo/v2
      tests:
        files:
          - $test
        allow: 
          - $gostd
          - github.com/crewlinker/clgo/
          - github.com/onsi/ginkgo/v2
          - github.com/joho/godotenv
          - github.com/caarlos0/env/v6
          - github.com/aws/aws-sdk-go-v2
          - github.com/go-logr/zapr
          - github.com/jackc/pgx/v5
          - github.com/redis/go-redis
          - github.com/XSAM/otelsql
          - ariga.io/atlas/sql
          - github.com/onsi/gomega

I would expect that if I remove !$test from the "main" rule it would starting to overlap and I wouldn't have to provide the same dependencies in the "tests" rule but it didn't work for me.

NOTE2: In the same repo above I didn't need to specify go.uber.org/fx or go.uber.org/zap at all, which confused me as well. I feel like I'm just not getting it.

ianlewis commented 1 year ago

Yeah, IIUC, you'd need to do something like the following if you want to dedup the allow list. Which is a bit ununintuitive to express.

depguard:
  rules:
    global:
      files:
        - $all
        - "!**/magefiles/*.go"
        - "!**/clzap/logs_testutil.go"
      allow:
        - $gostd
        - github.com/aws/aws-sdk-go-v2
        - github.com/crewlinker/clgo/
        - github.com/caarlos0/env/v6
        - github.com/go-logr/zapr
        - github.com/jackc/pgx/v5
        - github.com/redis/go-redis
        - github.com/XSAM/otelsql
        - ariga.io/atlas/sql

        # Allowed in non-test code
        - github.com/aws/aws-lambda-go/lambda

        # Allowed in test code.
        - github.com/onsi/ginkgo/v2
        - github.com/joho/godotenv
        - github.com/onsi/gomega

    tasks:
      files:
        - "**/magefiles/*.go"
      allow:
        - $gostd
        - github.com/magefile/mage

    zaplog:
      files:
        - "**/clzap/logs_testutil.go"
      allow:
        - $gostd
        - github.com/onsi/ginkgo/v2

    main:
      files:
        - $all
        - "!$test"
        - "!**/magefiles/*.go" # special exception for magefiles
        - "!**/clzap/logs_testutil.go" # special exception for ginkgo writer
      deny:
        - pkg: github.com/onsi/ginkgo/v2
          description: Don't use.
        - pkg: github.com/joho/godotenv
          description: Don't use.
        - pkg: github.com/onsi/gomega
          description: Don't use.

    tests:
      files:
        - $test
      deny:
        - pkg: github.com/aws/aws-lambda-go/lambda
          description: Don't use.
dixonwille commented 1 year ago

I have not forgotten about this, just have not had the time to look for, much less develop, a backwards compatible solution.

Fixing the loop I linked earlier is a breaking change as configurations that were setup to use it, were expecting that behavior... The loop assumes it needs to pass all lists a file matches for (which is most strict AND logic) but appears some want a less strict OR logic. Maybe add a configuration that defaults to the AND logic but can be configure to OR logic...

It seems 2 options are backwards compatible.

  1. Add variables that allow you to reference another list's allow/deny lists
  2. Create a configuration field that defaults to the "STRICT" mode but can be configured to a "LAX" mode (naming should be revised).

Open to other solutions or some pro/con conversation on either solution? In theory we could introduce both options.

dixonwille commented 9 months ago

So I have Strict and Lax mode for the package matching setup. But I would like to add reference variables as well to cover this issue more thoroughly.