Skip to content
This repository was archived by the owner on Jun 29, 2022. It is now read-only.

packet: Read BGP peer address from metadata service#1010

Merged
johananl merged 1 commit intomasterfrom
johananl/metallb-gw-address
Oct 8, 2020
Merged

packet: Read BGP peer address from metadata service#1010
johananl merged 1 commit intomasterfrom
johananl/metallb-gw-address

Conversation

@johananl
Copy link
Member

@johananl johananl commented Sep 24, 2020

In some Packet facilities the BGP peer address isn't the same as the gateway address allocated for a host. Rather, it is a loopback address that's reachable via the gateway.

The Packet metadata service now exposes BGP info to hosts, so we can query the metadata service for the BGP peer address.

The source address is explicitly specified since when the peer address is a loopback address, the source IP addresses which ends up getting selected by the kernel is the node's public address which doesn't work. In cases where the peer address is the gateway address there is no harm in explicitly specifying the source.

Fixes #1009.

@johananl johananl requested review from invidian and rata September 24, 2020 18:53
@johananl johananl force-pushed the johananl/metallb-gw-address branch from 87ebbc3 to c9ed8a3 Compare September 24, 2020 18:55
invidian
invidian previously approved these changes Sep 24, 2020
View reviewed changes
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

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

LGTM, I wonder if we should also add tests for it, so we can detect some issues in the future. Thanks for quick reaction @johananl !

@johananl
Copy link
Member Author

johananl commented Sep 24, 2020 •
edited
Loading

LGTM, I wonder if we should also add tests for it, so we can detect some issues in the future. Thanks for quick reaction @johananl !

Ideally such a test would be a "blackbox" ingress test (i.e. sending traffic via MetalLB). However, there doesn't seem to be a trivial way to handle EIP allocation (in combination with multiple parallel CI runs) so we would have to build something for it. Additionally, such a test wouldn't have caught this specific issue because only specific facilities were affected.

I can't think of a unit test which would have caught this problem. If you can, please share your thoughts.

@johananl
Copy link
Member Author

johananl commented Sep 24, 2020

There is a race condition with enabling BGP.

rata
rata previously approved these changes Sep 24, 2020
View reviewed changes
Copy link
Contributor

@rata rata left a comment

Choose a reason for hiding this comment

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

Added two comments about failure scenarios, but if this is needed ASAP we can handle those in a following PR. As you consider it is best.

@johananl johananl dismissed stale reviews from rata and invidian via 7f1d1cd September 24, 2020 19:31
@johananl johananl force-pushed the johananl/metallb-gw-address branch from c9ed8a3 to 7f1d1cd Compare September 24, 2020 19:31
@johananl johananl force-pushed the johananl/metallb-gw-address branch 2 times, most recently from de2f728 to feaa9ab Compare September 24, 2020 20:23
@johananl johananl requested review from invidian and rata September 24, 2020 20:26
@johananl
Copy link
Member Author

johananl commented Sep 24, 2020

There is a race condition with enabling BGP.

Fixed using a retry script.

@johananl johananl mentioned this pull request Sep 24, 2020
Open
@invidian
Copy link
Member

invidian commented Sep 24, 2020

Additionally, such a test wouldn't have caught this specific issue because only specific facilities were affected.

We randomize the locations to get best machines availability, so it would occur in some cases at least.

Ideally such a test would be a "blackbox" ingress test (i.e. sending traffic via MetalLB). However, there doesn't seem to be a trivial way to handle EIP allocation (in combination with multiple parallel CI runs) so we would have to build something for it.

Right... Given that it is not trivial to test, we should merge this to fix issue and perhaps raise the priority of testing it.

invidian
invidian previously approved these changes Sep 24, 2020
View reviewed changes
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

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

Just one thought, otherwise LGTM

@johananl
Copy link
Member Author

johananl commented Sep 24, 2020

Marking as "do not merge" until #1010 (comment) is resolved.

@johananl
Copy link
Member Author

johananl commented Sep 25, 2020 •
edited
Loading

@rata @invidian do you guys think the deployment should fail if BGP is enabled but we couldn't get the peer address? In other words, do we let the kubelet start without the BGP-related labels? Doing so would mean the user will get a working cluster but won't be able to use MetalLB.

@johananl johananl force-pushed the johananl/metallb-gw-address branch from feaa9ab to e26628c Compare September 25, 2020 11:34
@invidian
Copy link
Member

invidian commented Sep 25, 2020

