Support Gateway Selection using Ingress Labels #1250

pastequo · 2024-01-26T14:29:28Z

Changes

This PR was built on top of #1249

This is why there are a lot of changes (it includes 2 PRs in one)

Introduce LabelSelector on istio config
Filter gateway to use based on ingress labels & configuration label selector

/kind enhancement

Release Note

Services can be exposed by a specific istio gateway. Administrators, by using the new istio config format, are able to associate a label selector to their istio gateway. More information [here](https://docs.google.com/document/d/12X1-9nhjAhpf-Wlt3RbdoMbvx31zFBva/edit#heading=h.gjdgxs)

Docs

knative-prow · 2024-01-26T14:29:31Z

@pastequo: The label(s) kind/<kind> cannot be applied, because the repository doesn't have them.

In response to this:

Changes

This PR was built on top of #1249

Introduce LabelSelector on istio config

Filter gateway to use based on ingress labels & configuration label selector

/kind

Fixes #1248

Release Note
Docs

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.

knative-prow · 2024-01-26T14:29:38Z

Hi @pastequo. Thanks for your PR.

I'm waiting for a knative-extensions member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

codecov · 2024-01-26T16:06:22Z

Codecov Report

Attention: Patch coverage is 83.64780% with 26 lines in your changes are missing coverage. Please review.

Project coverage is 80.28%. Comparing base (e73d356) to head (1f02be8).
Report is 1 commits behind head on main.

Files	Patch %	Lines
pkg/reconciler/ingress/resources/gateway.go	76.74%	13 Missing and 7 partials ⚠️
pkg/reconciler/ingress/ingress.go	90.00%	2 Missing and 1 partial ⚠️
pkg/reconciler/ingress/lister.go	40.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1250      +/-   ##
==========================================
- Coverage   80.51%   80.28%   -0.24%     
==========================================
  Files          26       26              
  Lines        2022     2115      +93     
==========================================
+ Hits         1628     1698      +70     
- Misses        301      316      +15     
- Partials       93      101       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pastequo · 2024-01-30T17:37:22Z

Contrary to what was documented, I felt like it was wrong to implement here the check about the number of gateway (for example a maximum of one external gateway should be selected thru labels)

Even if it's not officially supported, a great part of the code acts as multiple gateway is possible. In particular, a lot of tests are using a configuration with multiple gateways.

If the change is confirmed, I think it should be done on a separated PR

dprotaso · 2024-02-06T19:35:33Z

You'll need to rebase since the prior PR merged.

I felt like it was wrong to implement here the check about the number of gateway (for example a maximum of one external gateway should be selected thru labels)

I'll bring it up at the WG group meeting tomorrow to see what others think.

My thoughts are given that we don't probe all gateways you could say allowing that behaviour is broken. If we have a plan to address the broken behaviour then we can drop the check.

dprotaso · 2024-02-06T21:55:31Z

/ok-to-test

dprotaso · 2024-02-07T15:28:56Z

Following up from the WG discussion - we'll want to add a check to ensure only a single gateway is selected (cause that's all we support). It's fine that this is done after this PR lands in a follow up change

pastequo · 2024-02-08T14:04:57Z

/retest

pkg/reconciler/ingress/resources/gateway.go

skonto · 2024-03-12T11:10:36Z

pkg/reconciler/ingress/resources/gateway.go

