Move mutable configmaps out of deployment manifest #1983

hty690 · 2021-03-23T06:20:13Z

Moving "antrea-ca" and "antrea-cluster-identity" out of deployment manifest.
Instead creating them in the code.

Fixes #1945

codecov-io · 2021-03-23T06:25:05Z

Codecov Report

Merging #1983 (79cc8e4) into main (e99a0bf) will increase coverage by 6.68%.
The diff coverage is 42.30%.

@@            Coverage Diff             @@
##             main    #1983      +/-   ##
==========================================
+ Coverage   60.10%   66.78%   +6.68%     
==========================================
  Files         197      197              
  Lines       17217    17259      +42     
==========================================
+ Hits        10348    11527    +1179     
+ Misses       5762     4543    -1219     
- Partials     1107     1189      +82

Flag	Coverage Δ
e2e-tests	`26.45% <38.46%> (?)`
kind-e2e-tests	`56.49% <40.38%> (+10.62%)`	⬆️
unit-tests	`41.74% <3.84%> (-0.11%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/flowaggregator/certificate.go	`0.00% <0.00%> (ø)`
pkg/apiserver/certificate/cacert_controller.go	`58.95% <64.70%> (+24.78%)`	⬆️
pkg/clusteridentity/clusteridentity.go	`71.25% <64.70%> (+3.06%)`	⬆️
pkg/agent/route/route_linux.go	`46.30% <0.00%> (+0.32%)`	⬆️
pkg/agent/controller/networkpolicy/cache.go	`85.29% <0.00%> (+0.65%)`	⬆️
pkg/agent/cniserver/pod_configuration.go	`54.68% <0.00%> (+0.78%)`	⬆️
pkg/controller/grouping/group_entity_index.go	`94.58% <0.00%> (+1.27%)`	⬆️
pkg/apiserver/storage/ram/store.go	`81.95% <0.00%> (+1.50%)`	⬆️
...ntroller/networkpolicy/networkpolicy_controller.go	`71.10% <0.00%> (+1.62%)`	⬆️
pkg/agent/agent.go	`48.92% <0.00%> (+1.67%)`	⬆️
... and 46 more

tnqn

What about flow-aggregator-ca?

tnqn · 2021-03-23T07:54:51Z

pkg/apiserver/certificate/cacert_controller.go

+		if !errors.IsNotFound(err) {
+			return fmt.Errorf("error getting ConfigMap %s: %v", CAConfigMapName, err)
+		}
+		caConfigMap, err = c.client.CoreV1().ConfigMaps(caConfigMapNamespace).Create(context.TODO(), c.createConfigMap(caConfigMapNamespace), metav1.CreateOptions{})


why don't create the configmap with the desired content directly which can avoid an extra call?
Also feel there is no need to have another method to construct a configmap, it's just one line code that is not shared by others.

jianjuns

Do you plan to cover flow-aggregator-ca as @antoninbas suggested?

hty690 · 2021-03-24T00:55:53Z

Do you plan to cover flow-aggregator-ca as @antoninbas suggested?

Yes. I'm working on it.

tnqn

LGTM overall

tnqn · 2021-03-24T05:07:04Z

pkg/clusteridentity/clusteridentity.go

+				Namespace: a.clusterIdentityConfigMapNamespace,
+			},
+			Data:       map[string]string{},
+			BinaryData: map[string][]byte{},


No need to specify the two fields if they are not set

tnqn · 2021-03-24T05:08:57Z

pkg/flowaggregator/certificate.go

+				Namespace: CAConfigMapNamespace,
+			},
+			Data:       map[string]string{},
+			BinaryData: map[string][]byte{},


tnqn

LGTM, thanks

tnqn · 2021-03-24T15:07:56Z

/test-all

antoninbas · 2021-03-24T20:59:17Z

build/yamls/base/controller-rbac.yml

+  - apiGroups:
+      - ""
+    resources:
+      - configmaps
+    verbs:
+      - create


I may be missing something obvious but why don't we just add the "create" verb to the list above:

- apiGroups: - "" resources: - configmaps resourceNames: - antrea-ca - antrea-cluster-identity verbs: - get - update - create

I wondered too, and thought maybe "create" does not work with resourceNames, but just checked and found it could. Then we should add create to the existing verb list.

I added "create" to this verb list and I met the error
error when creating 'kube-system/antrea-cluster-identity' ConfigMap: configmaps is forbidden: User "system:serviceaccount:kube-system:antrea-controller" cannot create resource "configmaps" in API group "" in the namespace "kube-system"

I found it works. I will recommit the changes.

Specifying resourceNames for "create" verb doesn't work: https://kubernetes.io/docs/reference/access-authn-authz/rbac/#referring-to-resources

Note: You cannot restrict create or deletecollection requests by resourceName. For create, this limitation is because the object name is not known at authorization time.

I did a test and it's consistent with the doc, how did you test?

I deleted and reapplied the yaml. And I found "antrea-ca" and "antrea-cluster-identity" was not deleted. and recreated. Maybe that's why I tested it with no error.

Ok, then the current yaml makes sense.

tnqn · 2021-03-25T05:27:56Z

pkg/flowaggregator/certificate.go

+		}
+		exists = false
+		caConfigMap = &v1.ConfigMap{
+			ObjectMeta: metav1.ObjectMeta{


Please add the label as the previous one in manifest yaml, in case we need a way to filter all antrea resources later.

Moving "antrea-ca", "antrea-cluster-identity" and "flow-aggregator-ca" out of deployment manifest. Instead creating them in the code. Fixes antrea-io#1945

tnqn

LGTM

tnqn · 2021-03-25T06:21:07Z

/test-all

tnqn · 2021-03-26T04:01:38Z

/test-e2e

tnqn reviewed Mar 23, 2021

View reviewed changes

jianjuns reviewed Mar 23, 2021

View reviewed changes

hty690 force-pushed the mutable_configmap branch from 1b96532 to 3a99782 Compare March 24, 2021 02:33

tnqn reviewed Mar 24, 2021

View reviewed changes

hty690 force-pushed the mutable_configmap branch from 3a99782 to 6226657 Compare March 24, 2021 05:25

tnqn previously approved these changes Mar 24, 2021

View reviewed changes

jianjuns previously approved these changes Mar 24, 2021

View reviewed changes

antoninbas reviewed Mar 24, 2021

View reviewed changes

hty690 dismissed stale reviews from jianjuns and tnqn via 91285dc March 25, 2021 01:15

hty690 force-pushed the mutable_configmap branch from 91285dc to 6226657 Compare March 25, 2021 04:54

tnqn reviewed Mar 25, 2021

View reviewed changes

Move mutable configmaps out of deployment manifest

79cc8e4

Moving "antrea-ca", "antrea-cluster-identity" and "flow-aggregator-ca" out of deployment manifest. Instead creating them in the code. Fixes antrea-io#1945

hty690 force-pushed the mutable_configmap branch from 6226657 to 79cc8e4 Compare March 25, 2021 05:56

tnqn approved these changes Mar 25, 2021

View reviewed changes

antoninbas added this to the Antrea v1.0 release milestone Mar 30, 2021

antoninbas merged commit 2ec1b53 into antrea-io:main Mar 30, 2021

hty690 deleted the mutable_configmap branch April 14, 2021 02:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move mutable configmaps out of deployment manifest #1983

Move mutable configmaps out of deployment manifest #1983

hty690 commented Mar 23, 2021

codecov-io commented Mar 23, 2021 •

edited

Loading

tnqn left a comment

tnqn Mar 23, 2021

jianjuns left a comment •

edited

Loading

hty690 commented Mar 24, 2021

tnqn left a comment

tnqn Mar 24, 2021

tnqn Mar 24, 2021

tnqn left a comment

tnqn commented Mar 24, 2021

antoninbas Mar 24, 2021

jianjuns Mar 24, 2021

hty690 Mar 25, 2021 •

edited

Loading

hty690 Mar 25, 2021

tnqn Mar 25, 2021

hty690 Mar 25, 2021

jianjuns Mar 25, 2021

tnqn Mar 25, 2021

tnqn left a comment

tnqn commented Mar 25, 2021

tnqn commented Mar 26, 2021

Move mutable configmaps out of deployment manifest #1983

Move mutable configmaps out of deployment manifest #1983

Conversation

hty690 commented Mar 23, 2021

codecov-io commented Mar 23, 2021 • edited Loading

Codecov Report

tnqn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianjuns left a comment • edited Loading

Choose a reason for hiding this comment

hty690 commented Mar 24, 2021

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

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Mar 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hty690 Mar 25, 2021 • 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

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Mar 25, 2021

tnqn commented Mar 26, 2021

codecov-io commented Mar 23, 2021 •

edited

Loading

jianjuns left a comment •

edited

Loading

hty690 Mar 25, 2021 •

edited

Loading