Skip to content

Copy configs into example images#12256

Merged
mattklein123 merged 18 commits intoenvoyproxy:masterfrom
phlax:copy-configs-into-example-images
Aug 3, 2020
Merged

Copy configs into example images#12256
mattklein123 merged 18 commits intoenvoyproxy:masterfrom
phlax:copy-configs-into-example-images

Conversation

@phlax
Copy link
Member

@phlax phlax commented Jul 23, 2020

Commit Message:
Copy configs into example docker images and fix remaining permission issues
Additional Description:

Since the envoy docker image no longer runs as root, a strict umask setting in a development environment can prevent the examples from working.

There were also some remaining permissions issues in the examples that are dealt with here.

There are some docs that are out of date or that can/should be updated - i will update these shortly

There is a test runner that I built for tracking down and testing the documented commands. It can be seen here:

https://github.com/phlax/envoy-examples

And the output from a successful test run using this PR can be seen here:

https://travis-ci.org/github/phlax/envoy-examples/builds/711188378

The grpc-bridge example didnt work at all - so i separated that to a different issue #12231

Edit: the jaeger-native-tracing example segfaults - but not as a result of this PR` see #12287

Update: binary for jaeger-native-tracing is included and the example works

Update: fixes for the grpc-bridge example are now included, but i'll leave docs updates to a separate PR

Risk Level: low
Testing: see above
Docs Changes: tba
Release Notes: n/a
[Optional Runtime guard:]
[Optional Fixes #Issue] Touch #12112 Fixes #12287
[Optional Deprecated:]

phlax added 10 commits July 22, 2020 17:52
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
@dio
Copy link
Member

dio commented Jul 24, 2020

@phlax I think we can run the run examples project as GitHub Actions workflow in this main repo or a separate repo in this org. @lizan WDYT?

@phlax
Copy link
Member Author

phlax commented Jul 24, 2020

@dio yep im just cleaning up the code now, but i think its a good idea. The only reservation is that its a 10 minutes test run, so not sure if we want it to trigger on every change - perhaps only if docs or examples change

@lizan
Copy link
Member

lizan commented Jul 24, 2020

@dio we used to run ci/verify_examples.sh so if we can bring that back it would be great.

dio
dio previously approved these changes Jul 25, 2020
Copy link
Member

@dio dio left a comment

Choose a reason for hiding this comment

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

Thanks. Let's plan for running the examples-verification in subsequent PR.

@dio
Copy link
Member

dio commented Jul 25, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

and copy configs into example image

Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax
Copy link
Member Author

phlax commented Jul 25, 2020

@dio @lizan i updated the jaeger-native-tracing example to include the binary downloaded from here:

https://github.com/tetratelabs/getenvoy-package/files/3518103/getenvoy-centos-jaegertracing-plugin.tar.gz

This seems to fix the problem and the example now works. Not sure if just including the binary is the best way.

Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax
Copy link
Member Author

phlax commented Jul 26, 2020

@dio i fixed the grpc-bridge example. The docs still need updating, but if its ok, ill update docs in a separate PR

Copy link
Member

@dio dio left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

@dio dio left a comment

Choose a reason for hiding this comment

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

Wait, sorry, missed the jaeger case.

Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax
Copy link
Member Author

phlax commented Jul 28, 2020

@dio PR updated with jaeger-native-tracing bin curled

latest test can be seen here:

https://travis-ci.org/github/phlax/envoy-examples/builds/712510401

@phlax phlax requested a review from dio July 30, 2020 17:29
dio
dio previously approved these changes Jul 30, 2020
Copy link
Member

@dio dio left a comment

Choose a reason for hiding this comment

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

Thanks!

lizan
lizan previously approved these changes Jul 30, 2020
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks so much for making our examples better. One comment. Thank you!

/wait

Comment on lines +118 to +125
layered_runtime:
layers:
- name: static_layer
static_layer:
envoy.deprecated_features:envoy.api.v2.route.CorsPolicy.allow_origin: true
envoy.deprecated_features:envoy.api.v2.route.Route.per_filter_config: true
envoy.deprecated_features:envoy.api.v2.route.VirtualHost.per_filter_config: true
envoy.deprecated_features:envoy.type.matcher.StringMatcher.regex: true
Copy link
Member

Choose a reason for hiding this comment

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

I think these configs are missing from https://github.com/envoyproxy/envoy/blob/master/examples/BUILD. Can we fix the configs? This will have to be updated also https://github.com/envoyproxy/envoy/blob/master/test/config_test/example_configs_test.cc.

Bonus points for figuring out how to automatically make sure new configs get added for testing. This is a consistent problem that we have. Thank you!

Copy link
Member Author

Choose a reason for hiding this comment

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

@mattklein123 i found the problem using this test runner

https://github.com/phlax/envoy-examples

weve been talking about how/if we could land it in envoy repo - i was waiting to land this first before thinking about it

Copy link
Member

Choose a reason for hiding this comment

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

OK sounds good. In the interim can you fix the configs and add to the config tests? Thank you.

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure if it would catch these configs tho

Copy link
Member Author

Choose a reason for hiding this comment

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

I think these configs are missing from https://github.com/envoyproxy/envoy/blob/master/examples/BUILD. Can we fix the configs?

I updated the BUILD script with all of the configs with yaml suffix.

Not entirely clear if this is correct, but also noticing that there is at least one non-yaml config listed, so there may be more

This will have to be updated also https://github.com/envoyproxy/envoy/blob/master/test/config_test/example_configs_test.cc.

Im afraid its not immediately obvious to me what needs to change here

Copy link
Member

Choose a reason for hiding this comment

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

You need to:

  1. Add the missing configs to the build config
  2. Fix the configs
  3. Update the test to account for the new number of configs (you will see the test will fail).

Thank you!

Copy link
Member Author

Choose a reason for hiding this comment

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

@mattklein123

i have fixed the deprecation issues in the example configs and updated the necessary test files

I wasnt able to include the jaeger-native config as it fails with

2020-08-01T17:22:05.6865108Z test/config_test/config_test.cc:134: Failure
2020-08-01T17:22:05.6865445Z Failed
2020-08-01T17:22:05.6866695Z '/b/f/w/_tmp/163cb06a4250265abc95a57e525238d7/test/config_test/jaeger-native-tracing_service1-envoy-jaeger.yaml' config failed. Error: opentracing: failed to load dynamic library: /usr/local/lib/libjaegertracing_plugin.so: cannot open shared object file: No such file or directory

im not sure how to mock/expect the files absence. Im happy to update if i know how.

i also updated the test runner i have tor the examples to test both the cors and csrf examples more thoroughly, and ensure i wasnt breaking anything.

regarding keeping the BUILD script up-to-date, we could do something like

cd examples
configs="$(find . -name "*.yaml" -o -name "*.lua" | grep -v docker-compose | cut  -d/ -f2-)"
for config in $configs; do
    grep "\"$config\"" BUILD || exit 1
done

it would fail on this PR because the jaeger configs arent included

Copy link
Member

Choose a reason for hiding this comment

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

Awesome thanks a ton. Agreed we can do something like the script you recommend as a follow up? (Perhaps with an exclusion list for configs we know won't work.)

Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax phlax dismissed stale reviews from lizan and dio via ac45889 July 31, 2020 18:37
phlax added 4 commits August 1, 2020 17:18
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
- erroneously added compose config removed
- jaeger native configs removed
  - these configs fail the tests because of missing binary

Signed-off-by: Ryan Northey <ryan@synca.io>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thank you!!!

@mattklein123 mattklein123 merged commit eec0e7e into envoyproxy:master Aug 3, 2020
chaoqin-li1123 pushed a commit to chaoqin-li1123/envoy that referenced this pull request Aug 7, 2020
Signed-off-by: Ryan Northey <ryan@synca.io>
chaoqin-li1123 pushed a commit to chaoqin-li1123/envoy that referenced this pull request Aug 7, 2020
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: chaoqinli <chaoqinli@google.com>
lizan pushed a commit that referenced this pull request Sep 8, 2020
This PR incorporates the tests that i created for the envoy examples (originally here: https://github.com/phlax/envoy-examples)

It also adds a test to ensure that example configs have been added to the examples `BUILD` config (ref: #12256 (comment))

Signed-off-by: Ryan Northey <ryan@synca.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Jaeger native tracing example segfaults

4 participants