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

helm chart should support setting internalTrafficPolicy service value #10798

Closed
abasalaev opened this issue Dec 22, 2023 · 19 comments
Closed

helm chart should support setting internalTrafficPolicy service value #10798

abasalaev opened this issue Dec 22, 2023 · 19 comments
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. triage/needs-information Indicates an issue needs more information in order to work on it.

Comments

@abasalaev
Copy link

This is a new value, see https://kubernetes.io/docs/concepts/services-networking/service-traffic-policy/

It should work in conjunction with https://github.com/kubernetes/ingress-nginx/blob/main/docs/user-guide/nginx-configuration/annotations.md#service-upstream

@abasalaev abasalaev added the kind/feature Categorizes issue or PR as related to a new feature. label Dec 22, 2023
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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 needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority labels Dec 22, 2023
@strongjz
Copy link
Member

strongjz commented Jan 4, 2024

/assign @Gacko

@Gacko
Copy link
Member

Gacko commented Jan 5, 2024

Hello @abasalaev!

Could you please elaborate on your use case? I can not see how internalTrafficPolicy is connected to the service-upstream annotation and feel like there's a misunderstanding of the both of them.

internalTrafficPolicy tells a service to be only reachable from pods on nodes actually having endpoints of this service running. So e.g. if you set a service's internalTrafficPolicy to Local, its endpoints are only reachable from pods of the same nodes. Keep in mind that it then is defined in the target service and not in the source.

The service-upstream annotation makes Ingress NGINX use a service's address instead of its endpoints, so it always calls the backend by its service address instead of single endpoints directly.

Ingress NGINX has no control over the configuration of services it's routing traffic to, only about the service routing traffic to itself. And this is all we can change: The way traffic is being routed to Ingress NGINX Controller pods.

Having internalTrafficPolicy configurable and set to Local apart from the default Cluster would only make it unreachable for pods not running on nodes having such a Ingress NGINX Controller pod.

Even though I'd still like to have more information on a use case then, I'd understand if you were asking for only implementing internalTrafficPolicy without connecting it to service-upstream. But having them in the same context makes me feel there's a misunderstanding.

Marco

@Gacko
Copy link
Member

Gacko commented Jan 5, 2024

/triage needs-information
/priority awaiting-more-evidence

@k8s-ci-robot k8s-ci-robot added triage/needs-information Indicates an issue needs more information in order to work on it. priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. and removed needs-priority labels Jan 5, 2024
@Gacko
Copy link
Member

Gacko commented Jan 15, 2024

/close

@k8s-ci-robot
Copy link
Contributor

@Gacko: Closing this issue.

In response to this:

/close

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.

@badgerspoke
Copy link

If I may necropost (sorry), there are valid reasons for setting local mode.

When using iptables mode for kube-proxy and Topology Aware Routing (TAR) one might want to ensure that traffic entering a node hosting ingress-nginx remains on that node - with the default of internalTrafficPolicy: Cluster on the ingress-nginx Service you'll get this in iptables for a node hosting ingress-nginx in zone A:

[root@ip-1-2-3-4 /]# iptables -vnt nat -L KUBE-SERVICES|grep nginx|grep http
    0     0 KUBE-SVC-FOO  tcp  --  *      *       0.0.0.0/0            192.168.11.22         /* ingress-nginx/ingress-nginx-controller:http cluster IP */ tcp dpt:80
    0     0 KUBE-SVC-BAR  tcp  --  *      *       0.0.0.0/0            192.168.11.22         /* ingress-nginx/ingress-nginx-controller:https cluster IP */ tcp dpt:443
[root@ip-1-2-3-4 /]# iptables -vnt nat -L KUBE-SVC-FOO
Chain KUBE-SVC-FOO (2 references)
 pkts bytes target     prot opt in     out     source               destination         
    0     0 KUBE-SEP-BAZ  all  --  *      *       0.0.0.0/0            0.0.0.0/0            /* ingress-nginx/ingress-nginx-controller:http -> 1.2.4.61:80 */ statistic mode random probability 0.50000000000
    0     0 KUBE-SEP-QAZ  all  --  *      *       0.0.0.0/0            0.0.0.0/0            /* ingress-nginx/ingress-nginx-controller:http -> 1.2.8.77:80 */
  • Note that actual chain names and IPs have been changed, but 1.2.4.0 is in zone A whereas 1.2.8.0 is in zone B.

