Update to v1.30 #333
Merged
Update to v1.30 #333
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was removed from upstream - see envoyproxy/envoy#32872
These flags are copied from upstream's .bazelrc. The most important one here is the `-fsized-deallocation` which was required to get CEL to compile. The rest may not be necessary, but since they changed in upstream from 1.29 to 1.30, I think it might be a good idea to include them.
There were some details in regex that were changed upstream that we had to incorporate
A few upstream Envoy functions that were being called in this library needed to be passed contexts, which required plumbing that object through the call hierarchy. Along the way, it seemed to appear that there were a few type signatures that were possibly not completely correct, and those have been adjusted to get things compiling successfully.
It seems that passing string arguments constructed with the `+` operator is not allowed in ENVOY_STREAM_LOG, and possibly other macros as well. This was my first encounter with this error, but there might be other instances lurking elsewhere in the repository.
The signature of this abstract class method was changed upstream - this change reflects the fix
|
Issues linked to changelog: |
Interesting lesson here - when calling `fmt::format`, we must use a constexpr, since the formatting function provides compile-time type checking. Neat!
|
Current status: known segmentation fault in one of the inja transformer tests: (EDIT: this is fixed by 0058721) have not yet had a chance to debug it |
Upstream ci calls this, so we put in a dummy target here
|
/kick |
nfuden
reviewed
May 13, 2024
| public: | ||
| Regex::CompiledMatcherPtr matcher(const std::string ®ex) const override { | ||
| return std::make_unique<CompiledStdMatcher>( | ||
| Solo::Regex::Utility::parseStdRegex(regex)); |
There was a problem hiding this comment.
Do we still need to do this if engine is now included by default in 1.30?
It sounds like we are cruising for a bruising in upcoming releases if we rely on this
There was a problem hiding this comment.
i don't believe std::regex is present in upstream envoy. i see a few references to that class, but no functions that are publicly exported that we can use for using std:regex. i could be wrong here though, let me know of a way that we can use it
| @@ -61,14 +67,6 @@ class ResponseMatcherImpl : public ResponseMatcher { | |||
| absl::optional<Matchers::StringMatcherImpl<envoy::type::matcher::v3::StringMatcher>> response_code_details_match_; | |||
| }; | |||
|
|
|||
| ResponseMatcherImpl::ResponseMatcherImpl( | |||
There was a problem hiding this comment.
if we're defining the constructor in the header file, i figured we might as well define it in the class declaration, but happy to revert. sorry about the fact that it messed up the diff though
test/extensions/filters/http/transformation/inja_transformer_test.cc
Outdated
Show resolved
Hide resolved
Co-authored-by: Nathan Fudenberg <nathan.fudenberg@solo.io>
…nsions/extensions_build_config.bzl Co-authored-by: Nathan Fudenberg <nathan.fudenberg@solo.io>
Co-authored-by: Nathan Fudenberg <nathan.fudenberg@solo.io>
Co-authored-by: Nathan Fudenberg <nathan.fudenberg@solo.io>
nfuden
reviewed
May 13, 2024
nfuden
reviewed
May 14, 2024
test/extensions/filters/http/transformation/inja_transformer_test.cc
Outdated
Show resolved
Hide resolved
Previous iterations were still using the `vformat` call so they may not have been using the `constexpr` versino of formatting correctly
nfuden
approved these changes
May 14, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Update envoy-gloo to use version 1.30.x of upstream envoy
The biggest change here is probably the update to C++20, which follows from this upstream commit.
A large amount of the changes come from navigating Envoy's labyrinth of context hierarchy. I figure that the easiest thing to do is just pass the full context object instead of passing portions of it into our objects. This allows us to keep our function signatures from having to change too much, and in my opinion, is a general best practice going forward as it seems that in the past, we had a habit of just passing more specific objects like
context.scope()instead.See here for an example, where we modify
TransformationFilterConfig::TransformationFilterConfigto accept aServer::Configuration::FactoryContext& contextargument instead of ascopeargument. This allows us to retrieve other fields from the context, likecontext.serverFactoryContext()which is a new parameter that needs to be passed toMatcherCopy::Matcher::create(this eventually gets plumbed down intobuildHeaderDataVector).Another example of that is here, where we need to pass
context.serverFactoryContext()as a new parameter into the StringMatcher constructor. In the process,dispatcher,runtime, andrandomwere removed fromAdvancedHttpHealthChecker::AdvancedHttpHealthChecker. And if any other functions from upstream Envoy also change in the future to require additional parameters from the context, those will be much easier to handle if we just always pass the full context object instead of extracting fields from it.Statusor: removes more exceptions which decreases the size
Serverfactorycontext: Removing singletons