Skip to content

Conversation

tgraf
Copy link
Member

@tgraf tgraf commented Aug 8, 2019

Due to adding the cilium_host IP address as scope link, the IP was skipped in:

func (a *addressFamilyIPv4) LocalAddresses() ([]net.IP, error) {

This further lead for func (d *Daemon) syncEndpointsAndHostIPs() to not add
the host IP to the lxcmap and ipcache. In fact, due to not being in the list
but previously being added to the lxcmap. The IP even got removed from the
lxcmap.

Depending on the configuration of Cilium, it was possible for this IP to be
added to the ipcache via other means but it was no longer guaranteed.

As soon as policy was involved, the IP was mapped to world and no host and thus
subject to drops as well.

The most obvious side effect of this was that host <-> pod communication could
be affected which often manifested itself in failing readiness probes.

Fixes: #8781
Fixes: #8776
Fixes: #8775

Signed-off-by: Martynas Pumputis m@lambda.lt
Signed-off-by: Thomas Graf thomas@cilium.io


This change is Reviewable

@tgraf tgraf requested a review from a team August 8, 2019 12:16
@tgraf
Copy link
Member Author

tgraf commented Aug 8, 2019

test-me-please

EDIT:

The following test failed:
K8sHealthTest checks cilium-health status between nodes [It]

None of the tests which this commit fixes failed.

Rerunning with real fix that can be merged.

@tgraf tgraf mentioned this pull request Aug 8, 2019
@coveralls
Copy link

coveralls commented Aug 8, 2019

Coverage Status

Coverage decreased (-0.005%) to 44.184% when pulling 53feecf on pr/tgraf/log-exclude-ip into 97fdb19 on master.

Due to adding the cilium_host IP address as scope link, the IP was skipped in:
```
func (a *addressFamilyIPv4) LocalAddresses() ([]net.IP, error) {
```

This further lead for `func (d *Daemon) syncEndpointsAndHostIPs()` to not add
the host IP to the lxcmap and ipcache. In fact, due to not being in the list
but previously being added to the lxcmap. The IP even got removed from the
lxcmap.

Depending on the configuration of Cilium, it was possible for this IP to be
added to the ipcache via other means but it was no longer guaranteed.

As soon as policy was involved, the IP was mapped to world and no host and thus
subject to drops as well.

The most obvious side effect of this was that host <-> pod communication could
be affected which often manifested itself in failing readiness probes.

Fixes: #8781
Fixes: #8776
Fixes: #8775

Signed-off-by: Martynas Pumputis <m@lambda.lt>
Signed-off-by: Thomas Graf <thomas@cilium.io>
@tgraf tgraf force-pushed the pr/tgraf/log-exclude-ip branch from e77f2c6 to 53feecf Compare August 8, 2019 14:40
@tgraf tgraf changed the title bpf: Do not add cilium_host IP as link scope address datapath: Always include IP of cilium_host in list of local IPs Aug 8, 2019
@tgraf
Copy link
Member Author

tgraf commented Aug 8, 2019

test-me-please

@tgraf tgraf added needs-backport/1.6 priority/high This is considered vital to an upcoming release. labels Aug 8, 2019
Copy link
Member

@borkmann borkmann left a comment

Choose a reason for hiding this comment

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

LGTM

@ianvernon ianvernon merged commit 234e851 into master Aug 8, 2019
@ianvernon ianvernon deleted the pr/tgraf/log-exclude-ip branch August 8, 2019 16:24
ianvernon pushed a commit that referenced this pull request Aug 8, 2019
This reverts commit 9721661.

PR #8839 revealed the cause of the Istio test failing; re-enable the test
accordingly by reverting the commit which disabled it.

Signed-off by: Ian Vernon <ian@cilium.io>
tgraf pushed a commit that referenced this pull request Aug 8, 2019
This reverts commit 9721661.

PR #8839 revealed the cause of the Istio test failing; re-enable the test
accordingly by reverting the commit which disabled it.

Signed-off by: Ian Vernon <ian@cilium.io>
@nebril nebril mentioned this pull request Aug 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/high This is considered vital to an upcoming release.
Projects
None yet
5 participants