@rata @invidian do you guys think the deployment should fail if BGP is enabled but we couldn't get the peer address? In other words, do we let the kubelet start without the BGP-related labels? Doing so would mean the user will get a working cluster but won't be able to use MetalLB.

I think it should fail, yes. I guess that would assume mis-configured project which has BGP disabled?

@johananl
Copy link
Member Author

johananl commented Sep 25, 2020

@rata @invidian do you guys think the deployment should fail if BGP is enabled but we couldn't get the peer address? In other words, do we let the kubelet start without the BGP-related labels? Doing so would mean the user will get a working cluster but won't be able to use MetalLB.

I think it should fail, yes. I guess that would assume mis-configured project which has BGP disabled?

For example. Could also be other things we haven't foreseen. The current behavior is to fail. I agree it's better to fail loudly than quietly here.

@johananl johananl force-pushed the johananl/metallb-gw-address branch 2 times, most recently from afeaa89 to 61fbe0e Compare September 25, 2020 17:02
@johananl
Copy link
Member Author

johananl commented Sep 25, 2020

I've refactored the script to poll for BGP metadata and write the peer address to /run/metadata/bgp. The kubelet unit now uses this file as an environment file when BGP is enabled.

I've tested a manual deployment with BGP both enabled and disabled and this seems to work well.

@johananl johananl requested review from invidian and rata September 25, 2020 17:46
invidian
invidian previously approved these changes Sep 25, 2020
View reviewed changes
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

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

LGTM

@johananl
Copy link
Member Author

johananl commented Sep 25, 2020

@invidian @rata I would like to merge this ASAP if there are no further concerns since we have a broken state in master right now. Are there any more blockers? I suggest that for non-blockers we open new issues and iterate later.

@johananl johananl force-pushed the johananl/metallb-gw-address branch from 61fbe0e to f483143 Compare September 25, 2020 18:51
iaguis
iaguis previously approved these changes Oct 6, 2020
View reviewed changes
Copy link
Contributor

@iaguis iaguis left a comment

Choose a reason for hiding this comment

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

lgtm

invidian
invidian previously approved these changes Oct 6, 2020
View reviewed changes
@johananl johananl dismissed stale reviews from invidian and iaguis via 4aae616 October 8, 2020 11:26
@johananl johananl force-pushed the johananl/metallb-gw-address branch from f483143 to 4aae616 Compare October 8, 2020 11:26
invidian
invidian suggested changes Oct 8, 2020
View reviewed changes
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

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

Rebase required.

@johananl
Copy link
Member Author

johananl commented Oct 8, 2020

Rebase required.

Thanks.

P.S. GitHub tells me that 😉

@johananl
packet: Read BGP peer address from metadata service
725d6fc 
In some Packet facilities the BGP peer address isn't the same as the
gateway address allocated for a host. Rather, it is a loopback
address that's reachable via the gateway.

The Packet metadata service now exposes BGP info to hosts, so we can
query the metadata service for the BGP peer address.

We currently use the first peer address only since MetalLB doesn't
support multiple node peers yet.

The source address is explicitly specified since when the peer
address is a loopback address, the source IP addresses which ends up
getting selected by the kernel is the node's *public* address which
doesn't work. In cases where the peer address is the gateway address
there is no harm in explicitly specifying the source.

Removing the `/bin/sh -c ""` wrapper in ExecStart because we no
longer need a shell since now we read the peer address from an
environment file.

Removing the TODO about using Afterburn for the peer address since we
no longer use the gateway address as the peer address.

Fixes #1009.
@johananl johananl force-pushed the johananl/metallb-gw-address branch from 4aae616 to 725d6fc Compare October 8, 2020 11:39
@invidian
Copy link
Member

invidian commented Oct 8, 2020

Thanks.

P.S. GitHub tells me that wink

I know, but it's still showing the PR status in the list as "Waiting for review", which is not really correct 😄

@johananl johananl requested a review from invidian October 8, 2020 13:12
@johananl
Copy link
Member Author

johananl commented Oct 8, 2020

Testing looks good. Need another LGTM.

@johananl johananl merged commit e84fed0 into master Oct 8, 2020
@johananl johananl deleted the johananl/metallb-gw-address branch October 8, 2020 13:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@rata rata Awaiting requested review from rata

2 more reviewers

@iaguis iaguis iaguis approved these changes

@invidian invidian invidian approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

components/metallb: Ensure correct discovery of BGP peer address

4 participants

@johananl @invidian @rata @iaguis