[ExternalNode] Implement SupportBundleCollection on Controller #4184

wenyingd · 2022-08-31T12:49:49Z

Add internal objects to sync the suport bundle collection request from
Controller to Agent.
Implement support bundle collection on Controller side.

Signed-off-by: wenyingd [email protected]

codecov · 2022-08-31T13:13:34Z

Codecov Report

Merging #4184 (e101c6c) into main (835e142) will decrease coverage by 2.51%.
The diff coverage is 75.31%.

❗ Current head e101c6c differs from pull request most recent head 76b96dd. Consider uploading reports for the commit 76b96dd to get more accurate results

@@            Coverage Diff             @@
##             main    #4184      +/-   ##
==========================================
- Coverage   64.38%   61.87%   -2.52%     
==========================================
  Files         393      389       -4     
  Lines       55538    55142     -396     
==========================================
- Hits        35760    34117    -1643     
- Misses      17188    18498    +1310     
+ Partials     2590     2527      -63

Flag	Coverage Δ
integration-tests	`34.89% <57.14%> (+0.28%)`	⬆️
kind-e2e-tests	`45.15% <10.95%> (-3.72%)`	⬇️
unit-tests	`44.21% <71.27%> (-3.94%)`	⬇️

Impacted Files	Coverage Δ
cmd/antrea-controller/controller.go	`0.00% <0.00%> (ø)`
pkg/apis/controlplane/types.go	`100.00% <ø> (ø)`
pkg/apis/controlplane/v1beta2/helper.go	`100.00% <ø> (ø)`
pkg/features/antrea_features.go	`60.00% <ø> (ø)`
pkg/controller/supportbundlecollection/validate.go	`70.83% <70.83%> (ø)`
...g/controller/supportbundlecollection/controller.go	`75.92% <75.92%> (ø)`
...istry/controlplane/supportbundlecollection/rest.go	`79.48% <79.48%> (ø)`
...roller/supportbundlecollection/store/collection.go	`81.69% <81.69%> (ø)`
pkg/apis/controlplane/register.go	`85.18% <100.00%> (-6.49%)`	⬇️
pkg/apis/controlplane/v1beta2/conversion.go	`66.66% <100.00%> (ø)`
... and 114 more

wenyingd · 2022-09-09T09:54:38Z

/test-all
/test-ipv6-all
/test-windows-all

mengdie-song · 2022-09-19T07:10:17Z

build/charts/antrea/templates/controller/rolebinding.yaml

+subjects:
+  - kind: ServiceAccount
+    name: antrea-controller
+    namespace: {{ .Release.Namespace }}


By this definition, we expect that the authSecret is under namespace kube-system? If so, we need to at least add some comments for this. Otherwise, controller will get RBAC problem if the namespace defined in the SupportBundleCollection is not kube-system.

User should modify the namespace for role antrea-read-secrets in the RoleBinding according to his own setup. This is a sample for antrea.

I would like to describe this in a future patch dedicated for documentation.

build/charts/antrea/templates/controller/clusterrole.yaml

jianjuns · 2022-09-19T23:02:26Z

build/charts/antrea/templates/controller/clusterrole.yaml

+    verbs:
+      - update
+---
+kind: ClusterRole


Could we publish these YAMLs with Nephe as they are not needed by Antrea for K8s?

In this way, the upstream user can not use this feature with on-prem VMs?

My concern is about adding permissions for controller to read all Secrets. We should not do that for a regular K8s cluster deployment. For non-Nephe use cases, could we just document the role binding requirement?

I remembered @reachjainrahul said that Nephe would not do much on the support bundle feature, so I doubt maybe it is not a better choice to place to the RBAC changes in Nephe.

If your concern is the impact on regular K8s cluster, I would prefer to move this special ClusterRole/RoleBinding out from the default antrea yaml, but place it to path build/yamls/externalnode/, just like "vm-agent-rbac.yaml". What is your thought?

Does it mean users (inc. Nephe) need to apply another YAML to use the feature? I am fine with that, but feel easier for Nephe to include it in its YAML.

Does it mean users (inc. Nephe) need to apply another YAML to use the feature?

Yes. But if we only have it in Nephe but do not maintain it in antrea, the non-Nephe users can not use this feature.

We can provide a YAML in Antrea too. My question is just should we include the role binding in the Nephe YAML to simplify Nephe deployment.

In my mind, Nephe is not planned with support bundle collection feature. @reachjainrahul please correct me, and share your thought?

jianjuns · 2022-09-19T23:04:35Z

cmd/antrea-controller/controller.go

@@ -168,8 +171,12 @@ func run(o *Options) error {
 		groupStore)

 	var externalNodeController *externalnode.ExternalNodeController
+	var bundleCollectionController *supportbundlecollection.Controller


Maybe we should add another feature gate for support bundle collection?

Sure, I will add it.

jianjuns · 2022-09-19T23:09:33Z

pkg/apis/controlplane/types.go

