[ExternalNode]Implement SupportBundleCollection on Agent #4338

mengdie-song · 2022-10-26T03:57:27Z

Implement SupportBundleCollection on Agent

Watch internal SupportBundleCollection events.
Refactor SupportBundle compression to util.
Let agent to collect SupportBundle for Node/ExternalNode.
Upload the collected SupportBundle to file server.
Only SFTP and basic authentication are supported for now.

Signed-off-by: Mengdie Song songm@vmware.com

codecov · 2022-10-26T04:13:12Z

Codecov Report

Merging #4338 (eedf1a2) into main (e94e4cf) will decrease coverage by 1.72%.
The diff coverage is 6.05%.

❗ Current head eedf1a2 differs from pull request most recent head 2f60340. Consider uploading reports for the commit 2f60340 to get more accurate results

@@            Coverage Diff             @@
##             main    #4338      +/-   ##
==========================================
- Coverage   65.53%   63.80%   -1.73%     
==========================================
  Files         400      402       +2     
  Lines       56950    57195     +245     
==========================================
- Hits        37321    36494     -827     
- Misses      16948    18007    +1059     
- Partials     2681     2694      +13

Flag	Coverage Δ		*Carryforward flag
e2e-tests	`38.05% <6.29%> (?)`
integration-tests	`34.62% <ø> (-0.03%)`	⬇️	Carriedforward from 19a7de0
kind-e2e-tests	`40.18% <0.00%> (-7.32%)`	⬇️	Carriedforward from 19a7de0
unit-tests	`49.78% <7.69%> (-0.50%)`	⬇️	Carriedforward from 19a7de0

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files	Coverage Δ
cmd/antrea-agent/agent.go	`0.00% <0.00%> (ø)`
...pportbundlecollection/support_bundle_controller.go	`0.00% <0.00%> (ø)`
pkg/util/compress/compress.go	`54.54% <54.54%> (ø)`
...kg/apiserver/registry/system/supportbundle/rest.go	`79.63% <100.00%> (+0.50%)`	⬆️
pkg/agent/proxy/endpointslicecache.go	`0.00% <0.00%> (-83.60%)`	⬇️
pkg/agent/flowexporter/exporter/certificate.go	`0.00% <0.00%> (-50.00%)`	⬇️
...g/agent/controller/serviceexternalip/controller.go	`35.83% <0.00%> (-45.74%)`	⬇️
pkg/agent/cniserver/ipam/antrea_ipam.go	`51.08% <0.00%> (-22.95%)`	⬇️
...nt/apiserver/handlers/serviceexternalip/handler.go	`29.62% <0.00%> (-22.23%)`	⬇️
pkg/agent/ipassigner/responder/arp_responder.go	`55.29% <0.00%> (-17.65%)`	⬇️
... and 64 more

lgtm-com · 2022-10-26T04:19:10Z

This pull request introduces 1 alert when merging 300dbab into 2971e09 - view on LGTM.com

new alerts:

1 for Use of insecure HostKeyCallback implementation

lgtm-com · 2022-10-26T08:53:23Z

This pull request introduces 1 alert when merging c34d1a3 into 2971e09 - view on LGTM.com

new alerts:

1 for Use of insecure HostKeyCallback implementation

lgtm-com · 2022-10-31T08:44:47Z

This pull request introduces 1 alert when merging e426eff into e7aba77 - view on LGTM.com

new alerts:

1 for Use of insecure HostKeyCallback implementation

lgtm-com · 2022-11-02T03:13:05Z

This pull request introduces 1 alert when merging 00f2e0f into e7aba77 - view on LGTM.com

new alerts:

1 for Use of insecure HostKeyCallback implementation

pkg/agent/controller/supportbundlecollection/support_bundle_controller.go

wenyingd · 2022-11-02T03:28:21Z

pkg/agent/controller/supportbundlecollection/support_bundle_controller.go

+		if strings.ToLower(arr[0]) != "sftp" {
+			return fmt.Errorf("only sftp is supported for now, protocol %s is not supported", strings.ToLower(arr[0]))
+		}
+		url = fileServer.URL[7:]


Better to calculate the index for url instead of hard-coded with 7 ?

I thought about it. But the prefix length is fixed which is seven. If we use index, it would be like strings.Index(fileServer.URL, "://") + 3, which is not necessary in my mind.

pkg/agent/controller/supportbundlecollection/support_bundle_controller.go

pkg/agent/controller/supportbundlecollection/support_bundle_controller_test.go

lgtm-com · 2022-11-02T03:58:20Z

This pull request introduces 1 alert when merging 39f2c7c into e7aba77 - view on LGTM.com

new alerts:

1 for Use of insecure HostKeyCallback implementation

