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

feat: add support for HTTPRouteTimeoutBackendRequest #5243

Merged
merged 1 commit into from
Jan 24, 2024

Conversation

tao12345666333
Copy link
Member

@tao12345666333 tao12345666333 commented Nov 28, 2023

What this PR does / why we need it:

Which issue this PR fixes:

fixes: #4914
Special notes for your reviewer:

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@tao12345666333 tao12345666333 added the area/gateway-api Relating to upstream Kubernetes SIG Networking Gateway API label Nov 28, 2023
Copy link

codecov bot commented Nov 28, 2023

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (main@887d0d2). Click here to learn what that means.

Files Patch % Lines
...ternal/dataplane/translator/translate_httproute.go 81.2% 3 Missing and 3 partials ⚠️
internal/admission/validation/gateway/httproute.go 86.3% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##             main   #5243   +/-   ##
======================================
  Coverage        ?   69.6%           
======================================
  Files           ?     176           
  Lines           ?   22710           
  Branches        ?       0           
======================================
  Hits            ?   15828           
  Misses          ?    5942           
  Partials        ?     940           

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

@tao12345666333 tao12345666333 force-pushed the feat/HTTPRouteTimeoutBackendRequest branch 3 times, most recently from 0b2b8bc to 1af087f Compare December 1, 2023 04:50
@tao12345666333 tao12345666333 marked this pull request as ready for review December 1, 2023 05:24
@tao12345666333 tao12345666333 requested a review from a team as a code owner December 1, 2023 05:24
@tao12345666333 tao12345666333 force-pushed the feat/HTTPRouteTimeoutBackendRequest branch from 1af087f to e81c3ee Compare December 1, 2023 05:55
@pull-request-size pull-request-size bot added size/M and removed size/S labels Dec 1, 2023
@tao12345666333 tao12345666333 force-pushed the feat/HTTPRouteTimeoutBackendRequest branch from e81c3ee to 7d63ac2 Compare December 1, 2023 05:57
CHANGELOG.md Outdated Show resolved Hide resolved
@tao12345666333 tao12345666333 force-pushed the feat/HTTPRouteTimeoutBackendRequest branch from e2cb0bd to 9889f6a Compare December 5, 2023 16:14
@pull-request-size pull-request-size bot added size/L and removed size/M labels Dec 5, 2023
@tao12345666333 tao12345666333 enabled auto-merge (squash) December 5, 2023 16:16
@tao12345666333 tao12345666333 added this to the KIC v3.1.x milestone Dec 5, 2023
@tao12345666333 tao12345666333 force-pushed the feat/HTTPRouteTimeoutBackendRequest branch 2 times, most recently from 49ba4a1 to d96c09a Compare December 5, 2023 16:33
Copy link
Contributor

@czeslavo czeslavo left a comment

Choose a reason for hiding this comment

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

I think that we may overlooked that we shouldn't simply iterate over all rules of an HTTPRoute and apply the timeouts to every Kong Service that originated from a given HTTPRoute. It's possible we'll have multiple Kong Services generated from a single HTTPRoute and I think we should apply the timeouts to them separately, taking into account whether a Kong Service originates from the rule with timeout defined ("belong to the same group"). The pattern used for generating Kong Services from an HTTPRoute is httpRoute.<httpRoute.namespace>.<httpRoute.name>.<firstRuleInGroup.idx> - we generate one Service per HTTPRoute rules group (where group is a set of rules that have common backends).

To make it clear I'll provide an example. Let's say we have an HTTPRoute with two rules, each pointing at different Kubernetes Service, and one more pointing at another:

apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  name: httproute-testing
  namespace: default
  annotations:
    konghq.com/strip-path: "true"
spec:
  parentRefs:
    - name: kong
  rules:
    - matches:
        - path:
            type: PathPrefix
            value: /httproute-testing
      backendRefs:
        - name: httpbin
          kind: Service
          port: 80
          weight: 75
        - name: nginx
          kind: Service
          port: 8080
          weight: 25
    - matches:
        - path:
            type: PathPrefix
            value: /httpbin-only-1
      backendRefs:
        - name: httpbin
          kind: Service
          port: 80
    - matches:
        - path:
            type: PathPrefix
            value: /httbin-only-2
      backendRefs:
        - name: httpbin
          kind: Service
          port: 80

KIC will translate them to more or less this configuration (only services extracted):

