Skip to content
This repository was archived by the owner on Oct 20, 2025. It is now read-only.

Don't leak old tunnel addresses#430

Merged
caseydavenport merged 2 commits into
projectcalico:masterfrom
caseydavenport:dont-leak-old-addresses
Mar 12, 2020
Merged

Don't leak old tunnel addresses#430
caseydavenport merged 2 commits into
projectcalico:masterfrom
caseydavenport:dont-leak-old-addresses

Conversation

@caseydavenport

@caseydavenport caseydavenport commented Feb 29, 2020

Copy link
Copy Markdown
Member

Spotted a bug where we'll leak addresses that don't have any allocation
attributes. Turns out we used to allocate all tunnel addresses without
attributes, and so tunnel addresses allocated prior to v3.6 will get
leaked once Calico is upgraded to v3.8+.

This PR aims to fix the leak. Subsequent PRs will try to clean up the
leaked addresses, and fix another potential leak spotted in code
reading.

There are a few cases we need to consider:

The node has an address in its spec, but the address doesn't exist in IPAM.

The current code handles this already simply by assigning a new address and updating the node.

The node has an address in its spec, the address exists in IPAM but has no attributes

This is the case that this PR is primarily trying to fix. Currently, we assign a new address and leak the old. In this PR, we will attempt to "correct" the allocation in IPAM to have attributes, and if that fails we'll just release it and assign a new one. We will only release it if we can see for sure the allocation has no handle. If it had a handle, it would suggest this is actually a WEP address.

The node has an address in its spec, the address exists and has attributes.

The current code handles this case already

The node has no address in its spec, the address exists in IPAM but has no attributes

In this case, the current code will simply allocate a new address and leak the old one. Since the old address has no attributes / handle, we have no way of identifying it and thus cannot clean it up.

A potential future enhancement would be to have kube-controllers detect addresses which have no handle and no attributes and garbage collect them.

The node has no address in its spec, the address exists in IPAM and has attributes

In this case, the current code will simply allocate a new address and leak the old one. However, the address does have a handle associated with it, so we can determine if we're hitting this scenario. I propose we do a defensive "ReleaseByHandle" if we detect that the node has no address in order to cover this case.

The node has no address in its spec, the address does not exist in IPAM.

The current code handles this case already.

  • Tests
  • Documentation
  • Release note
Fix potential tunnel address leak when upgrading from Calico < v3.6 to Calico  >= v3.8

Comment thread pkg/allocateip/allocateip.go
Comment thread pkg/allocateip/allocateip.go
@caseydavenport caseydavenport changed the title [WIP] Don't leak old tunnel addresses Don't leak old tunnel addresses Mar 5, 2020
@caseydavenport caseydavenport force-pushed the dont-leak-old-addresses branch from a329202 to a54cc19 Compare March 5, 2020 01:18

@nelljerram nelljerram left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Basically looks good, just a few minor points.

Comment thread pkg/allocateip/allocateip.go Outdated
Comment thread pkg/allocateip/allocateip.go Outdated
Comment thread pkg/allocateip/allocateip_test.go Outdated
Comment thread pkg/allocateip/allocateip_test.go
@caseydavenport caseydavenport force-pushed the dont-leak-old-addresses branch 2 times, most recently from 1108c18 to bdf43b4 Compare March 10, 2020 15:06

@nelljerram nelljerram left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks.

@caseydavenport caseydavenport force-pushed the dont-leak-old-addresses branch 2 times, most recently from f59d90c to ee330c1 Compare March 11, 2020 13:59
@caseydavenport caseydavenport force-pushed the dont-leak-old-addresses branch from ee330c1 to c38da99 Compare March 12, 2020 13:06
@caseydavenport caseydavenport merged commit f04a40b into projectcalico:master Mar 12, 2020
@caseydavenport caseydavenport deleted the dont-leak-old-addresses branch March 12, 2020 15:22
caseydavenport added a commit that referenced this pull request Mar 20, 2020
…#430-origin-release-v3.13

Automated cherry pick of #430 origin release v3.13
caseydavenport added a commit that referenced this pull request Mar 23, 2020
…#430-origin-release-v3.12

Automated cherry pick of #430: Add test which catches IP leak
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants