Ingress: Add support for is-default-class on IngressClass #23719

meyskens · 2023-02-13T15:40:10Z

This adds support for the ingressclass.kubernetes.io/is-default-class flag on the IngressClass. If set Cilium ingress wil pick up all ingress objects where no class is set also.

This also adds exposure to enable it in Helm under ingressController.default

Fixes: #23181

Signed-off-by: Maartje Eyskens maartje.eyskens@isovalent.com

Please ensure your pull request adheres to the following guidelines:

For first time contributors, read Submitting a pull request
All code is covered by unit and/or runtime tests where feasible.
All commits contain a well written commit description including a title,
description and a Fixes: #XXX line if the commit addresses a particular
GitHub issue.
If your commit description contains a Fixes: <commit-id> tag, then
please add the commit author[s] as reviewer[s] to this issue.
All commits are signed off. See the section Developer’s Certificate of Origin
Provide a title or release-note blurb suitable for the release notes.
Thanks for contributing!

Fixes: #issue-number

Add support for the `ingressclass.kubernetes.io/is-default-class` annotation on Cilium's IngressClass

nbusseneau

CI workflow LGTM, uses pull_request trigger so no specific risk in case of attack via malicious PR (forks do not have access to upstream secrets).

Helm changes are trivial, LGTM.

nbusseneau · 2023-02-14T13:44:54Z

.github/workflows/conformance-ingress-default.yaml

+      - name: Create minikube cluster with multiple nodes
+        uses: medyagh/setup-minikube@ab221dee176f8eabd8deddf849b5bf1d6244a6e8 # v0.0.11
+        with:
+          minikube-version: ${{ env.minikube_version }}
+          network-plugin: cni
+          cni: false
+          wait: false
+          start-args: --nodes 3 --docker-opt "default-ulimit=nofile=102400:102400"


Is there any specific reason to use Minikube here instead of Kind? Not a problem at all but I'm wondering if the pipeline would not run faster on a Kind-based cluster.

Happy to move it to kind (I was wondering the same thing as you...), but it builds upon the previous conformance tests which all use minikube, I only modified this one to use the default behavior

Your call, if not worth the trouble it's OK as is, and consistency with the other conformance workflows is also good. Probably @sayboras just needed Minikube for the other workflows.

I'll keep it minikube for now and will probably file an issue to consider kind (unless @sayboras has a reason)

Last time I can't get kind + metallb (required for LB service) to work from host (i.e. github runner), so I just use minikube with handy minikube tunnel command.

Recently, I noticed that we have labs prepared with Kind + MetalLB, so it's worth to give it a crack again.

as discussed offline, we can tackle this separately, we might even use LB IPAM feature if possible.

ldelossa · 2023-02-14T14:15:17Z

@meyskens can you please fill me in on motivation for this? I understand the change, but just looking for a bit more context.

meyskens · 2023-02-14T15:31:55Z

