Reduce permission of antrea-agent service account by xliuxu · Pull Request #3691 · antrea-io/antrea

xliuxu · 2022-04-27T04:44:57Z

Remove the update permission for services/status of antrea-agent
service account. Remove the optimization for ExternalTrafficPolicy
setting to Local cases in ServiceExternalIP feature accordingly.

Signed-off-by: Xu Liu xliu2@vmware.com

tnqn

LGTM

xliuxu · 2022-04-27T12:23:17Z

@tnqn The e2e test failed because with this change the testing code cannot determine which Node owns the external IP. Most of the test cases will need to check the assigned Node by the hostname field in the status of a Service. I cannot find an easy way to figure out which Node owns the IP as they are not physically assigned. Do you think it is OK to remove all related test cases until we have a new API for assigned Node checking?

antoninbas · 2022-04-27T18:33:11Z

@tnqn The e2e test failed because with this change the testing code cannot determine which Node owns the external IP. Most of the test cases will need to check the assigned Node by the hostname field in the status of a Service. I cannot find an easy way to figure out which Node owns the IP as they are not physically assigned. Do you think it is OK to remove all related test cases until we have a new API for assigned Node checking?

@xliuxu we don't assign the IP to the transport interface?

xliuxu · 2022-04-28T01:37:56Z

@antoninbas No. For LB IP of Services, we do not assign IP to any interfaces on the Node. Otherwise, it will conflict with the implementation of proxyAll feature. We use userspace ARP/NDP responders to handle ARP/NDP queries instead.

tnqn · 2022-04-28T08:25:49Z

@xliuxu we need a way to check which Node owns the IP for troubleshooting anyway. Could we add a antctl get serviceexternalip command to agent mode to dump something like the following? Ideally, every Node should have consistent result eventually. If the result is not consistent, we could use this command/API for troubleshooting too.

NAMESPACE  NAME        EXTERNAL-IP   NODE
default    service-1   1.1.1.1       worker-1
default    service-2   1.1.1.2       worker-2

xliuxu · 2022-04-29T08:58:25Z

@tnqn Thanks for the suggestion. I have added a new command antctl get serviceexternalip for checking the status of the assigned Node and adjusted the e2e codes.

codecov-commenter · 2022-04-29T11:26:48Z

Codecov Report

Merging #3691 (fea1ef5) into main (2065919) will decrease coverage by 6.68%.
The diff coverage is 82.19%.

@@            Coverage Diff             @@
##             main    #3691      +/-   ##
==========================================
- Coverage   64.60%   57.91%   -6.69%     
==========================================
  Files         278      393     +115     
  Lines       39640    56425   +16785     
==========================================
+ Hits        25608    32678    +7070     
- Misses      12043    21257    +9214     
- Partials     1989     2490     +501

