Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Panic in network policy deletion #3034

Merged
merged 2 commits into from Jun 27, 2022

Conversation

tssurya
Copy link
Member

@tssurya tssurya commented Jun 11, 2022

Fix panic in NP deletion

NOTE to reviewers: First commit adds a unit test that shows the panic, the second commit adds the fix and updates the test case to pass.

Fixes panic observed during NP deletion:

2022-05-27T15:48:27.957882179Z I0527 15:48:27.957839       1
policy.go:1251] Deleting network policy XXXXX in namespace XXXXX
2022-05-27T15:48:27.957950868Z E0527 15:48:27.957928       1
runtime.go:78] Observed a panic: "invalid memory address or nil pointer
dereference" (runtime error: invalid memory address or nil pointer
dereference)
2022-05-27T15:48:27.957950868Z goroutine 132 [running]:
2022-05-27T15:48:27.957950868Z
k8s.io/apimachinery/pkg/util/runtime.logPanic(0x1930dc0, 0x2907f50)
2022-05-27T15:48:27.957950868Z
/go/src/github.com/openshift/ovn-kubernetes/go-controller/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:74
+0x95
2022-05-27T15:48:27.957950868Z
k8s.io/apimachinery/pkg/util/runtime.HandleCrash(0x0, 0x0, 0x0)
2022-05-27T15:48:27.957950868Z
/go/src/github.com/openshift/ovn-kubernetes/go-controller/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:48
+0x86
2022-05-27T15:48:27.957950868Z panic(0x1930dc0, 0x2907f50)
2022-05-27T15:48:27.957950868Z 	/usr/lib/golang/src/runtime/panic.go:965
+0x1b9
2022-05-27T15:48:27.957950868Z
github.com/ovn-org/ovn-kubernetes/go-controller/pkg/ovn.(*Controller).destroyNetworkPolicy(0xc002fcc6c0,
0x0, 0xc000fc9901, 0x0, 0x0)
2022-05-27T15:48:27.957950868Z
/go/src/github.com/openshift/ovn-kubernetes/go-controller/pkg/ovn/policy.go:1287
+0x55
2022-05-27T15:48:27.957950868Z
github.com/ovn-org/ovn-kubernetes/go-controller/pkg/ovn.(*Controller).deleteNetworkPolicy(0xc002fcc6c0,
0xc0006cc580, 0x0, 0x0, 0x0)
2022-05-27T15:48:27.957950868Z
/go/src/github.com/openshift/ovn-kubernetes/go-controller/pkg/ovn/policy.go:1275
+0x451

Signed-off-by: Surya Seetharaman suryaseetharaman.9@gmail.com

When a network policy creation fails before it is
added to the `nsInfo.networkPolicies[policy.Name] = np`
cache, attempting to delete it or update it causes
panic since we end up passing nil pointer into
destroyNetworkPolicy.

Signed-off-by: Surya Seetharaman <suryaseetharaman.9@gmail.com>
Copy link
Member Author

@tssurya tssurya left a comment

Choose a reason for hiding this comment

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

go-controller/pkg/ovn/policy.go Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jun 11, 2022

Coverage Status

Coverage decreased (-0.01%) to 52.37% when pulling a679b63 on tssurya:panic-network-policy-deletion into 195e578 on ovn-org:master.

@tssurya tssurya force-pushed the panic-network-policy-deletion branch from 56449d2 to ac74d3a Compare June 14, 2022 17:21
@tssurya
Copy link
Member Author

tssurya commented Jun 14, 2022

I will work on the

2022-06-09T17:15:00.952174381Z E0609 17:15:00.952154       1 ovn.go:753] Failed to create network policy oit-ssi-fluentd/fluentd-input, error: failed to create default port groups and acls for policy: oit-ssi-fluentd/fluentd-input, error: unexpectedly found multiple equivalent ACLs: [{UUID:5b98f17d-789f-4de1-9beb-36741bfa40d1 Action:drop Direction:from-lport ExternalIDs:map[default-deny-policy-type:Egress] Label:0 Log:false Match:inport == @a12933912868060780448_egressDefaultDeny Meter:0xc001388aa0 Name:0xc001388ab0 Options:map[apply-after-lb:true] Priority:1000 Severity:0xc001388ac0} {UUID:b9349cb8-99b0-4130-92cf-ab11b4874a11 Action:drop Direction:from-lport ExternalIDs:map[default-deny-policy-type:Egress] Label:0 Log:false Match:inport == @a12933912868060780448_egressDefaultDeny Meter:0xc001388c90 Name:0xc001388ca0 Options:map[apply-after-lb:true] Priority:1000 Severity:0xc001388cb0}]

AND