So here there's a 50% chance of traffic entering this node actually going to another for the ingress (i.e. this is inbound to the controller and before it can decide how to route to a backend) - that's an additional/unnecessary hop potentially incurring latency and/or cost.

If we change the ingress-nginx Service property to internalTrafficPolicy: Local, still on that node in zone A we see:

[root@ip-1-2-3-4 /]# iptables -vnt nat -L KUBE-SERVICES|grep nginx|grep http
    0     0 KUBE-SVL-SOMETHINGHERE  tcp  --  *      *       0.0.0.0/0            192.168.11.22         /* ingress-nginx/ingress-nginx-controller:http cluster IP */ tcp dpt:80
    0     0 KUBE-SVL-SOMETHINGELSE  tcp  --  *      *       0.0.0.0/0            192.168.11.22         /* ingress-nginx/ingress-nginx-controller:https cluster IP */ tcp dpt:443
[root@ip-1-2-3-4 /]# iptables -vnt nat -L KUBE-SVL-SOMETHINGHERE
Chain KUBE-SVL-SOMETHINGHERE (2 references)
 pkts bytes target     prot opt in     out     source               destination         
   70  4200 KUBE-SEP-WHATEVER  all  --  *      *       0.0.0.0/0            0.0.0.0/0            /* ingress-nginx/ingress-nginx-controller:http -> 1.2.4.61:80 */

There are tradeoffs regarding concentration risk/availability but they can all be mitigated in various ways and are orthogonal to the issue at hand - the point is this is a reasonable option to control for folks who understand the consequences.

Can we please add it?

@Gacko
Copy link
Member

Gacko commented Aug 24, 2024

According to the docs internalTrafficPolicy is not affecting traffic ingressing from outside the cluster. This is what externalTrafficPolicy is for and we already support this.

internalTrafficPolicy of a Service only affects clients inside the cluster, so other pods. If you'd like to achieve Ingress NGINX only calling pods on the same node, this would need to be configured in the target service, not Ingress NGINX.

@badgerspoke
Copy link

Sorry to contradict you @Gacko but having tested this and checked the iptables rules, setting internalTrafficPolicy: Local on the nginx service changes how kube-proxy routes ingress traffic to that service - even from outside the cluster, at least in the case of EKS so perhaps it's different in other k8s implementations, I don't know.

We want it set so that traffic from the load balancer hits the node hosting nginx and then stays on that host in order to talk to nginx - with the setting of Cluster there is quite categorically a different iptables rule which allows that ingress traffic (and this is before nginx can decide what backend to talk to) to jump to a different node. We don't want 50% of that traffic jumping to a different node for various reasons (cost, latency etc).

Other folks might not understand this but try it for yourself and observe, I am not joking. Please can we now have the option in the chart so I don't need to maintain a fork forever. Thanks.

@Gacko
Copy link
Member

Gacko commented Sep 27, 2024

with the setting of Cluster there is quite categorically a different iptables rule which allows that ingress traffic (and this is before nginx can decide what backend to talk to) to jump to a different node. We don't want 50% of that traffic jumping to a different node for various reasons (cost, latency etc).

This is not how externalTrafficPolicy: Local should be implemented and therefore trying to "fix" it with internalTrafficPolicy is a hack to me. Please first double check the other components like kube-proxy, any alternatives and your providers network config then.

Other folks might not understand this but try it for yourself and observe, I am not joking. Please can we now have the option in the chart so I don't need to maintain a fork forever. Thanks.

Sorry, but based on your reasoning I still assume Ingress NGINX not to the root cause. So as long it hasn't been clarified that all your components are working Kubernetes API conform and the behavior you're observing is therefore Kubernetes API compliant, we don't want to implement a field that, if then implemented correctly in the user's environment/network, might also break Ingress NGINX if misconfigured. Additionally and apart from what you're observing I still don't see a valid use case for this.

@badgerspoke
Copy link

I'm not the author of kube-proxy etc I am an end user making observations about the system based on settings. This setting creates the environment I desire. What would you like me to check, exactly?

The iptables rules change to what is desired based on this single change. I can't really put it any other way.

