Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

[stable/nginx-ingress] Fix for "spec.clusterIP: Invalid value" error on upgrade #13646

Merged
merged 3 commits into from
May 20, 2019
Merged

[stable/nginx-ingress] Fix for "spec.clusterIP: Invalid value" error on upgrade #13646

merged 3 commits into from
May 20, 2019

Conversation

max-rocket-internet
Copy link
Contributor

@max-rocket-internet max-rocket-internet commented May 9, 2019

What this PR does / why we need it:

Fixes this error by allowing the user to set xxx.service.omitClusterIP=true in their values:

Error: UPGRADE FAILED: Service "nginx-ingress-controller" is invalid: spec.clusterIP: Invalid value: "": field is immutable

The workaround provided in #9227 where you need to retrieve the ClusterIP value from your cluster and insert into your values is not adequate 😃

Which issue this PR fixes

Fixes: #9227

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • DCO signed
  • Chart Version bumped
  • Variables are documented in the README.md
  • title of the PR contains starts with chart name e.g. [stable/chart]

@helm-bot helm-bot added Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 9, 2019
@k8s-ci-robot k8s-ci-robot requested a review from mgoodness May 9, 2019 10:50
@k8s-ci-robot
Copy link
Contributor

Hi @max-rocket-internet. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 9, 2019
@max-rocket-internet
Copy link
Contributor Author

/assign @jackzampolin

@k8s-ci-robot
Copy link
Contributor

@max-rocket-internet: GitHub didn't allow me to assign the following users: jackzampolin.

Note that only helm members and repo collaborators can be assigned and that issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @jackzampolin

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@max-rocket-internet
Copy link
Contributor Author

/assign mgoodness

