externalTrafficPolicy configuration support for Ingress/GwAPI K8s Service
#32873
Conversation
externalTrafficPolicy configuration support for Ingress/GwAPI K8s Service
eb7b59e to
0879c41
Compare
|
/test |
There was a problem hiding this comment.
Thanks for this PR @PhilipSchmid 🎉
I added some comments inline.
It looks like when changing the GwAPI eTP and doing a helm upgrade, the eTP on the actual Gateway LB Services is not updated on demand (at least for LB services not). A Cilium Operator restart fixes this and patches the new eTP in the Service.
Does the Operator gets automatically restarted during the Helm Upgrade (after changing the Helm values)? If not, this would be the issue. (That's why it's working after a manual Operator restart)
Changing ingressController.service.externalTrafficPolicy=Cluster to ingressController.service.externalTrafficPolicy=Local and re-running make kind-install-cilium causes the Helm deployment to fail. It looks like it's just a make kind-install-cilium "issue" as it runs cilium install in the background, and if I manually run cilium upgrade with the same parameters, it works. Is this a known behavior? Just double-checking it's not something related to this change.
Probably not related to this PR 🤞
-> I tried to address the other questions in my comments inline
0879c41 to
ecef14f
Compare
|
Rebased to |
ecef14f to
7211c32
Compare
|
Thanks for the feedback, @mhofstetter. I just implemented it and force-pushed the fixed commits. |
7211c32 to
55b69fc
Compare
|
@mhofstetter Regarding this 👇
No, the Ciliium Operator isn't automatically restarted, as it's only a cilium/install/kubernetes/cilium/templates/cilium-configmap.yaml Lines 276 to 286 in 4391c8f For example, if I change |
|
I did some manual E2E testing, and it looks good, but I couldn't test it 100%. The main reason for that is that a GwAPI bug was introduced with the commits of #32744 because with the commit right before it still worked. GwAPI engineers are aware of this and provide a fix anytime soon. However, this should not impact or delay this PR. Detailed Instructions for GwAPI on AKS BYOCNI E2E TestingGwAPI on AKS (BYOCNI) E2E Testing## Cluster Setup az extension add --name aks-preview
az extension update --name aks-preview
az feature register --namespace "Microsoft.ContainerService" --name "KubeProxyConfigurationPreview"
az provider register --namespace Microsoft.ContainerServiceset -gx CLUSTERNAME "etp-test-philip"
set -gx RESOURCEGROUP "etp-test-philip"
set -gx LOCATION "eastus2"
az group create --name $RESOURCEGROUP --location $LOCATION
echo >kube-proxy.json '{
"enabled": false,
"mode": "IPVS",
"ipvsConfig": {
"scheduler": "LeastConnection",
"TCPTimeoutSeconds": 900,
"TCPFINTimeoutSeconds": 120,
"UDPTimeoutSeconds": 300
}
}'
az aks create --name $CLUSTERNAME --resource-group $RESOURCEGROUP \
--network-plugin none \
--kube-proxy-config kube-proxy.json \
--kubernetes-version 1.29
az aks get-credentials -g $RESOURCEGROUP --name $CLUSTERNAME
kubectl get nodes -o wideCilium InstallationInstall GwAPI CRDs: kubectl apply -f https://raw.githubusercontent.com/kubernetes-sigs/gateway-api/v1.0.0/config/crd/standard/gateway.networking.k8s.io_gatewayclasses.yaml
kubectl apply -f https://raw.githubusercontent.com/kubernetes-sigs/gateway-api/v1.0.0/config/crd/standard/gateway.networking.k8s.io_gateways.yaml
kubectl apply -f https://raw.githubusercontent.com/kubernetes-sigs/gateway-api/v1.0.0/config/crd/standard/gateway.networking.k8s.io_httproutes.yaml
kubectl apply -f https://raw.githubusercontent.com/kubernetes-sigs/gateway-api/v1.0.0/config/crd/standard/gateway.networking.k8s.io_referencegrants.yaml
kubectl apply -f https://raw.githubusercontent.com/kubernetes-sigs/gateway-api/v1.0.0/config/crd/experimental/gateway.networking.k8s.io_grpcroutes.yaml
kubectl apply -f https://raw.githubusercontent.com/kubernetes-sigs/gateway-api/v1.0.0/config/crd/experimental/gateway.networking.k8s.io_tlsroutes.yamlInstall Cilium: echo >etp-test-values.yaml "\
image:
override: "quay.io/cilium/cilium-ci:0879c41b0c952972f781af4c44c14b78b8e07f61@sha256:57d4325ab70d2e6b4de6bc17b17d3af27693b26adf5dd8933e00b403cfdae382"
operator:
image:
override: "quay.io/cilium/operator-generic-ci:0879c41b0c952972f781af4c44c14b78b8e07f61@sha256:e0485aecc65d3f0eef8fa67030104a5f5a0cb2d1f2c95f723cd14423433390bf"
kubeProxyReplacement: \"true\"
k8sServiceHost: <TBD>
k8sServicePort: 443
ingressController:
enabled: true
loadbalancerMode: shared
service:
externalTrafficPolicy: Local
gatewayAPI:
enabled: true
externalTrafficPolicy: Local"
cilium install --datapath-mode aks-byocni \
--chart-directory=$HOME/git/github.com/philipschmid/cilium/install/kubernetes/cilium \
--helm-values=etp-test-values.yaml
# Wait a few seconds
cilium statusWorkloadTest infra workload: ---
apiVersion: v1
kind: Namespace
metadata:
name: infra
---
apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
name: shared-gateway
namespace: infra
spec:
gatewayClassName: cilium
listeners:
- name: http
protocol: HTTP
port: 80
allowedRoutes:
namespaces:
from: AllGet the LB IP/name (wait a few seconds): set -gx LB_IP (kubectl get services \
--namespace infra \
cilium-gateway-shared-gateway \
--output jsonpath='{.status.loadBalancer.ingress[0].ip}')
echo $LB_IP
4.153.48.103 # <- IP in my test caseTest workload (replace ---
apiVersion: v1
kind: Namespace
metadata:
name: workload
---
apiVersion: apps/v1
kind: Deployment
metadata:
name: echo-app-deployment
namespace: workload
labels:
app: echo-app
spec:
replicas: 3 # 3 replicas because my testing AKS cluster has 3 nodes
selector:
matchLabels:
app: echo-app
template:
metadata:
labels:
app: echo-app
spec:
affinity:
podAntiAffinity:
requiredDuringSchedulingIgnoredDuringExecution:
- labelSelector:
matchExpressions:
- key: "app"
operator: In
values:
- echo-app
topologyKey: "kubernetes.io/hostname"
containers:
- name: echo-app
image: ghcr.io/philipschmid/echo-app:main
ports:
- containerPort: 8080
env:
- name: PRINT_HTTP_REQUEST_HEADERS
value: "true"
---
apiVersion: v1
kind: Service
metadata:
name: echo-app-service
namespace: workload
labels:
app: echo-app
spec:
selector:
app: echo-app
ports:
- protocol: TCP
port: 8080
targetPort: 8080
type: ClusterIP
---
apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
name: workload
namespace: workload
spec:
parentRefs:
- name: shared-gateway
namespace: infra
hostnames:
- gwapi.$LB_IP.sslip.io # gwapi.4.153.48.103.sslip.io
rules:
- backendRefs:
- name: echo-app-service
port: 8080
---
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
name: workload
namespace: workload
spec:
ingressClassName: cilium
rules:
- host: ingress.$LB_IP.sslip.io # ingress.4.153.48.103.sslip.io
http:
paths:
- pathType: Prefix
path: "/"
backend:
service:
name: echo-app-service
port:
number: 8080TestingTest GwAPI: curl http://gwapi.4.153.48.103.sslip.ioTest Ingress: curl http://ingress.4.153.48.103.sslip.ioClean Upaz aks delete --name $CLUSTERNAME --resource-group $RESOURCEGROUP
az group delete --name $RESOURCEGROUP |
There was a problem hiding this comment.
Doc changes look good, thanks!
A few (non-blocking) observations on the commit formatting:
- 3rd commit description has a typo: globaly -> globally
- 3rd commit title (
gwapi: eTP support for GwAPI) is also not super easy to read because of the many abbreviations, I'd consider expanding at least one of the two and using consistent case on gwapi/GwAPI if you keep it abbreviated - We usually wrap commit descriptions on 72-character wide lines (see most commit descriptions in
git log) - On your way to become the first
@cisco.comcommit author! 😎
Added configuration option to explicitly configure the externalTrafficPolicy for the Cilium Ingress Kubernetes Service. Signed-off-by: Philip Schmid <phisch@cisco.com>
Added externalTrafficPolicy support for dedicated Cilium Ingress instances. Configurable via new `ingress.cilium.io/service-external-traffic-policy` Ingress annotation. Signed-off-by: Philip Schmid <phisch@cisco.com>
Added externalTrafficPolicy (eTP) support for Cilium GatewayAPI. eTP is globally configurable via `gatewayAPI.externalTrafficPolicy` Helm flag. Signed-off-by: Philip Schmid <phisch@cisco.com>
55b69fc to
a7c3bcf
Compare
|
Thanks a lot for the input, @qmonnet! I just reworded the commit messages and force-pushed once again. |
|
Great, thanks! /test |
This PR adds support for configuring the
externalTrafficPolicyKubernetes Service spec field of the Cilium Ingress/GatewayAPILoadBalancer/NodePortService.ingress.cilium.io/service-external-traffic-policyIngress annotation.gatewayAPI.externalTrafficPolicyHelm flag.If possible, let's please try to merge this before the 1.16 feature freeze 🤞.
Open questions based on the outcome of manual E2E testing:
ingressController.service.externalTrafficPolicy=ClustertoingressController.service.externalTrafficPolicy=Localand re-runningmake kind-install-ciliumcauses the Helm deployment to fail. It looks like it's just amake kind-install-cilium"issue" as it runscilium installin the background, and if I manually runcilium upgradewith the same parameters, it works. Is this a known behavior? Just double-checking it's not something related to this change.ingress.cilium.io/service-external-traffic-policyvalue on aningress.cilium.io/loadbalancer-mode=dedicatedIngress resource "spams" the Cilium Operator logs with multiple (~10)Reconciler errormessages. Even after the initial apply, we'll see such an error every other few seconds/minutes. Is there any need to mitigate this? I don't think so, but just checking.gatewayAPI.externalTrafficPolicyvalue causes the Cilium Operator to log an error and fail. Does this behavior make sense? I think so, but I'd like to hear other folks' opinions on that.helm upgrade, the eTP on the actual Gateway LB Services is not updated on demand (at least for<pending>LB services not). A Cilium Operator restart fixes this and patches the new eTP in the Service.