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

Support Path and QueryParams in http route matches #204

Merged

Conversation

lujiajing1126
Copy link
Contributor

@lujiajing1126 lujiajing1126 commented Mar 12, 2024

Ⅰ. Describe what this PR does

Add support for Path and QueryParams

Ⅱ. Does this pull request fix one issue?

Fixes #205

Ⅲ. Special notes for reviews

Supported list for Path:

  • Istio

Supported list for QueryParams:

  • Gateway API
  • Istio
  • MSE Ingress

@kruise-bot
Copy link

Welcome @lujiajing1126! It looks like this is your first PR to openkruise/rollouts 🎉

Copy link

codecov bot commented Mar 12, 2024

Codecov Report

Attention: Patch coverage is 78.26087% with 5 lines in your changes missing coverage. Please review.

Project coverage is 43.45%. Comparing base (07c1731) to head (48bd9ea).
Report is 1 commits behind head on master.

Current head 48bd9ea differs from pull request most recent head 5720914

Please upload reports for the commit 5720914 to get more accurate results.

Files Patch % Lines
pkg/trafficrouting/network/gateway/gateway.go 50.00% 1 Missing and 2 partials ⚠️
pkg/trafficrouting/manager.go 71.42% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #204      +/-   ##
==========================================
- Coverage   43.63%   43.45%   -0.19%     
==========================================
  Files          52       52              
  Lines        5681     5682       +1     
==========================================
- Hits         2479     2469      -10     
- Misses       2778     2787       +9     
- Partials      424      426       +2     
Flag Coverage Δ
unittests 43.45% <78.26%> (-0.19%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

lujiajing1126 and others added 9 commits March 15, 2024 16:59
Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
…idering case-insensitivity. (openkruise#200)

* Fixed a bug caused by NOT considering case-insensitivity.

Signed-off-by: yunbo <yunbo10124scut@gmail.com>

* add DisableGenerateCanaryService for CanaryStrategy

amend1: update crd yaml

amend2: add DisableGenerateCanaryService for v1alpha1

Signed-off-by: yunbo <yunbo10124scut@gmail.com>

---------

Signed-off-by: yunbo <yunbo10124scut@gmail.com>
Co-authored-by: yunbo <yunbo10124scut@gmail.com>
Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
@lujiajing1126 lujiajing1126 force-pushed the support-more-http-route-matches branch from 778ffc5 to 48bd9ea Compare March 15, 2024 08:59
api/v1alpha1/rollout_types.go Outdated Show resolved Hide resolved
api/v1beta1/rollout_types.go Outdated Show resolved Hide resolved
pkg/trafficrouting/manager.go Outdated Show resolved Hide resolved
@lujiajing1126
Copy link
Contributor Author

I have some misunderstanding on the logical operator used for RouteMatcher. I have checked it again last week. Now I believe Istio and GatewayAPI have similar semantics.

I will refactor the proposal and the implementation soon.

Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
@lujiajing1126
Copy link
Contributor Author

lujiajing1126 commented May 15, 2024

I've refactored semantics and implemented for MSE and istio scenarios. PTAL @zmberg

Also the proposal has been updated. E2E tests are added as requested.

@zmberg
Copy link
Member

zmberg commented Jun 12, 2024

/lgtm

@zmberg
Copy link
Member

zmberg commented Jun 12, 2024

/approve

@kruise-bot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zmberg

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:

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

@kruise-bot kruise-bot merged commit 62794dc into openkruise:master Jun 12, 2024
19 checks passed
@lujiajing1126 lujiajing1126 deleted the support-more-http-route-matches branch June 12, 2024 05:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Proposal] 流量路由规则支持Path和QueryParams
4 participants