Isolate Secure VirtualHosts On Separate FilterChains (Part 2) #6622
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
|
Issues linked to changelog: |
github-actions
bot
added
the
keep pr updated
sam-heilbron
force-pushed
the
gateway/virtual-service-isolation-part-2
branch
from
0d46ca3
to
1409fc8
Compare
sam-heilbron
commented
Jun 24, 2022
gunnar-solo
reviewed
Jun 24, 2022
|
Visit the preview URL for this PR (updated for commit 42a3155): https://gloo-edge--pr6622-gateway-virtual-serv-7tctsgnl.web.app (expires Fri, 01 Jul 2022 21:19:57 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
sam-heilbron
commented
Jun 24, 2022
| @@ -145,6 +158,24 @@ var _ = Describe("ReconcileGatewayProxies", func() { | |||
| Expect(vhosts).To(HaveLen(1)) | |||
| Expect(vhosts).To(ContainName(translator.VirtualHostName(goodVs))) | |||
| }) | |||
|
|
|||
| It("only creates the valid virtual hosts (using IsolateVirtualHosts Feature)", func() { | |||
There was a problem hiding this comment.
These tests mirror the test above it. But we generate the proxy using our new feature flag, to generate a different type of listener (aggregate). The test then verifies that the listener has the same features as expected. If you modify the actual proxy_reconciler code taht I added, these tests fail (which is good!)
…ub.com/solo-io/gloo into gateway/virtual-service-isolation-part-2
…ub.com/solo-io/gloo into gateway/virtual-service-isolation-part-2
|
Another instance where the final test (helm regression test) actually completed and passed, but github never signaled that the job was complete |
sam-heilbron
added a commit
that referenced
this pull request
Jun 26, 2022
* workflow changes * first pass * fix hybrid translator to not modify resources, and instead tests expect output config * update translator tests to match new behavior * fix validation report test * move merge code into shared file * cleanup github action changes * add e2e test for hybrid gateway * unfocus test * fix comment * attempt to move shared inheritance logic to separate functions that do 1 thing * add support for delegated gateways * selector supports ssl matching * git checkout master .github/workflows --> back out changes that are done in separate PR * fix tests to support only selecting gateways wiwth ssl that matches gateway * support aggregate listener in proxy reconciler * add tests for proxy reconciler * codegen * unfocus test Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>
8 tasks
soloio-bulldozer bot
added a commit
that referenced
this pull request
Jun 26, 2022
* Clarify Gateway Translation, xDS and e2e tests (#6504) * Part 1: introduce predicate, improve some comments around translation * more comments around translation * add comments around testing * small cleanup * function cleanup * cleanup mock function * pass writeNamesapce properly * pass writeNamesapce properly * gofmt * update readme * Apply suggestions from code review Co-authored-by: Bernie Birnbaum <bewebi@earthlink.net> * Update projects/gloo/pkg/xds/README.md Co-authored-by: Jacob Cukjati <jakecukjati@gmail.com> * codegen Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com> Co-authored-by: Bernie Birnbaum <bewebi@earthlink.net> Co-authored-by: Jacob Cukjati <jakecukjati@gmail.com> * Isolate Secure VirtualHosts On Separate FilterChains (#6558) * Introduce AggregateListener to the Listener API * generated-code * add listener report, initial handilng of aggregate listener * initialize aggregate listener report * codegen * beging to support httpTranslator for isolation virtual hosts * continue with http translator * add setting * inject setting into translator * move code to ensure new translation is only run if opted into, and to reuse virtualservice translation * add test for translator branching logic for new aggregate translator * add tests for listener subsystem * begin report assertion tests * more report processing tests * begin e2e tests * Add HttpGateway support for AggregateTranslator, e2e tests passing * cleanup comment * fix test for unsupported feature * codegen * convert setup/cleanup into utility so other tests can benefit * support per gw annotation, with tests * update comments in proxy api * codegen * upgrade solo-kit and protoc-gen-openapi deps * configure new codegen settings for validation schema * run codegen to descrease proxy CRD size * fix import * fix comparison between string and type * fix comment * support proxy validation * cleanup terminology around HttpFilter * clarify todo * add changelog * add kube utility * fix port overlap in SimpleGlooSnapshot * codegen * Adding changelog file to new location * Deleting changelog file from old location * update deps and changelog * Adding changelog file to new location * Deleting changelog file from old location Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com> Co-authored-by: changelog-bot <changelog-bot> * delete clientset - unused * Isolate Secure VirtualHosts On Separate FilterChains (Part 2) (#6622) * workflow changes * first pass * fix hybrid translator to not modify resources, and instead tests expect output config * update translator tests to match new behavior * fix validation report test * move merge code into shared file * cleanup github action changes * add e2e test for hybrid gateway * unfocus test * fix comment * attempt to move shared inheritance logic to separate functions that do 1 thing * add support for delegated gateways * selector supports ssl matching * git checkout master .github/workflows --> back out changes that are done in separate PR * fix tests to support only selecting gateways wiwth ssl that matches gateway * support aggregate listener in proxy reconciler * add tests for proxy reconciler * codegen * unfocus test Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com> * move changelog * process setting at setup time * more fixes backported from gloo/gateway merge code that i had missed during backport * fix args * instantte http gateway client * initialize gateway test client with new setting property * include httpgateways in ingress cluster role (already supported in gateway Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com> Co-authored-by: Bernie Birnbaum <bewebi@earthlink.net> Co-authored-by: Jacob Cukjati <jakecukjati@gmail.com>
8 tasks
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.
Description
Allow VirtualHosts with distinct SslConfig to be isolated from one another by exposing them on different FilterChains.
Remaining Work
Before Merge
After Merge
Solution
Builds off of work that was introduced in #6558
Hybrid Translator
There are some slight changes to how the HybridTranslator is implemented, but the functionality has remained the same.
Previously, we would reconcile (merge) sslConfigurations from the VirtualService and Gateway, and then apply the merged config back on the original resource, modifying it in place. I didn't write a test to verify this, but my concern was that by modifying the original Gateway resource, we would be affecting translation. I restructured the code and split off functions, since I would need these functions in my new functionality. I also ensured that the functions return instead of modifying.
Due to this change, I had to update the tests that validated this merge logic. The tests compared an expected gateway with what resulted after translation. Since we are no longer modifying the gateway in place, I updated the test to use the outputted listener and compare the fields there. The values were the same.
Aggregate Listener
Wrote a bunch of code for this, but it's largely copy/reuse/paste from the Hybrid Translator
Aggregate Listener Builder
An aggregate listener attempts to reduce space by storing a single copy of a resource, and then references whenever it is used in a filter chain. This means that the process of indexing the resource can be verbose and make the code harder to read. Also, since some code requires performing this action multiple times, I added a utility builder that just aggregates the state. It allows us to keep the intention of the code clearer (I think) and reduces the amount of variables we need to maintain/return.
Proxy Reconciler
Added support for the new listener type and added corresponding tests
Gateway Selector
Previously, it was possible to have a Gateway CR without SSL, select a MatchableHttpGateway with SSL. Now it's no longer possible. A few tests needed to be updated to reflect this.
Envoy End to End Tests
I added test cases to demonstrate the same behavior that we previously had demonstrated for HttpGateways, but this time for:
Checklist:
make -B install-go-tools generated-codeto ensure there will be no code diff