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

Introduce time-based policy rules #738

Open
lcarva opened this issue Sep 2, 2022 · 3 comments
Open

Introduce time-based policy rules #738

lcarva opened this issue Sep 2, 2022 · 3 comments
Labels
enhancement New feature or request

Comments

@lcarva
Copy link
Contributor

lcarva commented Sep 2, 2022

Introducing a new policy rule can be disruptive. One approach to minimize disruption is to introduce the new policy rule first as a warning, then after a period of time, change it to a failure. This, however, has its shortcomings:

  1. Someone/something has to remember to make the change.
  2. The change may be delayed due to external factors.
  3. Users may already be conditioned to ignore warnings.
  4. A warning may not properly convey the message that it will turn into a failure in the future.

Proposal

Introduce the concept of "future_failures", which is a deny policy rule that, when met, is not reported as a failure until a certain period in time. We can leverage rego annotations to specify when that time is. For example:

# METADATA
# custom:
#   effective_on: "2023-01-01T00:00:00Z"
deny {
  true
}

Prior to 2023-01-01T00:00:00Z, conftest reports it under future_failures. As soon as the year turns, conftest reports it under failures.

@jalseth
Copy link
Member

jalseth commented Sep 5, 2022

Hello, thanks for filing this. I don't see a benefit this provides over using the existing warn and deny rules (example below), and it would have a major downside of only working in conftest and would not work when using, for example, OPA Gatekeeper in K8s.

package main

enforcement_time := "2022-09-05"
yyyymmdd_format := "2006-01-02"

deny[msg] {
    time.now_ns() > time.parse_ns(yyyymmdd_format, enforcement_time)
    some_detection_rule
    
    msg := "$FOO is misconfigured."
}

warn[msg] {
    time.now_ns() <= time.parse_ns(yyyymmdd_format, enforcement_time)
    some_detection_rule
    
    msg := sprintf("$FOO is misconfigured. This will be denied starting on %v.", [enforcement_time])
}

some_detection_rule {
   true
}

Please elaborate on what the proposed solution is solving that the example above does not.

@lcarva
Copy link
Contributor Author

lcarva commented Sep 6, 2022

@jalseth, thanks for your reply!

I think the proposal has a few advantages.

It simplifies rule writing. As you demonstrated, the detection logic can be abstracted into another rule to avoid code duplication. We also need to do that for the time comparison if we we have multiple time-based rules. No arguing that this can be solved with the existing constructs though. It's just more verbose and less intuitive IMO.

It also allows de-duplicating annotations more easily. In your suggestion, someone would have to either duplicate the annotations for the "deny" and "warn" rule, or move them to their own doc/package and rely on doc/package annotations instead. This could become quite cumbersome with many rules.

Although we can write the message to indicate that a warning will turn into a failure at some point, this makes it difficult to programmatically highlight them. Again, it's possible to do that today, but it requires more custom code.

@jalseth
Copy link
Member

jalseth commented Sep 9, 2022

it's possible to do that today, but it requires more custom code.

I'm not really following this part. If we implemented this, every other tool (such as one that generates documentation based on OPA Metadata annotations) would also need to add support for this custom annotation as it is non-standard. If anything, I think adding this would require more custom code to be built in the OPA ecosystem.

this makes it difficult to programmatically highlight them.

You can add anything you want to the objects returned by the deny/warn rules that can be programmatically parsed when using conftest's JSON output, such as an enforce_time field.

less intuitive IMO

We will just have to disagree there :) I can't imagine anything more intuitive and straight-forward than a time comparison assertion in the deny/warn rules themselves.

All that aside, nobody would be required to use this if they didn't want to and it would not break any existing workflows so I'd welcome a PR to add this feature.

@jalseth jalseth added the enhancement New feature or request label Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants