aws-cloudformation / cloudformation-guard

Guard offers a policy-as-code domain-specific language (DSL) to write rules and validate JSON- and YAML-formatted data such as CloudFormation Templates, K8s configurations, and Terraform JSON plans/configurations against those rules. Take this survey to provide feedback about cfn-guard: https://amazonmr.au1.qualtrics.com/jfe/form/SV_bpyzpfoYGGuuUl0
Apache License 2.0
1.29k stars 180 forks source link

feat(pre-commit-hook): Initial implementation in python #22 #524

Closed dannyvassallo closed 4 months ago

dannyvassallo commented 4 months ago

Issue #, if available: 22

Description of changes:

Adds a python based cross platform pre-commit-hook with unit and integration tests for cfn-guard.

Will follow up with docs updates and a tag for the hook to update the pre-commit-config.yml used by tests.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

benbridts commented 4 months ago

I love this!

Would it be possible to pass files to the python wrapper, and have that dynamically construct the --data argument? That way only changed files would be scanned.

Usage would be

# TODO: Update this to use the release from CFN once merged
repos:
  - repo: https://github.com/dannyvassallo/cloudformation-guard
    rev: pre-commit-v0.0.62
    hooks:
      - id: cfn-guard
        args:
          - validate
          - --rules=./guard/resources/validate/rules-dir/

I'd argue that you'd only want to run vaildate anyway, so that could also be moved inside the wrapper, giving

# TODO: Update this to use the release from CFN once merged
repos:
  - repo: https://github.com/dannyvassallo/cloudformation-guard
    rev: pre-commit-v0.0.62
    hooks:
      - id: cfn-guard
        args:
          - --rules=./guard/resources/validate/rules-dir/
dannyvassallo commented 4 months ago

I love this!

Would it be possible to pass files to the python wrapper, and have that dynamically construct the --data argument? That way only changed files would be scanned.

Usage would be

# TODO: Update this to use the release from CFN once merged
repos:
  - repo: https://github.com/dannyvassallo/cloudformation-guard
    rev: pre-commit-v0.0.62
    hooks:
      - id: cfn-guard
        args:
          - validate
          - --rules=./guard/resources/validate/rules-dir/

I'd argue that you'd only want to run vaildate anyway, so that could also be moved inside the wrapper, giving

# TODO: Update this to use the release from CFN once merged
repos:
  - repo: https://github.com/dannyvassallo/cloudformation-guard
    rev: pre-commit-v0.0.62
    hooks:
      - id: cfn-guard
        args:
          - --rules=./guard/resources/validate/rules-dir/

I considered only using validate and setting up the arguments manually before passing them in. I didn't want to remove the ability to run tests pre-commit and the path of least resistance (mapping all of the args) was to just foward them along and allow customers to use the binary as they would outside of the framework. This is a nice take though. I'm not opposed.

benbridts commented 4 months ago

I'd argue that you'd only want to run vaildate anyway, so that could also be moved inside the wrapper, giving

# TODO: Update this to use the release from CFN once merged
repos:
  - repo: https://github.com/dannyvassallo/cloudformation-guard
    rev: pre-commit-v0.0.62
    hooks:
      - id: cfn-guard
        args:
          - --rules=./guard/resources/validate/rules-dir/

I considered only using validate and setting up the arguments manually before passing them in. I didn't want to remove the ability to run tests pre-commit and the path of least resistance (mapping all of the args) was to just foward them along and allow customers to use the binary as they would outside of the framework. This is a nice take though. I'm not opposed.

I like the way https://github.com/python-jsonschema/check-jsonschema/blob/main/.pre-commit-hooks.yaml handles that by defining multiple hooks. For cfn-guard the usage could look like:

# TODO: Update this to use the release from CFN once merged
repos:
  - repo: https://github.com/dannyvassallo/cloudformation-guard
    rev: pre-commit-v0.0.62
    hooks:
      - id: cfn-guard-validate # could also be just cfn-guard, assuming most people want this
        args:
          - --rules=./guard/resources/validate/rules-dir/
      - id: cfn-guard-base  # could als be just cfn-guard
        args:
          - test  # or validate
          - --test-data ...
          - --rules=./guard/resources/validate/rules-dir/

If it all possible: an option(or options) that accepts filenames as input, makes pre-commit a lot easier to use

dannyvassallo commented 4 months ago

@benbridts After some tinkering I settled on this:

-   repo: hook.url
    rev: pre-commit-vX.X.X
    hooks:
        -   id: cfn-guard
            args:
                - "--operation=validate"
                - "--rules=guard/resources/validate/rules-dir/"
            files: ^guard/resources/validate/data-dir/.*
        -   id: cfn-guard
            args:
                - "--operation=test"
                - "--dir=pre_commit_hooks_tests/resources/"
            files: ^pre_commit_hooks_tests/resources.*

This leverages the filenames arg from pre-commit and allows the user to specify the pattern for the directories/files that change through the framework rather than guard itself.

I also hardcoded the version number in the pre-commit hook so it's no longer pulling in the latest and the pre-commit rev will be tied to a specific version of guard starting with 3.1.1