Report policy revision implemented by the proxy in Endpoint model #3151

joestringer · 2018-03-15T06:51:31Z

This PR plumbs the Repository revision and endpoints into the NetworkPolicyDiscoveryService, where these are cached and associated with NetworkPolicyCache versions. When Envoy receives the relevant resources and acknowledges them, it looks up in this cache to determine which policy revision is being acked and calls back into the endpoint objects to update a new field, proxyPolicyRevision.

For more details, see #3144.

Fixes: #3122
Fixes: #3144

houndci-bot · 2018-03-15T06:51:44Z

test/k8sT/Updates.go

+	"github.com/cilium/cilium/test/helpers"
+
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"


should not use dot imports

houndci-bot · 2018-03-15T06:51:44Z

test/k8sT/Updates.go

+
+	"github.com/cilium/cilium/test/helpers"
+
+	. "github.com/onsi/ginkgo"


should not use dot imports

houndci-bot · 2018-03-15T06:51:44Z

pkg/envoy/endpoint.go

+	return
+}
+
+func NewNetworkPolicyEventList() *NetworkPolicyEventList {


exported function NewNetworkPolicyEventList should have comment or be unexported

houndci-bot · 2018-03-15T06:51:44Z

pkg/envoy/cilium/npds.pb.go

@@ -68,6 +72,13 @@ func (m *NetworkPolicy) GetEgressPerPortPolicies() []*PortNetworkPolicy {
 	return nil
 }

+func (m *NetworkPolicy) GetPolicy() uint64 {


exported method NetworkPolicy.GetPolicy should have comment or be unexported

houndci-bot · 2018-03-15T06:51:44Z

pkg/envoy/cilium/npds.pb.go

 }

 func (m *NetworkPolicy) Reset()                    { *m = NetworkPolicy{} }
 func (m *NetworkPolicy) String() string            { return proto.CompactTextString(m) }
 func (*NetworkPolicy) ProtoMessage()               {}
 func (*NetworkPolicy) Descriptor() ([]byte, []int) { return fileDescriptor3, []int{0} }

-func (m *NetworkPolicy) GetPolicy() uint64 {
+func (m *NetworkPolicy) GetName() string {


exported method NetworkPolicy.GetName should have comment or be unexported

rlenglet · 2018-03-15T18:24:14Z

api/v1/openapi.yaml

@@ -823,6 +823,9 @@ definitions:
      policy-revision:
        description: The policy revision this endpoint is running on
        type: integer
+      proxy-policy-revision:
+        description: The policy revision currently in use in the proxy


in use -> enforced (?)

rlenglet · 2018-03-15T18:32:48Z

pkg/envoy/endpoint.go

+)
+
+var (
+	defaultipToBindingSize = 64


rlenglet · 2018-03-15T18:33:57Z

pkg/envoy/endpoint.go

+	GetIdentity() identity.NumericIdentity
+
+	// OnProxyPolicyUpdate is called when the proxy confirms that it
+	// has implemented a policy.


implemented -> applied

rlenglet · 2018-03-15T18:34:49Z

pkg/envoy/endpoint.go

+
+var (
+	defaultipToBindingSize = 64
+	unackedNPDSUpdates     *NetworkPolicyEventList


Add a TODO(rlenglet) comment: move this into the XDSServer.

rlenglet · 2018-03-15T18:37:05Z

pkg/envoy/endpoint.go

+	}
+}
+
+// Remove an endpoint from the list so that it no longer receives callbacks


Remove removes ...

rlenglet · 2018-03-15T19:23:46Z

pkg/envoy/server.go

-	name := strconv.FormatUint(uint64(id), 10)
-	NetworkPolicyCache.Upsert(NetworkPolicyTypeURL, name, networkPolicy, false)
+	// When successful, push them into the cache.
+	unackedNPDSUpdates.Insert(ep, policy.Revision, NetworkPolicyCache.GetVersion())


Call wg.AddCompletion() to get a Completion and pass that to the Insert call to store it in the binding.

rlenglet · 2018-03-15T19:25:21Z

daemon/daemon.go

@@ -148,21 +148,21 @@ func (d *Daemon) RemoveProxyRedirect(e *endpoint.Endpoint, id string) error {

 // UpdateNetworkPolicy adds or updates a network policy in the set
 // published to L7 proxies.
-func (d *Daemon) UpdateNetworkPolicy(id identity.NumericIdentity, policy *policy.L4Policy,
+func (d *Daemon) UpdateNetworkPolicy(e *endpoint.Endpoint, policy *policy.L4Policy,


Add a wg *completion.WaitGroup parameter.

rlenglet · 2018-03-15T19:26:36Z

envoy/cilium/npds.proto

+  // The policy identifier associated with the network policy. Corresponds to
+  // remote_policies entries in PortNetworkPolicyRule.
+  // Optional.
+  uint64 policy = 4;


Just a nitpick. I would move this up and use the following labels:

string name = 1; uint64 policy = 2; repeated PortNetworkPolicy ingress_per_port_policies = 3; repeated PortNetworkPolicy egress_per_port_policies = 4;

rlenglet · 2018-03-15T19:27:00Z

pkg/endpoint/endpoint.go

@@ -1883,6 +1885,17 @@ func (e *Endpoint) bumpPolicyRevision(revision uint64) {
 	e.Mutex.Unlock()
 }

+// OnProxyPolicyUpdate is a callback used to update the Endpoint's
+// proxyPolicyRevision when the specified revision has been implemented in the


implemented -> applied

rlenglet · 2018-03-15T19:27:42Z

pkg/endpoint/owner.go

@@ -53,12 +53,12 @@ type Owner interface {

 	// UpdateNetworkPolicy adds or updates a network policy in the set
 	// published to L7 proxies.
-	UpdateNetworkPolicy(id identity.NumericIdentity, policy *policy.L4Policy,
+	UpdateNetworkPolicy(e *Endpoint, policy *policy.L4Policy,


Add a wg *completion.WaitGroup parameter.

Signed-off-by: Joe Stringer <joe@covalent.io>

houndci-bot · 2018-03-15T22:53:14Z

pkg/envoy/cilium/npds.pb.go

 }

 func (m *NetworkPolicy) Reset()                    { *m = NetworkPolicy{} }
 func (m *NetworkPolicy) String() string            { return proto.CompactTextString(m) }
 func (*NetworkPolicy) ProtoMessage()               {}
 func (*NetworkPolicy) Descriptor() ([]byte, []int) { return fileDescriptor3, []int{0} }

+func (m *NetworkPolicy) GetName() string {


exported method NetworkPolicy.GetName should have comment or be unexported

Addressed comments and rewrote to reuse AckingResourceMutatorWrapper. Please review.

joestringer · 2018-03-15T22:57:38Z

daemon/daemon.go

 	labelsMap identity.IdentityCache, deniedIngressIdentities, deniedEgressIdentities map[identity.NumericIdentity]bool) error {
 	if d.l7Proxy == nil {
 		return fmt.Errorf("can't update network policy, proxy disabled")
 	}
-	return d.l7Proxy.UpdateNetworkPolicy(id, policy, labelsMap, deniedIngressIdentities, deniedEgressIdentities)
+	// TODO(jrajahalme): Pass e.ProxyWaitGroup down


@jrajahalme you'll need to pass e.ProxyWaitGroup down as part of your changes.

rlenglet · 2018-03-15T22:59:21Z

daemon/daemon.go

 	labelsMap identity.IdentityCache, deniedIngressIdentities, deniedEgressIdentities map[identity.NumericIdentity]bool) error {
 	if d.l7Proxy == nil {
 		return fmt.Errorf("can't update network policy, proxy disabled")
 	}
-	return d.l7Proxy.UpdateNetworkPolicy(id, policy, labelsMap, deniedIngressIdentities, deniedEgressIdentities)
+	// TODO(jrajahalme): Pass e.ProxyWaitGroup down


@jrajahalme heads up. We can't pass a WaitGroup yet, because no proxy is querying + acking the NetworkPolicy resources yet, so all regenerations would fail if we did.

rlenglet

LGTM. Thanks for doing this! I just have 4 nitpicks. Nothing blocking.

rlenglet · 2018-03-15T23:04:23Z

pkg/envoy/server.go

+		ep.GetIPv4Address(),
+	}
+	policies := []*cilium.NetworkPolicy{}
+	for i := range ips {


Nitpick: for _, ip := range ips { then use ip instead of ips[i].

rlenglet · 2018-03-15T23:04:44Z

pkg/envoy/server.go

-	name := strconv.FormatUint(uint64(id), 10)
-	NetworkPolicyCache.Upsert(NetworkPolicyTypeURL, name, networkPolicy, false)
+	// When successful, push them into the cache.
+	for i := range policies {


Nitpick: for _, p := range policies { then use p instead of policies[i].

rlenglet · 2018-03-15T23:06:52Z

pkg/envoy/server.go

+		} else {
+			c = wg.AddCompletionWithCallback(callback)
+		}
+		nodeIDs := []string{}


Nitpick:

nodeIDs := make([]string, 0, 1)

rlenglet · 2018-03-15T23:09:48Z

pkg/envoy/xds/cache.go

@@ -254,3 +254,8 @@ func (c *Cache) GetResources(ctx context.Context, typeURL string, lastVersion *u
 	cacheLog.Debugf("returning %d resources out of %d requested", len(res.Resources), len(resourceNames))
 	return res, nil
 }
+
+// GetVersion returns the version for the cache at the current point in time.
+func (c *Cache) GetVersion() uint64 {


Remove. That seems unused.
Anyway it is not thread-safe as-is. This would require locking.

joestringer · 2018-03-15T23:54:21Z

Something fishy happening in the unit tests, upserts into AckingNetworkPolicyMutator are not showing up in NetworkPolicyCache.

rlenglet · 2018-03-15T23:56:44Z

@joestringer Maybe the order of initialization of variables in resources.go?

rlenglet · 2018-03-15T23:57:50Z

You could move the initialization of the variables into an init()? Or move that into the StartXDSServer() in pkg/envoy/server.go.

rlenglet · 2018-03-15T23:58:23Z

@joestringer which unit tests are failing?

joestringer · 2018-03-16T00:00:07Z

@rlenglet DaemonEtcdSuite.TestRemovePolicy is failing, it sees an empty NetworkPolicyCache after inserting network policies.

I tried shifting the mutator update into an init() in pkg/envoy/resources.go, that didn't work. Will try the StartXDSServer() approach next.

joestringer · 2018-03-16T00:30:59Z

I think the problem is that we moved endpoint.updateNetworkPolicy() but the tests are expecting it to be updated during dry runs. Fix incoming.

rlenglet · 2018-03-16T00:51:50Z

Good call.
You should just call d.l7Proxy.UpdateNetworkPolicy with a nil WaitGroup in the dry run block.

Since there is endpoint-specific configuration which affects policy, this patch changes network policies to be applied per endpoint (via IPs) rather than per identity. Do this by defining an interface of the relevant portions of the endpoint that are used when updating network policy (NetworkPolicyEndpoint), and simplify UpdateNetworkPolicy() and RemoveNetworkPolicy() to pass this new NetworkPolicyEndpoint around. Fixes: cilium#3122 Signed-off-by: Joe Stringer <joe@covalent.io>

The existing path was updating the L7 Network Policies in the potentially dry-run path inside regeneratePolicy(), however it should only be applied when we are actually going to apply the policy changes to the datapath. Refactor the function and call it later from regenerateBPF(). Signed-off-by: Joe Stringer <joe@covalent.io>

A while ago, the spec was updated to use double graves to indicate fields, to be compatible with sphinx notation for where these fields end up in the documentation. We didn't update the API definitions at the time. Do so. This should be a functional no-op. Signed-off-by: Joe Stringer <joe@covalent.io>

An upcoming commit will need to identify which policy revision was used to generate an L4/L7 policy, so when we construct the L4Policy, cache the current policy repository revision in the L4Policy object. Signed-off-by: Joe Stringer <joe@covalent.io>

This new field will report the policy revision that has been acknowledged by the proxy, so it is clear to see whether the proxy is synchronised with Cilium or not. Signed-off-by: Joe Stringer <joe@covalent.io>

This callback will be called when the completion socket is closed. It allows an interested party to immediately apply some behaviour at the time that the completion occurs, rather than waiting on the completion in the outer code. It will be used in an upcoming commit to reflect the policy revision from proxy acknowledgements up to the endpoint. Signed-off-by: Joe Stringer <joe@covalent.io>

When the NetworkPolicyDiscoveryService acknowledges oustanding NetworkPolicyUpdates for an endpoint, push the corresponding policy revision number into the endpoint. Fixes: cilium#3144 Signed-off-by: Joe Stringer <joe@covalent.io>

rlenglet · 2018-03-16T01:37:03Z

test-me-please

rlenglet · 2018-03-16T01:37:53Z

The last test failed. A priori unrelated to this PR. This test seems flaky.

joestringer · 2018-03-16T03:29:43Z

Suspecting another test flake in tests PolicyEnforcement updates, will re-run.

joestringer · 2018-03-16T03:29:47Z

test-me-please

rlenglet · 2018-03-16T05:16:37Z

@tgraf could you please review this? Thanks!

joestringer added wip area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Mar 15, 2018

joestringer requested a review from a team March 15, 2018 06:51

joestringer requested review from a team as code owners March 15, 2018 06:51

joestringer requested a review from a team March 15, 2018 06:51

joestringer requested a review from a team as a code owner March 15, 2018 06:51

joestringer requested a review from a team March 15, 2018 06:51

joestringer requested a review from a team as a code owner March 15, 2018 06:51

houndci-bot reviewed Mar 15, 2018

View reviewed changes

joestringer force-pushed the submit/report-npds-revision branch from 021447f to b52efd1 Compare March 15, 2018 16:07

joestringer added pending-review and removed wip labels Mar 15, 2018

rlenglet mentioned this pull request Mar 15, 2018

Filter HTTP in Istio sidecar proxy #3065

Closed

30 tasks

rlenglet previously requested changes Mar 15, 2018

View reviewed changes

joestringer force-pushed the submit/report-npds-revision branch from b52efd1 to 18be553 Compare March 15, 2018 20:55

joestringer mentioned this pull request Mar 15, 2018

Make xDS network policies per-endpoint instead of per-identity #3130

Closed

endpoint: Fix typo

d438f93

Signed-off-by: Joe Stringer <joe@covalent.io>

joestringer force-pushed the submit/report-npds-revision branch from 18be553 to f7f8ba6 Compare March 15, 2018 22:52

houndci-bot reviewed Mar 15, 2018

View reviewed changes

joestringer force-pushed the submit/report-npds-revision branch from f7f8ba6 to 9631842 Compare March 15, 2018 22:54

joestringer commented Mar 15, 2018

View reviewed changes

rlenglet reviewed Mar 15, 2018

View reviewed changes

rlenglet approved these changes Mar 15, 2018

View reviewed changes

joestringer force-pushed the submit/report-npds-revision branch 2 times, most recently from e36829f to 5830763 Compare March 16, 2018 00:42

joestringer added 7 commits March 15, 2018 17:55

endpoint: Add proxyPolicyRevision

d500305

This new field will report the policy revision that has been acknowledged by the proxy, so it is clear to see whether the proxy is synchronised with Cilium or not. Signed-off-by: Joe Stringer <joe@covalent.io>

envoy: Update revision upon acks from NPDS

d4024cd

When the NetworkPolicyDiscoveryService acknowledges oustanding NetworkPolicyUpdates for an endpoint, push the corresponding policy revision number into the endpoint. Fixes: cilium#3144 Signed-off-by: Joe Stringer <joe@covalent.io>

joestringer force-pushed the submit/report-npds-revision branch from 5830763 to d4024cd Compare March 16, 2018 00:56

tgraf approved these changes Mar 16, 2018

View reviewed changes

tgraf merged commit 99a73a2 into cilium:master Mar 16, 2018

joestringer deleted the submit/report-npds-revision branch March 16, 2018 15:01


		"github.com/cilium/cilium/test/helpers"

		. "github.com/onsi/ginkgo"

Report policy revision implemented by the proxy in Endpoint model #3151

Report policy revision implemented by the proxy in Endpoint model #3151

Conversation

joestringer commented Mar 15, 2018 • edited

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

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

rlenglet 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

joestringer commented Mar 15, 2018

rlenglet commented Mar 15, 2018

rlenglet commented Mar 15, 2018

rlenglet commented Mar 15, 2018

joestringer commented Mar 16, 2018 • edited

joestringer commented Mar 16, 2018 • edited

rlenglet commented Mar 16, 2018 • edited

rlenglet commented Mar 16, 2018

rlenglet commented Mar 16, 2018

joestringer commented Mar 16, 2018

joestringer commented Mar 16, 2018

rlenglet commented Mar 16, 2018

joestringer commented Mar 15, 2018 •

edited

joestringer commented Mar 16, 2018 •

edited

joestringer commented Mar 16, 2018 •

edited

rlenglet commented Mar 16, 2018 •

edited