Skip to content
This repository has been archived by the owner on Jun 20, 2024. It is now read-only.

npc: Track ipset entries per user and refcount default-allow ipsets #3181

Merged
merged 1 commit into from
Nov 27, 2017

Conversation

brb
Copy link
Contributor

@brb brb commented Nov 21, 2017

There was a bug in handling default-allow ipsets - it could not cope with out of order receive of Pod updates. E.g:

  1. AddPod(Pod_A, PodIP_A),
  2. AddPod(Pod_B, PodIP_A),
  3. DeletePod(Pod_A),

would result with PodIP_A being removed from default-allow ipset, as such ipset entries were not refcounted. Bypassing of refcounting was due to the fact that adding a netpol which dst-selects a Pod which IP addr is in default-allow ipset has to remove the IP addr from the ipset regardless of refcount value of the IP addr entry in the ipset. So, refcounting in such case didn't make sense and it was bypassed.

This commit changes the way entries are refcounted in ipsets, and it makes default-allow ipset the same citizen as other ipsets, so from now default-allow ipset is refcounted.

Fix #3177

There was a bug in handling "default-allow" ipsets - it could not
cope with out of order receive of Pod updates. E.g:

    1) AddPod(Pod_A, PodIP_A),
    2) AddPod(Pod_B, PodIP_A),
    3) DeletePod(Pod_A),

would result with PodIP_A being removed from "default-allow" ipset,
as such ipset entries were not refcounted. Bypassing of refcounting
was due to the fact that adding a netpol which dst-selects a Pod which
IP addr is in "default-allow" ipset has to remove the IP addr from
the ipset regardless of refcount value of the IP addr entry in the
ipset. So, refcounting in such case didn't make sense and it was
bypassed.

This commit changes the way entries are refcounted in ipsets, and
it makes "default-allow" ipset the same citizen as other ipsets, so
from now "default-allow" ipset is refcounted.
@brb brb added this to the 2.1.2 milestone Nov 21, 2017
Copy link
Contributor

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

One question over logging. Otherwise seems ok; I have run this in the environment where I saw #3177 and failed to reproduce the bug. 👍

return nil
}

i.Logger.Printf("added entry %s to %s of %s", entry, ipsetName, user)

This comment was marked as abuse.

This comment was marked as abuse.

return nil
}
return i.DelEntry(ipsetName, entry)
i.Logger.Printf("deleted entry %s from %s of %s", entry, ipsetName, user)

This comment was marked as abuse.

@bboreham bboreham changed the base branch from master to 2.1 November 22, 2017 13:47
@bboreham bboreham merged commit 8e16b03 into 2.1 Nov 27, 2017
@brb brb deleted the issues/3177-ooo branch January 6, 2018 17:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants