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.

Sign up for GitHub

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

Jump to bottom

fix(plugin/azure-function): clear upstream uri and request uri inject plugin logic #11850

Merged
merged 6 commits into from
Nov 16, 2023
Merged

fix(plugin/azure-function): clear upstream uri and request uri inject plugin logic #11850

merged 6 commits into from
Nov 16, 2023

Conversation

oowl
Copy link
Member

@oowl oowl commented Oct 26, 2023 •
edited by windmgc
Loading

Summary

Checklist

  • The Pull Request has tests
  • A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
  • There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Full changelog

  • [Implement ...]

Issue reference

KAG-2841

@oowl
Copy link
Member Author

oowl commented Oct 26, 2023 •
edited
Loading

@bungle Hi Aapo, I do not know how this plugin is designed, but I think these upstream URIs and request URIs can not inject azure function plugin logic, Which do you mean?

@pull-request-size pull-request-size bot added size/L and removed size/M labels Oct 26, 2023
@oowl oowl force-pushed the fix/azure-function-handler branch from bc1121c to 8a57d00 Compare October 26, 2023 07:27
@pull-request-size pull-request-size bot added size/M and removed size/L labels Oct 26, 2023
@oowl oowl added this to the 3.5.0 milestone Oct 26, 2023
@oowl oowl requested a review from bungle October 26, 2023 09:39
windmgc
windmgc approved these changes Oct 27, 2023
View reviewed changes
Copy link
Member

@windmgc windmgc left a comment

Choose a reason for hiding this comment

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

The change seems reasonable to me since concatenating the upstream URI/request URI with the conf.routeprefix is quite unnecessary, it seems we only need the routeprefix to construct the Azure API request
Approved although it might be a potential behavior change

changelog/unreleased/kong/fix-upstream-uri-azure-function-plugin.yml Outdated Show resolved Hide resolved
@AndyZhang0707 AndyZhang0707 removed this from the 3.5.0 milestone Oct 27, 2023
@oowl oowl force-pushed the fix/azure-function-handler branch from 91cea88 to 10a73a8 Compare October 27, 2023 14:44
@kikito kikito requested a review from brentos October 31, 2023 18:05
@oowl oowl force-pushed the fix/azure-function-handler branch from 10a73a8 to 4aabbd5 Compare November 3, 2023 06:51
@brentos
Copy link
Contributor

brentos commented Nov 3, 2023 •
edited
Loading

@Tieske Do you happen to know / remember why this plugin concatenated the upstream path? This PR changes the existing behavior.

@jschmid1
Copy link
Contributor

jschmid1 commented Nov 7, 2023

@Tieske do you have any further information?

@Tieske
Copy link
Member

Tieske commented Nov 7, 2023

once upon a time... the tests passed, and I committed...

Really, don't recall the details. It mimicked the Lambda one, and I had test functions that worked...

@oowl oowl force-pushed the fix/azure-function-handler branch 2 times, most recently from 3631b93 to efbcdae Compare November 14, 2023 05:52
@bungle
Copy link
Member

bungle commented Nov 14, 2023

I don't recall anything about this either.

@gszr
Copy link
Member

gszr commented Nov 14, 2023

Is there any further action items here folks? Can we merge it?

brentos
brentos reviewed Nov 14, 2023
View reviewed changes
Copy link
Contributor

@brentos brentos left a comment

Choose a reason for hiding this comment

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

I'm okay with the changes but I think we should make this a "breaking_change" in the changelog, since some users could depend on this behavior

@oowl oowl force-pushed the fix/azure-function-handler branch from efbcdae to d8e5eb4 Compare November 15, 2023 02:54
@oowl
Copy link
Member Author

oowl commented Nov 15, 2023

@brentos I have changed the entry to breaking change type. Please help me review it again.

@oowl oowl force-pushed the fix/azure-function-handler branch from d8e5eb4 to 0a55508 Compare November 15, 2023 02:55
@oowl oowl force-pushed the fix/azure-function-handler branch from 0a55508 to 1953c64 Compare November 15, 2023 08:47
@windmgc windmgc merged commit 13dbed3 into master Nov 16, 2023
23 checks passed
@windmgc windmgc deleted the fix/azure-function-handler branch November 16, 2023 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tzssangglass tzssangglass tzssangglass approved these changes

@brentos brentos brentos approved these changes

@windmgc windmgc windmgc approved these changes

@bungle bungle Awaiting requested review from bungle

Assignees

@oowl oowl

Labels
plugins/azure-functions size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@oowl @brentos @jschmid1 @Tieske @bungle @gszr @windmgc @tzssangglass @AndyZhang0707