Skip to content

extended request options to support passing metadata match criteria#15863

Merged
alyssawilk merged 3 commits intoenvoyproxy:mainfrom
wbpcode:request-option
Apr 14, 2021
Merged

extended request options to support passing metadata match criteria#15863
alyssawilk merged 3 commits intoenvoyproxy:mainfrom
wbpcode:request-option

Conversation

@wbpcode
Copy link
Copy Markdown
Member

@wbpcode wbpcode commented Apr 7, 2021

Signed-off-by: wbpcode comems@msn.com

Commit Message:

extended request options to support passing metadata match criteria

Additional Description:

Extended request options to support passing metadata match criteria. Check #15163 for more info about that why we need this PR. This PR is just a possible solution. I would be willing to modify it if there is a more elegant solution.

Risk Level: Low.
Testing: Added.
Docs Changes: N/A.
Release Notes: N/A.

@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Apr 7, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15863 (comment) was created by @wbpcode.

see: more, trace.

Copy link
Copy Markdown
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

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

Thanks! See my comment below. Otherwise this looks good.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this method should follow the example of setHashPolicy and accept a const envoy::config::core::v3::Metadata&. That way callers don't need to use the implementation of MetadataMatchCriteria and aren't tempted to provide their own implementation.

@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Apr 8, 2021

@zuercher Thanks. 😄
I chose to pass MetadataMatchCriteria instead of envoy::config::core::v3::Metadata because of the following reasons:

  1. In some cases, we can directly reuse the MetadataMatchCriteria from the route.
auto options = 
  RequestOptions().setsetMetadataMatchCriteria(decoder_callbacks_->route()->routeEntry()->metadataMatchCriteria());
  1. The MetadataMatchCriteria is generally fixed when we send http requests to external services via http calls in a filter. The MetadataMatchCriteria may change only when the configuration changes. Passing the MetadataMatchCriteria directly allows the developer of filter to decide if the MetadataMatchCriteriaImpl object needs to be constructed, avoiding the need to reconstruct the MetadataMatchCriteriaImpl object each time a request is sent.

@zuercher
Copy link
Copy Markdown
Member

zuercher commented Apr 9, 2021

I discussed this briefly with another maintainer and the conclusion was that AsyncClient is one of the primary interfaces that a user of envoy as a library has to contend with. So I still think it would be better to limit this to using the proto types used to configure the feature for a route. Perhaps adding a method on the RouteEntry that allows access to the metadata_match proto objects would make it easier to implement your use case?

@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Apr 12, 2021

🤔 I agree with you, we can improve this part later.

Signed-off-by: wbpcode <comems@msn.com>
@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Apr 12, 2021

Only one commit. I refactored it to create a cleaner implementation.

Copy link
Copy Markdown
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

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

Nice! I had one small comment, but otherwise looks good to me.

wbpcode added 2 commits April 13, 2021 11:21
Signed-off-by: wbpcode <comems@msn.com>
Signed-off-by: wbpcode <comems@msn.com>
Copy link
Copy Markdown
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

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

Thanks!

@zuercher
Copy link
Copy Markdown
Member

@alyssawilk can you take a second pass?

Copy link
Copy Markdown
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

Code and tests look great!
@ggreenway is this worth a release note, or do you think it's fine as is.

@ggreenway
Copy link
Copy Markdown
Member

@alyssawilk did you mean @zuercher ?

Either way, I took a look; I don't think there's any public-facing change, so probably fine with no release note.

@alyssawilk alyssawilk merged commit 69b97db into envoyproxy:main Apr 14, 2021
douglas-reid pushed a commit to douglas-reid/envoy that referenced this pull request Apr 19, 2021
…nvoyproxy#15863)

Commit Message:

extended request options to support passing metadata match criteria

Additional Description:

Extended request options to support passing metadata match criteria. Check envoyproxy#15163 for more info about that why we need this PR. This PR is just a possible solution. I would be willing to modify it if there is a more elegant solution.

Risk Level: Low.
Testing: Added.
Docs Changes: N/A.

Signed-off-by: wbpcode <comems@msn.com>
Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>
Monkeyanator pushed a commit to Monkeyanator/envoy that referenced this pull request Apr 20, 2021
…nvoyproxy#15863)

Commit Message:

extended request options to support passing metadata match criteria

Additional Description:

Extended request options to support passing metadata match criteria. Check envoyproxy#15163 for more info about that why we need this PR. This PR is just a possible solution. I would be willing to modify it if there is a more elegant solution.

Risk Level: Low.
Testing: Added.
Docs Changes: N/A.

Signed-off-by: wbpcode <comems@msn.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants