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

istioctl doesn't correctly patch CPU requests/limits #50990

Open
2 tasks done
iouri-s opened this issue May 11, 2024 · 2 comments
Open
2 tasks done

istioctl doesn't correctly patch CPU requests/limits #50990

iouri-s opened this issue May 11, 2024 · 2 comments
Assignees
Labels
area/environments area/upgrade Issues related to upgrades

Comments

@iouri-s
Copy link

iouri-s commented May 11, 2024

Is this the right place to submit this?

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

Bug Description

./ClusterInit/istioctl-1.20.3-linux-amd64/istioctl upgrade --set values.global.proxy.resources.limits.cpu=200
Run the command with the --force flag if you want to ignore the validation error and proceed.
Error: generate config: json: cannot unmarshal number into Go value of type string

Then I add quotes - same result:
./ClusterInit/istioctl-1.20.3-linux-amd64/istioctl upgrade --set values.global.proxy.resources.limits.cpu="200"

Run the command with the --force flag if you want to ignore the validation error and proceed.
Error: generate config: json: cannot unmarshal number into Go value of type string

Then I add escaped quotes - it looks good at first:

./ClusterInit/istioctl-1.20.3-linux-amd64/istioctl upgrade --set values.global.proxy.resources.limits.cpu=\"200\"

WARNING: Istio is being upgraded from 1.20.0 to 1.20.3.
Running this command will overwrite it; use revisions to upgrade alongside the existing version.
Before upgrading, you may wish to use 'istioctl x precheck' to check for upgrade warnings.
This will install the Istio 1.20.3 "default" profile (with components: Istio core, Istiod, and Ingress gateways) into the cluster. Proceed? (y/N) y
✔ Istio core installed
✔ Istiod installed
✔ Ingress gateways installed
✔ Installation complete
Made this installation the default for injection and validation.

Now - let's check what it created - cpu is double-escaped. This is not going to work!
kubectl get configmap -n istio-system istio-sidecar-injector -o yaml

...

          "resources": {
            "limits": {
              "cpu": "\"200\"",
              "memory": "1024Mi"
            },

Version

$ istioctl version && kubectl version
client version: 1.17.1
control plane version: 1.20.3
data plane version: 1.20-dev (12 proxies), 1.20.3 (1 proxies)
+ kubectl version
Client Version: v1.28.3
Kustomize Version: v5.0.4-0.20230601165947-6ce0bf390ce3
Server Version: v1.27.9

Additional Information

No response

@istio-policy-bot istio-policy-bot added area/environments area/upgrade Issues related to upgrades labels May 11, 2024
@hzxuzhonghu
Copy link
Member

@istio/wg-environments-maintainers

@hanxiaop
Copy link
Member

There should be dup issues about this marshaling error, but I cannot find now. I'd prefer to include the unit, which should work without any fixes.

@dgn dgn self-assigned this May 14, 2024
dgn added a commit to dgn/istio that referenced this issue May 14, 2024
This uses reflection to check field types of our values. The performance
hit should be negligible as this only runs as part of istioctl. The
function turned out to be a bit more complex because we a) use a map
for values instead of the struct in our IstioOperator CRD and b)
protoc-gen-go does not generate proper json tags.

Fixes istio#50990
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/environments area/upgrade Issues related to upgrades
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants