Feature Request: Support for a single-step verification

[TechDufus]

Is your feature request related to a problem?

This policy as-is requires that default max values be provided for either/both CPU and Memory.

I would like to be able to accept/reject based on the existence of CPU/Memory requests/limits at all, and don't care what they are set to. I only care that they are set.

In Rancher™, we specify namespace / project resource limits and do not need to enforce them individually.

Solution you'd like

I'd like to be able to provide the following YAML for the policy settings, which would respectively be a two-step verification, or a single.

# optional
memory:
  defaultRequest: "100M"
  defaultLimit: "500M"
  maxLimit: "4G"
# optional - Just needs to exist.
cpu:
  defaultRequest: "0M"
  defaultLimit: "0M"
  maxLimit: "0G"

Alternatives you've considered

No response

Anything else?

No response

[TechDufus]

I took a swing at this in my fork. Is there any way to see if I'm on the right track? https://github.com/TechDufus/container-resources-policy/tree/feature/Allow_Any_as_0

[flavio]

I think the feature is reasonable, but I'm not 100% convinced about having the 0 values turn on a special behavior.

I would prefer something more explicit inside of the configuration.

I cannot come up with anything that feels good to me, right now the best thing I got is:

# optional
memory:
  constraints: # optional
    defaultRequest: "100M"
    defaultLimit: "500M"
    maxLimit: "4G"
# optional
cpu:
  constraints: # optional
    defaultRequest: 100m
    defaultLimit: 200m
    maxLimit: 500m
# optional
ignoreImages: ["ghcr.io/foo/bar:1.23", "myimage", "otherimages:v1"]

In this way, a configuration like that:

memory:
    constraints:
      defaultRequest: "100M"
      defaultLimit: "500M"
      maxLimit: "4G"
cpu:

You ensure CPU and memory limits are set by the user. In the context of memory limits we would do a check against the constrained values of the policy. While, in the context of CPU, any value provided by the user would be fine.

What do you think?

@kubewarden/kubewarden-developers : please take a look too

[jvanz]

Maybe we can have a boolean flag inside if of the memory and cpu field like enforce. If enforce is true, validate the values using the values defined in the policy settings. Otherwise, just check if the fields has something but does not care about the values.

For example, something like this:

memory:
  enforce: false
  defaultRequest: ""
  defaultLimit: ""
  maxLimit: ""

Just verifies if the memory resources configuration from the admission request is set. Not matter what is there. But something like this:

memory:
  enforce: true
  defaultRequest: "1G"
  defaultLimit: "1G"
  maxLimit: "2G"

Enforces that not only the memory resource configuration is there, but it also verifies if it respects the values defined by the user in the policy settings.

[TechDufus]

I think enforce false makes sense, but then I'd expect to NOT have to provide the other values, even if blank.

Also if enforce is not provided, it can be assumed to be true, which makes the most sense with how the original implementation works.

memory:
  enforce: false
cpu:
  defaultRequest: "1G"
  defaultLimit: "1G"
  maxLimit: "2G"

My original thought was to provide a blank cpu: just as @flavio showed in his example, but I assumed the yaml would freak out (is that legal in yaml), so I opted for my 0 example.

Again, I agree with @flavio that this example makes the MOST sense to me.

memory:
    defaultRequest: "100M"
    defaultLimit: "500M"
    maxLimit: "4G"
cpu:

Or even:

memory:
cpu:

[flavio]

I like @jvanz's proposal. I also agree with @TechDufus, enforce should be true by default.

Recap

Replicate current behavior

Make sure memory settings are set. If the user provided values, make sure they are valid given the policy constraints. If the user didn't sent any value, use the ones defined by the policy:

memory:
  defaultRequest: "100M"
  defaultLimit: "500M"
  maxLimit: "4G"

Given this configuration, there's no check done against the CPU settings.

Note, the configuration from above is equivalent to the following one:

memory:
  enforce: true
  defaultRequest: "100M"
  defaultLimit: "500M"
  maxLimit: "4G"

Make sure some value is set

The following example behaves like the previous one when it comes to the memory settings. In addition to that, the policy now ensures some value is specified with regards to the CPU settings. The policy won't care about the values provided by the user, there must be a value set:

memory:
  defaultRequest: "100M"
  defaultLimit: "500M"
  maxLimit: "4G"
cpu:
  enforce: true

A new type of non valid configuration

The enforce value cannot be set to false while some values are set. The following configuration is not valid:

memory:
  defaultRequest: "100M"
  defaultLimit: "500M"
  maxLimit: "4G"
  enforce: false

Conclusion

If we all agree about this new format of the configuration values, we can move ahead implementing the changes.

@TechDufus do you want to be the one implementing the change?

[TechDufus]

I'd love to work on this, yes.

Let me cook something up and see what the team thinks of it.

[TechDufus]

I'm throwing a PR together for this, but I wanted to quickly explain a change I've made to the above.

I've changed the name enforce to ignoreValues for several reasons:

1. It pairs nicely with the existing ignoreImages value
2. Go bools default to false, and to add the least amount of code changes, having the default 'enforce' behavior be a false value required inverting the naming.

Here is a blurb that my PR will be adding to the README that explains this:

---

If you would only like to enforce that requests and limits are set (ie, you do not care what they are set to, just that they have been set), you can set ignoreValues to true. This will skip the enforcement of specific values and only enforce that requests and limits are set. Here is an example of how to configure this:

# optional
memory:
  ignoreValues: true
# optional
cpu:
  defaultRequest: 100m
  defaultLimit: 200m
  maxLimit: 500m
# optional
ignoreImages: ["ghcr.io/foo/bar:1.23", "myimage", "otherimages:v1"]

Please note from the above example, that when ignoreValues is set to true, the defaultRequest, defaultLimit, and maxLimit fields must not be set. Additionally, ignoreValues default value is false, so it's recommended to only provide it when you want to set it to true.

---

[TechDufus]

Can anyone here build and run the bats tests on my branch? My local env is borked at the moment for some reason...

https://github.com/TechDufus/container-resources-policy/tree/feature/Enforce

[flavio]

@jvanz can you tag a new release to include this feature?