+	Items           []SupportBundleCollection `json:"items" protobuf:"bytes,2,rep,name=items"`
+}
+
+type BundleFileServer struct {


Can we reuse the CRD definition?

Updated to reuse CRD definition for BundleFileServer.

jianjuns · 2022-09-19T23:10:04Z

pkg/apis/controlplane/types.go

+	Password string
+}
+
+type BundleServerAuthConfiguration struct {


I don't think we can reuse CRD definition for BundleServerAuthConfiguration, because a Secret reference is used in the CRD definition for the authenticator, while in the control plane API, the field is configured with the actual authenticator parsed from the Secret by Controller.

jianjuns · 2022-09-19T23:10:59Z

pkg/apis/controlplane/types.go

+	NodeName string
+	// The Namespace of the Node produces the status. It is set only when NodeType is externalNode
+	NodeNamespace string
+	// The type of the Node that produces the status. The values include Node and ExternalNode.


The supported values are "Node" and "ExternalNode". Could we reuse the CRD definition?

Updated the comment information. I don't think we can use the CRD definition for SupportBundleCollectionNodeStatus: the CRD definition does not provide per Node status, but uses Conditions to describe the summarized SupportBundleCollection status. But for control plane API, we need per Node/ExternalNode to report its own status first, and then aggregated in Controller and update in the CRD status. And some existing fields ( like desiredNodes/succeededNodes) in CRD definition are not suitable for per Node status report.

pkg/controller/supportbundlecollection/store/supportbundlecollection.go

pkg/controller/types/supportbundlecollection.go

build/charts/antrea/conf/antrea-controller.conf

pkg/controller/supportbundlecollection/controller.go

jianjuns · 2022-09-22T01:04:19Z

pkg/controller/supportbundlecollection/controller.go

+	klog.V(2).InfoS("Processed Node Add event", "name", node.Name)
+}
+
+func (c *Controller) updateNode(oldObj interface{}, newObj interface{}) {


Do we need to handle Node updates? Could we ignore Node updates after CRD is processed for simplicity? Anyway, we have no way to guarantee all Node updates are covered as update events are async.

I removed updateNode and updateExternalNode.

pkg/controller/supportbundlecollection/validate.go

pkg/controller/supportbundlecollection/controller.go

tnqn · 2022-09-29T13:21:34Z

pkg/apis/controlplane/types.go

+type BundleFileServer struct {
+	// The URL of the bundle file server. It is set with format: scheme://host[:port][/path],
+	// e.g, https://api.example.com:8443/v1/supportbundles/. If scheme is not set, https is used by default.
+	URL string `json:"url"`


The unversioned structs don't need json tag

pkg/apis/crd/v1alpha1/types.go

pkg/controller/supportbundlecollection/controller.go

tnqn · 2022-09-29T14:18:09Z

pkg/controller/supportbundlecollection/controller.go

+		klog.ErrorS(err, "Failed to get authentication defined in the SupportBundleCollection CR", "name", bundle.Name, "authentication", bundle.Spec.Authentication)
+		return err
+	}
+	c.addInternalSupportBundleCollection(bundle, nodeSpan, *authentication, metav1.NewTime(expiredAt))


It seems it never updates started condition to true?

I moved it to status_controller part #4249 after the internal object is created.

pkg/controller/supportbundlecollection/store/collection.go

tnqn · 2022-09-29T14:23:10Z

pkg/controller/supportbundlecollection/validate.go

+				allowed = false
+			}
+			if !allowed {
+				msg = fmt.Sprintf("SupportBundleCollection %s is started, cannot be updated or deleted", oldObj.Name)


should the message be updated? can a started request be deleted?
How do we know it has started here?

tnqn · 2022-09-30T15:51:12Z

pkg/antctl/raw/multicluster/join.go

@@ -217,13 +217,13 @@ func NewJoinCommand() *cobra.Command {
 	command.Flags().StringVarP(&joinOpts.LeaderNamespace, "leader-namespace", "", "", "Namespace of the leader cluster")
 	command.Flags().StringVarP(&joinOpts.LeaderClusterID, "leader-clusterid", "", "", "Cluster ID of the leader cluster")
 	command.Flags().StringVarP(&joinOpts.TokenSecretName, "token-secret-name", "", "", "Name of the Secret resource that contains the member token. "+
-		"Token Secret name takes precedence over token Secret file and the Secret manifest in the join config file")
+		"BearerToken Secret name takes precedence over token Secret file and the Secret manifest in the join config file")


I think this is not an intended change?

No, it is replaced by IDE automatically, thanks for catching it.

pkg/config/flowaggregator/config.go

pkg/controller/supportbundlecollection/controller.go

wenyingd · 2022-10-17T06:18:12Z

/test-all

pkg/controller/supportbundlecollection/controller_test.go

tnqn · 2022-10-25T03:44:45Z

pkg/controller/supportbundlecollection/controller_test.go

+	return nodeConfigs, externalNodeConfigs
+}
+
+func TestAddSupportBundleCollection(t *testing.T) {


The test seems more complicated than it needs to be. To test addSupportBundleCollection, I think it just needs to check whether the queue contains expected items with specific inputs:

tests := []struct{}{ name string supportBundleCollection *SupportBundleCollection expectedItem string } { ... } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { controller := ... controller.addSupportBundleCollection(tt.supportBundleCollection) if tt.expectedItem != "" { assert.Equal(t, 1, controller.queue.Len()) gotItem := controller.queue.Get() assert.Equal(tt.expectedItem, gotItem) } else { assert.Equal(t, 0, controller.queue.Len()) } } }

