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.

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

feat(domains): file domain #688

Closed
wants to merge 9 commits into from
Closed

feat(domains): file domain #688

wants to merge 9 commits into from

Conversation

mildwonkey
Copy link
Contributor

@mildwonkey mildwonkey commented Sep 27, 2024

Description

This adds a files domain which use's OPA's conftest parser to read in a long list of configuration file types * so that rego validations can be written against the contents.

domain:
  type: file
  file-spec:
    filepaths:
    - name: grafna-config
       path: badconfig.ini

I'm not thrilled with the UX right now and I'm not sure what I want to do about it, so here's a draft PR so we can discuss! The file domain takes an array of files, but the implications are that the output DomainResources map uses the file name as a key, so you need to include the filename in the validation:

      package validate
      import rego.v1

      # Default values
      default validate := false
      default msg := "Not evaluated"
      
      validate if {
       check_grafana_config.result
      }
      msg = check_grafana_config.msg
      
      check_grafana_config = {"result": true, "msg": msg} if {
        config := input["badconfig.ini"]
        protocol := config.server.protocol
        protocol == "https"
        msg := "Server protocol is set to https"
      } else = {"result": false, "msg": msg} if {
        config := input["badconfig.ini"]
        protocol := config.server.protocol
        protocol == "http"
        msg := "Protocol is http. That's bad, mmkay?"
      }

I can make a nice list of arguments why this is good, and why this is bad: on the one hand, we don't risk collision if multiple files have the same field. On the other hand, maybe it's ok if the files domain can only read a single file at a time and then we can leave the filename out of the map, or if we choose to merge all the data into a single "file" first. Both feel like big footguns to me. In the former case - it's entirely valid to have for e.g. a dozen terraform files where one will do. you still want to validate it all at once. in the latter - who's responsibility is it to ensure we aren't losing data due to collisions?.

Also this still needs work, tests, and documentation.

From https://www.conftest.dev/:

As of today Conftest supports:

CUE
CycloneDX
Dockerfile
EDN
Environment files (.env)
HCL and HCL2
HOCON
Ignore files (.gitignore, .dockerignore)
INI
JSON
Jsonnet
Property files (.properties)
SPDX
TextProto (Protocol Buffers)
TOML
VCL
XML
YAML

Related Issue

Relates to # #337

  • Creation of a files domain
  • Ability to specify a file path (local or remote) in a validation
    • local
    • remote
  • Ability to process a json file (otherwise error)
  • Ability for each provider to accurately evaluate the data
    • OPA
    • Kyverno

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Other (security config, docs update, etc)

Checklist before merging

@mildwonkey mildwonkey changed the title [DRAFT] feature: file domain feature(domains): file domain Sep 27, 2024
@mildwonkey mildwonkey changed the title feature(domains): file domain feat(domains): file domain Sep 27, 2024
@brandtkeller brandtkeller linked an issue Sep 27, 2024 that may be closed by this pull request
9 tasks
Copy link
Member

@brandtkeller brandtkeller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate the direction this is driving and the conciseness of the current implementation.

Your identified constraints for what we do/do-not allow are certainly worth working through. I believe we're at a phase where constraints are valid if documented such that we can point to them - but would love to continue to weigh approaches.

When it comes to testing - I do want to call out that this should work with any provider - the conftest/opa overlay is obviously very opa heavy (as is much of our implementation with Lula - as OPA/rego is quite powerful) but we do want to ensure we're also testing against the Kyverno baseline. Something we need to practice more often.

@mildwonkey
Copy link
Contributor Author

@brandtkeller yep! I am VERY CONCERNED about how this works with kyverno - honestly I am a bit surprised by how separate the domains and providers are, because I can see a world where certain providers can only support certain domains, or where the domain resources need to be gathered by the provider so we're certain we're getting a provider-compliant output. That concerns me!

src/pkg/domains/files/files.go Outdated Show resolved Hide resolved
src/pkg/domains/files/files.go Show resolved Hide resolved
src/pkg/domains/files/files_spec.go Outdated Show resolved Hide resolved
@meganwolf0
Copy link
Collaborator

we do want to ensure we're also testing against the Kyverno baseline

Does kyverno polices support any data schema or just Kubernetes resources? I should probably know the answer but a quick parsing of their docs seems to indicate it's highly coupled to that format... if so I don't know how we'd use it here

@mildwonkey
Copy link
Contributor Author

we do want to ensure we're also testing against the Kyverno baseline

Does kyverno polices support any data schema or just Kubernetes resources? I should probably know the answer but a quick parsing of their docs seems to indicate it's highly coupled to that format... if so I don't know how we'd use it here

kyverno-json can be used with any arbitrary json-formatted data, so it does work reasonably well with the output of DomainResources. The UX is icky though, IMO, but I barely know kyverno and OPA so I'm hoping I come to like it with time (and of course as far as I know I'm doing this the clunkiest way possible):

provider:
  type: kyverno
  kyverno-spec:
    policy:
      apiVersion: json.kyverno.io/v1alpha1
      kind: ValidatingPolicy
      metadata:
        name: grafana-config
      spec:
        rules:
        - name: protocol-is-https
          assert:
            all:
            - check:
                goodconfig.ini:
                  server:
                    protocol: https

@mildwonkey mildwonkey marked this pull request as ready for review September 30, 2024 18:09
@mildwonkey mildwonkey requested a review from a team as a code owner September 30, 2024 18:09
Copy link
Member

@brandtkeller brandtkeller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appreciative of the code comments and not withstanding the mention of E2E tests am quite excited about this capability.

* XML
* YAML

## Validations
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't yet have a good pattern for the mapping of domain -> provider.

That said - I do like the Kyverno example below. Could we add an OPA provider example as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the writing is very good but I added an OPA example! please nitpick freely.

src/pkg/domains/files/files.go Outdated Show resolved Hide resolved
@mildwonkey
Copy link
Contributor Author

I'm still working on this, but I'm going to split it up into a couple of smaller PRs (probably, either way I'm cleaning this mess up)(messing with directories is always a fun time)

@mildwonkey mildwonkey closed this Oct 1, 2024
@mildwonkey mildwonkey mentioned this pull request Oct 3, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Support for a file domain
3 participants