Propagate status field to cilium network policy by aanm · Pull Request #1354 · cilium/cilium

aanm · 2017-08-23T03:19:34Z

kubectl get ciliumnetworkpolicies multi-rules -o json

...
    "status": {
        "Nodes": {
            "cilium-k8s-master": {
                "LastSeen": "2017-08-22T20:16:23.471822307-07:00",
                "Message": "\u003cnil\u003e",
                "OK": false
            },
            "cilium-k8s-node-2": {
                "LastSeen": "2017-08-22T20:15:59.540742656-07:00",
                "Message": "\u003cnil\u003e",
                "OK": false
            }
        }
    }

houndci-bot · 2017-08-23T03:19:44Z

comment on exported const CustomResourceDefinitionPluralName should be of the form "CustomResourceDefinitionPluralName ..."

tgraf · 2017-08-23T04:03:20Z

 	// Skip currently unused IDs
 	if ctx.From == nil || len(ctx.From) == 0 {
-		log.Debugf("[%s] Ignoring unused ID %v", e.PolicyID(), ctx)
+		//log.Debugf("[%s] Ignoring unused ID %v", e.PolicyID(), ctx)


Remove this for real and submit as separate PR

tgraf · 2017-08-23T04:04:03Z

+	Message string
+
+	// LastSeen contains the last time this status was updated
+	LastSeen time.Time


Let's call this LastUpdated

tgraf · 2017-08-23T04:04:15Z

+	OK bool
+
+	// Message describes the error condition if OK is false
+	Message string


Let's call this Error

tgraf · 2017-08-25T14:49:33Z

Move this status update into its own function in pkg/k8s

Will the status only work if CRD are in use?

Will the status only work if CRD are in use?

You got confused because of the variable name? It works for both, but I'll change the variable name to cnpClient

// crdClient is the interface for CRD and TPR crdClient k8s.CRDInterface

Renaming will help. I'm also fine keeping it as-is as CRD will be the only interface soon and TPR will be gone. I was just wondering in general.

tgraf · 2017-08-25T14:52:18Z

Must be a const in pkg/k8s/const.go we should use a single value for all backoff loops so we can adjust globally.

tgraf · 2017-08-25T14:53:09Z

Doesn't Update() give you the latest revision of the resource when it fails? You can use it instead of hoping for a race free get & update.

Doesn't Update() give you the latest revision of the resource when it fails? You can use it instead of hoping for a race free get & update.

It doesn't and it took me a while to figure out that. How can we still get a race with the get & update? I was hopping this would prevent it:

if serverRule.Metadata.UID != rule.Metadata.UID { // Although the policy was found this means it was deleted, // and re-added with the same name. log.Debugf("k8s: rule %s/%s changed while updating node status, stopping retry", ns, name) break }

but I can modify it to this as well:

if serverRule.Metadata.UID != rule.Metadata.UID || !serverRule.SpecEquals(rule) { // Although the policy was found this means it was deleted, // and re-added with the same name. log.Debugf("k8s: rule %s/%s changed while updating node status, stopping retry", ns, name) break }

The code is fine but it looked inefficient. It's unexpected that Update() does not provide the latest version.

tgraf · 2017-08-25T14:54:06Z

You can remove the FIXME here now

Signed-off-by: André Martins <andre@cilium.io>

When using kubernetes to manage CRD CNP, it was difficult to know on which nodes was the policy being placed. With this commit it will be possible to check with `kubectl get ciliumnetworkpolicies <name> -o json` or with `kubectl describe ciliumnetworkpolicies <name>` when was the policy received by which node. Signed-off-by: André Martins <andre@cilium.io>

Signed-off-by: André Martins <andre@cilium.io>

Since cilium was not checking if the TPR resource definition after being created, it could cause false positives in the detection of conflicting TPR and CRDs installed in the same cluster. With this commit, cilium makes sure it TPR is installed before continue any operation. Signed-off-by: André Martins <andre@cilium.io>

aanm · 2017-08-30T21:48:57Z

Finally, this was failing because of the CI tests failures but one of them was caused by me. I've fix it and it has passed.

tgraf · 2017-08-30T22:31:11Z

Great work!

As the previous commit mentions, this does not occur on main, v1.16, and v1.15 due to cilium#29036. However, in v1.14 and v1.13, we need to take special care because the NameManager and SelectorCache lock can be taken while the Endpoint lock is held during Endpoint deletion. Here are the relevant stacktraces regarding the deadlock: ``` 1: sync.Mutex.Lock [75 minutes] [Created by http.(*Server).Serve in goroutine 1699 @ server.go:3086] sync sema.go:77 runtime_SemacquireMutex(*uint32(0x5), false, 43690) sync mutex.go:171 (*Mutex).lockSlow(*Mutex(cilium#1733)) sync mutex.go:90 (*Mutex).Lock(...) sync rwmutex.go:147 (*RWMutex).Lock(*RWMutex(0xb0)) fqdn name_manager.go:70 (*NameManager).Lock(0xffffffffffffffff) policy selectorcache.go:964 (*SelectorCache).RemoveSelectors(cilium#1088, {cilium#28569, 0xb, 1}, {cilium#643, cilium#32582}) policy l4.go:810 (*L4Filter).removeSelectors(cilium#32582, cilium#29992) policy l4.go:817 (*L4Filter).detach(cilium#719, cilium#29993) policy l4.go:988 L4PolicyMap.Detach(...) policy l4.go:1179 (*L4Policy).Detach(cilium#20318, cilium#1383) policy resolve.go:103 (*selectorPolicy).Detach(...) policy distillery.go:81 (*PolicyCache).delete(cilium#1354, cilium#19354) policy distillery.go:138 (*PolicyCache).LocalEndpointIdentityRemoved(cilium#523, cilium#1163) identitymanager manager.go:167 (*IdentityManager).remove(cilium#706, cilium#19354) identitymanager manager.go:147 (*IdentityManager).Remove(cilium#706, cilium#19354) identitymanager manager.go:52 Remove(...) endpoint endpoint.go:1146 (*Endpoint).leaveLocked(cilium#1883, cilium#12221, {0x30, 0}) endpoint endpoint.go:2192 (*Endpoint).Delete(cilium#1883, {0x80, 0xaa}) endpointmanager manager.go:380 (*EndpointManager).removeEndpoint(cilium#1161, 0, {0xff, 0xff}) endpointmanager manager.go:394 (*EndpointManager).RemoveEndpoint(...) cmd endpoint.go:684 (*Daemon).deleteEndpointQuiet(...) cmd endpoint.go:666 (*Daemon).deleteEndpoint(cilium#1155, cilium#1883) cmd endpoint.go:713 (*Daemon).DeleteEndpoint(cilium#1155, {cilium#27399, cilium#8108}) cmd endpoint.go:770 (*deleteEndpointID).Handle(cilium#21193, {cilium#2452, {cilium#27399, 0x4d}}) endpoint delete_endpoint_id.go:66 (*DeleteEndpointID).ServeHTTP(cilium#5934, {cilium#666, cilium#2242}, cilium#2452) middleware operation.go:28 (*Context).RoutesHandler.NewOperationExecutor.func1({cilium#666, cilium#2242}, cilium#2452) http server.go:2136 HandlerFunc.ServeHTTP(ReadCloser(cilium#130), func{cilium#2242, 0x3}) middleware router.go:78 NewRouter.func1({cilium#666, cilium#2242}, cilium#2451) http server.go:2136 HandlerFunc.ServeHTTP(ReadCloser(cilium#718), func{cilium#2242, #59}) middleware redoc.go:72 Redoc.func1({cilium#666, cilium#2242}, cilium#1251) http server.go:2136 HandlerFunc.ServeHTTP(ReadCloser(cilium#4920), func{cilium#2242, #45}) middleware spec.go:46 Spec.func1({cilium#666, cilium#2242}, cilium#4921) http server.go:2136 HandlerFunc.ServeHTTP(ReadCloser(cilium#10532), func{cilium#2242, cilium#23015}) metrics middleware.go:64 (*APIEventTSHelper).ServeHTTP(cilium#1459, {cilium#668, cilium#10533}, cilium#2451) api apipanic.go:42 (*APIPanicHandler).ServeHTTP(cilium#722, {cilium#668, cilium#10533}, cilium#4922) http server.go:2938 serverHandler.ServeHTTP(*Server(cilium#8105), cilium#668, cilium#10533, 0x6) http server.go:2009 (*conn).serve(*conn(cilium#16005), Context{cilium#673, cilium#1554}) 8: sync.Mutex.Lock [74 minutes] [Created by http.(*Server).Serve in goroutine 1699 @ server.go:3086] sync sema.go:77 runtime_SemacquireMutex(*, 0x47, cilium#1154) sync mutex.go:171 (*Mutex).lockSlow(cilium#706) sync mutex.go:90 (*Mutex).Lock(...) sync rwmutex.go:147 (*RWMutex).Lock(*) identitymanager manager.go:99 (*IdentityManager).RemoveOldAddNew(cilium#706, 0, cilium#1154) identitymanager manager.go:123 RemoveOldAddNew(...) endpoint policy.go:852 (*Endpoint).SetIdentity(*, cilium#1154, 0) endpoint endpoint.go:1932 (*Endpoint).identityLabelsChanged(*, {cilium#674, *}, 1) endpoint endpoint.go:1780 (*Endpoint).runIdentityResolver(*, {cilium#674, *}, 1, 1) endpoint endpoint.go:1720 (*Endpoint).UpdateLabels(*, {cilium#674, *}, *, *, 8) cmd endpoint.go:477 (*Daemon).createEndpoint(cilium#1155, {cilium#673, *}, {cilium#683, cilium#1155}, *) cmd endpoint.go:542 (*putEndpointID).Handle(cilium#21192, {*, *, {*, 0xe}}) endpoint put_endpoint_id.go:58 (*PutEndpointID).ServeHTTP(cilium#3961, {cilium#666, *}, *) middleware operation.go:28 (*Context).RoutesHandler.NewOperationExecutor.func1({cilium#666, *}, *) http server.go:2136 HandlerFunc.ServeHTTP(*, {cilium#666, *}, *) middleware router.go:78 NewRouter.func1({cilium#666, *}, *) http server.go:2136 HandlerFunc.ServeHTTP(*, {cilium#666, *}, *) middleware redoc.go:72 Redoc.func1({cilium#666, *}, cilium#1251) http server.go:2136 HandlerFunc.ServeHTTP(*, {cilium#666, *}, #45) middleware spec.go:46 Spec.func1({cilium#666, *}, *) http server.go:2136 HandlerFunc.ServeHTTP(*, {cilium#666, *}, *) metrics middleware.go:64 (*APIEventTSHelper).ServeHTTP(cilium#1459, {cilium#668, *}, *) api apipanic.go:42 (*APIPanicHandler).ServeHTTP(#49, {cilium#668, *}, *) http server.go:2938 serverHandler.ServeHTTP({cilium#653}, {cilium#668, *}, 6) http server.go:2009 (*conn).serve(*, {cilium#673, cilium#1554}) 5: sync.Mutex.Lock [75 minutes] [Created by eventqueue.(*EventQueue).Run in goroutine 1482 @ eventqueue.go:229] sync sema.go:77 runtime_SemacquireMutex(cilium#142, 0xe8, *) sync mutex.go:171 (*Mutex).lockSlow(cilium#1733) sync mutex.go:90 (*Mutex).Lock(...) sync rwmutex.go:147 (*RWMutex).Lock(0x68) fqdn name_manager.go:70 (*NameManager).Lock(*) policy selectorcache.go:798 (*SelectorCache).AddFQDNSelector(cilium#1088, {cilium#643, *}, {{*, 0x4d}, {0, 0}}) policy l4.go:628 (*L4Filter).cacheFQDNSelector(...) policy l4.go:623 (*L4Filter).cacheFQDNSelectors(*, {*, 4, cilium#193}, cilium#536) policy l4.go:725 createL4Filter({cilium#680, *}, {*, 1, 1}, 0, {cilium#660, *}, {{*, 4}, ...}, ...) policy l4.go:879 createL4EgressFilter(...) policy rule.go:717 mergeEgressPortProto({cilium#680, *}, #44, {*, 0xa, 0}, *, {cilium#660, *}, {{*, ...}, ...}, ...) policy rule.go:672 mergeEgress.func1({cilium#660, *}) api l4.go:284 PortRules.Iterate({*, 1, cilium#546}, *) policy rule.go:624 mergeEgress({cilium#680, *}, *, {*, 1, 1}, 0, {cilium#661, *}, {cilium#662, ...}, ...) policy rule.go:753 (*rule).resolveEgressPolicy(*, {cilium#680, *}, *, *, *, {0, 0, 0}, {0, ...}) policy rules.go:103 ruleSlice.resolveL4EgressPolicy({*, *, *}, {cilium#680, *}, *) policy repository.go:718 (*Repository).resolvePolicyLocked(cilium#1089, *) policy distillery.go:119 (*PolicyCache).updateSelectorPolicy(cilium#1354, *) policy distillery.go:153 (*PolicyCache).UpdatePolicy(...) endpoint policy.go:262 (*Endpoint).regeneratePolicy(*) endpoint bpf.go:744 (*Endpoint).runPreCompilationSteps(*, *, *) endpoint bpf.go:589 (*Endpoint).regenerateBPF(*, *) endpoint policy.go:457 (*Endpoint).regenerate(*, *) endpoint events.go:53 (*EndpointRegenerationEvent).Handle(*, *) eventqueue eventqueue.go:245 (*EventQueue).run.func1() sync once.go:74 (*Once).doSlow(*, *) sync once.go:65 (*Once).Do(...) eventqueue eventqueue.go:233 (*EventQueue).run(*) 1: select [75 minutes] [Created by eventqueue.(*EventQueue).Run in goroutine 1482 @ eventqueue.go:229] semaphore semaphore.go:60 (*Weighted).Acquire(cilium#1092, {cilium#671, cilium#722}, cilium#766) lock semaphored_mutex.go:30 (*SemaphoredMutex).Lock(...) ipcache ipcache.go:140 (*IPCache).Lock(...) ipcache cidr.go:56 (*IPCache).AllocateCIDRs(cilium#1316, {0, 0, cilium#2037}, {0, 0, 0}, 0) ipcache cidr.go:103 (*IPCache).AllocateCIDRsForIPs(0, {0, cilium#697, 0}, 0xffffffffffffffff) cmd identity.go:114 cachingIdentityAllocator.AllocateCIDRsForIPs(...) policy selectorcache.go:509 (*SelectorCache).allocateIdentityMappings(cilium#1088, {{0, 0}, {cilium#5036, 0x2b}}, #45) policy selectorcache.go:843 (*SelectorCache).AddFQDNSelector(cilium#1088, {cilium#643, cilium#27811}, {{0, 0}, {cilium#5036, 0x2b}}) policy l4.go:628 (*L4Filter).cacheFQDNSelector(...) policy l4.go:623 (*L4Filter).cacheFQDNSelectors(cilium#27811, {cilium#42936, 0x287, cilium#193}, cilium#536) policy l4.go:725 createL4Filter({cilium#680, cilium#22826}, {cilium#22828, 1, 1}, 0, {cilium#660, cilium#24510}, {{cilium#4487, 3}, ...}, ...) policy l4.go:879 createL4EgressFilter(...) policy rule.go:717 mergeEgressPortProto({cilium#680, cilium#22826}, #44, {cilium#22828, 0xa, #78536}, #79633, {cilium#660, cilium#24510}, {{cilium#4487, ...}, ...}, ...) policy rule.go:672 mergeEgress.func1({cilium#660, cilium#24510}) api l4.go:284 PortRules.Iterate({cilium#24510, 1, cilium#546}, cilium#11741) policy rule.go:624 mergeEgress({cilium#680, cilium#22826}, cilium#18687, {cilium#22828, 1, 1}, 0, {cilium#661, cilium#5624}, {cilium#662, ...}, ...) policy rule.go:753 (*rule).resolveEgressPolicy(cilium#24575, {cilium#680, cilium#22826}, cilium#18687, cilium#29345, cilium#4782, {0, 0, 0}, {0, ...}) policy rules.go:103 ruleSlice.resolveL4EgressPolicy({cilium#10690, 0xb, 5}, {cilium#680, cilium#22826}, cilium#18687) policy repository.go:718 (*Repository).resolvePolicyLocked(cilium#1089, cilium#18461) policy distillery.go:119 (*PolicyCache).updateSelectorPolicy(cilium#1354, cilium#18461) policy distillery.go:153 (*PolicyCache).UpdatePolicy(...) endpoint policy.go:262 (*Endpoint).regeneratePolicy(cilium#1748) endpoint bpf.go:744 (*Endpoint).runPreCompilationSteps(cilium#1748, cilium#27542, cilium#4781) endpoint bpf.go:589 (*Endpoint).regenerateBPF(cilium#1748, cilium#27542) endpoint policy.go:457 (*Endpoint).regenerate(cilium#1748, cilium#27542) endpoint events.go:53 (*EndpointRegenerationEvent).Handle(cilium#18609, cilium#703) eventqueue eventqueue.go:245 (*EventQueue).run.func1() sync once.go:74 (*Once).doSlow(*Once(cilium#45414), func(cilium#69)) sync once.go:65 (*Once).Do(...) eventqueue eventqueue.go:233 (*EventQueue).run(cilium#6023) ``` Generated from pp tool: https://github.com/maruel/panicparse Signed-off-by: Chris Tarazi <chris@isovalent.com>

aanm added kind/enhancement This would improve or streamline existing functionality. pending-review labels Aug 23, 2017

aanm requested a review from a team August 23, 2017 03:19

houndci-bot reviewed Aug 23, 2017

View reviewed changes

aanm force-pushed the 1222-adding-cnp-status branch from f59b30f to 1d41fc1 Compare August 23, 2017 03:26

tgraf suggested changes Aug 23, 2017

View reviewed changes

aanm force-pushed the 1222-adding-cnp-status branch from 1d41fc1 to 193a0c2 Compare August 24, 2017 22:27

aanm changed the title ~~[DO NOT MERGE] Propagate status field to cilium network policy~~ Propagate status field to cilium network policy Aug 24, 2017

aanm force-pushed the 1222-adding-cnp-status branch 3 times, most recently from abeeea8 to 5471028 Compare August 25, 2017 01:10

aanm requested review from a team and tgraf August 25, 2017 01:10

aanm force-pushed the 1222-adding-cnp-status branch 2 times, most recently from 5d551f1 to 16c946a Compare August 25, 2017 12:45

tgraf changed the title ~~Propagate status field to cilium network policy~~ DO NOT MERGE: Propagate status field to cilium network policy Aug 25, 2017

tgraf suggested changes Aug 25, 2017

View reviewed changes

aanm changed the title ~~DO NOT MERGE: Propagate status field to cilium network policy~~ Propagate status field to cilium network policy Aug 25, 2017

aanm force-pushed the 1222-adding-cnp-status branch from 16c946a to 549810a Compare August 25, 2017 17:03

aanm requested a review from tgraf August 25, 2017 17:05

tgraf approved these changes Aug 25, 2017

View reviewed changes

aanm force-pushed the 1222-adding-cnp-status branch 4 times, most recently from 57ff4bc to e12ab24 Compare August 26, 2017 01:49

aanm mentioned this pull request Aug 26, 2017

tests: Fix wait_for_k8s_endpoints #1390

Merged

aanm force-pushed the 1222-adding-cnp-status branch 3 times, most recently from 9625374 to b1a3ded Compare August 29, 2017 00:13

aanm force-pushed the 1222-adding-cnp-status branch 6 times, most recently from 7563de3 to a5d8bcf Compare August 30, 2017 15:05

aanm added wip and removed pending-review labels Aug 30, 2017

aanm force-pushed the 1222-adding-cnp-status branch 2 times, most recently from 18c8e44 to a916199 Compare August 30, 2017 16:20

aanm changed the title ~~Propagate status field to cilium network policy~~ DO NOT MERGE Propagate status field to cilium network policy Aug 30, 2017

aanm added 5 commits August 30, 2017 21:57

k8s: moved CreateCustomResourceDefinitions to pkg/k8s

6f14d11

Signed-off-by: André Martins <andre@cilium.io>

Create CNP dedicated client with public interface

211cc5d

Signed-off-by: André Martins <andre@cilium.io>

tests: adding e2e tests for CNP in kubernetes

6392a56

Signed-off-by: André Martins <andre@cilium.io>

aanm force-pushed the 1222-adding-cnp-status branch from a916199 to ca9583d Compare August 30, 2017 19:57

aanm changed the title ~~DO NOT MERGE Propagate status field to cilium network policy~~ Propagate status field to cilium network policy Aug 30, 2017

aanm added pending-review and removed wip labels Aug 30, 2017

aanm requested a review from tgraf August 30, 2017 21:48

tgraf approved these changes Aug 30, 2017

View reviewed changes

tgraf merged commit 3361224 into master Aug 30, 2017

tgraf deleted the 1222-adding-cnp-status branch August 30, 2017 22:31

tgraf removed the stable/needs-backport label Dec 5, 2017

Conversation

aanm commented Aug 23, 2017 • edited by tgraf Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aanm Aug 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aanm commented Aug 30, 2017

Uh oh!

tgraf commented Aug 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aanm commented Aug 23, 2017 •

edited by tgraf

Loading

aanm Aug 25, 2017 •

edited

Loading

tgraf commented Aug 30, 2017 •

edited

Loading