_format_version: "3.0"
services:
# This service is generated out of `/httpbin-only-1` and  `/httbin-only-2` rules that had the same backendRefs and settings.
- connect_timeout: 60000
  host: httproute.default.httproute-testing.1
  id: 672dc964-a7f8-5a80-a518-96fc39500cc8
  name: httproute.default.httproute-testing.1
  port: 80
  protocol: http
  read_timeout: 60000
  retries: 5
  routes:
  - https_redirect_status_code: 426
    id: 4deff45c-4095-55d2-80ab-aea40c0ac7bd
    name: httproute.default.httproute-testing.1.0
    path_handling: v0
    paths:
    - ~/httpbin-only-1$
    - /httpbin-only-1/
    - ~/httbin-only-2$
    - /httbin-only-2/
    preserve_host: true
    protocols:
    - http
    - https
    strip_path: true
  write_timeout: 60000
# This service is generated out of `/httproute-testing` rule that has multiple backendRefs.
- connect_timeout: 60000
  host: httproute.default.httproute-testing.0
  id: 4e3cb785-a8d0-5866-aa05-117f7c64f24d
  name: httproute.default.httproute-testing.0
  port: 8080
  protocol: http
  read_timeout: 60000
  retries: 5
  routes:
  - https_redirect_status_code: 426
    id: 073fc413-1c03-50b4-8f44-43367c13daba
    name: httproute.default.httproute-testing.0.0
    path_handling: v0
    paths:
    - ~/httproute-testing$
    - /httproute-testing/
    preserve_host: true
    protocols:
    - http
    - https
    strip_path: true

Please note the comments in the resulting config. If there were multiple rules in a single HTTPRoute that would use the same Kubernetes Service, they'd be merged into one Service, but it must be the same one (or the same set of Services when multiple backends are used). This part of the Translator is responsible for this grouping mechanism.

Given that, I think we have to make sure that we do not override other Kong Service's timeouts unintentionally like it's currently implemented.

internal/dataplane/translator/translate_httproute.go Outdated Show resolved Hide resolved
@tao12345666333
Copy link
Member Author

#4914 (comment)
This is my consideration before implementing this feature.

@tao12345666333
Copy link
Member Author

After discussing with @czeslavo and @mflendrich, we have reached a consensus.

This feature is experimental and in an early stage. Both the upstream specification and KIC may make modifications in the future. More thought is needed to change the default behavior of KIC for it.

Currently, we can keep this implementation but add a restriction that users can only set them to the same value, otherwise, it will be rejected, so there will be no overlapping situations.

e.g.

apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  name: timeout-example
spec:
  ...
  rules:
  - backendRefs:
    - name: some-service
      port: 8080
    timeouts:
      backendRequest: 5s
  - backendRefs:
    - name: some-service
      port: 8080

this one will be rejected.

apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  name: timeout-example
spec:
  ...
  rules:
  - backendRefs:
    - name: some-service
      port: 8080
    timeouts:
      backendRequest: 5s
  - backendRefs:
    - name: some-service
      port: 8080
    timeouts:
      backendRequest: 5s

this one will be allowed

I will increase this restriction through admission and create an issue for tracking. Once we have other modifications, the admission can be removed.

@czeslavo czeslavo added the release/not-required this is not a hard requirement for the release, it can be considered a "nice to have" label Jan 22, 2024
@tao12345666333 tao12345666333 force-pushed the feat/HTTPRouteTimeoutBackendRequest branch from d96c09a to c39fa90 Compare January 23, 2024 06:57
Copy link
Contributor

@czeslavo czeslavo left a comment

Choose a reason for hiding this comment

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

Overall looks good to me now! 👍 Just minor nits regarding TODOs and code cosmetics.

CHANGELOG.md Show resolved Hide resolved
internal/admission/validation/gateway/httproute.go Outdated Show resolved Hide resolved
internal/dataplane/translator/translate_httproute.go Outdated Show resolved Hide resolved
Signed-off-by: Jintao Zhang <zhangjintao9020@gmail.com>
Co-authored-by: Grzegorz Burzyński <czeslavo@gmail.com>
Copy link
Contributor

@czeslavo czeslavo left a comment

Choose a reason for hiding this comment

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

🚢

@tao12345666333 tao12345666333 merged commit 910b8b9 into main Jan 24, 2024
37 checks passed
@tao12345666333 tao12345666333 deleted the feat/HTTPRouteTimeoutBackendRequest branch January 24, 2024 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/gateway-api Relating to upstream Kubernetes SIG Networking Gateway API release/not-required this is not a hard requirement for the release, it can be considered a "nice to have" size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support conformance test HTTPRouteTimeoutBackendRequest
4 participants