endpoint: sync endpoint IP-SecurityID mapping to kvstore #2875

ianvernon · 2018-02-19T20:56:54Z

Add controllers within the endpoint that synchronize its IPv4 and IPv6 addresses with the key-value store.

Add a watcher for this mapping in the key-value store, and cache it locally with the new ipcache package. When a new entry is added to this local cache, trigger policy updates for endpoints.

Signed-off by: Ian Vernon ian@cilium.io

Fixes: #2552

Add mapping of endpoint IPs to security identities in the key-value store. Watch the key-value store for updates and cache them locally per agent.

ianvernon · 2018-02-19T20:57:04Z

test-me-please

ianvernon · 2018-02-19T21:17:42Z

test-me-please

ianvernon · 2018-02-19T22:34:11Z

test-me-please

ianvernon · 2018-02-20T00:21:37Z

test-me-please

rlenglet · 2018-02-19T23:27:21Z

pkg/endpoint/policy.go

+				}
+				return nil
+			},
+			RunInterval: time.Duration(1) * time.Minute,


1 * time.Minute is more readable. There's no need to convert the constant.

rlenglet · 2018-02-19T23:29:57Z

pkg/policy/identity.go

+// NewIPCache returns a new IPCache with the mappings of endpoint IP to security
+// identity (and vice-versa) initialized.
+// TODO (ianvernon) - populate proxyMutator here / add parameter for it.
+func NewIPCache() *IPCache {


Pass in the xds.ResourceMutator.

rlenglet · 2018-02-19T23:30:35Z

pkg/policy/identity.go

+	identityToIPCache map[NumericIdentity]map[string]struct{}
+	// TODO (ianvernon)
+	proxyMutator        xds.ResourceMutator
+	proxyMutatorTypeURL string


Replace this field with the xds.NetworkPolicyHostsTypeURL constant. That doesn't need to be configurable.

rlenglet · 2018-02-19T23:35:01Z

pkg/policy/identity.go

+	// TODO (ianvernon) - mapping of security identity to set of IPs.
+	identityToIPCache map[NumericIdentity]map[string]struct{}
+	// TODO (ianvernon)
+	proxyMutator        xds.ResourceMutator


proxyMutator -> xdsResourceMutator

rlenglet · 2018-02-19T23:37:52Z

pkg/policy/identity.go

+	// IPIdentityCache caches the mapping of endpoint IPs to their corresponding
+	// security identities across the entire cluster in which this instance of
+	// Cilium is running.
+	IPIdentityCache  = NewIPCache()


Pass in the xds.ResourceMutator, after FIXME is merged:

IPIdentityCache = NewIPCache(xds.NetworkPolicyHostsCache)

rlenglet · 2018-02-20T00:23:35Z

pkg/policy/identity.go

@@ -28,6 +28,11 @@ import (
 	"github.com/cilium/cilium/pkg/logging/logfields"
 	"github.com/cilium/cilium/pkg/u8proto"

+	"encoding/json"
+	// TODO - uncomment this when we actually update the proxyMutator for IP Cache
+	//"github.com/cilium/cilium/pkg/envoy/api"


FYI, that import path will change with #2781.

rlenglet · 2018-02-20T00:34:45Z

pkg/policy/identity.go

+	// IPIdentityCache caches the mapping of endpoint IPs to their corresponding
+	// security identities across the entire cluster in which this instance of
+	// Cilium is running.
+	IPIdentityCache = NewIPCache()


Pass in the cache, which is defined in #2879:

IPIdentityCache = NewIPCache(envoy.NetworkPolicyHostsCache)

rlenglet · 2018-02-20T00:36:08Z

pkg/policy/identity.go

+	"encoding/json"
+	// TODO - uncomment this when we actually update the proxyMutator for IP Cache
+	//"github.com/cilium/cilium/pkg/envoy/api"
+	"github.com/cilium/cilium/pkg/envoy/xds"


You'll also need to import "github.com/cilium/cilium/pkg/envoy" to refer to the cache.

ianvernon · 2018-02-20T02:31:41Z

test-me-please

Factor out code from policy package related to identity allocation, types, etc. into a separate package. This was motivated by cyclic import issues faced in PR #2875. Update code to use this package accordingly. No change in functionality should occur as part of this commit. Signed-off by: Ian Vernon <ian@cilium.io>

ianvernon · 2018-02-20T23:03:44Z

This is blocked by #2890, which was created to resolve cyclic import issues.

Factor out code from policy package related to identity allocation, types, etc. into a separate package. This was motivated by cyclic import issues faced in PR #2875. Update code to use this package accordingly. No change in functionality should occur as part of this commit. Signed-off by: Ian Vernon <ian@cilium.io>

tgraf · 2018-03-10T11:19:53Z

pkg/endpoint/policy.go

+				return nil
+			},
+			StopFunc: func() error {
+				if err := kvstore.Delete(ipKey); err != nil {


Not related to your PR directly but StopFunc() is not being retried. Your code is an example of where it would make sense to retry for at least a bit. I'm still a bit undecided as the lease will cause it to be removed if the agent reconnects and creates a new lease.

I thought the lease was created on a per-key basis? I.e., if the entry fails to be deleted, and there is no such endpoint once the agent reconnects, after the lease expires, the key-value pair will get deleted from the key-value store. Is this assumption incorrect?

Lease is created per client and attached to all keys which request it. Lease renewal thus applies to all keys of an agent right now.

Wouldn't it follow our controller pattern to then create a new controller on stop to handle the delete? We've been treating the controllers as both periodic functions and long lived "operations". In any case, we can explicitly retry in StopFunc, since it is called by the goroutine dedicated to this controller, can't we?

ianvernon · 2018-03-11T22:36:19Z

@rlenglet can you give a pass at this please to confirm that the XDS cache work is done correctly?

joestringer

common/ changes LGTM. Not sure why bpf-loader or cli owners are being pulled in.

joestringer · 2018-03-12T18:03:35Z

pkg/identity/identity.go

+//
+// WARNING - STABLE API
+// This structure is written as JSON to the key-value store. Do NOT modify this
+// structure in ways which are not JSON forward compatible.


Mind adding field tags for the JSON formatting?

Sorry about that, done.

Should this say" in ways which are not JSON forward and backward compatible"? We would, in any case, roll the version path component for the keys in ipcache when we changed this field, wouldn't we? If we don't, the change would have to be backwards compatible to continue working with old nodes (and forwards compatibility wouldn't matter).
In any case, mentioning that we have version variables that might also need to change might be worthwhile (assuming what I said above is correct).

ianvernon · 2018-03-12T19:42:32Z

test-me-please

raybejjani

nothing blocking :)

raybejjani · 2018-03-13T14:14:06Z

pkg/endpoint/policy.go

+func (e *Endpoint) runIPIdentitySync(endpointIP addressing.CiliumIP) {
+
+	if endpointIP == nil {
+		return


We should probably log a debug that the controller isn't running, at least (although if someone is calling this with nil it would be a little odd).

raybejjani · 2018-03-13T14:23:45Z

pkg/endpoint/policy.go

@@ -923,6 +931,74 @@ func (e *Endpoint) runIdentityToK8sPodSync() {
 	)
 }

+// FormatGlobalEndpointID returns the global ID of endpoint in the format
+// / <global ID Prefix>:<cluster name>:<node name>:<endpoint ID> as a string.
+func (e *Endpoint) FormatGlobalEndpointID() string {


Do we expect to use this again elsewhere? or is this a function because node.GetLocalNode() returns an *node.Node that we need to drop?

raybejjani · 2018-03-13T14:26:02Z

pkg/endpoint/policy.go

+
+				// Release lock as we do not want to have long-lasting key-value
+				// store operations resulting in lock being held for a long time.
+				e.Mutex.RUnlock()


Would it be ok if, at the start of DoFunc, you took the lock, grabbed copies of all the relevant things, and released the lock without any branches? I think it's basically e.state, e.SecurityIdentity.ID and e.ID (via FormatGlobalEndpointID that we need to the lock to read. This is mostly to remove the need to reason about the lock while reading this code.

raybejjani · 2018-03-13T14:30:34Z

pkg/endpoint/policy.go

+				return nil
+			},
+			StopFunc: func() error {
+				if err := kvstore.Delete(ipKey); err != nil {


Wouldn't it follow our controller pattern to then create a new controller on stop to handle the delete? We've been treating the controllers as both periodic functions and long lived "operations". In any case, we can explicitly retry in StopFunc, since it is called by the goroutine dedicated to this controller, can't we?

raybejjani · 2018-03-13T14:44:13Z

pkg/endpoint/policy.go

@@ -913,6 +983,11 @@ func (e *Endpoint) SetIdentity(owner Owner, identity *identityPkg.Identity) {

 	e.runIdentityToK8sPodSync()

+	// Whenever the identity is updated, propagate change to key-value store


A more general question: Does this mean that we rely on the kvstore access controls to stop someone/something messing the IP->Security Identity mapping (and thus, the egress security enforcement)?
More specifically, we rely on the v1 part of IPIdentitiesPath to avoid collisions during upgrades between cilium versions that change this implementation, right? If so, do we need to guard against misconfigurations anywhere? The most likely example I can think of would be multiple clusters using the same IP range and the same etcd.

raybejjani · 2018-03-13T14:46:34Z

pkg/identity/identity.go

+//
+// WARNING - STABLE API
+// This structure is written as JSON to the key-value store. Do NOT modify this
+// structure in ways which are not JSON forward compatible.


Should this say" in ways which are not JSON forward and backward compatible"? We would, in any case, roll the version path component for the keys in ipcache when we changed this field, wouldn't we? If we don't, the change would have to be backwards compatible to continue working with old nodes (and forwards compatibility wouldn't matter).
In any case, mentioning that we have version variables that might also need to change might be worthwhile (assuming what I said above is correct).

raybejjani · 2018-03-13T14:59:54Z

pkg/ipcache/ipcache.go

+// GetIPIdentityMapModel returns all known endpoint IP to security identity mappings
+// stored in the key-value store.
+func GetIPIdentityMapModel() {
+	// TODO (ianvernon) return model of ip to identity mapping. For use in CLI.


I think this was discussed earlier in the PRs life but I'm not sure if I'm missing something. If we have an issue to implement this why do we need the stub? From what I can tell it isn't actually used anywhere.

raybejjani · 2018-03-13T15:26:26Z

pkg/ipcache/ipcache.go

+						ipStrings = append(ipStrings, endpointIP)
+					}
+					sort.Strings(ipStrings)
+					envoy.NetworkPolicyHostsCache.Upsert(envoy.NetworkPolicyHostsTypeURL, ipIDPair.ID.StringID(), &envoyAPI.NetworkPolicyHosts{Policy: uint64(ipIDPair.ID), HostAddresses: ipStrings}, false)


An alternative is to define a callback type in this package, and have others register conforming callbacks. This has the advantage that the dependency direction reflects the flow of data (in that the envoy package knows about ipcache and is using it, so it imports it), instead of a push scheme like we have now. It also allows this attachment to happen in a third package, allowing generic types to be wired together without the type system needing to know.
If we intended to have a global host cache then what I said is less useful, but it still retains the niceties around reflecting which direction data is flowing.

rlenglet · 2018-03-13T21:34:22Z

pkg/endpoint/policy.go

 	"github.com/cilium/cilium/pkg/policy"
 	"github.com/cilium/cilium/pkg/policy/api"

 	"github.com/sirupsen/logrus"
+	"strconv"


Move this above.

rlenglet · 2018-03-13T21:37:18Z

pkg/identity/identity.go

+// This structure is written as JSON to the key-value store. Do NOT modify this
+// structure in ways which are not JSON forward compatible.
+type IPIdentityPair struct {
+	IP       net.IP          `json:"IP"`


We usually start JSON field names with lowercase, instead of using the Go capitalization. Not a big deal though.

rlenglet · 2018-03-13T21:41:00Z

pkg/ipcache/ipcache.go

+
+// IPIdentityMappingOwner is the interface the owner of an identity allocator
+// must implement
+type IPIdentityMappingOwner interface {


Rename ...Owner into ...Observer?

rlenglet · 2018-03-13T21:45:15Z

pkg/ipcache/ipcache.go

+					cacheChanged = true
+
+					// Update XDS Cache as well.
+					ipStrings := make([]string, 0, len(endpointIPs))


Why do you call IPIdentityCache.LookupByIdentity before calling IPIdentityCache.upsert? Aren't you missing the newly added IP?

rlenglet · 2018-03-13T21:47:00Z

pkg/ipcache/ipcache.go

+			case kvstore.EventTypeDelete:
+				if exists {
+					// Delete from XDS Cache as well.
+					envoy.NetworkPolicyHostsCache.Delete(envoy.NetworkPolicyHostsTypeURL, cachedIdentity.StringID(), false)


This is not correct. You only deleted one IP-ID pair. Not the whole identity.
You should delete form this cache only if there are no IPs left associated with an identity.
Otherwise, you must call Upsert with the set resulting from removing the IP in the deleted pair.

ianvernon · 2018-03-13T21:54:56Z

test-me-please

Whenever an endpoint is assigned a new security identity, update the key-value store with a mapping of the endpoint's IP(s) to its newly assigned identity. Add a new cache which caches this information from the key-value store locally. Signed-off by: Ian Vernon <ian@cilium.io>

Signed-off by: Ian Vernon <ian@cilium.io>

ianvernon · 2018-03-13T21:57:37Z

test-me-please

ianvernon added the wip label Feb 19, 2018

ianvernon requested a review from a team February 19, 2018 20:56

ianvernon requested review from a team as code owners February 19, 2018 20:56

ianvernon requested a review from a team February 19, 2018 20:56

ianvernon force-pushed the endpoint-ip-kvstore branch from caa1ecd to 9247b01 Compare February 19, 2018 21:15

ianvernon force-pushed the endpoint-ip-kvstore branch from 9247b01 to 286f076 Compare February 19, 2018 22:33

ianvernon force-pushed the endpoint-ip-kvstore branch 2 times, most recently from 438f18d to dd6168a Compare February 20, 2018 00:17

ianvernon requested a review from a team February 20, 2018 00:17

ianvernon force-pushed the endpoint-ip-kvstore branch 2 times, most recently from 6fe770a to 6483d56 Compare February 20, 2018 00:21

rlenglet mentioned this pull request Feb 20, 2018

envoy: Create and configure the caches for NPDS and NPHDS #2879

Merged

rlenglet reviewed Feb 20, 2018

View reviewed changes

ianvernon force-pushed the endpoint-ip-kvstore branch from 6483d56 to a3eb5e7 Compare February 20, 2018 02:31

ianvernon requested a review from a team as a code owner February 20, 2018 02:31

ianvernon mentioned this pull request Feb 20, 2018

pkg/identity: create identity package #2890

Merged

rlenglet mentioned this pull request Feb 22, 2018

New Envoy xDS APIs for L7 policies and endpoint IP to ID mapping #2596

Closed

5 tasks

ianvernon force-pushed the endpoint-ip-kvstore branch from a3eb5e7 to fe9b234 Compare February 23, 2018 18:11

cilium deleted a comment from houndci-bot Mar 10, 2018

ianvernon requested a review from tgraf March 10, 2018 03:51

tgraf reviewed Mar 10, 2018

View reviewed changes

tgraf approved these changes Mar 10, 2018

View reviewed changes

joestringer approved these changes Mar 12, 2018

View reviewed changes

ianvernon force-pushed the endpoint-ip-kvstore branch from ff7e4bc to b476e48 Compare March 12, 2018 19:42

ianvernon added the requires-janitor-review label Mar 12, 2018

raybejjani approved these changes Mar 13, 2018

View reviewed changes

rlenglet suggested changes Mar 13, 2018

View reviewed changes

ianvernon force-pushed the endpoint-ip-kvstore branch from b476e48 to a34e2a3 Compare March 13, 2018 21:53

Ian Vernon added 2 commits March 13, 2018 14:56

test/helpers: get all logs with journalctl for cilium

a827153

Signed-off by: Ian Vernon <ian@cilium.io>

ianvernon force-pushed the endpoint-ip-kvstore branch from a34e2a3 to a827153 Compare March 13, 2018 21:56

rlenglet approved these changes Mar 13, 2018

View reviewed changes

ianvernon merged commit 2cc32fd into master Mar 14, 2018

ianvernon deleted the endpoint-ip-kvstore branch March 14, 2018 01:04

		@@ -913,6 +983,11 @@ func (e Endpoint) SetIdentity(owner Owner, identity identityPkg.Identity) {

		e.runIdentityToK8sPodSync()

		// Whenever the identity is updated, propagate change to key-value store

endpoint: sync endpoint IP-SecurityID mapping to kvstore #2875

endpoint: sync endpoint IP-SecurityID mapping to kvstore #2875

Conversation

ianvernon commented Feb 19, 2018 • edited

ianvernon commented Feb 19, 2018

ianvernon commented Feb 19, 2018

ianvernon commented Feb 19, 2018

ianvernon commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianvernon commented Feb 20, 2018

ianvernon commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianvernon commented Mar 11, 2018

joestringer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianvernon commented Mar 12, 2018

raybejjani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianvernon commented Mar 13, 2018

ianvernon commented Mar 13, 2018

ianvernon commented Feb 19, 2018 •

edited