Skip to content

listener: match rebalancer to listener IP family type#16914

Merged
ggreenway merged 13 commits intoenvoyproxy:mainfrom
jacob-delgado:match-ip-family-outbound-listener
Jul 7, 2021
Merged

listener: match rebalancer to listener IP family type#16914
ggreenway merged 13 commits intoenvoyproxy:mainfrom
jacob-delgado:match-ip-family-outbound-listener

Conversation

@jacob-delgado
Copy link
Copy Markdown
Contributor

@jacob-delgado jacob-delgado commented Jun 9, 2021 •
edited
Loading

Commit Message:
When getting a rebalancer by address and a wild card match is being
used, the first match in the list is returned. However, if there are
listeners with addresses "0.0.0.0" and "::" then the first active
listener found will be used, irrespective of the IP family type. Change
the behavior to always return the listener of the same IP family type as
the rebalancer.

Co-authored-by: yingchun.cai@volunteers.acasi.info

Additional Description: N/A
Risk Level: Medium
Testing: TBD
Docs Changes: TBD
Release Notes: inline
Platform Specific Features: N/A
Runtime guard: envoy.reloadable_features.listener_wildcard_match_ip_family
Fixes #16804

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Jun 9, 2021

Hi @jacob-delgado, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #16914 was opened by jacob-delgado.

see: more, trace.

@jacob-delgado
listener: match rebalancer to listener IP family type
be7b2f6 
Commit Message:
When getting a rebalancer by address and a wild card match is being
used, the first match in the list is returned. However, if there are
listeners with addresses "0.0.0.0" and "::" then the first active
listener found will be used, irrespective of the IP family type. Change
the behavior to always return the listener of the same IP family type as
the rebalancer.

Risk Level: Medium
Testing: TBD
Docs Changes: TBD
Release Notes: inline
Fixes #16804
Runtime guard: envoy.reloadable_features.listener_wildcard_match_ip_family

Co-authored-by: yingchun.cai@volunteers.acasi.info
Signed-off-by: Jacob Delgado <jacob.delgado@volunteers.acasi.info>
@jacob-delgado
Copy link
Copy Markdown
Contributor Author

jacob-delgado commented Jun 9, 2021

cc @lambdai Please review.

I'm also looking for places to add a test, either in unit test or integration test, but could not find a suitable test to modify. Are you aware of any that I can modify or will I have to write a new one? Any guidance would be helpful.

It's also unclear where I need to modify docs, if at all.

@jacob-delgado
Fix formatting
1d7bf67 
Signed-off-by: Jacob Delgado <jacob.delgado@volunteers.acasi.info>
@jacob-delgado
capture result of std::find_if
9e22bbd 
Signed-off-by: Jacob Delgado <jacob.delgado@volunteers.acasi.info>
soulxu
soulxu reviewed Jun 10, 2021
View reviewed changes
lambdai
lambdai reviewed Jun 10, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@lambdai lambdai left a comment

Choose a reason for hiding this comment

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

LGTM!

Can you add the test? When the test env declares IPv6, it's actually ipv4+ipv6 dual stack, or ipv6 only.

You can add a test case that works for both cases and gain more eyes watching it.

@jacob-delgado
Code review comments
6f5436a 
Signed-off-by: Jacob Delgado <jacob.delgado@volunteers.acasi.info>
@jacob-delgado
Add a new environment variable
693624f 
...to support dual stack listeners when running integration tests.

When ENVOY_IP_FAMILY_TEST is set to true then listeners are not changed
when usting 0.0.0.0 or :: to be of the IP type that is under test.

Signed-off-by: Jacob Delgado <jacob.delgado@volunteers.acasi.info>
@jacob-delgado
Add a new environment variable
3a0f8fd 
...to support dual stack listeners when running integration tests.

When ENVOY_IP_FAMILY_TEST is set to true then listeners are not changed
when usting 0.0.0.0 or :: to be of the IP type that is under test.

Signed-off-by: Jacob Delgado <jacob.delgado@volunteers.acasi.info>
@jacob-delgado
Copy link
Copy Markdown
Contributor Author

jacob-delgado commented Jun 14, 2021

LGTM!

Can you add the test? When the test env declares IPv6, it's actually ipv4+ipv6 dual stack, or ipv6 only.

You can add a test case that works for both cases and gain more eyes watching it.

So after reaching out to Yuchen he pointed me to listener_lds_integration_test.cc:560 for modification. After some investigation I found that listeners are always modified to be of the same IP familly type. Is this approach appropriate? My plan is to get a fakeUpstream that is listening on both IPv4 and IPv6 and to check that the appropriate upstream received the incoming data. Would that be a good approach for getting this merged?

Will I also need a unit test for this to be approved? I did take a look at connection_handler_impl.cc, but it didn't seem I could easily modify for testing. The BalancedConnectionHandlerOptRef that it gave me back (through some dynamtic casting of handler_ to ConnectionHandlerImpl was difficult to parse and verify I was getting back the correct listener. Looking at some of the unit tests (even for active_tcp_listener) there's a lot of setup/mocking code. But maybe I'm just looking in the wrong places.

@ggreenway
Copy link
Copy Markdown
Member

ggreenway commented Jun 22, 2021

@lambdai can you answer the above questions?

@lambdai
Copy link
Copy Markdown
Contributor

lambdai commented Jun 22, 2021

So after reaching out to Yuchen he pointed me to listener_lds_integration_test.cc:560 for modification. After some investigation I found that listeners are always modified to be of the same IP familly type

Sorry I may give the improper advice since I didn't know the trick here.

