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

mobile: moving the c++ integration test to use default config #2293

Merged
merged 2 commits into from
May 20, 2022
Merged

mobile: moving the c++ integration test to use default config #2293

merged 2 commits into from
May 20, 2022

Conversation

alyssawilk
Copy link
Contributor

Risk Level: n/a
Testing: test still passes
Docs Changes: n/a (test only)
Release Notes: n/a (test only)

@alyssawilk
mobile: moving the c++ integration test to use default config
7ff1f33 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk
Copy link
Contributor Author

cc @abeyad

@alyssawilk
h/t ali
2bbc4c6 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk
Copy link
Contributor Author

Also question for Lyft, it took me ages to figure out that my generated config was invalid because I wasn't prepending the config header. I was going to add comments in engine.h about that, but I notice it's not exposed via engine.h only internal.h which implies it generally shouldn't be used.

Can y'all think of a clarifying comment I could add to the Engine's generateConfigStr which would make it more clear what one has to do to make it valid?

@alyssawilk alyssawilk enabled auto-merge (squash) May 18, 2022 20:31
jpsim
jpsim reviewed May 18, 2022
View reviewed changes
test/common/integration/client_integration_test.cc
use_lds_ = false;
autonomous_upstream_ = true;
defer_listener_finalization_ = true;
HttpTestUtility::addDefaultHeaders(default_request_headers_);
config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) {
// The default stats config has overenthusiastic filters.
Copy link
Contributor

Choose a reason for hiding this comment

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

What filters cause issues exactly? Should this be adjusted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I added this as agenda to today's meeting as I wanted to get context on your stats filters. the stats config here:
https://github.com/envoyproxy/envoy-mobile/blob/main/library/common/config/config.cc#L421
filters out http.client.stream_success and friends which is used in the integration test.

I'm inclined to land this one as-is, and then work out with you folks if we should remove the stats matcher entirely (and move it to Lyft internal config), make it configurable, or what.

@jpsim
Copy link
Contributor

jpsim commented May 18, 2022

it took me ages to figure out that my generated config was invalid because I wasn't prepending the config header

I don't have the historical context here, but if the config header is necessary, why not always include it? Seems like we should always try to produce valid configs when going through our APIs.

@goaway
Copy link
Contributor

goaway commented May 19, 2022

Thanks for doing this @alyssawilk! It's actually an item I'd had on my TODO list for a while.

Also question for Lyft, it took me ages to figure out that my generated config was invalid because I wasn't prepending the config header. I was going to add comments in engine.h about that, but I notice it's not exposed via engine.h only internal.h which implies it generally shouldn't be used.

Can y'all think of a clarifying comment I could add to the Engine's generateConfigStr which would make it more clear what one has to do to make it valid?

It's been a while since I've looked at the cc builder, but in general, the builders aren't meant to output a complete valid Envoy config. Rather they're meant to output formatted overrides to values initially defined in the config_header, all of which will eventually be prepended to the config_template. They still currently have access to the template, because there's a few cases where anchors/overrides don't yet suffice, and so some limited interpolation is still done.

The only thing that should need to be done to produce a complete valid config is concatenating the config_header with the config_template. (This bypasses the means by which any EngineBuilder configuration is applied.) The header is not exposed because it's actually very large, and in normal use one should never actually need to touch it. This test case, however, is perhaps a bit special, since you're trying to independently produce a complete valid config.

I don't have the historical context here, but if the config header is necessary, why not always include it? Seems like we should always try to produce valid configs when going through our APIs.

The config_header is added as part of the startup process when preparing to run the main thread. It's a last step because the different pieces of config can be very large at that point and we want to avoid unnecessary data copying/manipulation. But in the sense of running EM in the typical fashion, it is always included.

My suggestion for this would be to not use the cc Platform::EngineBuilder at all, and instead simply concatenate the config_header and config_template.

@alyssawilk
Copy link
Contributor Author

@goaway would love to touch base with you today but I think I want to use the builder because as I understand it it's the place we'd mimic java and kotlin APIs for changing config? I want to be able to do things like enable http/3 not through explicit config modifiers in the test, but through the same mechanisms we use elsewhere and I thought builder was the way to do that (though I could easily misunderstand)

@alyssawilk alyssawilk assigned goaway and unassigned jpsim May 19, 2022
@alyssawilk
Copy link
Contributor Author

for example one of the upcoming tasks I've got is to add optional brotli support. I think I want the builder for that.

@alyssawilk alyssawilk mentioned this pull request May 19, 2022
Closed
@alyssawilk
Copy link
Contributor Author

ok, filed #2303 and I think this is otherwise good to go?

@alyssawilk alyssawilk merged commit b62165b into envoyproxy:main May 20, 2022
jpsim added a commit that referenced this pull request May 26, 2022
@jpsim
Merge remote-tracking branch 'origin/main' into jp-real-engine-handles
24cb769 
* origin/main: (32 commits)
  Compress xcframework release asset (#2324)
  bazel: update rules_apple (#2326)
  Merge `android_dist` with `android_dist_ci` (#2323)
  kotlin: fix flaky receive error test (#2317)
  kotlin: fix flaky grpc test (#2316)
  build(deps): bump pyjwt from 2.1.0 to 2.4.0 in /.github/actions/pr_notifier (#2314)
  test: making C++ integration test more authentic (#2315)
  reverting override to override_request_timeout_by_gateway_timeout (#2296)
  Update Envoy (#2309)
  release: use `CREDENTIALS_GITHUB_RELEASE_DEPLOY_KEY`
  Use `CREDENTIALS_GITHUB_PUSH_TOKEN`
  Push to branch before tagging the release
  Cronvoy: preparation to unittest certificate verification JNI (#2251)
  Corrected typo in documentation for Android building requirements (#2313)
  instrumentation: add timers and warnings to platform callbacks (#2300)
  Bump Lyft Support Rotation (#2310)
  Revert "android: use local addresses as opposed to prefix (#2081)" (#2307)
  Fix release GitHub workflow (#2306)
  mobile: moving the c++ integration test to use default config (#2293)
  config: cleaning up deprecated configs (#2295)
  ...

Signed-off-by: JP Simard <jp@jpsim.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpsim jpsim jpsim left review comments

@goaway goaway goaway approved these changes

Assignees

@goaway goaway

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@alyssawilk @jpsim @goaway