Flag	Coverage Δ
e2e-tests	`46.09% <80.82%> (?)`
integration-tests	`38.34% <ø> (?)`
kind-e2e-tests	`53.30% <80.82%> (+0.27%)`	⬆️
unit-tests	`43.79% <88.09%> (+0.05%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/antctl/antctl.go	`50.00% <ø> (ø)`
pkg/querier/querier.go	`55.00% <ø> (ø)`
...nt/apiserver/handlers/serviceexternalip/handler.go	`51.85% <51.85%> (ø)`
pkg/agent/apiserver/apiserver.go	`66.66% <100.00%> (+0.36%)`	⬆️
...g/agent/controller/serviceexternalip/controller.go	`81.56% <100.00%> (+0.19%)`	⬆️
pkg/agent/multicast/mcast_controller.go	`45.98% <0.00%> (-21.47%)`	⬇️
pkg/controller/networkpolicy/tier.go	`50.00% <0.00%> (-5.00%)`	⬇️
pkg/controller/serviceexternalip/controller.go	`68.67% <0.00%> (-4.82%)`	⬇️
...ntroller/networkpolicy/networkpolicy_controller.go	`70.17% <0.00%> (-0.88%)`	⬇️
pkg/agent/openflow/framework.go	`86.27% <0.00%> (-0.73%)`	⬇️
... and 138 more

tnqn

LGTM overall

tnqn

LGTM

tnqn · 2022-05-05T11:33:51Z

/test-all

tnqn · 2022-05-05T11:34:07Z

@antoninbas will you take another look?

antoninbas

some small comments and questions, but overall LGTM

antoninbas · 2022-05-05T18:11:23Z

 	s.Handler.NonGoRestfulMux.HandleFunc("/addressgroups", addressgroup.HandleFunc(npq))
 	s.Handler.NonGoRestfulMux.HandleFunc("/ovsflows", ovsflows.HandleFunc(aq))
 	s.Handler.NonGoRestfulMux.HandleFunc("/ovstracing", ovstracing.HandleFunc(aq))
+	s.Handler.NonGoRestfulMux.HandleFunc("/serviceexternalip", serviceexternalip.HandleFunc(seipq))


you may want to add the /serviceexternalip URL to the antctl RBAC ClusterRole, just for consistency (and in case someone is using the antctl ServiceAccount token to access antrea APIs directly)

antoninbas · 2022-05-05T18:13:20Z

+	info := make([]querier.ServiceExternalIPInfo, len(c.externalIPStates))
+	idx := 0
+	for k, v := range c.externalIPStates {
+		info[idx].ServiceName = k.Name
+		info[idx].Namespace = k.Namespace
+		info[idx].ExternalIP = v.ip
+		info[idx].ExternalIPPool = v.ipPool
+		info[idx].AssignedNode = v.assignedNode
+		idx++
+	}


nit: I think something like this is more elegant:

info := make([]querier.ServiceExternalIPInfo, 0, len(c.externalIPStates)) for k, v := range c.externalIPStates { info = append(info, querier.ServiceExternalIPInfo{ ServiceName: k.Name, // ... })

thanks. updated.

antoninbas · 2022-05-05T18:15:19Z

 	queue workqueue.RateLimitingInterface

-	externalIPStates      map[apimachinerytypes.NamespacedName]externalIPState
+	externalIPStates      map[apimachinerytypes.NamespacedName]*externalIPState


is there a reason for making this is a pointer? Based on usage in GetServiceExternalIPStatus, it doesn't seem necessary.

It is just for the defer statement to save the externalIPState. I have reverted this change since it is not necessary and can also be achieved by changing the signature of func saveServiceState.

tnqn

LGTM

Remove the update permission for services/status of antrea-agent service account. Remove the optimization for ExternalTrafficPolicy setting to Local cases in ServiceExternalIP feature accordingly. Introduce "antctl get serviceexternalip" command for the agent to make checking the assigned Node of external IPs easier. Signed-off-by: Xu Liu <xliu2@vmware.com>

xliuxu · 2022-05-06T03:48:09Z

 		t.Run(cmd, func(t *testing.T) {
 			stdout, stderr, err := runAntctl(podName, args, data)
-			if err != nil {
+			if err != nil && !strings.HasSuffix(stderr, "not enabled\n") {


@tnqn I just pushed a new commit with a single line change here to fix the e2e error for antctl.

tnqn · 2022-05-06T04:21:42Z

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

xliuxu · 2022-05-06T09:49:18Z

/test-networkpolicy
/test-windows-conformance
/test-ipv6-only-networkpolicy
/test-ipv6-e2e

antoninbas · 2022-05-06T18:51:38Z

@xliuxu please cherry-pick this to release-1.6

tnqn previously approved these changes Apr 27, 2022

View reviewed changes

tnqn requested a review from antoninbas April 27, 2022 05:20

tnqn added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. action/release-note Indicates a PR that should be included in release notes. action/backport Indicates a PR that requires backports. labels Apr 27, 2022

xliuxu dismissed tnqn’s stale review via ddb269f April 29, 2022 08:53

xliuxu force-pushed the permission_agent_service_status branch 2 times, most recently from ddb269f to ccbc603 Compare April 29, 2022 08:56

xliuxu requested a review from tnqn April 29, 2022 08:58

xliuxu force-pushed the permission_agent_service_status branch from ccbc603 to 0be085e Compare April 29, 2022 12:05

tnqn reviewed Apr 29, 2022

View reviewed changes

Comment thread cmd/antrea-agent/agent.go Outdated

Comment thread pkg/querier/querier.go Outdated

Comment thread test/e2e/service_externalip_test.go Outdated

xliuxu force-pushed the permission_agent_service_status branch 3 times, most recently from 7448c54 to 28d0dac Compare April 29, 2022 15:56

tnqn previously approved these changes May 5, 2022

View reviewed changes

antoninbas reviewed May 5, 2022

View reviewed changes

xliuxu dismissed tnqn’s stale review via fba7770 May 5, 2022 23:55

xliuxu force-pushed the permission_agent_service_status branch 3 times, most recently from 157d878 to 03b44ae Compare May 6, 2022 01:46

tnqn previously approved these changes May 6, 2022

View reviewed changes

xliuxu dismissed tnqn’s stale review via fea1ef5 May 6, 2022 03:46

xliuxu force-pushed the permission_agent_service_status branch from 03b44ae to fea1ef5 Compare May 6, 2022 03:46

xliuxu commented May 6, 2022

View reviewed changes

tnqn approved these changes May 6, 2022

View reviewed changes

antoninbas approved these changes May 6, 2022

View reviewed changes

antoninbas merged commit 627239c into antrea-io:main May 6, 2022

This was referenced May 7, 2022

Automated cherry pick of #3691: Reduce permission of antrea-agent service account #3743

Merged

Automated cherry pick of #3691: Reduce permission of antrea-agent service account #3749

Merged

antoninbas mentioned this pull request May 12, 2022

[Security] Too many RBAC permissions for the antrea-agent ServiceAccount #3777

Closed

Conversation

xliuxu commented Apr 27, 2022

Uh oh!

tnqn left a comment

Choose a reason for hiding this comment

Uh oh!

xliuxu commented Apr 27, 2022

Uh oh!

antoninbas commented Apr 27, 2022

Uh oh!

xliuxu commented Apr 28, 2022

Uh oh!

tnqn commented Apr 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xliuxu commented Apr 29, 2022

Uh oh!

codecov-commenter commented Apr 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tnqn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tnqn left a comment

Choose a reason for hiding this comment

Uh oh!

tnqn commented May 5, 2022

Uh oh!

tnqn commented May 5, 2022

Uh oh!

antoninbas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

antoninbas May 5, 2022

Choose a reason for hiding this comment

Uh oh!

xliuxu May 6, 2022

Choose a reason for hiding this comment

Uh oh!

antoninbas May 5, 2022

Choose a reason for hiding this comment

Uh oh!

xliuxu May 6, 2022

Choose a reason for hiding this comment

Uh oh!

antoninbas May 5, 2022

Choose a reason for hiding this comment

Uh oh!

xliuxu May 6, 2022

Choose a reason for hiding this comment

Uh oh!

tnqn left a comment

Choose a reason for hiding this comment

Uh oh!

xliuxu May 6, 2022

Choose a reason for hiding this comment

Uh oh!

tnqn commented May 6, 2022

Uh oh!

xliuxu commented May 6, 2022

Uh oh!

antoninbas commented May 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tnqn commented Apr 28, 2022 •

edited

Loading

codecov-commenter commented Apr 29, 2022 •

edited

Loading