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.

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

Closed
antoniovicente opened this issue Jul 28, 2020 · 10 comments

Comments

@antoniovicente
Copy link
Contributor

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

@alyssawilk
Copy link
Contributor

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?
cc @lizan

@kyessenov
Copy link
Contributor

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.

@alyssawilk
Copy link
Contributor

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?

@kyessenov
Copy link
Contributor

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

@antoniovicente
Copy link
Contributor Author

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.

@alyssawilk
Copy link
Contributor

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?

@kyessenov
Copy link
Contributor

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.

@alyssawilk
Copy link
Contributor

yeah, that's on us for not documenting more clearly. I mainly know about it because @yanavlasov has been cleaning it up :-)

@mattklein123
Copy link
Member

There seem to be some exceptions, this is apparently not the first time that envoy core ends up depending on specific extensions.

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.

@alyssawilk
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants