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

Add support for HTTPOption #437

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ReToCode
Copy link
Member

@ReToCode ReToCode commented Jan 18, 2023

This PR introduces support for HTTPOption (introduced in knative/networking#415).

Changes

  • 🎁 Add support for HTTPOption
  • 🎁 Introduces a new configuration to specify the name of the http-listener for the gateways
  • 🧹 Cleanup some redundancies in test code
  • 🧹 Use new functions where deprecations were used

The implementation is based on the gateway-api example for redirects: https://github.com/kubernetes-sigs/gateway-api/blob/main/examples/standard/http-redirect.yaml. It was necessary to restructure some code, as two HTTPRoute objects need to be created, each referencing only a specific section in the gateway using sectionName. We should further discuss if we should use that approach also for the "normal" use-case (as we currently bind to all the sections in the gateway only differentiated by the hostname in the TLS case). Also The configuration key visibility seems now a bit off, maybe we can find a better name?

A full example of how the resources look like can be found here: https://gist.github.com/ReToCode/2e4d2b3223752c6f380348ef027c55b3

/kind cleanup
/kind enhancement

Fixes #130

@knative-prow
Copy link

knative-prow bot commented Jan 18, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@knative-prow knative-prow bot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/enhancement size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 18, 2023
@ReToCode
Copy link
Member Author

/test all

@ReToCode
Copy link
Member Author

/cc @nak3 , @dprotaso could you please look over this. I'm a bit unsure if this is the way to go.

@codecov
Copy link

codecov bot commented Jan 18, 2023

Codecov Report

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

Project coverage is 79.02%. Comparing base (217f929) to head (5256377).
Report is 12 commits behind head on main.

Files Patch % Lines
pkg/reconciler/ingress/reconcile_resources.go 52.50% 15 Missing and 4 partials ⚠️
pkg/reconciler/ingress/resources/httproute.go 90.41% 3 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #437      +/-   ##
==========================================
- Coverage   79.12%   79.02%   -0.11%     
==========================================
  Files          17       17              
  Lines        1260     1373     +113     
==========================================
+ Hits          997     1085      +88     
- Misses        227      245      +18     
- Partials       36       43       +7     

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

@ReToCode
Copy link
Member Author

Also not really sure about this change: 663d28b. Either unit tests complain about header mismatch (https://github.com/knative-sandbox/net-gateway-api/actions/runs/3949133837/jobs/6759963500#step:7:173) or golangci-lint complains about not preallocating the slice (https://github.com/knative-sandbox/net-gateway-api/actions/runs/3947688057/jobs/6756800214).

@ReToCode
Copy link
Member Author

/retest

@ReToCode
Copy link
Member Author

/test integration-tests-istio

@nak3
Copy link
Contributor

nak3 commented Jan 19, 2023

Thank you so much, Reto!

It was necessary to restructure some code, as two HTTPRoute objects need to be created, each referencing only a specific section in the gateway using sectionName.

I wondered that upstream gateway-api can improve the two HTTPRoute objects for HTTPS redirection (I believe it is too redundant) and actually there are some on-going discussion:

Once the upstream's latest example httproute-redirect-https.yaml (currently invalid!) work, we can simply implement this feature, can't we?

@ReToCode
Copy link
Member Author

Interesting, haven't seen that. You are right about the verbosity, I agree there. But the discussion seems also unsure on how to resolve this. Seems like the current stable state is also the last discussed state (kubernetes-sigs/gateway-api#1185 (comment))?
Also I haven't found an issue that is related to the example you linked, so not sure if there is work going in that direction. But I'd be fine to hold this feature here until this is more clear.

@nak3
Copy link
Contributor

nak3 commented Jan 19, 2023

Also I haven't found an issue that is related to the example you linked

Hmm... As far as I tested, it keeps redirecting to HTTPS as explained in kubernetes-sigs/gateway-api#1185 and never reached out to the backendRefs(example-svc). To solve it, it was necessary to create two HTTPRoutes.

Anyway, kubernetes-sigs/gateway-api#1185 was tagged to 0.7.0 milestone so maybe it is better to wait a little bit for the upstream.

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 18, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 7, 2023

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Reopen with /reopen. Mark as fresh by adding the
comment /remove-lifecycle stale.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 7, 2023
@nak3
Copy link
Contributor

nak3 commented Aug 7, 2023

/remove-lifecycle stale

@knative-prow knative-prow bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 7, 2023
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 8, 2023
@ReToCode
Copy link
Member Author

ReToCode commented Aug 8, 2023

As it does not look like upstream gw-api is going to change this behaviour soon (the issue was moved out of GA) and the current redirect behaviour is perceived as stable, let's give the current approach another go.

PTAL: @nak3 , @dprotaso

@ReToCode ReToCode marked this pull request as ready for review August 8, 2023 12:05
@knative-prow knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 8, 2023
@knative-prow knative-prow bot requested review from dprotaso and upodroid August 8, 2023 12:05
@ReToCode
Copy link
Member Author

ReToCode commented Aug 8, 2023

/retest

@ReToCode
Copy link
Member Author

/assign @nak3
/assign @dprotaso

@ReToCode ReToCode removed the request for review from upodroid March 18, 2024 08:10
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 13, 2024
@skonto
Copy link

skonto commented Jun 19, 2024

@ReToCode gentle ping when you are back.

@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 3, 2024
@ReToCode
Copy link
Member Author

ReToCode commented Jul 3, 2024

@dprotaso @skonto rebased, PTAL.

@knative-extensions knative-extensions deleted a comment from knative-prow bot Jul 3, 2024
@ReToCode
Copy link
Member Author

ReToCode commented Jul 3, 2024

/hold

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 3, 2024
@knative-prow knative-prow bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 3, 2024
@ReToCode
Copy link
Member Author

ReToCode commented Jul 3, 2024

HTTP-Option test does not yet work on envoy gateway because of envoyproxy/gateway#2149.

/unhold

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 3, 2024
@knative-extensions knative-extensions deleted a comment from knative-prow bot Jul 3, 2024
@knative-extensions knative-extensions deleted a comment from knative-prow bot Jul 3, 2024
@knative-extensions knative-extensions deleted a comment from knative-prow bot Jul 3, 2024
@arkodg
Copy link

arkodg commented Aug 9, 2024

HTTP-Option test does not yet work on envoy gateway because of envoyproxy/gateway#2149.

/unhold

hey @ReToCode thanks for flagging this, the issue has been fixed, can we retry the test for Envoy Gateway ?

@ReToCode
Copy link
Member Author

ReToCode commented Aug 9, 2024

@arkodg looks great. Thanks for the fix and the ping 👍

if err != nil {
return err
}

if isHTTPRouteReady(httproute) {
// For now, we only generate the redirected HTTPRoute for external visibility,
// because currently we do not (yet) support HTTPs for cluster-local domains in net-gateway-api.
Copy link

@skonto skonto Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we support that for internal encryption and Kourier? 🤔
What is the status for the rest of the ingress implementations?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes Kourier supports it, net-istio does not need it and net-contour work was started but never finished.
But as this is quite niche and not in the conformance spec, I think we are fine without it here.

@skonto
Copy link

skonto commented Oct 23, 2024

@dprotaso gentle ping, should we merge this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/enhancement size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add TestHTTPOption for gateway-api
6 participants