@@ -19,7 +19,9 @@ metadata:
release: {{ .Release.Name }}
name: {{ template "nginx-ingress.controller.fullname" . }}-metrics
spec:
{{- if not .Values.controller.metrics.service.omitClusterIP }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of a new value that can be set lets have these check if the current value of clusterIP contains anything.

{{- if .Values.controller.metrics.service.clusterIP }}

https://github.com/helm/helm/blob/master/docs/chart_template_guide/control_structures.md#ifelse

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @ChiefAlexander
Of course that's the obvious solution but it's not usable here. This is all covered in #9227

This was tried in #7933 and reverted in #10993

All users who installed the chart between version 0.31.0 and 1.2.2 cannot upgrade.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole thing is a disaster. Add a section in the readme to document this and how users need to use it to upgrade from those versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK done

@ChiefAlexander
Copy link
Collaborator

/assign

…on upgrade

Signed-off-by: Max Williams <max.williams@deliveryhero.com>
Signed-off-by: Max Williams <max.williams@deliveryhero.com>
Signed-off-by: Max Williams <max.williams@deliveryhero.com>
@ChiefAlexander
Copy link
Collaborator

/ok-to-test
/lgtm

@k8s-ci-robot k8s-ci-robot added ok-to-test lgtm Indicates that a PR is ready to be merged. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 20, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ChiefAlexander, max-rocket-internet

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 20, 2019
@k8s-ci-robot k8s-ci-robot merged commit b36ee76 into helm:master May 20, 2019
@max-rocket-internet max-rocket-internet deleted the nginx-ingress_upgrade_fix branch May 21, 2019 08:21
eyenx pushed a commit to eyenx/charts that referenced this pull request May 28, 2019
…on upgrade (helm#13646)

* [stable/nginx-ingress] Fix for "spec.clusterIP: Invalid value" error on upgrade

Signed-off-by: Max Williams <max.williams@deliveryhero.com>

* update README and values.yaml

Signed-off-by: Max Williams <max.williams@deliveryhero.com>

* adding note to readme

Signed-off-by: Max Williams <max.williams@deliveryhero.com>
@kwaazaar
Copy link

kwaazaar commented Jul 29, 2019

I installed 1.6.0 and am now upgrading to 1.10.1 and get this same error. How am I suppose to configure this, because the readme.md is not clear to me.

This is the error I get:
Error: Service "internal-ingresscontroller-nginx-ingress-controller" is invalid: spec.clusterIP: Invalid value: "": field is immutable

On the cmdline I have add this:
--set controller.service.omitClusterIP=true

But that makes no different. I also use a values-yaml which has these fields (partial):
controller:
service:
omitClusterIP: "true"

Also doesn;t help. This makes it impossible to upgrade. All I can do is remove the chart en reinstall.

@max-rocket-internet
Copy link
Contributor Author

@kwaazaar AFAIK version 1.6 wasn't affected by this issue.

Can you paste helm diff output?

@kwaazaar
Copy link

@kwaazaar AFAIK version 1.6 wasn't affected by this issue.

Can you paste helm diff output?

How do I do that?

@heidemn
Copy link

heidemn commented Dec 4, 2019

@naseemkullah I tried all combinations of clusterIP ("", null) and omitClusterIP (true, "true") - no success.

At least I could get out of the situation:
Before, even downgrades back to Chart v1.24.4 no longer worked.
I could go back to this working version using helm rollback nginx-ingress <revision>.
With this Chart version, I can again make updates to the Chart values. The Invalid value: "" error does not occur.

What is funny: I looked at the output of helm get manifest nginx-ingress. The working version and the broken version don't show any difference. Both have spec.clusterIP: "" for controller-service.yaml and default-backend-service.yaml.
Also the output of kubectl get svc nginx-ingress-controller -o yaml doesn't show any difference.

@naseemkullah
Copy link
Collaborator

@naseemkullah I tried all combinations of clusterIP ("", null) and omitClusterIP (true, "true") - no success.

At least I could get out of the situation:
Before, even downgrades back to Chart v1.24.4 no longer worked.
I could go back to this working version using helm rollback nginx-ingress <revision>.
With this Chart version, I can again make updates to the Chart values. The Invalid value: "" error does not occur.

What is funny: I looked at the output of helm get manifest nginx-ingress. The working version and the broken version don't show any difference. Both have spec.clusterIP: "" for controller-service.yaml and default-backend-service.yaml.
Also the output of kubectl get svc nginx-ingress-controller -o yaml doesn't show any difference.

Just to confirm did you try commenting out/removing clusterIP from your values file?

@Jyrno42
Copy link
Contributor

Jyrno42 commented Dec 4, 2019

A similar issue is also occurring for stable/openldap on helm 3.

@bluemalkin
Copy link

Hi I just want to report that I'm experiencing exactly the same issue as described by @heidemn

I'm trying to upgrade from the chart v1.6.4

@ffais
Copy link

ffais commented Dec 5, 2019

I'm experiencing exactly the same issue as described by @heidemn .

I tried to upgrade from Chart 1.24.7 to 1.26.2.
Helm v2.15.1.
Kubernetes AKS v1.14.6.

@naseemkullah
Copy link
Collaborator

naseemkullah commented Dec 5, 2019

From what I gather only fresh installs, either as of 1.6.8 with omitClusterIP values set to true, OR as of 1.26.0 with no values set for the various x.service.clusterIPs in values file will not experience the clusterIP problem.

My understanding is, and I may be wrong but:

If you can afford downtime (probably not) uninstall and reinstall the latest version of the chart without any clusterIP related values (Not even ""). Default chart values have the cluserIP values commented out.

If you cannot afford downtime, no choice but to hardcode all clusterIP values, until you do get some allocated downtime, at which point do the above.

(I'm curious as to why clusterIPs were ever made configurable in the first place?)

@stefanandres
Copy link
Contributor

stefanandres commented Dec 5, 2019

This is even a problem from 1.25.0 -> 1.26.2 for me now. I had this problem before when I had to purge the nginx-ingress release and redeploy it. Now even setting service.omitClusterIP: true for controller, controller.metrics and default-backend does not help for this upgrade.

Also setting ClusterIP to "" does not help for a migration as mentioned in the README

As of version 1.26.0 of this chart, by simply not providing any clusterIP value, invalid: spec.clusterIP: Invalid value: "": field is immutable will no longer occur since clusterIP: "" will not be rendered. If you do wish to provide a clusterIP value in your values file, ensure that it is quoted.

I had to manually get the ClusterIP of each of the 3 services and manually set them into my values. With 1.25.0 I did not have to do this, so this seems like a regression to me. Am I missing something? The goal (for migrating from 1.25.0 to 1.26.2) should still be, to not need to set manually the ClusterIP of existing services.

@naseemkullah
Copy link
Collaborator

This is even a problem from 1.25.0 -> 1.26.2 for me now. I had this problem before when I had to purge the nginx-ingress release and redeploy it. Now even setting service.omitClusterIP: true for controller, controller.metrics and default-backend does not help for this upgrade.

Also setting ClusterIP to "" does not help for a migration as mentioned in the README

The README is unclear then, it states to not set any clusterIP values, (not even ""), I will make a PR to make this a bit more clear/

Please note, I was able to run

helm2 install --name okok stable/nginx-ingress --version 1.25.0 --set controller.service.omitClusterIP=true,controller.stats.service.omitClusterIP=true,controller.metrics.service.omitClusterIP=true,defaultBackend.service.omitClusterIP=true

Followed up with a

helm2 upgrade --reset-values  okok stable/nginx-ingress --version 1.26.1

sucessfully.

However if omitClusterIP was not already being used in the pre 1.25.0 version, the upgrade failed with the infamous clusterIP error.

In other words, when you initially redeployed, had you set the omitClusterIP then and there current upgrade would work.

Both omitClusterIP and current method of simply not

As of version 1.26.0 of this chart, by simply not providing any clusterIP value, invalid: spec.clusterIP: Invalid value: "": field is immutable will no longer occur since clusterIP: "" will not be rendered. If you do wish to provide a clusterIP value in your values file, ensure that it is quoted.

I had to manually get the ClusterIP of each of the 3 services and manually set them into my values. With 1.25.0 I did not have to do this, so this seems like a regression to me. Am I missing something? The goal (for migrating from 1.25.0 to 1.26.2) should still be, to not need to set manually the ClusterIP of existing services.

My understanding is that same thing would happen today if omitClusterIP was not set from the get go.

So to sum up, a freshly installed release as of this commit should set the omitClusterIPs right away and any version installed since 1.26.1 need not set those, but must make sure there is no explicit clusterIP="" that is set.

@pierluigilenoci
Copy link
Contributor

pierluigilenoci commented Dec 6, 2019

I still have this problem. The only solution to update is with these instructions:

CLUSTER_IP=$(kubectl -n kube-system get services nginx-ingress-release-controller -o jsonpath="{.spec.clusterIP}")

helm upgrade -f ./nginx-ingress.yaml nginx-ingress-release stable/nginx-ingress --set controller.service.clusterIP=${CLUSTER_IP}

I'm using chart 0.26.1.

Jyrno42 added a commit to Jyrno42/charts that referenced this pull request Dec 6, 2019
…grade

see

- helm#13646

Signed-off-by: Jyrno Ader <jyrno42@gmail.com>
Jyrno42 added a commit to Jyrno42/charts that referenced this pull request Dec 10, 2019
…grade

see

- helm#13646

Signed-off-by: Jyrno Ader <jyrno42@gmail.com>
Jyrno42 added a commit to Jyrno42/charts that referenced this pull request Dec 10, 2019
…grade

see

- helm#13646

Signed-off-by: Jyrno Ader <jyrno42@gmail.com>
Jyrno42 added a commit to Jyrno42/charts that referenced this pull request Dec 10, 2019
…grade

see

- helm#13646

Signed-off-by: Jyrno Ader <jyrno42@gmail.com>
Jyrno42 added a commit to Jyrno42/charts that referenced this pull request Dec 10, 2019
…grade

see

- helm#13646

Signed-off-by: Jyrno Ader <jyrno42@gmail.com>
@heidemn
Copy link

heidemn commented Dec 14, 2019

Just to confirm did you try commenting out/removing clusterIP from your values file?

Yes, I tried that.

Might it be possible to edit the internal state of Helm2/Tiller to get rid of the offending clusterIP?

@erihanse
Copy link
Contributor

We're also experiencing problems with upgrading from v0.22.1. We have spec.clusterIP: "" on our service manifests. The only way for us to do an upgrade is to do what @pierluigilenoci said, by setting the clusterIP for the services to the current assigned clusterIP. Will that "hack" have any implications in the future if this is fixed properly?

@volwei
Copy link

volwei commented Jan 22, 2020

Hi.
I tried to update from chart version 1.18.0 to 1.29.2. I got the "Service "xyz" is invalid: spec.clusterIP: Invalid value: "": field is immutable" error for the 3 services.
I set clusterIP like described by @pierluigilenoci for the 3 services. After that, the upgrade was successfull.
Now, with version 1.29.2 i got the same error if i execute "helm upgrade -i ... --version 1.29.2". Do i have to set clusterIP always if i want to upgrade? Even with the current version? That is not practicable because my ingress is running on spot instances. There is always the possibility that the ec2 instance will be destroyed because of different reasons and the ingress will start on a new instance. Then, a new ClusterIP will be assigned to the services.
Is there any other way to fix this?

@blurpy
Copy link

blurpy commented Feb 11, 2020

I've been trying to figure out a way to upgrade without hard coding a clusterIP, and it looks like the solution is upgrading the chart with "--force".

Initial attempt the normal way. First install 0.22.1 like it is currently:

# helm upgrade --install "nginx-ingress" stable/nginx-ingress --namespace "ingress" --version "0.22.1" -f nginx-ingress-controller-values-0.22.1.yaml
Release "nginx-ingress" does not exist. Installing it now.
NAME:   nginx-ingress
LAST DEPLOYED: Tue Feb 11 12:19:33 2020
NAMESPACE: ingress
STATUS: DEPLOYED

Then upgrade, including omitClusterIP in the new values:

# helm upgrade --install "nginx-ingress" stable/nginx-ingress --namespace "ingress" --version "1.30.1" -f nginx-ingress-controller-values-1.30.1.yaml 
Error: UPGRADE FAILED: Service "nginx-ingress-controller-metrics" is invalid: spec.clusterIP: Invalid value: "": field is immutable && Service "nginx-ingress-controller" is invalid: spec.clusterIP: Invalid value: "": field is immutable && Service "nginx-ingress-default-backend" is invalid: spec.clusterIP: Invalid value: "": field is immutable

Getting the familiar error.

After cleanup, do the same, except upgrade using --force.

# helm upgrade --install "nginx-ingress" stable/nginx-ingress --namespace "ingress" --version "0.22.1" -f nginx-ingress-controller-values-0.22.1.yaml
Release "nginx-ingress" does not exist. Installing it now.
NAME:   nginx-ingress
LAST DEPLOYED: Tue Feb 11 12:23:05 2020
NAMESPACE: ingress
STATUS: DEPLOYED
# helm upgrade --install "nginx-ingress" stable/nginx-ingress --namespace "ingress" --version "1.30.1" -f nginx-ingress-controller-values-1.30.1.yaml --force
Release "nginx-ingress" has been upgraded. Happy Helming!
LAST DEPLOYED: Tue Feb 11 12:24:03 2020
NAMESPACE: ingress
STATUS: DEPLOYED

Much better!

Not sure if there are any downsides to this yet. This works even without adding omitClusterIP to the values as well, but I'm including it just in case.

@stefanandres
Copy link
Contributor

--force didn't work for me, but this did (#9227 (comment)):
I have done to fix this issue for my ingresses (had a lof of them, so I scripted it).
Because helm3 made this easier (no need for protobuf), I also took the chance to migrate from helm2 to helm3.

patch_nginx_release() {
  set -v
  local secret=$1
  # patch ClusterIP
  patched=$(kubectl get secrets $secret -o jsonpath='{.data.release}' | base64 -d | base64 -d | gzip -d - | gsed 's#  clusterIP: \\"\\"\\n##g' | gzip - | base64 | base64)
  kubectl patch secret $secret -p "{\"data\":{\"release\":\"$patched\"}}" --type=merge
  # patch rbac v1beta deprecation needed for 1.29.x
  patched=$(kubectl get secrets $secret -o jsonpath='{.data.release}' | base64 -d | base64 -d | gzip -d - | gsed 's#rbac.authorization.k8s.io/v1beta1#rbac.authorization.k8s.io/v1#g'  | gzip - | base64 | base64)
  kubectl patch secret $secret -p "{\"data\":{\"release\":\"$patched\"}}" --type=merge
  set +v
}

# Convert helm2 to helm3
kubectl get cm -l NAME=MYHELMRELEASE -o yaml -n kube-system > backup-MYHELMRELEASE-helm2.yaml
helm3 2to3 convert --delete-v2-releases MYHELMRELEASE

# get latest helm3 release via: kubectl get secrets
patch_nginx_release sh.helm.release.v1.MYHELMRELEASE.REVISION

# See that there should be no ClusterIP change
helm3 diff upgrade --namespace NAMESPACE -f values.yaml MYHELMRELEASE stable/nginx --version 1.29.6

# Update labels for helm3 and update from 1.25 to 1.29
helm3 upgrade --namespace NAMESPACE -f values.yaml MYHELMRELEASE stable/nginx --version 1.29.6

I used this like 10-20 times to upgrade from nginx-ingress 1.25.0 to 1.29.6 now.
(I do not give any guarantee that this code works for anyone, please test it before you want to apply it in another test cluster)

@blurpy
Copy link

blurpy commented Feb 11, 2020

Good to have another alternative!

@mmerrill3
Copy link

I've been trying to figure out a way to upgrade without hard coding a clusterIP, and it looks like the solution is upgrading the chart with "--force".

Initial attempt the normal way. First install 0.22.1 like it is currently:

# helm upgrade --install "nginx-ingress" stable/nginx-ingress --namespace "ingress" --version "0.22.1" -f nginx-ingress-controller-values-0.22.1.yaml
Release "nginx-ingress" does not exist. Installing it now.
NAME:   nginx-ingress
LAST DEPLOYED: Tue Feb 11 12:19:33 2020
NAMESPACE: ingress
STATUS: DEPLOYED

Then upgrade, including omitClusterIP in the new values:

# helm upgrade --install "nginx-ingress" stable/nginx-ingress --namespace "ingress" --version "1.30.1" -f nginx-ingress-controller-values-1.30.1.yaml 
Error: UPGRADE FAILED: Service "nginx-ingress-controller-metrics" is invalid: spec.clusterIP: Invalid value: "": field is immutable && Service "nginx-ingress-controller" is invalid: spec.clusterIP: Invalid value: "": field is immutable && Service "nginx-ingress-default-backend" is invalid: spec.clusterIP: Invalid value: "": field is immutable

Getting the familiar error.

After cleanup, do the same, except upgrade using --force.

# helm upgrade --install "nginx-ingress" stable/nginx-ingress --namespace "ingress" --version "0.22.1" -f nginx-ingress-controller-values-0.22.1.yaml
Release "nginx-ingress" does not exist. Installing it now.
NAME:   nginx-ingress
LAST DEPLOYED: Tue Feb 11 12:23:05 2020
NAMESPACE: ingress
STATUS: DEPLOYED
# helm upgrade --install "nginx-ingress" stable/nginx-ingress --namespace "ingress" --version "1.30.1" -f nginx-ingress-controller-values-1.30.1.yaml --force
Release "nginx-ingress" has been upgraded. Happy Helming!
LAST DEPLOYED: Tue Feb 11 12:24:03 2020
NAMESPACE: ingress
STATUS: DEPLOYED

Much better!

Not sure if there are any downsides to this yet. This works even without adding omitClusterIP to the values as well, but I'm including it just in case.

The downsides with this approach is that there will be downtime, since a load balancer will be recreated for the nginx service.

@mmerrill3
Copy link

I made this utility to be able to change helm 2 charts in their configmaps, if you want to remove ClusterIP: "" from the old manifests.
This was intended for our k8s 1.16 upgrade, to update API groups, but you can really update anything with this.
https://github.com/mmerrill3/helm-apigroup-fixer

@mmerrill3
Copy link

I tested out removing the ClusterIP: "" from all the services in the old helm2 release using the utility above, and the upgrade worked for me.

@blurpy
Copy link

blurpy commented Feb 24, 2020

The downsides with this approach is that there will be downtime, since a load balancer will be recreated for the nginx service.

Not sure I understand. Do you mean if you have a service of type LoadBalancer?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). lgtm Indicates that a PR is ready to be merged. ok-to-test size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[stable/nginx-ingress] ClusterIP set to ""