It's probably easier to unit test it by creating a ConnectionHandlerImpl, add 1 dispatching listener(virtual) and 2 wildcard listener(ipv4 and ipv6) and accept a MockConnection at dispatching listener. The wildcard listeners doesn't require OS ipv4/ipv6 socket now so you should see fewer obstacles. Search for "ActiveTcpListenerTest, RedirectedRebalancer" for reference. That test case confirmed the balance between 2 thread of the same listener but you may want to confirm the target listener is the correct wildcard listener.

Ping me if you are seeing unexpected challenges.

@jacob-delgado
wip: add integration test for rebalancer
93d9ce4 
Signed-off-by: Jacob Delgado <jacob.delgado@volunteers.acasi.info>
@jacob-delgado
Manually revert to code at 6f5436a
8f29ecc 
Signed-off-by: Jacob Delgado <jacob.delgado@volunteers.acasi.info>
@jacob-delgado
Add unit test
8ec1a83 
Signed-off-by: Jacob Delgado <jacob.delgado@volunteers.acasi.info>
@jacob-delgado
Merge branch 'match-ip-family-outbound-listener' of github.com:jacob-…
62435e5 
…delgado/envoy into match-ip-family-outbound-listener

Signed-off-by: Jacob Delgado <jacob.delgado@volunteers.acasi.info>
@jacob-delgado
Remove integration framework modifications
ee81915 
Signed-off-by: Jacob Delgado <jacob.delgado@volunteers.acasi.info>
jacob-delgado
jacob-delgado commented Jun 24, 2021
View reviewed changes
lambdai
lambdai previously approved these changes Jul 1, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@lambdai lambdai left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

test/server/connection_handler_test.cc Outdated
listener_callbacks1->onAccept(Network::ConnectionSocketPtr{accepted_socket});
EXPECT_EQ(1UL, handler_->numConnections());

// metrics aren't sufficient for testing; need a better way to identify the socket
Copy link
Copy Markdown
Contributor

@lambdai lambdai Jul 1, 2021

Choose a reason for hiding this comment

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

If you like, you can create dedicate FilterChainManager for ipv6_any_listener and ipv4_any_listener
You will expect manager_ of ipv6_any_listener be called with findFilterChain

@jacob-delgado
Fix unit test
f834f73 
Signed-off-by: Jacob Delgado <jacob.delgado@volunteers.acasi.info>
@jacob-delgado
Remove unnecessary comment
7845d2b 
Signed-off-by: Jacob Delgado <jacob.delgado@volunteers.acasi.info>
@jacob-delgado jacob-delgado requested a review from lambdai July 7, 2021 15:38
@jacob-delgado
Copy link
Copy Markdown
Contributor Author

jacob-delgado commented Jul 7, 2021

cc @lambdai @ggreenway Yuchen had approved it, but I had to make some changes for coverage and a few other changes to make sure it's working as intended. Looking for a final review.

@ggreenway
Copy link
Copy Markdown
Member

ggreenway commented Jul 7, 2021

Looking for a final review.

I'll aim to review today.

ggreenway
ggreenway approved these changes Jul 7, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

LGTM

@ggreenway ggreenway merged commit 19e7879 into envoyproxy:main Jul 7, 2021
baojr added a commit to baojr/envoy that referenced this pull request Jul 7, 2021
@baojr
Merge branch 'main' into baojr/grpc-reverse-bridge-stream
2a09e60 
* main:
  listener: match rebalancer to listener IP family type (envoyproxy#16914)
  jwt_authn: implementation of www-authenticate header (envoyproxy#16216)
  listener: reset the file event in framework instead of listener filter doing itself (envoyproxy#17227)
  Small typo fix (envoyproxy#17247)
  Doc: Clarify request/response attributes are http-only (envoyproxy#17204)
  bazel/README.md: add aspell comment (envoyproxy#17072)
  docs: Fix broken URL links in HTTP upgrades doc (envoyproxy#17225)
  remove the wrong comment on test (envoyproxy#17233)
  upstream: allow clusters to skip waiting on warmup for initialization (envoyproxy#17179)
  JwtAuthn: support completing padding on forward jwt payload header (envoyproxy#16752)
  remove support for v2 UNSUPPORTED_REST_LEGACY (envoyproxy#16968)
  metrics service: fix wrong argument arrange on MetricsServiceSink (envoyproxy#17127)
  deps: update cel-cpp to 0.6.1 (envoyproxy#16293)
  Add ability to filter ConfigDump. (envoyproxy#16774)
  examples: fix Wasm example. (envoyproxy#17218)
  upstream: update host's socket factory when metadata is updated. (envoyproxy#16708)

Signed-off-by: Garrett Bourg <bourg@squareup.com>
leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
@jacob-delgado
listener: match rebalancer to listener IP family type (envoyproxy#16914)
2dda525 
When getting a rebalancer by address and a wild card match is being
used, the first match in the list is returned. However, if there are
listeners with addresses "0.0.0.0" and "::" then the first active
listener found will be used, irrespective of the IP family type. Change
the behavior to always return the listener of the same IP family type as
the rebalancer.

Fixes envoyproxy#16804

Co-authored-by: yingchun.cai@volunteers.acasi.info
Signed-off-by: Jacob Delgado <jacob.delgado@volunteers.acasi.info>
@soulxu soulxu mentioned this pull request Dec 30, 2021
Merged
@alyssawilk alyssawilk mentioned this pull request Jan 12, 2022
Closed
@zhlsunshine zhlsunshine mentioned this pull request Jul 12, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ggreenway ggreenway ggreenway approved these changes

@lambdai lambdai Awaiting requested review from lambdai

+1 more reviewer

@soulxu soulxu soulxu left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@lambdai lambdai

@ggreenway ggreenway

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Matching IP Family for Outbound Listener

4 participants

@jacob-delgado @ggreenway @lambdai @soulxu