-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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.
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
Add CI to verify that envoy builds and works correctly when most extensions are disabled. #12314
Comments
I wonder if we can fix this with visibility rules rather than a separate build. Maybe some envoy extension macros which default to only having visibility to other extensions? |
Can we make the list of extensions that are OK to use in integration tests be more visible? It's valuable to use "real" extensions in xDS integrations since the interactions are more complex. |
I think the goal is to extend filters test/integration/filters/ as we need to test specific logic. I think we could have a test filter with config as complex as we need, to avoid the dependency issue, WDYT? |
That would work if you want to test things in isolation. But then there are issues with 2 components (a filter and some xDS). For example, we found multiple issues with Wasm + LDS once we put it into a black box that were completely missed by integration tests (mostly around stats). Perhaps, we need several integration tests, one per component, and then some that exercise cross-component interactions |
Without some kind of enforcement, it will eventually become impossible to disable most extensions and the extension mechanism will diminish in value. There is currently an expectation that you can disable extensions. There seem to be some exceptions, this is apparently not the first time that envoy core ends up depending on specific extensions. |
I agree some extensions will want their own DS tests, but if it's for testing extension functionality (rather than ds functionality) I think it goes in the extension directory. So core ds reloads can be tested with synthetic filters in //test/integration/ and extension-specific reloads can be tested along with the extension. make sense? |
Yes, I'd fine if we add more policy enforcement to make sure real extensions are not used in integration tests. I was simply not aware. |
yeah, that's on us for not documenting more clearly. I mainly know about it because @yanavlasov has been cleaning it up :-) |
All of the exceptions that I know of are for tests (other than possibly some of the transport sockets). We should definitely clean this up where possible. |
Unfortunately, it's not as clean as that. I'm putting together a visibility PR which at least makes it more clear what's used and what's not, and makes it harder for folks to accidentally add new "core" tests depending on extensions. I think I'm going to close this off as a dup of #9953 which does include "prevent" and tag my build rules change there. |
While trying to import PR #11826 into our environment which disables most extensions, I noticed a direct dependency between //test/integration:extension_discovery_integration_test and //source/extensions/filters/http/rbac:config. This direct dependency puts us in an difficult situation: either we bring in RBAC despite we not depending on it, or lose coverage for the code added in PR #11826
It seems like CI does not verify that envoy core does not develop unexpected hard dependencies on extensions. It is also not clear which subset of the extensions are considered "essential" to envoy core and without which we should not expect tests to pass. Would it be possible to add a CI that verifies that envoy builds and tests pass when minimal extensions are enabled?
cc: @kyessenov @yanavlasov
The text was updated successfully, but these errors were encountered: