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

supportbundle: fix nil pointer error #2598

Merged
merged 1 commit into from
Aug 23, 2021

Conversation

liu4480
Copy link
Contributor

@liu4480 liu4480 commented Aug 16, 2021

when iproute2 is not installed in docker image, and run antctl supportbundle in
antrea-agent, it will panic, this is because, collectAgent will return a nil pointer
for systemv1beta1.SupportBundle, and func (r *supportBundleREST) Create,
antrea handles the nil pointer directly which leads to the panic.

@codecov-commenter
Copy link

codecov-commenter commented Aug 16, 2021

Codecov Report

Merging #2598 (3a5402c) into main (bdddc01) will increase coverage by 0.34%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2598      +/-   ##
==========================================
+ Coverage   60.27%   60.62%   +0.34%     
==========================================
  Files         282      285       +3     
  Lines       22345    23050     +705     
==========================================
+ Hits        13469    13973     +504     
- Misses       7452     7610     +158     
- Partials     1424     1467      +43     
Flag Coverage Δ
kind-e2e-tests 47.58% <0.00%> (+0.39%) ⬆️
unit-tests 41.69% <0.00%> (-0.51%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...kg/apiserver/registry/system/supportbundle/rest.go 74.56% <0.00%> (-1.02%) ⬇️
pkg/ipfix/ipfix_process.go 81.81% <0.00%> (-18.19%) ⬇️
pkg/ovs/openflow/ofctrl_group.go 38.09% <0.00%> (-7.86%) ⬇️
pkg/controller/networkpolicy/crd_utils.go 86.04% <0.00%> (-3.12%) ⬇️
pkg/controller/egress/controller.go 84.12% <0.00%> (-3.08%) ⬇️
pkg/controller/egress/ipallocator/allocator.go 80.41% <0.00%> (-2.57%) ⬇️
pkg/controller/traceflow/controller.go 72.72% <0.00%> (-2.43%) ⬇️
pkg/monitor/controller.go 27.61% <0.00%> (-1.50%) ⬇️
pkg/agent/openflow/client.go 57.90% <0.00%> (-1.18%) ⬇️
pkg/agent/openflow/network_policy.go 75.57% <0.00%> (-0.72%) ⬇️
... and 20 more

Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

it's not great to reference line numbers in a commit message, as code changes over time and it's not helpful when searching through the git history

Comment on lines 162 to 164
if b != nil {
r.clean(ctx, b.Filepath, bundleExpireDuration)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we check the value of err after calling collectAgent / collectController above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, I moved it into the annomous function which wraps to handle error returned from collectAgent / collectController

               func() {
			r.statusLocker.Lock()
			defer r.statusLocker.Unlock()
			if err != nil {
				klog.Errorf("Error when collecting supportBundle: %v", err)
				r.cache.Status = systemv1beta1.SupportBundleStatusNone
				return
			}
			...
		}()

antoninbas
antoninbas previously approved these changes Aug 20, 2021
@antoninbas
Copy link
Contributor

/test-all

Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

TestAntctl failed because of this change on all testbeds.
Looking at r.clean, it also acquired r.statusLocker, so the change leads to a deadlock.

when iproute2 is not installed in docker image, and run `antctl supportbundle` in
antrea-agent, it will panic, this is because, collectAgent will return a nil pointer
for systemv1beta1.SupportBundle, and in `func (r *supportBundleREST) Create`,
antrea handles the nil pointer directly which leads to the panic.

Signed-off-by: Bin Liu <biliu@vmware.com>
@liu4480
Copy link
Contributor Author

liu4480 commented Aug 23, 2021

/test-all

Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM, will defer to @antoninbas since he had comment.

@antoninbas antoninbas merged commit b1a357d into antrea-io:main Aug 23, 2021
liu4480 added a commit to liu4480/antrea that referenced this pull request Sep 16, 2021
When iproute2 is not installed in docker image, and one runs `antctl supportbundle` in
antrea-agent, it will panic. This is because collectAgent will return a nil pointer
for systemv1beta1.SupportBundle, and in `func (r *supportBundleREST) Create`,
antrea calls a method on the nil pointer directly which leads to the panic.

Signed-off-by: Bin Liu <biliu@vmware.com>
tnqn added a commit to tnqn/antrea that referenced this pull request Oct 17, 2022
The code should only schedule file cleanup when the returned error is
empty (meaning the returned supportbundle is not nil), otherwise the
supportbundle file would never be deleted and the program would panic
when there is any error encountered during supportbundle collection.

antrea-io#2598 tried to fix it but did
the opposite.

Signed-off-by: Quan Tian <qtian@vmware.com>
tnqn added a commit to tnqn/antrea that referenced this pull request Oct 19, 2022
The code should only schedule file cleanup when the returned error is
empty (meaning the returned supportbundle is not nil), otherwise the
supportbundle file would never be deleted and the program would panic
when there is any error encountered during supportbundle collection.

antrea-io#2598 tried to fix it but did
the opposite.

Signed-off-by: Quan Tian <qtian@vmware.com>
tnqn added a commit to tnqn/antrea that referenced this pull request Oct 20, 2022
The code should only schedule file cleanup when the returned error is
empty (meaning the returned supportbundle is not nil), otherwise the
supportbundle file would never be deleted and the program would panic
when there is any error encountered during supportbundle collection.

antrea-io#2598 tried to fix it but did
the opposite.

Signed-off-by: Quan Tian <qtian@vmware.com>
tnqn added a commit that referenced this pull request Oct 20, 2022
The code should only schedule file cleanup when the returned error is
empty (meaning the returned supportbundle is not nil), otherwise the
supportbundle file would never be deleted and the program would panic
when there is any error encountered during supportbundle collection.

#2598 tried to fix it but did
the opposite.

Signed-off-by: Quan Tian <qtian@vmware.com>
hongliangl pushed a commit to hongliangl/antrea that referenced this pull request Oct 22, 2022
…ea-io#4306)

The code should only schedule file cleanup when the returned error is
empty (meaning the returned supportbundle is not nil), otherwise the
supportbundle file would never be deleted and the program would panic
when there is any error encountered during supportbundle collection.

antrea-io#2598 tried to fix it but did
the opposite.

Signed-off-by: Quan Tian <qtian@vmware.com>
heanlan pushed a commit to heanlan/antrea that referenced this pull request Mar 29, 2023
…ea-io#4306)

The code should only schedule file cleanup when the returned error is
empty (meaning the returned supportbundle is not nil), otherwise the
supportbundle file would never be deleted and the program would panic
when there is any error encountered during supportbundle collection.

antrea-io#2598 tried to fix it but did
the opposite.

Signed-off-by: Quan Tian <qtian@vmware.com>
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.

4 participants