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

Daemon: fix hardcode envoy listener addr #17281

Merged
merged 1 commit into from Oct 4, 2021
Merged

Conversation

rock-andy
Copy link

@rock-andy rock-andy commented Sep 1, 2021

Without this patch, NetworkPolicy with Envoy would failed if
AF_INET6 address family not supported, and Envoy would report
error like:

xdsDetail="Error adding/updating listener(s) cilium-http-egress:14978:
IPv6 addresses are not supported on this machine: [::]:14978\n"
xdsNonce=196 xdsStreamID=1 xdsTypeURL=type.googleapis.com/envoy.config.listener.v3.Listener

Fixes: #17156

Signed-off-by: Zhang Qiang qiangzhang@qiyi.com

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Thanks for contributing!

Fixes: #issue-number

Envoy configuration is fixed to work also when IPv6 is disabled.

@rock-andy rock-andy requested a review from a team as a code owner September 1, 2021 14:46
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 1, 2021
@jrajahalme
Copy link
Member

jrajahalme commented Sep 9, 2021

test-me-please

Job 'Cilium-PR-K8s-1.21-kernel-4.9' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sPolicyTest Basic Test Traffic redirections to proxy Tests proxy visibility interactions with policy lifecycle operations

Failure Output

FAIL: traffic was not redirected to the proxy when it should have been

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.21-kernel-4.9 so I can create a new GitHub issue to track it.

Job 'Cilium-PR-K8s-1.16-net-next' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sPolicyTest Basic Test Traffic redirections to proxy Tests proxy visibility interactions with policy lifecycle operations

Failure Output

FAIL: traffic was not redirected to the proxy when it should have been

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.16-net-next so I can create a new GitHub issue to track it.

Copy link
Member

@jrajahalme jrajahalme left a comment

Choose a reason for hiding this comment

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

Looks good, one question below, thanks!

@@ -414,7 +419,7 @@ func (s *XDSServer) AddMetricsListener(port uint16, wg *completion.WaitGroup) {
Address: &envoy_config_core.Address_SocketAddress{
SocketAddress: &envoy_config_core.SocketAddress{
Protocol: envoy_config_core.SocketAddress_TCP,
Address: "::",
Address: listenerAddr,
Ipv4Compat: 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 suppose having Ipv4Compat: true is harmless when listening on an IPv4 is address?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your time @jrajahalme . I have just checked the related code of envoy about ipv4Compat, and here's the only usage of socket_address.ipv4_compat, that means IPv4Compat only affect for ipv6 connection. Here's the code:

https://github.com/envoyproxy/envoy/blob/main/source/common/network/utility.cc#L137

@jrajahalme jrajahalme added release-note/bug This PR fixes an issue in a previous release of Cilium. area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Sep 9, 2021
@jrajahalme
Copy link
Member

Added a release note to the description.

@aanm
Copy link
Member

aanm commented Sep 15, 2021

/test

@aanm
Copy link
Member

aanm commented Sep 15, 2021

it looks a little bit suspicious the same test failed in 2 different CI runs. Are we sure this isn't a real failure?

Without this patch, NetworkPolicy with Envoy would failed if
AF_INET6 address family not supported, and Envoy would report
error like:

```
xdsDetail="Error adding/updating listener(s) cilium-http-egress:14978:
IPv6 addresses are not supported on this machine: [::]:14978\n"
xdsNonce=196 xdsStreamID=1 xdsTypeURL=type.googleapis.com/envoy.config.listener.v3.Listener
```

Fixes: cilium#17156

Signed-off-by: Zhang Qiang <qiangzhang@qiyi.com>
@aanm
Copy link
Member

aanm commented Oct 2, 2021

/test

@aanm aanm self-requested a review October 2, 2021 12:58
@aanm aanm merged commit b36c93b into cilium:master Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Layer 7 protocol visibility failed with annotate io.cilium.proxy-visibility
3 participants