Apply service changes fails when load balancer is removed #33766
Comments
|
@jeremywadsack how did you create the original service? using |
|
Hi @adohe. Yes, I've always used I originally created the service without the load balancer. Then I updated it to add the load balancer and the firewall rules about a month ago. Let me know if there are more details I can provide to help diagnose this if it's not expected behavior. |
|
I just tested this again, completely outside an application and reproduced the issue. My guess is that Again, using GKE I created a service description, I then created the service: Next I created t2.yml as follows (i.e. the same thing without a load balancer): And once the test service was up and had created the load balancer I tried removing it: |
|
@adohe I just ran into this again. Any further thoughts? I'm now running: To resolve this I ended up having to |
|
Having the same issue, I use apply command to create service and deployment. Updated service to LoadBalancer type, then wanted to move back to default type and getting such error: |
|
FWIW, I ran into this as well. |
|
@bodepd what's your kubernetes version? |
|
@jeremywadsack There are no sig labels on this issue. Please add a sig label by: |
k8s-github-robot
added
the
needs-sig
|
This is still an issue with the test case documented above. /sig sig-node |
|
/sig network |
k8s-ci-robot
added
sig/network
k8s-github-robot
removed
the
needs-sig
|
Ran into this as well |
thockin
added
kind/bug
|
@mengqiy a fun one - guidance on this would be helpful. We may need to change our validation or something? |
|
Same issue in here any fix ? Actually I have a service with Loadbalancer as type wish to change it to ClusterIP, I got that error : |
|
Some help on this would be great...even mitigation steps?...I can confirm that this is still happening today |
|
I was able to mitigate by deleting the service and then applying the new
configuration.
On Sun, Jan 28, 2018 at 8:47 PM Inspiravetion ***@***.***> wrote:
Some help on this would be great...even mitigation steps?...I can confirm
that this is still happening today
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#33766 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAOurFBacTsbDQhT7y83whB0EsB7BbAHks5tPU1ogaJpZM4KKOzb>
.
--
Jeremy Wadsack
|
|
The issue is caused by defaulting: API server defaults field The following is an example after defaulting. You can use |
|
In my case "kubectl apply -f <file.yml> --force" worked. |
|
I'm running into this with Helm Our solution was also to delete the old service before doing the helm upgrade. |
|
@mengqiy @lavalamp Should we have a pattern for this? The user didn't set this field, but they need to unset it. That seems bad. If I don't force them to unset it, it implies that it is in use, but its not, even though it is allocated. We could automatically unset it, but that is somewhat surprising if they actually intended to set it (clearing instead of getting an error). This seems like the least wrong option, but in general we do not mutate user requested fields... |
|
Agreed, it's bad. I think we could fix this by having a proper union semantic somehow (though the API might need several changes). It's always good to have new examples of how things can break. We're working on trying to fix this. Thank you! |
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
|
"Unions" is kind of stalled for now, but we've made a lot of progress on the necessary work to make this happen. I'll keep this issue posted when we make progress, thanks for your patience! |
|
Hi, Any fix on this. I am having the same issue when i delete the LoadBalancer and re- Error Message: Thanks |
|
How are you applying this change?
…On Sun, Jun 21, 2020, 4:04 AM rangapv ***@***.***> wrote:
Hi,
Any fix on this. I am having the same issue when i delete the LoadBalancer
and re-
apply the service as ClusterIP it gives me the error ...
Error Message:
The Service "web2-service" is invalid: spec.ports[0].nodePort: Forbidden:
may not be used when type is 'ClusterIP'
Thanks
Ranga
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#33766 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKWAVC624EX7UZCCBAWCJDRXXSNBANCNFSM4CRI5TNQ>
.
|
|
I did a kubectl apply -f ./web20service.yaml |
|
"--force" is very poorly named: it deletes and recreates the object, which
for service might lose an external LB IP address.
But it's very difficult to say what went wrong without the original object
(including last applied annotation) and the manifest you attempted to
apply. Did you try to change the service type?
…On Mon, Jun 22, 2020 at 7:07 AM rangapv ***@***.***> wrote:
I did a kubectl apply -f ./web20service.yaml
However when I add a --force flag as mentioned by @sivaprakash123
<https://github.com/sivaprakash123> ; it seems to work, I don't know if
that is advisable to do .
Regards
Ranga
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#33766 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAE6BFVVZQZLH7S7ZC4DG6LRX5QRJANCNFSM4CRI5TNQ>
.
|
|
Yes , I did change the service type ... is it forbidden ... |
|
No, but you have to unset a field the system set for you. Mengqi explained
the situation earlier in the thread.
This is likely to remain a friction point until we implement some form of
union. Which is on the list, but won't make the next release.
…On Mon, Jun 22, 2020, 8:47 AM rangapv ***@***.***> wrote:
Yes , I did change the service type ... is it forbidden ...
Regards
Ranga
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#33766 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAE6BFUGMRQTCYN3BJ636ADRX54HTANCNFSM4CRI5TNQ>
.
|
|
Will do! so basically follow Mengqi solution for now.... use kubectl edit to remove type: LoadBalancer, loadBalancerSourceRanges and spec.ports[0].nodePort. It should work. Regards |
This is also a problem when using terraform. Removed the type = LoadBalancer , then applied the change and got: Service "xxx" is invalid: spec.ports[0].nodePort: Forbidden: may not be used when |
|
TF is probably triggering the same bug; you'll have to figure out how to get it to clear the nodePort fields when you make that change. |
|
This has been an issue for a long time. I'm not 100% confident in my
understanding of apply, but maybe it makes sense toi consider a more
radical solution?
What if we changed Service updates to auto-clear NodePort fields ? It's
strictly a breaking change, and we'd need to be really sure about apply and
patch semantics, but it seems more correct?
…On Tue, Sep 22, 2020 at 10:59 AM Daniel Smith ***@***.***> wrote:
TF is probably triggering the same bug; you'll have to figure out how to
get it to clear the nodePort fields when you make that change.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#33766 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKWAVHUTMLS47F6K2X7MHLSHDQX5ANCNFSM4CRI5TNQ>
.
|
|
I think the long-term plan is to use a union, and the short-term workaround is to send empty/null nodePort? |
|
A union for what? Sorry, you lost me. Service has too many
inter-related fields, but they are not all grouped together.
…On Tue, Sep 22, 2020 at 12:17 PM Antoine Pelisse ***@***.***> wrote:
I think the long-term plan is to use a union, and the short-term
workaround is to send empty/null nodePort?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#33766 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKWAVHVDZC2YPUTZ65NUETSHDZ43ANCNFSM4CRI5TNQ>
.
|
|
Yeah, that's why it's long term. My understanding is that some fields only make sense for specific "discriminator" values, they really look like they should be part of a union, but that'd need unions to actually work and probably the whole service fields to be re-arranged differently. It's very very long term ;-) Are we unhappy with the null approach for node port? Does that fail specifically because of how terraform would handle null value? |
|
On Tue, Sep 22, 2020 at 2:16 PM Antoine Pelisse ***@***.***> wrote:
Yeah, that's why it's long term. My understanding is that some fields only
make sense for specific "discriminator" values, they really look like they
should be part of a union, but that'd need unions to actually work and
probably the whole service fields to be re-arranged differently. It's very
very long term ;-)
Are we unhappy with the null approach for node port? Does that fail
specifically because of how terraform would handle null value?
I think what is unhappy is that people
a) didn't set nodePort values (allocated) and don't have them in their YAML
b) change from type=LB to type=ClusterIP
c) we are forcing them to clear nodePort fields
It seems like it would be better all around if we can discern a change in
`type` to auto-clear the nodePort values.
There are other transforms that require fields to be cleared, but they are
less common. I have heard this one repeatedly.
But where would be the right place to do that? defaults.go? That doesn't
know about an "update". Somewhere in the registry I guess, before
validation?
While this is a strictly breaking change, what trouble could it ACTUALLY
cause?
|
|
E.g.
```
diff --git a/pkg/registry/core/service/strategy.go
b/pkg/registry/core/service/strategy.go
index 4e160b3cf046..403ffc106822 100644
--- a/pkg/registry/core/service/strategy.go
+++ b/pkg/registry/core/service/strategy.go
@@ -138,11 +138,26 @@ func (svcStrategy) AllowCreateOnUpdate() bool {
}
func (strategy svcStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList {
- allErrs := validation.ValidateServiceUpdate(obj.(*api.Service), old.(*api.Service))
- allErrs = append(allErrs,validation.ValidateConditionalService(obj.(*api.Service), old.(*api.Service), strategy.ipFamilies)...)
+ before := old.(*api.Service)
+ after := obj.(*api.Service)
+ if needsNodePort(before) && !needsNodePort(after) {
+ for i := range after.Spec.Ports {
+ after.Spec.Ports[i].NodePort = 0
+ }
+ }
+ // FIXME: repeat for ClusterIP
+ allErrs := validation.ValidateServiceUpdate(after, before)
+ allErrs = append(allErrs, validation.ValidateConditionalService(after, before, strategy.ipFamilies)...)
return allErrs
}
+func needsNodePort(svc *api.Service) bool {
+ if svc.Spec.Type == api.ServiceTypeNodePort || svc.Spec.Type == api.ServiceTypeLoadBalancer {
+ return true
+ }
+ return false
+}
+
func (svcStrategy) AllowUnconditionalUpdate() bool {
return true
}
```
|
|
For some reason, your comment doesn't format properly, reproduced here: |
|
If I read this properly, you're modifying the object in the validation function, is that OK? |
|
I don't know if it is "OK", but it works. Is there a better hook?
In general we don't mutate a user-provided spec, so this is a bit off the
beaten path. I could do it in defaults.go, but that would apply even on
create. More consistent but more impactful.
E.g. today a user who creates a service with `type: ClusterIP` and
specifies a `ports[*].nodePort` value will get a validation error - what
they specified is ambiguous. If they are CHANGING `type` from NodePort to
ClusterIP it seems more reasonable to nuke the nodePort value, but I could
be convinced...
This is still very hand-waving and not in any way a plan - I have not
thought hard about possible negative impact.
…On Wed, Sep 23, 2020 at 8:40 AM Antoine Pelisse ***@***.***> wrote:
If I read this properly, you're modifying the object in the validation
function, is that OK?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#33766 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKWAVDBZJDOBILL26WZJJ3SHIJIVANCNFSM4CRI5TNQ>
.
|
Every instance of doing that is an apply bug waiting to happen :-) |
|
NodePort can be set by the user. It's rarely useful but I know people do use it. So TL;DR: a) we clear ports[].nodePort whenever they are not needed (even if the user set them) @khenidak because I know he loves this |
|
It is a ux issue really. If I have always thought of this as fyi the above also applies on changing to mutability is hard... |
|
Was thinking more on this today. What we really want to express is the
idea that we can auto-clear fields IF and ONLY IF we set them in the first
place. In other words, what is we tracked which fields were auto-assigned
and do our best to auto-clear them.
Most users (99.9% or more) do NOT set clusterIP or nodePort values. So in
those corner-cases where those need to be cleared, we CAN clear them. For
those users that set them in the first place, well then need to unset them
manually.
Is that nuts? Is there any precedent for this? I know Service is an
oddball API...
…On Fri, Sep 25, 2020 at 12:58 PM Khaled Henidak (Kal) < ***@***.***> wrote:
It is a ux issue really. If NodePorts are to be treated like ClusterIP;
e.g. allocated by system or set by user then we must expect the same user
to clear them as they change services to types that do not require
NodePorts. a la setting Nonefor headless or clearing ClusterIP when usingExternalName`.
What i am trying to say is same argument can be made for ClusterIP. But
argument is not made due to the fact that ClusterIP has more visibility to
end user.
I have always thought of this as clearer user intent meaning. As a user i
fully understand that i am releasing critical resources that i may not get
back because the system may allocate them immediately else where. The
validation error protects against situations where the user expects
resources to remain allocated, even when types has changed or the type
change is a mistake.
fyi the above also applies on changing to require NodePort service types
to ExternalName.
mutability is hard...
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#33766 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKWAVC53SGGJZ3NBRSVVADSHTY57ANCNFSM4CRI5TNQ>
.
|
I don't know if I agree with that. I understand that this problem Now, the problem is real, and my solution is not so I'm happy to |
|
@piotr-napadlek @jeremywadsack I had the same issue and the workaround of setting nodePort to empty works for me as well. Note that this works on a plain service yml file that I can directly edit. However, I am using Kustomize for my orchestration setup and it seems that setting nodePort to empty, Kustomize just ignores that entry and does not include it on the build. The nodePort is missing from the generated yml file and therefore fail to get applied. Anyone experience this as well? |
|
@yalegria I have successfully worked around this issue using |
|
FTR: This is fixed is v1.20.x |
and others
What keywords did you search in Kubernetes issues before filing this one? (If you have found any duplicates, you should instead reply there.): 'remove load balancer'
BUG REPORT
Kubernetes version (use
kubectl version):Environment:
uname -a):What happened:
I have a service that I configured with an external load balancer with specific firewall rules.
We were ready to remove the external access so I changed the configuration to remove the load balancer:
And ran
kubectl apply -fand got an error about an invalid port:What you expected to happen:
I expected Kubernetes to remove the load balancer from the service and convert back to internal balancing for access from the cluster only.
How to reproduce it (as minimally and precisely as possible):
See configuration files above.
Anything else do we need to know:
I tried this with another service and got repeated result, just a different port:
The text was updated successfully, but these errors were encountered: