Skip to content

Conversation

@github-actions
Copy link
Contributor

@github-actions github-actions bot commented Mar 12, 2023

Backport of #47123 to release/7.0

/cc @JamesNK

Fix verb in route template with gRPC transcoding

gRPC JSON transcoding routes can have a "verb" at the end of the route. It's indicated by a colon. For example: /v1/greeter/{name}:calculate. It's used for the custom method pattern.

The verb was incorrectly not being used when creating ASP.NET Core endpoints. The result is routing errors. For example, /v1/frames:start and /v1/frames:get both end up with a route /v1/frames, which creates an ambiguous match exception.

Fixes grpc/grpc-dotnet#2054

Customer Impact

Reported by a customer at grpc/grpc-dotnet#2054. They have an error and there isn't a good workaround. Routes could be changed to not use a verb, but forcing developers to change their API address and calling clients isn't good.

Regression?

  • Yes
  • No

[If yes, specify the version the behavior has regressed from]

Risk

  • High
  • Medium
  • Low

Change only impacts gRPC JSON transcoding, and only if someone is using verb in route.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

When servicing release/2.1

  • Make necessary changes in eng/PatchConfig.props

@ghost ghost added the area-grpc Includes: GRPC wire-up, templates label Mar 12, 2023
@ghost ghost added this to the 7.0.x milestone Mar 12, 2023
@ghost
Copy link

ghost commented Mar 12, 2023

Hi @github-actions[bot]. If this is not a tell-mode PR, please make sure to follow the instructions laid out in the servicing process document.
Otherwise, please add tell-mode label.

@JamesNK JamesNK added the Servicing-consider Shiproom approval is required for the issue label Mar 12, 2023
@ghost
Copy link

ghost commented Mar 12, 2023

Hi @github-actions[bot]. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

@JamesNK
Copy link
Member

JamesNK commented Mar 12, 2023

Note: Includes changes from #47165 (discovered while doing manual testing)

@leecow leecow added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Mar 14, 2023
@ghost
Copy link

ghost commented Mar 14, 2023

Hi @github-actions[bot]. This PR was just approved to be included in the upcoming servicing release. Somebody from the @dotnet/aspnet-build team will get it merged when the branches are open. Until then, please make sure all the CI checks pass and the PR is reviewed.

@leecow leecow modified the milestones: 7.0.x, 7.0.5 Mar 14, 2023
@leecow
Copy link
Member

leecow commented Mar 14, 2023

@wtgodbe to confirm with James on confidence of the fix. Last day for checking in and we don't want to elevate risk.

@danmoseley
Copy link
Member

danmoseley commented Mar 14, 2023

If confidence in risk is "very high" we're OK taking this today for next month. If it's not quite that high, please check in after branch reopens for subsequent servicing release

@JamesNK
Copy link
Member

JamesNK commented Mar 14, 2023

Ok, can delay it to 7.0.6.

@JamesNK JamesNK modified the milestones: 7.0.5, 7.0.x Mar 14, 2023
@ghost
Copy link

ghost commented Mar 22, 2023

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no breaking changes are introduced, please leave an /azp run comment here to rerun the CI pipeline and confirm success before merging the change.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Mar 22, 2023
@RussKie
Copy link
Contributor

RussKie commented Mar 22, 2023

Ok, can delay it to 7.0.6.

To confirm, 7.0.6 is slated for May, meaning we expect to merge this in early April (when branches open), right?

@wtgodbe
Copy link
Member

wtgodbe commented Mar 22, 2023

we expect to merge this in early April (when branches open), right?

Yes

@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Apr 4, 2023
@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Apr 4, 2023
@wtgodbe
Copy link
Member

wtgodbe commented Apr 4, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@wtgodbe wtgodbe merged commit 5c477ff into release/7.0 Apr 4, 2023
@wtgodbe wtgodbe deleted the backport/pr-47123-to-release/7.0 branch April 4, 2023 21:18
@ghost ghost modified the milestones: 7.0.x, 7.0.6 Apr 4, 2023
@rbhanda rbhanda modified the milestones: 7.0.6, 7.0.7 Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-grpc Includes: GRPC wire-up, templates area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework Servicing-approved Shiproom has approved the issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants