Add FilterConfigurationBase#518
Conversation
Base class for test server http filter extension configuration. Provides support for shared configuration and consolidates shared code. Potentially useful in that it provides a path to cache parsed and interpreted configuration accross extensions as it flows through the server pipeline. This isn't a goal for now however, this is a prelude to envoyproxy#512, which includes it and provides a means to see the end goal. Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
|
note to reviewers: to avoid a dip in coverage, this doesn't wire up in BUILD yet. There's no separate testing on this around yet, though it will get tested once we're done via extensions as the extensions rely on this (see #512). |
dubious90
left a comment
dubious90
left a comment
There was a problem hiding this comment.
Looks good. Have a couple comments.
Just to make sure I follow, there is a PR coming soon that will integrate these changes into our existing filters, and that's when we'll bring in both the BUILD changes and the testing?
| * or a shared pointer to the effective configuration. We use a shared pointer to avoid copying | ||
| * in the static configuration flow. | ||
| */ | ||
| using EffectiveFilterConfiguration = |
There was a problem hiding this comment.
I think including StatusOr as part of this is a little confusing. Can we make EffectiveFilterConfiguration just the shared pointer, and then return StatusOr?
There was a problem hiding this comment.
done, let me know if you feel this was properly addressed.
| * @param filter_name name of the extension that is consuming this. Used during error response | ||
| * generation. | ||
| */ | ||
| FilterConfigurationBase(nighthawk::server::ResponseOptions static_proto_config, |
There was a problem hiding this comment.
can this be a const reference?
| namespace Nighthawk { | ||
| namespace Server { | ||
|
|
||
| FilterConfigurationBase::FilterConfigurationBase(nighthawk::server::ResponseOptions proto_config, |
There was a problem hiding this comment.
nit: proto_config has a different name in the header file.
| } | ||
|
|
||
| bool FilterConfigurationBase::maybeSendErrorReply( | ||
| Envoy::Http::StreamDecoderFilterCallbacks& decoder_callbacks) const { |
There was a problem hiding this comment.
Can this be const? Or is sendLocalReply a non-const function?
There was a problem hiding this comment.
sendLocalReply is a non-const function, so we can't const this.
dubious90
added
waiting-for-changes
…r-prelude-2 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
oschaaf
left a comment
oschaaf
left a comment
There was a problem hiding this comment.
@dubious90 review feedback addressed in 562a1d9, including the updated canonical representation which uses absl::StatusOr, so we can open up the discussion on that.
| } | ||
|
|
||
| bool FilterConfigurationBase::maybeSendErrorReply( | ||
| Envoy::Http::StreamDecoderFilterCallbacks& decoder_callbacks) const { |
There was a problem hiding this comment.
sendLocalReply is a non-const function, so we can't const this.
| * or a shared pointer to the effective configuration. We use a shared pointer to avoid copying | ||
| * in the static configuration flow. | ||
| */ | ||
| using EffectiveFilterConfiguration = |
There was a problem hiding this comment.
done, let me know if you feel this was properly addressed.
| * @param filter_name name of the extension that is consuming this. Used during error response | ||
| * generation. | ||
| */ | ||
| FilterConfigurationBase(nighthawk::server::ResponseOptions static_proto_config, |
that is correct. this is a subset of #512 which has the BUILD file and everything else. The BUILD file is excluded to avoid coverage from failing because of dipping below the threshold. |
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
|
@dubious90 as discussed, backed out the absl::StatusOr typedef in b894e96 |
oschaaf
added
waiting-for-review
2 participants
Base class for test server http filter extension configuration.
Provides support for shared configuration and consolidates
shared code.
Potentially useful in that it provides a path to cache parsed
and interpreted configuration accross extensions as it flows
through the server pipeline. This isn't a goal for now however,
this is a prelude to #512, which includes it and provides a means
to see the end goal.
Signed-off-by: Otto van der Schaaf oschaaf@we-amp.com