This PR came out of #23181 (which was passed onto be by my team as good first issue).
As far as I understand the motivation (@sayboras feel free to add onto this if I missed something) is to add the ability for the user to say they want Cilium as their default ingress controller which is useful when not setting the ingressClassName on all ingress objects. In which case the default one will always pick it up. This was standardized with an annotation in the Kubernetes spec (info at https://kubernetes.io/docs/concepts/services-networking/ingress/#default-ingress-class) however we did not implement this yet thus lacking an upstream compatibility with the ingress API.

qmonnet

I found a couple of typos, but looks good otherwise from my side. Thanks!

Documentation/network/servicemesh/installation.rst

operator/pkg/ingress/ingress_class.go

operator/pkg/ingress/ingress.go

qmonnet

Thanks!

operator/pkg/ingress/ingress.go

squeed

One minor thing, then LGTM!

operator/pkg/ingress/ingress.go

This adds support for the ingressclass.kubernetes.io/is-default-class flag on the IngressClass. If set Cilium ingress wil pick up all ingress objects where no class is set also. This also adds exposure to enable it in Helm under ingressController.default Fixes: cilium#23181 Signed-off-by: Maartje Eyskens <maartje.eyskens@isovalent.com>

squeed

Nice!

sayboras

Thanks a lot for taking up this task, LGTM ✔️

sayboras · 2023-02-16T14:58:38Z

/test

sayboras · 2023-02-17T05:56:42Z

Suite-k8s-1.16.K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master failure due to the below error. I don't think it's related to the changes here (only on operator and Ingress).

Re-run to confirm the flake.

DNS entry is not ready after timeout
Expected
    <*errors.errorString | 0xc0006f6fb0>: {
        s: "DNS 'app1-service.default.svc.cluster.local' is not ready after timeout: 7m0s timeout expired",
    }
to be nil

sayboras · 2023-02-17T05:56:50Z

/test-1.16-4.19

sayboras · 2023-02-17T09:04:10Z

Reviews are in, all tests passed (ci-verifier test is not required for this change). Mark this ready to merge.

pchaigno · 2023-02-17T11:29:56Z

Suite-k8s-1.16.K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master failure due to the below error. I don't think it's related to the changes here (only on operator and Ingress).

@sayboras Do we already have a flake issue for that?

sayboras · 2023-02-17T11:50:05Z

Suite-k8s-1.16.K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master failure due to the below error. I don't think it's related to the changes here (only on operator and Ingress).

Do we already have a flake issue for that?

Good point, confirmed that its a flake as re-run was good. Here is it #23845

pchaigno · 2023-02-17T15:28:49Z

Thanks Tam!

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 13, 2023

meyskens force-pushed the meyskens/ingress-class branch 6 times, most recently from f460e7e to 0663468 Compare February 14, 2023 10:26

meyskens marked this pull request as ready for review February 14, 2023 13:19

meyskens requested review from a team as code owners February 14, 2023 13:19

meyskens requested review from ldelossa, squeed, sayboras, qmonnet and nbusseneau February 14, 2023 13:19

meyskens mentioned this pull request Feb 14, 2023

Docs: Add part on updating the Helm chart #23739

Merged

nbusseneau approved these changes Feb 14, 2023

View reviewed changes

meyskens added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Feb 14, 2023

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 14, 2023

qmonnet requested changes Feb 14, 2023

View reviewed changes

Documentation/network/servicemesh/installation.rst Outdated Show resolved Hide resolved

Documentation/network/servicemesh/installation.rst Outdated Show resolved Hide resolved

operator/pkg/ingress/ingress_class.go Outdated Show resolved Hide resolved

ldelossa approved these changes Feb 14, 2023

View reviewed changes

squeed requested changes Feb 14, 2023

View reviewed changes

operator/pkg/ingress/ingress.go Show resolved Hide resolved

meyskens force-pushed the meyskens/ingress-class branch from 0663468 to 32d730c Compare February 15, 2023 14:42

meyskens requested review from squeed and qmonnet February 15, 2023 14:44

meyskens force-pushed the meyskens/ingress-class branch from 32d730c to 6a76d5d Compare February 15, 2023 15:02

qmonnet approved these changes Feb 15, 2023

View reviewed changes

operator/pkg/ingress/ingress.go Outdated Show resolved Hide resolved

meyskens force-pushed the meyskens/ingress-class branch 2 times, most recently from 56ce9a1 to 651c6c1 Compare February 16, 2023 08:15

squeed requested changes Feb 16, 2023

View reviewed changes

operator/pkg/ingress/ingress.go Outdated Show resolved Hide resolved

meyskens force-pushed the meyskens/ingress-class branch from 651c6c1 to f37de60 Compare February 16, 2023 08:50

meyskens requested a review from squeed February 16, 2023 08:50

meyskens force-pushed the meyskens/ingress-class branch from f37de60 to 9302b3f Compare February 16, 2023 09:21

squeed approved these changes Feb 16, 2023

View reviewed changes

sayboras approved these changes Feb 16, 2023

View reviewed changes

sayboras added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 17, 2023

pchaigno removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 17, 2023

sayboras added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. area/servicemesh GH issues or PRs regarding servicemesh labels Feb 17, 2023

pchaigno merged commit 85671ad into cilium:master Feb 17, 2023

mhofstetter mentioned this pull request Feb 20, 2023

bpf & envoy: Add support for authentication on ingress policies #23839

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ingress: Add support for is-default-class on IngressClass #23719

Ingress: Add support for is-default-class on IngressClass #23719

meyskens commented Feb 13, 2023 •

edited

nbusseneau left a comment •

edited

nbusseneau Feb 14, 2023

meyskens Feb 14, 2023

nbusseneau Feb 14, 2023

meyskens Feb 14, 2023

sayboras Feb 14, 2023

sayboras Feb 16, 2023

ldelossa commented Feb 14, 2023

meyskens commented Feb 14, 2023

qmonnet left a comment •

edited

qmonnet left a comment

squeed left a comment

squeed left a comment

sayboras left a comment

sayboras commented Feb 16, 2023

sayboras commented Feb 17, 2023

sayboras commented Feb 17, 2023

sayboras commented Feb 17, 2023

pchaigno commented Feb 17, 2023 •

edited

sayboras commented Feb 17, 2023

pchaigno commented Feb 17, 2023

Ingress: Add support for is-default-class on IngressClass #23719

Ingress: Add support for is-default-class on IngressClass #23719

Conversation

meyskens commented Feb 13, 2023 • edited

nbusseneau left a comment • edited

Choose a reason for hiding this comment

nbusseneau Feb 14, 2023

Choose a reason for hiding this comment

meyskens Feb 14, 2023

Choose a reason for hiding this comment

nbusseneau Feb 14, 2023

Choose a reason for hiding this comment

meyskens Feb 14, 2023

Choose a reason for hiding this comment

sayboras Feb 14, 2023

Choose a reason for hiding this comment

sayboras Feb 16, 2023

Choose a reason for hiding this comment

ldelossa commented Feb 14, 2023

meyskens commented Feb 14, 2023

qmonnet left a comment • edited

Choose a reason for hiding this comment

qmonnet left a comment

Choose a reason for hiding this comment

squeed left a comment

Choose a reason for hiding this comment

squeed left a comment

Choose a reason for hiding this comment

sayboras left a comment

Choose a reason for hiding this comment

sayboras commented Feb 16, 2023

sayboras commented Feb 17, 2023

sayboras commented Feb 17, 2023

sayboras commented Feb 17, 2023

pchaigno commented Feb 17, 2023 • edited

sayboras commented Feb 17, 2023

pchaigno commented Feb 17, 2023

meyskens commented Feb 13, 2023 •

edited

nbusseneau left a comment •

edited

qmonnet left a comment •

edited

pchaigno commented Feb 17, 2023 •

edited