-			Name:      parts[0],
-			Namespace: parts[1],
+// GetIngressGatewaySvcNameNamespaces gets the Istio ingress namespaces from ConfigMap for gateways that should expose the service.
+func GetIngressGatewaySvcNameNamespaces(ctx context.Context, obj kmeta.Accessor) ([]metav1.ObjectMeta, error) {


It seems to me that wherever we have a kmeta.Accessor we could replace it with ing *v1alpha1.Ingress, correct?

True. I used to have ing *v1alpha1.Ingress & @dprotaso made me change for this. Which is imho better to depends on a small interface rather than an implementation

See #1250 (comment)

It depends generalizing for one case it is still weird but maybe a good fir for the GetLabels method.

skonto · 2024-03-12T15:28:09Z

test/e2e/exposition_test.go

+	}
+}
+
+func replaceTabs(s string) string {


We could have as an alternative, something like:

func toYamlNewFormatGateways(data []Gateway) string { bytes, _ := yaml.Marshal(data) return string(bytes) } "external-gateways": toYamlNewFormatGateways([]Gateway{ { Namespace: "unused", Name: "unused", ServiceURL: "default.default.svc.cluster.local", }, { Namespace: "namespace", Name: "gateway", ServiceURL: "istio-gateway.istio-system.svc.cluster.local", LabelSelector: &metav1.LabelSelector{ MatchExpressions: []metav1.LabelSelectorRequirement{ { Key: "key", Operator: metav1.LabelSelectorOpIn, Values: []string{"value"}, }, }, }, }}, ),

I think I rather have a string that reflects exactly what a user would have written, rather than generate it

For the scenarios you want to check just the user syntax yes, for the rest where you want to run test cases based on valid content it is safer. Anyway no strong preference.

pastequo · 2024-03-13T10:11:57Z

I think we should document this feature. Is this something we plan to do?

Do you mean aside from the google doc ? https://docs.google.com/document/d/12X1-9nhjAhpf-Wlt3RbdoMbvx31zFBva/edit#heading=h.gjdgxs

Something here https://github.com/knative/docs ? Or somewhere else ?

skonto · 2024-03-13T10:12:44Z

Something here https://github.com/knative/docs ?

yes

pastequo · 2024-03-13T14:58:23Z

Issue created: knative/docs#5905

dprotaso · 2024-03-21T13:50:08Z

/approve

Leaving lgtm for stavros

knative-prow · 2024-03-21T13:50:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, pastequo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [dprotaso]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

skonto · 2024-03-27T11:32:01Z

@pastequo could you rebase pls so we can stamp it?

skonto · 2024-03-27T13:01:49Z

/lgtm
/hold for tests

skonto · 2024-03-27T13:44:55Z

/unhold

dprotaso · 2024-03-27T14:46:58Z

🎉 thanks @pastequo for your help on landing this

knative-prow bot requested review from skonto and upodroid January 26, 2024 14:29

knative-prow bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 26, 2024

pastequo force-pushed the feat/gateway-filtering branch from 139dd4f to 1363f09 Compare January 29, 2024 16:25

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 29, 2024

pastequo force-pushed the feat/gateway-filtering branch from 1363f09 to 23a0d7e Compare January 29, 2024 16:53

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 29, 2024

pastequo force-pushed the feat/gateway-filtering branch from 23a0d7e to 1682c45 Compare January 29, 2024 17:22

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 30, 2024

pastequo force-pushed the feat/gateway-filtering branch from 8548431 to 060b8f9 Compare January 30, 2024 17:22

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 30, 2024

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2024

knative-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 6, 2024

pastequo force-pushed the feat/gateway-filtering branch 2 times, most recently from 0682f4b to 4985d97 Compare February 8, 2024 09:45

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 8, 2024

pastequo force-pushed the feat/gateway-filtering branch 2 times, most recently from c237ceb to af986a0 Compare February 8, 2024 10:12

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 12, 2024

pastequo force-pushed the feat/gateway-filtering branch from af986a0 to 27c845a Compare February 16, 2024 11:53

skonto reviewed Mar 12, 2024

View reviewed changes

pkg/reconciler/ingress/resources/gateway.go Show resolved Hide resolved

skonto reviewed Mar 12, 2024

View reviewed changes

pastequo force-pushed the feat/gateway-filtering branch from f06b35a to 72c817c Compare March 12, 2024 16:15

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 12, 2024

pastequo mentioned this pull request Mar 13, 2024

Net-istio: Capability to filter on istio gateway knative/docs#5905

Closed

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 14, 2024

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 21, 2024

pastequo and others added 5 commits March 27, 2024 13:59

feat: Gateway filtering based on labels (knative-extensions#1248)

65ac941

chore: Fix various MR comments

1921320

feat: Ensure exactly one default gateway is set

ec0d6d8

chore: MR comments resolution

a857cb6

chore: Update comments

1f02be8

pastequo force-pushed the feat/gateway-filtering branch from 0d92608 to 1f02be8 Compare March 27, 2024 12:59

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 27, 2024

knative-prow bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Mar 27, 2024

knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 27, 2024

knative-prow bot merged commit 52c9a7a into knative-extensions:main Mar 27, 2024
40 checks passed

dprotaso changed the title ~~Feat/gateway filtering~~ Support Gateway Selection using Ingress Labels Mar 27, 2024

SaschaSchwarze0 mentioned this pull request Jul 19, 2024

Remove useless if #1346

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Gateway Selection using Ingress Labels #1250

Support Gateway Selection using Ingress Labels #1250

pastequo commented Jan 26, 2024 •

edited by ReToCode

Loading

knative-prow bot commented Jan 26, 2024

Changes

knative-prow bot commented Jan 26, 2024

codecov bot commented Jan 26, 2024 •

edited

Loading

pastequo commented Jan 30, 2024

dprotaso commented Feb 6, 2024

dprotaso commented Feb 6, 2024

dprotaso commented Feb 7, 2024 •

edited

Loading

pastequo commented Feb 8, 2024

skonto Mar 12, 2024

pastequo Mar 12, 2024

skonto Mar 13, 2024

skonto Mar 12, 2024 •

edited

Loading

pastequo Mar 12, 2024

skonto Mar 13, 2024

pastequo commented Mar 13, 2024

skonto commented Mar 13, 2024 •

edited

Loading

pastequo commented Mar 13, 2024

dprotaso commented Mar 21, 2024

knative-prow bot commented Mar 21, 2024

skonto commented Mar 27, 2024 •

edited

Loading

skonto commented Mar 27, 2024

skonto commented Mar 27, 2024

dprotaso commented Mar 27, 2024

Support Gateway Selection using Ingress Labels #1250

Support Gateway Selection using Ingress Labels #1250

Conversation

pastequo commented Jan 26, 2024 • edited by ReToCode Loading

Changes

knative-prow bot commented Jan 26, 2024

Changes

knative-prow bot commented Jan 26, 2024

codecov bot commented Jan 26, 2024 • edited Loading

Codecov Report

pastequo commented Jan 30, 2024

dprotaso commented Feb 6, 2024

dprotaso commented Feb 6, 2024

dprotaso commented Feb 7, 2024 • edited Loading

pastequo commented Feb 8, 2024

skonto Mar 12, 2024

Choose a reason for hiding this comment

pastequo Mar 12, 2024

Choose a reason for hiding this comment

skonto Mar 13, 2024

Choose a reason for hiding this comment

skonto Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

pastequo Mar 12, 2024

Choose a reason for hiding this comment

skonto Mar 13, 2024

Choose a reason for hiding this comment

pastequo commented Mar 13, 2024

skonto commented Mar 13, 2024 • edited Loading

pastequo commented Mar 13, 2024

dprotaso commented Mar 21, 2024

knative-prow bot commented Mar 21, 2024

skonto commented Mar 27, 2024 • edited Loading

skonto commented Mar 27, 2024

skonto commented Mar 27, 2024

dprotaso commented Mar 27, 2024

pastequo commented Jan 26, 2024 •

edited by ReToCode

Loading

codecov bot commented Jan 26, 2024 •

edited

Loading

dprotaso commented Feb 7, 2024 •

edited

Loading

skonto Mar 12, 2024 •

edited

Loading

skonto commented Mar 13, 2024 •

edited

Loading

skonto commented Mar 27, 2024 •

edited

Loading