tnqn

LGTM, typo in commit message: Controlelr

wenyingd · 2022-10-26T08:23:38Z

LGTM, typo in commit message: Controlelr

updated.

wenyingd · 2022-10-26T08:23:48Z

/test-all

Add internal objects to sync the suport bundle request from Controller to Agent, and report status from Agent to Controller. Implement support bundle collection on Controller side. Signed-off-by: wenyingd <[email protected]>

wenyingd · 2022-10-26T10:36:21Z

Update commit message only.

tnqn

LGTM

wenyingd · 2022-10-27T02:18:15Z

/test-all

…antrea-io#4184) Add internal objects to sync the suport bundle request from Controller to Agent, and report status from Agent to Controller. Implement support bundle collection on Controller side. Signed-off-by: wenyingd <[email protected]>

wenyingd force-pushed the supportbundle_internalapi branch 8 times, most recently from f498493 to 038afe2 Compare September 6, 2022 11:35

wenyingd force-pushed the supportbundle_internalapi branch 5 times, most recently from fa9ba32 to 01b4976 Compare September 9, 2022 09:27

wenyingd changed the title ~~[WIP] [ExternalNode] Implement SupportBundleCollection on Controller~~ [ExternalNode] Implement SupportBundleCollection on Controller Sep 9, 2022

wenyingd requested review from tnqn, mengdie-song and jianjuns September 13, 2022 03:18

wenyingd force-pushed the supportbundle_internalapi branch 2 times, most recently from 3318649 to ae3aba0 Compare September 19, 2022 03:02

mengdie-song reviewed Sep 19, 2022

View reviewed changes

wenyingd force-pushed the supportbundle_internalapi branch 2 times, most recently from 936ed8a to 88d17fc Compare September 19, 2022 11:36

jianjuns reviewed Sep 19, 2022

View reviewed changes

wenyingd force-pushed the supportbundle_internalapi branch 4 times, most recently from f6b019e to 8b094ea Compare September 21, 2022 06:12

jianjuns reviewed Sep 22, 2022

View reviewed changes

wenyingd requested a review from mengdie-song September 28, 2022 02:54

tnqn reviewed Sep 29, 2022

View reviewed changes

wenyingd force-pushed the supportbundle_internalapi branch from 060eae1 to 46b3ba5 Compare September 30, 2022 03:03

wenyingd requested a review from tnqn September 30, 2022 10:29

tnqn reviewed Sep 30, 2022

View reviewed changes

wenyingd force-pushed the supportbundle_internalapi branch from 46b3ba5 to eae14df Compare October 6, 2022 01:16

wenyingd requested a review from tnqn October 6, 2022 02:07

tnqn added this to the Antrea v1.9 release milestone Oct 11, 2022

wenyingd force-pushed the supportbundle_internalapi branch 2 times, most recently from f8fc31e to f72f7d4 Compare October 17, 2022 03:53

wenyingd force-pushed the supportbundle_internalapi branch from f72f7d4 to 930fd5d Compare October 18, 2022 10:49

wenyingd modified the milestones: Antrea v1.9 release, Antrea v1.10 release Oct 20, 2022

tnqn reviewed Oct 25, 2022

View reviewed changes

wenyingd force-pushed the supportbundle_internalapi branch from 930fd5d to 38a3330 Compare October 25, 2022 09:20

wenyingd requested a review from tnqn October 25, 2022 09:22

tnqn previously approved these changes Oct 25, 2022

View reviewed changes

wenyingd dismissed tnqn’s stale review via 76b96dd October 26, 2022 10:34

wenyingd force-pushed the supportbundle_internalapi branch from 38a3330 to 76b96dd Compare October 26, 2022 10:34

tnqn approved these changes Oct 26, 2022

View reviewed changes

tnqn merged commit 53d253a into antrea-io:main Oct 27, 2022

wenyingd deleted the supportbundle_internalapi branch October 27, 2022 12:27

[ExternalNode] Implement SupportBundleCollection on Controller #4184

[ExternalNode] Implement SupportBundleCollection on Controller #4184

Conversation

wenyingd commented Aug 31, 2022 • edited Loading

codecov bot commented Aug 31, 2022 • edited Loading

Codecov Report

wenyingd commented Sep 9, 2022

Choose a reason for hiding this comment

wenyingd Sep 19, 2022 • edited Loading

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 Sep 21, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenyingd commented Oct 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

wenyingd commented Oct 26, 2022

wenyingd commented Oct 26, 2022

wenyingd commented Oct 26, 2022

tnqn left a comment

Choose a reason for hiding this comment

wenyingd commented Oct 27, 2022

wenyingd commented Aug 31, 2022 •

edited

Loading

codecov bot commented Aug 31, 2022 •

edited

Loading

wenyingd Sep 19, 2022 •

edited

Loading

wenyingd Sep 21, 2022 •

edited

Loading