Gateway helm chart rendering no longer succeeds when setting CPU resource field to ~

[shydefoo]

Is this the right place to submit this?

• [X] This is not a security vulnerability or a crashing bug
• [X] This is not a question about how to use Istio

Bug Description

On previous versions of istio (before v1.21.0), this configuration had no issues:

  resources:
    requests:
      cpu: 500m
      memory: 1Gi
    limits:
      cpu: ~
      memory: 1Gi

However, on v1.21.0, passing this additional values file throws this error:

➜ helm template . -f my_values.yaml
Error: values don't meet the specifications of the schema(s) in the following chart(s):
gateway:
- resources.limits.cpu: Invalid type. Expected: string, given: null

Upon inspecting the helm chart, it seems that a new defaults field is introduced, and fields under defaults get overwritten by the top level fields of the same name. The workaround seems to be to overwrite the fields under defaults, ie:

defaults: 
  resources:
    requests:
      cpu: 500m
      memory: 1Gi
    limits:
      cpu: ~
      memory: 1Gi

Is there a way to unset the cpu field without having to move resources under defaults?

Version

➜ istioctl version
client version: 1.19.3
control plane version: 1.20.3
data plane version: 1.20.3 (7 proxies)
➜ kubectl version
Client Version: v1.28.3
Kustomize Version: v5.0.4-0.20230601165947-6ce0bf390ce3
Server Version: v1.28.3-gke.1286000
➜ helm version --short
v3.13.1+g3547a4b

Additional Information

No response

[howardjohn]

So the root cause is a difference in how Helm directly merges vs how we are merging with the 'defaults' stuff.

I think basically caused by this line: https://github.com/helm/helm/blob/847369c184d93fc4d36e9ec86a388b60331ab37a/pkg/chartutil/coalesce.go#L225.

We could solve this one-off by allowing it in the schema:

diff --git a/manifests/charts/gateway/values.schema.json b/manifests/charts/gateway/values.schema.json
index c97d84c1e5..4c4f0836d7 100644
--- a/manifests/charts/gateway/values.schema.json
+++ b/manifests/charts/gateway/values.schema.json
@@ -99,10 +99,10 @@
               "type": "object",
               "properties": {
                 "cpu": {
-                  "type": "string"
+                  "type": ["string", "null"]
                 },
                 "memory": {
-                  "type": "string"
+                  "type": ["string", "null"]
                 }
               }
             },
@@ -110,10 +110,10 @@
               "type": "object",
               "properties": {
                 "cpu": {
-                  "type": "string"
+                  "type": ["string", "null"]
                 },
                 "memory": {
-                  "type": "string"
+                  "type": ["string", "null"]
                 }
               }
             }

but that is not very general

[shydefoo]

Sflr @howardjohn and thanks for the information provided! Would it be possible to allow it in the schema? Since this would allow unsetting the default cpu limits specified in values.yaml

[morremeyer]

It should also allow integer for the CPU. I'll try to open a PR in the next few days.

@howardjohn I'm not sure what you mean with

So the root cause is a difference in how Helm directly merges vs how we are merging with the 'defaults' stuff.

can you elaborate please?

[morremeyer]

Another resolution would be to not set resources by default, which is what most charts tend to do since no chart author can know the resource needs in a specific deployment.

[howardjohn]

can you elaborate please?

We do merging in our chart with mergeOverwrite. It behaves differently from how helm internally merges