lgtm-com · 2022-11-04T08:28:39Z

This pull request introduces 1 alert when merging 7ca6995 into 85e144c - view on LGTM.com

new alerts:

1 for Use of insecure HostKeyCallback implementation

lgtm-com · 2022-11-08T08:21:17Z

This pull request introduces 1 alert when merging 5094000 into 339ed6f - view on LGTM.com

new alerts:

1 for Use of insecure HostKeyCallback implementation

lgtm-com · 2022-11-08T11:43:55Z

This pull request introduces 1 alert when merging 345a488 into 339ed6f - view on LGTM.com

new alerts:

1 for Use of insecure HostKeyCallback implementation

mengdie-song · 2022-11-09T07:45:01Z

/test-all

lgtm-com · 2022-11-09T07:52:51Z

This pull request introduces 1 alert when merging c72beea into e516aca - view on LGTM.com

new alerts:

1 for Use of insecure HostKeyCallback implementation

lgtm-com · 2022-11-10T09:24:48Z

This pull request introduces 1 alert when merging 9471a3c into e516aca - view on LGTM.com

new alerts:

1 for Use of insecure HostKeyCallback implementation

lgtm-com · 2022-11-11T07:51:37Z

This pull request introduces 1 alert when merging 5803c73 into 5051f54 - view on LGTM.com

new alerts:

1 for Use of insecure HostKeyCallback implementation

mengdie-song · 2022-12-02T09:28:24Z

/test-all

lgtm-com · 2022-12-02T09:52:13Z

This pull request introduces 1 alert when merging ba22c7f into 19a7de0 - view on LGTM.com

new alerts:

1 for Use of insecure HostKeyCallback implementation

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

lgtm-com · 2022-12-02T12:15:25Z

This pull request introduces 1 alert when merging 0b07cfe into 19a7de0 - view on LGTM.com

new alerts:

1 for Use of insecure HostKeyCallback implementation

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

wenyingd · 2022-12-05T01:24:40Z

pkg/agent/supportbundlecollection/support_bundle_controller.go