Looks like you're not going to be helpful so we will fork and move on, thanks anyways.

@Gacko
Copy link
Member

Gacko commented Sep 27, 2024

From my understanding the behavior you're expecting (having traffic coming from the outside of your cluster, so load balancer traffic e.g., stay on the same node and getting directed to pods on this node instead of getting distributed to other pods on different nodes inside the same cluster) can be achieved by setting externalTrafficPolicy to Local.

At least and from my understanding this is what externalTrafficPolicy was made for.

internalTrafficPolicy on the other hand was made for keeping traffic egressing from one pod inside your cluster to a service configured with internalTrafficPolicy: Local on the same node. If there's no pod backing this particular service on the same node, your packets won't go anywhere and the connection won't be established.

You're telling, that traffic coming from the outside of your cluster is still getting distributed across your cluster. This is clearly in the responsibility of externalTrafficPolicy to me and if your cluster is behaving differently than how externalTrafficPolicy is meant to be implemented, I'd first check your environment.

Of course I can also setup a cluster and see if I can observe the same behavior you're describing. I didn't do this for this particular issue here, but in my day to day job I used to support Ingress NGINX in a lot of different environments (everything from tiny clusters to huge clusters, different cloud providers, different Kubernetes versions) and I never observed the issue you're describing and trying to change the Ingress NGINX chart for.

@Gacko
Copy link
Member

Gacko commented Sep 27, 2024

Also, are you taking into account that internalTrafficPolicy would make the whole Ingress NGINX unavailable via the cluster internal service for all pods that are not having an Ingress NGINX pod running on their node, at least as long as your Kubernetes flavor (EKS) is implementing internalTrafficPolicy the way it should work?

@Gacko
Copy link
Member

Gacko commented Sep 27, 2024

Here's the KEP for internalTrafficPolicy. The section about motivation is pretty clear about what it was made for and definitely does not cover your use case: https://github.com/kubernetes/enhancements/blob/master/keps/sig-network/2086-service-internal-traffic-policy/README.md#motivation

@Gacko
Copy link
Member

Gacko commented Sep 27, 2024

And here's what the Kubernetes docs are telling about externalTrafficPolicy. This is clearly covering your use case: https://kubernetes.io/docs/tasks/access-application-cluster/create-external-load-balancer/#preserving-the-client-source-ip

@abasalaev
Copy link
Author

abasalaev commented Sep 27, 2024

Hi @Gacko

This request doesn't relate to the externalTrafficPolicy at all.

My case was the following - there is a special type of nodes that always includes two pods: ingress-nginx and app-server. Ingress-nginx proxies requests to the app-server.

If I enable nginx.ingress.kubernetes.io/service-upstream annotation, then ingress-nginx uses kube-proxy to find a pod with app-server and proxies request there. By default, it uses round-robin and selects the pod across all nodes. BUT I want it to use the pod from the same node and internalTrafficPolicy: Local allows me to do this.

Hope it helps

@longwuyuan
Copy link
Contributor

Hi @abasalaev , when you can alter the manifests and charts in your own fork, would you feel that its is a feasible practical solution when compared to altering the project itself. We don't have any resources and hence we don't have any interest to support/maintain features/capabilities that meet this kind of criteria. We are actually deprecating multiple popular and much used features like tcp/udp forwarding.

@abasalaev
Copy link
Author

Hi @longwuyuan

I just shared this case and am not insisting on including this change. I appreciate your efforts.

@Gacko
Copy link
Member

Gacko commented Sep 27, 2024

Hi @Gacko

This request doesn't relate to the externalTrafficPolicy at all.

My case was the following - there is a special type of nodes that always includes two pods: ingress-nginx and app-server. Ingress-nginx proxies requests to the app-server.

If I enable nginx.ingress.kubernetes.io/service-upstream annotation, then ingress-nginx uses kube-proxy to find a pod with app-server and proxies request there. By default, it uses round-robin and selects the pod across all nodes. BUT I want it to use the pod from the same node and internalTrafficPolicy: Local allows me to do this.

Hope it helps

For this use case you need to set internalTrafficPolicy: Local on the app service - not on Ingress NGINX.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. triage/needs-information Indicates an issue needs more information in order to work on it.
Projects
Archived in project
Development

No branches or pull requests

6 participants