2022-06-01T08:17:44.635401164Z I0601 08:17:44.634030       1 policy.go:1092] Adding network policy allow-from-other-namespaces in namespace mdh-old
2022-06-01T08:17:44.635401164Z E0601 08:17:44.634509       1 ovn.go:753] Failed to create network policy mdh-old/allow-from-other-namespaces, error: failed to create default port groups and acls for policy: mdh-old/allow-from-other-namespaces, error: unexpectedly found multiple equivalent ACLs: [{UUID:3bc36c0e-ee1a-4609-a240-c211f14f379b Action:allow Direction:to-lport ExternalIDs:map[default-deny-policy-type:Ingress] Label:0 Log:false Match:outport == @a13985064446031893020_ingressDefaultDeny && arp Meter:0xc003245210 Name:0xc003245220 Options:map[] Priority:1001 Severity:0xc003245230} {UUID:41702278-fb49-4418-abca-6425912d1e63 Action:allow Direction:to-lport ExternalIDs:map[default-deny-policy-type:Ingress] Label:0 Log:false Match:outport == @a13985064446031893020_ingressDefaultDeny && (arp || nd) Meter:0xc0032452a0 Name:0xc0032452b0 Options:map[] Priority:1001 Severity:0xc0032452c0}]

in a new PR. Don't want to mix up creation bugs with deletion bugs.

@tssurya tssurya changed the title Panic network policy deletion Fix Panic in network policy deletion Jun 14, 2022
@tssurya
Copy link
Member Author

tssurya commented Jun 14, 2022

/retest-failed

@tssurya
Copy link
Member Author

tssurya commented Jun 14, 2022

test failure due to #2988 hence retesting.

@tssurya
Copy link
Member Author

tssurya commented Jun 14, 2022

/retest

@ovn-robot
Copy link
Collaborator

Oops, something went wrong:

This workflow is already running

go-controller/pkg/ovn/policy.go Outdated Show resolved Hide resolved
@tssurya tssurya force-pushed the panic-network-policy-deletion branch from ac74d3a to 93d4889 Compare June 23, 2022 13:12
go-controller/pkg/ovn/policy.go Show resolved Hide resolved
go-controller/pkg/ovn/policy.go Show resolved Hide resolved
@tssurya tssurya force-pushed the panic-network-policy-deletion branch from 93d4889 to c6eea75 Compare June 23, 2022 20:03
go-controller/pkg/ovn/policy.go Outdated Show resolved Hide resolved
@tssurya tssurya force-pushed the panic-network-policy-deletion branch from c6eea75 to 7024e2c Compare June 23, 2022 20:38
Fixes panic observed during NP deletion:

2022-05-27T15:48:27.957882179Z I0527 15:48:27.957839       1
policy.go:1251] Deleting network policy XXXXX in namespace XXXXX
2022-05-27T15:48:27.957950868Z E0527 15:48:27.957928       1
runtime.go:78] Observed a panic: "invalid memory address or nil pointer
dereference" (runtime error: invalid memory address or nil pointer
dereference)
2022-05-27T15:48:27.957950868Z goroutine 132 [running]:
2022-05-27T15:48:27.957950868Z
k8s.io/apimachinery/pkg/util/runtime.logPanic(0x1930dc0, 0x2907f50)
2022-05-27T15:48:27.957950868Z
/go/src/github.com/openshift/ovn-kubernetes/go-controller/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:74
+0x95
2022-05-27T15:48:27.957950868Z
k8s.io/apimachinery/pkg/util/runtime.HandleCrash(0x0, 0x0, 0x0)
2022-05-27T15:48:27.957950868Z
/go/src/github.com/openshift/ovn-kubernetes/go-controller/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:48
+0x86
2022-05-27T15:48:27.957950868Z panic(0x1930dc0, 0x2907f50)
2022-05-27T15:48:27.957950868Z 	/usr/lib/golang/src/runtime/panic.go:965
+0x1b9
2022-05-27T15:48:27.957950868Z
github.com/ovn-org/ovn-kubernetes/go-controller/pkg/ovn.(*Controller).destroyNetworkPolicy(0xc002fcc6c0,
0x0, 0xc000fc9901, 0x0, 0x0)
2022-05-27T15:48:27.957950868Z
/go/src/github.com/openshift/ovn-kubernetes/go-controller/pkg/ovn/policy.go:1287
+0x55
2022-05-27T15:48:27.957950868Z
github.com/ovn-org/ovn-kubernetes/go-controller/pkg/ovn.(*Controller).deleteNetworkPolicy(0xc002fcc6c0,
0xc0006cc580, 0x0, 0x0, 0x0)
2022-05-27T15:48:27.957950868Z
/go/src/github.com/openshift/ovn-kubernetes/go-controller/pkg/ovn/policy.go:1275
+0x451

Signed-off-by: Surya Seetharaman <suryaseetharaman.9@gmail.com>
@tssurya tssurya force-pushed the panic-network-policy-deletion branch from 7024e2c to a679b63 Compare June 23, 2022 20:44
@tssurya
Copy link
Member Author

tssurya commented Jun 23, 2022

@tssurya
Copy link
Member Author

tssurya commented Jun 23, 2022

/retest-failed

@tssurya
Copy link
Member Author

tssurya commented Jun 24, 2022

@tssurya
Copy link
Member Author

tssurya commented Jun 24, 2022

/retest-failed

@tssurya
Copy link
Member Author

tssurya commented Jun 24, 2022

/retest-failed

@trozet trozet merged commit 4c03ae8 into ovn-org:master Jun 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants