ingress: Support NodePort for dedicated Ingress

[sayboras]

Description

This commit is to support NodePort service in dedicated mode. Shared service NodePort can be configured via helm as per #22583.

Relates: #22583
Signed-off-by: Tam Mach tam.mach@cilium.io

Testing

Testing was done locally as per below

# Basic ingress for istio bookinfo demo application, which can be found in below
# https://raw.githubusercontent.com/istio/istio/release-1.11/samples/bookinfo/platform/kube/bookinfo.yaml
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: basic-ingress
  namespace: default
  annotations:
    io.cilium.ingress/service-type: "NodePort"
    io.cilium.ingress/insecure-node-port: "30000"
    io.cilium.ingress/secure-node-port: "30001"
    io.cilium.ingress/loadbalancer-mode: "dedicated"
spec:
  ingressClassName: cilium
  rules:
  - http:
      paths:
      - backend:
          service:
            name: details
            port:
              number: 9080
        path: /details
        pathType: Prefix
      - backend:
          service:
            name: productpage
            port:
              number: 9080
        path: /
        pathType: Prefix

Sending traffic to NodePort service

$ kgsvc       
NAME                           TYPE        CLUSTER-IP       EXTERNAL-IP   PORT(S)                      AGE
cilium-ingress-basic-ingress   NodePort    10.106.157.226   <none>        80:30000/TCP,443:30001/TCP   3s
details                        ClusterIP   10.96.85.227     <none>        9080/TCP                     26m
kubernetes                     ClusterIP   10.96.0.1        <none>        443/TCP                      35m
productpage                    ClusterIP   10.104.152.26    <none>        9080/TCP                     26m
ratings                        ClusterIP   10.105.230.48    <none>        9080/TCP                     26m
reviews                        ClusterIP   10.102.73.248    <none>        9080/TCP                     26m

$ curl http://192.168.49.2:30000/details/1
{"id":1,"author":"William Shakespeare","year":1595,"type":"paperback","pages":200,"publisher":"PublisherA","language":"English","ISBN-10":"1234567890","ISBN-13":"123-1234567890"}

[sayboras]

/test

Job 'Cilium-PR-K8s-1.25-kernel-4.19' failed:

Click to show.

Test Name

K8sAgentChaosTest Connectivity demo application Endpoint can still connect while Cilium is not running

Failure Output

FAIL: Endpoints are not ready after timeout

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.25-kernel-4.19 so I can create one.

Job 'Cilium-PR-K8s-1.16-kernel-4.9' failed:

Click to show.

Test Name

K8sKafkaPolicyTest Kafka Policy Tests KafkaPolicies

Failure Output

FAIL: Found 1 io.cilium/app=operator logs matching list of errors that must be investigated:

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.16-kernel-4.9 so I can create one.

[qmonnet]

Looks good as far as I can tell

[joestringer]

@sayboras Why was release-blocker/1.13 added to this feature PR? We closed the window for new features about a month ago.

[sayboras]

Why was release-blocker/1.13 added to this feature PR? We closed the window for new features about a month ago.

We partially added this feature in #22583, hence I think we can just wrap it off for NodePort support.

[joestringer]

Looking back over the PR, it looks like just adding parsing for a couple of extra service annotations and adding some minor translation code for that, and hence it fleshes out the remaining pieces of a brand new feature (ie no risk of breaking existing features). I'm not a big fan of sneaking PRs in this late during the cycle, but the PR does look innocent enough 😇

How do we test this and know it works? Is it just ensuring that when certain objects are ingested into operator/pkg/model/ingress, they are properly translated? Do we need some sort of dataplane / integration testing?

[sahid]

Thank you Tam, looks good, just one suggestion, but I may not not noticed a check that is already present.

[sayboras]

/test

Job 'Cilium-PR-K8s-1.24-kernel-5.4' failed:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: cannot import l7 policy: /home/jenkins/workspace/Cilium-PR-K8s-1.24-kernel-5.4/src/github.com/cilium/cilium/test/k8s/manifests/l7-policy.yaml

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.24-kernel-5.4 so I can create one.

[youngnick]

The change itself LGTM, but I agree with Joe that it would be worth adding some extra tests:

• an e2e test that tests what happens when there's a mistake with the config (from the other comment). We should lock in the "you get an LB service instead" behavior with a test of some sort, so that if someone "fixes" that, we'll know.
• Some unit testing of the GetAnnotation functions would also be nice.

[sayboras]

/mlh new-flake Cilium-PR-K8s-1.24-kernel-5.4

👍 created #23150

[joestringer]

/test-1.24-5.4

[sayboras]

I haved address the lack of testing for this change, mainly unit tests on individual phases (e.g. annotations, ingestion, and translation).

The lack of control plane tests will be tracked in #23234

[sayboras]

/test

[sayboras]

/test

[sayboras]

/test