Allow to skip resources on agent images through annotations

[sarahhenkens]

Currently, it's not possible to skip creating the resource requests and limits on the injected containers via the injector. At most, you can set them to empty strings which will emit a 0 as the limits.

This pull request ensures that resource configuration can be skipped if the user has disabled them in their annotations by setting their value to "" (empty string).

• Specifying vault.hashicorp.com/agent-limits-cpu: "" will skip assigning the resource limit all together
• Manually setting vault.hashicorp.com/agent-limits-cpu: "0" will still allow you to set it to 0
• No changes to the default resources when they are omitted from the annonations

Current Behavior

The current behavior will emit a literal 0 for the limits and requests if setting them to "", which has some unexpected behavior with the scheduler and will cause the agents to get OOM killed.

annotations:
  vault.hashicorp.com/agent-inject: "true"
  vault.hashicorp.com/role: your-vault-role
  vault.hashicorp.com/agent-limits-cpu: ""
  vault.hashicorp.com/agent-limits-mem: ""
  vault.hashicorp.com/agent-requests-cpu: ""
  vault.hashicorp.com/agent-requests-mem: ""

Will result in:

Init Containers:
  vault-agent-init:
    Restart Count:  0
    Limits:
      cpu:     0
      memory:  0
    Requests:
      cpu:     0

Updated behavior:

annotations:
  vault.hashicorp.com/agent-inject: "true"
  vault.hashicorp.com/role: your-vault-role
  vault.hashicorp.com/agent-limits-cpu: ""
  vault.hashicorp.com/agent-limits-mem: ""
  vault.hashicorp.com/agent-requests-cpu: ""
  vault.hashicorp.com/agent-requests-mem: ""

Will result in no resources set all together. As its the default for pods in Kubernetes.

Specifying only CPU

When selecting only CPU limits, it is working as expected:

annotations:
  vault.hashicorp.com/agent-inject: "true"
  vault.hashicorp.com/role: your-vault-role
  vault.hashicorp.com/agent-limits-cpu: ""
  vault.hashicorp.com/agent-limits-mem: ""
  vault.hashicorp.com/agent-requests-cpu: "150m"
  vault.hashicorp.com/agent-requests-mem: "64Mi"

Will result in:

    Restart Count:  0
    Requests:
      cpu:     150m
      memory:  64Mi
    Environment:

Testing

• Updated the unit tests to ensure tests pass and check for the new behavior
• Confirmed that the build passes locally
• Manually tested in my local kubernetes cluster

[hashicorp-cla]

All committers have signed the CLA.

[sarahhenkens]

@tvoran, @jasonodonnell, friendly bump up on this pending PR :) Would love your 👀 , feedback and thoughts 🙌

[jasonodonnell]

Hi @sarahhenkens, I had taken a look at this but got distracted, my apologies! In general this looks fine but I'll need to do some functionality testing and a thorough code review.

I'm wondering what your use case is for disabling resource limits? Typically we don't see uncapped containers.

[sarahhenkens]

I'm wondering what your use case is for disabling resource limits? Typically we don't see uncapped containers.

The current version of Kubernetes has some inefficient mechanisms in how it executes CPU throttling through cgroups. Because of this, I decided to not use limits at all in my setup and just use requests.

Kubernetes doesn't enforce users to set resource limits, so the use case is generally: Make it compatible with Kubernetes own capabilities without being overly opinionated on resource best practices.

The current version of vault-k8s supports disabling of resource configuration by passing in "" but it incorrectly sets it to 0 instead of omitting the resource definitions from the Yaml Resource config. This has some unexpected behavior with the scheduler and will cause the agents to get OOM killed.

Since this PR doesn't change the default behavior of vault-k8s, I see it more of an enhancement to give end-users of this library more choices by allowing them to do any of the following:

• Don't set any overrides, use the default
• Set explicit overrides
• Set them to 0 if they really have a reason for it
• Disable resources if they don't want vault-k8s to add resource limits to the sidecar init-container.

[jasonodonnell]

Code looks good, tested fine locally and unit tests are passing. LGTM, thank you for the contribution!

[sarahhenkens]

Thank you for accepting the change! 🙌