+
+func (c *SupportBundleController) addSupportBundleCollection(supportBundle *cpv1b2.SupportBundleCollection) {
+	c.supportBundleCollectionsMutex.Lock()
+	defer c.supportBundleCollectionsMutex.Unlock()


Unlock the mutex after cosuming resource, no need to wait until it is added into queue?

c.supportBundleCollectionsMutex.Lock() c.supportBundleCollection = supportBundle c.supportBundleCollectionsMutex.Unlock() c.queue.Add(supportBundle.Name)

wenyingd · 2022-12-05T01:24:48Z

pkg/agent/supportbundlecollection/support_bundle_controller.go

+}
+
+func (c *SupportBundleController) deleteSupportBundleCollection(supportBundle *cpv1b2.SupportBundleCollection) {
+	c.supportBundleCollectionsMutex.Lock()


wenyingd · 2022-12-05T01:30:15Z

pkg/agent/supportbundlecollection/support_bundle_controller.go

+	antreaClientGetter            agent.AntreaClientProvider
+	queue                         workqueue.Interface
+	supportBundleCollection       *cpv1b2.SupportBundleCollection
+	supportBundleCollectionsMutex sync.RWMutex


Suggested change

supportBundleCollectionsMutex sync.RWMutex

supportBundleCollectionMutex sync.RWMutex

luolanzone · 2022-12-05T06:42:43Z

@ceclinux Could you address the latest comments of this PR and move forward? thanks.

lgtm-com · 2022-12-06T06:12:44Z

This pull request introduces 1 alert when merging 1f5f0e4 into b0e9636 - view on LGTM.com

new alerts:

1 for Use of insecure HostKeyCallback implementation

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

tnqn

LGTM, two minor comments

tnqn · 2022-12-06T06:30:52Z

pkg/agent/supportbundlecollection/support_bundle_controller.go

+	klog.Info("Started watch for SupportBundleCollections")
+	eventCount := 0
+	defer func() {
+		klog.InfoS("Stopped watch for SupportBundleCollections", "total items received", eventCount)


sturctured logging's keys shouldn't be space connected words, see https://github.com/tnqn/code-review-comments#use-structure-logging.
"totalItemsReceived"

tnqn · 2022-12-06T12:29:15Z

...lientset/versioned/typed/controlplane/v1beta2/fake/fake_supportbundlecollection_expansion.go

+	obj, err := c.Fake.
+		Invokes(testing.NewUpdateSubresourceAction(supportbundlecollectionsResource, "status", "", status), &v1beta2.SupportBundleCollectionStatus{})
+
+	if obj == nil {
+		return nil
+	}
+	return err


Suggested change

obj, err := c.Fake.

Invokes(testing.NewUpdateSubresourceAction(supportbundlecollectionsResource, "status", "", status), &v1beta2.SupportBundleCollectionStatus{})

if obj == nil {

return nil

}

return err

_, err := c.Fake.

Invokes(testing.NewUpdateSubresourceAction(supportbundlecollectionsResource, "status", "", status), &v1beta2.SupportBundleCollectionStatus{})

return err

wenyingd · 2022-12-06T14:25:18Z

pkg/agent/supportbundlecollection/support_bundle_controller.go

+		return
+	}
+	options := metav1.ListOptions{
+		FieldSelector: fields.OneTermEqualSelector("nodeName", c.nodeName).String(),


Use k8s.NamespacedName(c.namespace, c.nodeName) as the ListOption with the "ExternalNode" case, which is expected after this PR #4401 is merged. Otherwise, ExternalNode would use an incorrect value when controller calculates the span.

lgtm-com · 2022-12-07T00:39:16Z

This pull request introduces 1 alert when merging 424a31b into e94e4cf - view on LGTM.com

new alerts:

1 for Use of insecure HostKeyCallback implementation

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

wenyingd · 2022-12-07T01:17:56Z

pkg/agent/supportbundlecollection/support_bundle_controller.go

+		klog.ErrorS(err, "Failed to get antrea client")
+		return
+	}
+	options := metav1.ListOptions{


You check use namespace/name as filter key only with ExternalNode case by checking c.supportBundleNodeType == controlplane.SupportBundleCollectionNodeTypeExternalNode. But for K8s worker Node, c.nodeName is expected.

1. Watch internal SupportBundleCollection events. 2. Refactor SupportBundle compression to util. 3. Let agent to collect SupportBundle for Node/ExternalNode. 4. Upload the collected SupportBundle to file server. Only SFTP and basic authentication are supported for now. Signed-off-by: Mengdie Song <songm@vmware.com> Co-authored-by: Ruochen Shen <src655@gmail.com>

wenyingd

LGTM

tnqn

LGTM

tnqn · 2022-12-07T02:06:31Z

/test-all

lgtm-com · 2022-12-07T02:19:47Z

This pull request introduces 1 alert when merging 2f60340 into e94e4cf - view on LGTM.com

new alerts:

1 for Use of insecure HostKeyCallback implementation

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

ceclinux · 2022-12-07T04:38:57Z

pkg/agent/supportbundlecollection/support_bundle_controller.go

+	cfg := &ssh.ClientConfig{
+		User: serverAuth.BasicAuthentication.Username,
+		Auth: []ssh.AuthMethod{ssh.Password(serverAuth.BasicAuthentication.Password)},
+		// #nosec G106: skip host key check here and users can specify their own checks if needed


I think there might be some potential issues with the ssh ClientConfig

The comment is not working to pass the security checker [ExternalNode]Implement SupportBundleCollection on Agent #4338 (comment).

I am confused about the phrase "users specify their own checks if needed."

Also, is the security vulnerability critical in this case? Is it possible that the supportbunble tarball can be captured by the attackers through this security vulnerability?

I might have missed some context. Appreciate it if you provide some comments. @tnqn @wenyingd @mengdie-song @jianjuns

I think your question is about the ssh configuration option “HostKeyCallback: ssh.InsecureIgnoreHostKey()“. Our choice is to ignore the public key returned by the server because we do not perform check on the key in our implementation. A candidate security risk for a general SSH connection is the target SFTP server connected with the IP address is not the one who has a different public key from the one client used to connect to. But in support bundle collection scenario, the user should ensure the sftp server is valid and accessible from Node/ExternalNode when creating the CR, and Agent is not supposed to connect to an incorrect server using the url provided by user. So I don't think it may introduce security risk to Agent. In another word, even Agent perform actions to cache and validate the public key, nothing could perform in the next step when we found they are dismatched.

) 1. Watch internal SupportBundleCollection events. 2. Refactor SupportBundle compression to util. 3. Let agent to collect SupportBundle for Node/ExternalNode. 4. Upload the collected SupportBundle to file server. Only SFTP and basic authentication are supported for now. Signed-off-by: Mengdie Song <songm@vmware.com> Co-authored-by: Ruochen Shen <src655@gmail.com>

mengdie-song force-pushed the vmbm-supportbundle-agentimpl branch 2 times, most recently from 6ad148a to c34d1a3 Compare October 26, 2022 08:22

mengdie-song force-pushed the vmbm-supportbundle-agentimpl branch from c34d1a3 to e426eff Compare October 31, 2022 08:22

mengdie-song changed the title ~~[WIP]Implement SupportBundleCollection on Agent~~ Implement SupportBundleCollection on Agent Nov 1, 2022

mengdie-song changed the title ~~Implement SupportBundleCollection on Agent~~ [ExternalNode]Implement SupportBundleCollection on Agent Nov 2, 2022

mengdie-song requested review from wenyingd, tnqn and jianjuns November 2, 2022 02:39

XinShuYang force-pushed the vmbm-supportbundle-agentimpl branch from e426eff to 00f2e0f Compare November 2, 2022 02:48

XinShuYang force-pushed the vmbm-supportbundle-agentimpl branch from 00f2e0f to 39f2c7c Compare November 2, 2022 03:33

wenyingd reviewed Nov 2, 2022

View reviewed changes

XinShuYang force-pushed the vmbm-supportbundle-agentimpl branch from 39f2c7c to 7ca6995 Compare November 4, 2022 08:05

XinShuYang force-pushed the vmbm-supportbundle-agentimpl branch from 7ca6995 to 5094000 Compare November 8, 2022 07:58

XinShuYang force-pushed the vmbm-supportbundle-agentimpl branch from 5094000 to 345a488 Compare November 8, 2022 11:21

XinShuYang force-pushed the vmbm-supportbundle-agentimpl branch from 345a488 to c72beea Compare November 9, 2022 07:26

XinShuYang force-pushed the vmbm-supportbundle-agentimpl branch from c72beea to 9471a3c Compare November 10, 2022 09:00

XinShuYang force-pushed the vmbm-supportbundle-agentimpl branch from 9471a3c to 5803c73 Compare November 11, 2022 07:25

vicky-liu added this to the Antrea v1.10 release milestone Dec 2, 2022

mengdie-song force-pushed the vmbm-supportbundle-agentimpl branch from ba22c7f to 0b07cfe Compare December 2, 2022 11:52

wenyingd reviewed Dec 5, 2022

View reviewed changes

luolanzone assigned ceclinux Dec 5, 2022

luolanzone closed this Dec 6, 2022

luolanzone removed this from the Antrea v1.10 release milestone Dec 6, 2022

ceclinux mentioned this pull request Dec 6, 2022

[ExternalNode] Implement SupportBundleCollection on Agent #4441

Closed

mengdie-song reopened this Dec 6, 2022

ceclinux force-pushed the vmbm-supportbundle-agentimpl branch from 0b07cfe to 1f5f0e4 Compare December 6, 2022 05:47

luolanzone added this to the Antrea v1.10 release milestone Dec 6, 2022

tnqn reviewed Dec 6, 2022

View reviewed changes

wenyingd reviewed Dec 6, 2022

View reviewed changes

ceclinux force-pushed the vmbm-supportbundle-agentimpl branch from 1f5f0e4 to 424a31b Compare December 7, 2022 00:01

wenyingd reviewed Dec 7, 2022

View reviewed changes

ceclinux force-pushed the vmbm-supportbundle-agentimpl branch from 424a31b to 2f60340 Compare December 7, 2022 01:50

wenyingd approved these changes Dec 7, 2022

View reviewed changes

tnqn approved these changes Dec 7, 2022

View reviewed changes

tnqn merged commit 1c73bd1 into antrea-io:main Dec 7, 2022

ceclinux reviewed Dec 7, 2022

View reviewed changes

	supportBundleCollectionsMutex sync.RWMutex
	supportBundleCollectionMutex sync.RWMutex

[ExternalNode]Implement SupportBundleCollection on Agent #4338

[ExternalNode]Implement SupportBundleCollection on Agent #4338

Conversation

mengdie-song commented Oct 26, 2022

codecov bot commented Oct 26, 2022 • edited Loading

Codecov Report

lgtm-com bot commented Oct 26, 2022

lgtm-com bot commented Oct 26, 2022

lgtm-com bot commented Oct 31, 2022

lgtm-com bot commented Nov 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Nov 2, 2022

lgtm-com bot commented Nov 4, 2022

lgtm-com bot commented Nov 8, 2022

lgtm-com bot commented Nov 8, 2022

mengdie-song commented Nov 9, 2022

lgtm-com bot commented Nov 9, 2022

lgtm-com bot commented Nov 10, 2022

lgtm-com bot commented Nov 11, 2022

mengdie-song commented Dec 2, 2022

lgtm-com bot commented Dec 2, 2022

lgtm-com bot commented Dec 2, 2022

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

luolanzone commented Dec 5, 2022

lgtm-com bot commented Dec 6, 2022

tnqn 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

wenyingd Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Dec 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenyingd left a comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Dec 7, 2022

lgtm-com bot commented Dec 7, 2022

Choose a reason for hiding this comment

wenyingd Dec 21, 2022 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Oct 26, 2022 •

edited

Loading

wenyingd Dec 6, 2022 •

edited

Loading

wenyingd Dec 21, 2022 •

edited

Loading