Audit Logging support K8s Networkpolicy #4047

qiyueyao · 2022-07-21T17:56:35Z

Previously audit logging only works for Antrea NetworkPolicy and ClusterNetworkPolicy when setting the enableLogging field in the rule. This PR is to address this issue.

Method overview of this PR is at this design doc.

To enable logging for K8s NetworkPolicy, set annotations in metadata for the Namespace as policy.antrea.io/enable-np-logging: "true". Then K8s NetworkPolicy applied to this Namespace will be logged in the directory /var/log/antrea/networkpolicy/np.log.

codecov · 2022-07-21T18:07:44Z

Codecov Report

Merging #4047 (dca7cee) into main (454df5d) will increase coverage by 3.89%.
The diff coverage is 70.54%.

❗ Current head dca7cee differs from pull request most recent head 6b8a815. Consider uploading reports for the commit 6b8a815 to get more accurate results

@@            Coverage Diff             @@
##             main    #4047      +/-   ##
==========================================
+ Coverage   64.25%   68.15%   +3.89%     
==========================================
  Files         294      297       +3     
  Lines       44805    45699     +894     
==========================================
+ Hits        28789    31145    +2356     
+ Misses      13687    12202    -1485     
- Partials     2329     2352      +23

Flag	Coverage Δ		*Carryforward flag
e2e-tests	`40.68% <58.75%> (?)`
integration-tests	`35.44% <42.46%> (?)`		Carriedforward from 26ac871
kind-e2e-tests	`51.08% <71.42%> (+0.79%)`	⬆️	Carriedforward from 26ac871
unit-tests	`44.37% <51.30%> (+0.04%)`	⬆️	Carriedforward from 26ac871

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

Impacted Files	Coverage Δ
multicluster/cmd/multicluster-controller/leader.go	`0.00% <0.00%> (ø)`
multicluster/cmd/multicluster-controller/member.go	`0.00% <0.00%> (ø)`
pkg/agent/openflow/client.go	`70.30% <ø> (+4.26%)`	⬆️
pkg/agent/controller/networkpolicy/reconciler.go	`71.51% <16.66%> (ø)`
...g/controller/networkpolicy/clusternetworkpolicy.go	`61.85% <55.55%> (-6.33%)`	⬇️
...uster/controllers/multicluster/stale_controller.go	`69.09% <64.51%> (+17.37%)`	⬆️
pkg/agent/openflow/network_policy.go	`79.46% <88.57%> (+1.89%)`	⬆️
...kg/agent/controller/networkpolicy/audit_logging.go	`90.71% <90.00%> (+6.68%)`	⬆️
...lers/multicluster/commonarea/remote_common_area.go	`69.79% <100.00%> (+42.04%)`	⬆️
pkg/agent/controller/networkpolicy/packetin.go	`76.35% <100.00%> (+3.37%)`	⬆️
... and 66 more

pkg/agent/openflow/pipeline.go

docs/antrea-network-policy.md

pkg/agent/openflow/network_policy.go

pkg/agent/openflow/pipeline.go

pkg/controller/networkpolicy/networkpolicy_controller.go

pkg/controller/networkpolicy/networkpolicy_controller_test.go

test/e2e/framework.go

pkg/agent/controller/networkpolicy/audit_logging.go

docs/antrea-network-policy.md

pkg/agent/controller/networkpolicy/audit_logging.go

pkg/agent/openflow/network_policy.go

pkg/controller/networkpolicy/networkpolicy_controller.go

qiyueyao · 2022-08-04T01:57:24Z

Reviewers, there's a problem for the Namespace UPDATE event. When modifying the drop flow in an UPDATE event, we might need to compare the previous drop flow and the newly generated one to determine if a modification is necessary. Code here.
Currently the method is to always modify the drop flow due to lack of available information. If we want to compare the flows, we need to change the Flow struct to add useful methods, which involves much more changes to the OVS struct.
Any suggestions or thoughts? Thanks.

pkg/controller/networkpolicy/clusternetworkpolicy.go

tnqn · 2022-08-04T14:40:16Z

pkg/controller/networkpolicy/clusternetworkpolicy.go

+	if oldNamespace.Annotations[EnableNPLoggingAnnotationKey] != curNamespace.Annotations[EnableNPLoggingAnnotationKey] {
+		affectedNPs, err := n.networkPolicyLister.NetworkPolicies(curNamespace.Name).List(labels.Everything())
+		if err != nil {
+			klog.Errorf("Error fetching NetworkPolicies in the Namespace %s: %v", curNamespace.Name, err)


please use structured logging for new logs, but here the error can be ignored, as this is in-memory operation, List call will never return an error.

test/e2e/framework.go

tnqn · 2022-08-04T15:29:20Z

pkg/controller/networkpolicy/clusternetworkpolicy.go

+			return
+		}
+		for _, np := range affectedNPs {
+			n.updateNetworkPolicy(np, np)


Calling the event handler directly would introduce another race condition similar to the ones I'm trying to fix via #4028:

updateNamespace gets NP from lister and calls updateNetworkPolicy

A NP update event updates NP in lister and triggers updateNetworkPolicy with new object

updateNetworkPolicy triggered in step 2 finished and updated internal NP

updateNetworkPolicy triggered in step 1 finished and overrided internal NP with a stale state

But I don't have a good solution to quickly fix it in this patch without refactoring the workflow like I did in #4028. Perhaps we could handle the annotation update case later and wait for #4028 refactoring the workflow first, or we would risky NetworkPolicies working in stale state.

tnqn · 2022-08-05T16:10:57Z

Reviewers, there's a problem for the Namespace UPDATE event. When modifying the drop flow in an UPDATE event, we might need to compare the previous drop flow and the newly generated one to determine if a modification is necessary. Code here. Currently the method is to always modify the drop flow due to lack of available information. If we want to compare the flows, we need to change the Flow struct to add useful methods, which involves much more changes to the OVS struct. Any suggestions or thoughts? Thanks.

If enableLogging changes, the ID of rule also changes, the original rule would be deleted and the new rule would be installed:

antrea/pkg/agent/controller/networkpolicy/cache.go

Lines 650 to 665 in 2a37aec

    
           rule := &rule{ 
        
           	Direction:       r.Direction, 
        
           	From:            r.From, 
        
           	To:              r.To, 
        
           	Services:        r.Services, 
        
           	Action:          r.Action, 
        
           	Priority:        r.Priority, 
        
           	PolicyPriority:  policy.Priority, 
        
           	TierPriority:    policy.TierPriority, 
        
           	AppliedToGroups: appliedToGroups, 
        
           	Name:            r.Name, 
        
           	PolicyUID:       policy.UID, 
        
           	SourceRef:       policy.SourceRef, 
        
           	EnableLogging:   r.EnableLogging, 
        
           } 
        
           rule.ID = hashRule(rule)

So it's already handled. Otherwise current code won't work because reconciler doesn't call functions like AddPolicyRuleAddress for a previously install rule if there is no address change.

I guess Code is not needed?

qiyueyao · 2022-08-06T22:52:14Z

So it's already handled. Otherwise current code won't work because reconciler doesn't call functions like AddPolicyRuleAddress for a previously install rule if there is no address change.
I guess Code is not needed?

Probably I didn't express it clearly, for the flow generated by the rule directly, it indeed changes due to ruleID update, so the IngressRuleTable is updated correspondingly.
However, for K8s NetworkPolicies, the default drop flow in IngressDefaultTable would not update since address was not changed. The pathway is Reconcile->add->installOFRule->InstallPolicyRuleFlows->calculateMatchFlowChangesForRule, calculateChangesForRuleCreation->addAddrFlows->addConjunctiveMatchFlow. In this function, it searches for globalConjMatchFlowCache where the key only depends on tableID, priority and matchPairs, it does not depend on enableLogging.
I did some tests as well, Code here is the place to update default drop flow by K8s NP, but the problem remains whether to compare the flows so that we only update when it is necessary. If we need to compare but there is no easy way to modify Flow, then I believe the UPDATE event should be marked TODO, to be solved together with the race condition in a future PR. Thanks.

tnqn · 2022-08-08T13:33:54Z

@qiyueyao thanks for the explanation. Now I understand the problem is the dropFlow is not specific to a rule but specific to a match condition. Then does it work if we store enableLogging in conjMatchFlowContext and update the dropFlow when it changes?

GraysonWu · 2022-08-08T19:21:29Z

I think Quan's suggestion could work. Since we only use it for dropFlow, maybe name it as something like: dropFlowEnableLogging?

Signed-off-by: Qiyue Yao <yaoq@vmware.com>

qiyueyao · 2022-08-09T04:53:16Z

Storing enableLogging in conjMatchFlowContext works, the latest commit updates that and is ready. Thanks!

salv-orlando · 2022-08-09T05:18:26Z

docs/antrea-network-policy.md

@@ -659,6 +659,9 @@ for all NetworkPolicies in the Namespace. Packets of any connection that match
 a NetworkPolicy rule will be logged with a reference to the NetworkPolicy name,
 but packets dropped by the implicit "default drop" (not allowed by any NetworkPolicy)
 will only be logged with consistent name `K8sNetworkPolicy` for reference.
+Note that currently, Antrea only retrieves the logging Annotation once when adding
+NetworkPolicies and in case of agent restart, users should not update Namespace
+logging Annotations, otherwise it would risk NetworkPolicies working in a stale state.


As a reader (a probably bad one :)) I understand that if an agent is restarted, users should not be updated the namespace logging annotation anymore.

If I am not mistaken, the concept to convey is that events where the namespace annotation is processed concurrently with the network policy, such as agent restart, might trigger a risk of having stale NetworkPolicy info realized. If my understanding is correct, we might consider a little rephrasing above.

@salv-orlando The race condition was explained here: #4047 (comment). I think we should remove this restriction in this release. It would be a simple change after #4028 is merged. I was waiting for these feature PRs to be merged first, otherwise all of them need to address many conflicts caused by the refactoring by #4028

I do not get what exact issues can happen under what exact conditions either. Could you explain more? @qiyueyao

I guess we are talking about two issues here:

We do not handle annotation changes after a NetworkPolicies are created.

For agent restart (before? after? during?), users cannot change the Namespace annotation, otherwise we can run into some issues (but what issues? What does "working in a stale state means"?

Discussed offline and opened follow up PR #4099.

salv-orlando · 2022-08-09T05:21:42Z

pkg/agent/openflow/network_policy.go

@@ -731,6 +734,15 @@ func (c *clause) addConjunctiveMatchFlow(featureNetworkPolicy *featureNetworkPol
 				changeType: insertion,
 			}
 		}
+	} else if context.dropFlowEnableLogging != enableLogging {


I assume this will also work if the namespace annotation is changed while the antrea agent is down, as this code will be invoked upon agent restart. Is that correct?

Actually there is another pathway for agent restart BatchInstallPolicyRuleFlows -> addRuleToConjunctiveMatch -> addActionToConjunctiveMatch especially for batch install, which is triggered upon Antrea agent restart.

tnqn · 2022-08-05T15:43:39Z

test/e2e/framework.go

 		if ns.Status.Phase == corev1.NamespaceTerminating {
 			return fmt.Errorf("error when creating '%s' Namespace: namespace exists but is in 'Terminating' phase", namespace)
 		}
 	}
 	return nil
 }

-// createNamespaceWithAnnotations creates the namespace with Annotations.
+func (data *TestData) UpdateNamespace(namespace string, mutateFunc func(*corev1.Namespace)) error {
+	ns, _ := data.clientset.CoreV1().Namespaces().Get(context.TODO(), namespace, metav1.GetOptions{})


It should not ignore the error since this access K8s API, which could fail because of several reasons, otherwise the following code would panic because ns is nil

tnqn · 2022-08-05T15:46:13Z

test/e2e/antreapolicy_test.go

+// testAuditLoggingEnableNP tests that audit logs are generated when K8s NP is applied
+// tests both Allow traffic by K8s NP and Drop traffic by implicit K8s policy drop
+func testAuditLoggingEnableNP(t *testing.T, data *TestData) {
+	data.updateNamespaceWithAnnotations(namespaces["x"], map[string]string{networkpolicy.EnableNPLoggingAnnotationKey: "true"})


The returned error should checked

failOnError(data.updateNamespaceWithAnnotations(namespaces["x"], map[string]string{networkpolicy.EnableNPLoggingAnnotationKey: "true"}), t)

Changed in follow up PR #4099.

tnqn · 2022-08-09T05:34:43Z

docs/antrea-network-policy.md

@@ -659,6 +659,9 @@ for all NetworkPolicies in the Namespace. Packets of any connection that match
 a NetworkPolicy rule will be logged with a reference to the NetworkPolicy name,
 but packets dropped by the implicit "default drop" (not allowed by any NetworkPolicy)
 will only be logged with consistent name `K8sNetworkPolicy` for reference.
+Note that currently, Antrea only retrieves the logging Annotation once when adding
+NetworkPolicies and in case of agent restart, users should not update Namespace
+logging Annotations, otherwise it would risk NetworkPolicies working in a stale state.


@salv-orlando The race condition was explained here: #4047 (comment). I think we should remove this restriction in this release. It would be a simple change after #4028 is merged. I was waiting for these feature PRs to be merged first, otherwise all of them need to address many conflicts caused by the refactoring by #4028

tnqn · 2022-08-09T07:07:41Z

/test-all

tnqn

Will merge the PR as is. We could improve the tests via separate PR

tnqn · 2022-08-09T09:13:57Z

@qiyueyao I had to rewrite the commit message and close the corresponding issue manually when merging the PR. Could you please write detailed commit message (and perhaps squash commits timely) for future PRs? If you prefer appending commits instead of force pushing, the first commit should have a proper commit message which could be used when merging.

The PR description and commit message should use the well known format to link the issue, like "Fixes #XXX", instead of a hyperlink "This PR is to address this issue", which couldn't close issue automatically. You could find more details here.

qiyueyao force-pushed the log-k8s-drop branch 2 times, most recently from 185d85e to 66fdf99 Compare July 26, 2022 21:48

qiyueyao added the area/network-policy Issues or PRs related to network policies. label Jul 26, 2022

qiyueyao added this to the Antrea v1.8 release milestone Jul 26, 2022

qiyueyao requested review from GraysonWu, salv-orlando and tnqn July 26, 2022 23:02

qiyueyao force-pushed the log-k8s-drop branch from 66fdf99 to a45eaf7 Compare July 27, 2022 07:05

GraysonWu reviewed Jul 27, 2022

View reviewed changes

pkg/agent/openflow/pipeline.go Outdated Show resolved Hide resolved

GraysonWu reviewed Jul 27, 2022

View reviewed changes

pkg/agent/openflow/pipeline.go Outdated Show resolved Hide resolved

tnqn reviewed Jul 28, 2022

View reviewed changes

jianjuns reviewed Jul 29, 2022

View reviewed changes

salv-orlando reviewed Jul 29, 2022

View reviewed changes

pkg/agent/controller/networkpolicy/audit_logging.go Show resolved Hide resolved

pkg/agent/openflow/network_policy.go Outdated Show resolved Hide resolved

pkg/controller/networkpolicy/networkpolicy_controller.go Show resolved Hide resolved

qiyueyao requested a review from wenyingd August 2, 2022 05:54

tnqn reviewed Aug 4, 2022

View reviewed changes

qiyueyao force-pushed the log-k8s-drop branch from 923fc6e to b0d9815 Compare August 5, 2022 09:36

qiyueyao added 4 commits August 8, 2022 15:34

Controller K8s Enablelogging

a610e2d

Signed-off-by: Qiyue Yao <yaoq@vmware.com>

log k8s drop

852aae1

Signed-off-by: Qiyue Yao <yaoq@vmware.com>

unit test

f15717d

Signed-off-by: Qiyue Yao <yaoq@vmware.com>

e2e tests

bef0b24

Signed-off-by: Qiyue Yao <yaoq@vmware.com>

qiyueyao added 7 commits August 8, 2022 15:34

documentation

b28c370

Signed-off-by: Qiyue Yao <yaoq@vmware.com>

address comment

ce50e6c

Signed-off-by: Qiyue Yao <yaoq@vmware.com>

improve code style

0f26769

Signed-off-by: Qiyue Yao <yaoq@vmware.com>

address comments code style

a1efbca

Signed-off-by: Qiyue Yao <yaoq@vmware.com>

address comments UPDATE event

7804551

Signed-off-by: Qiyue Yao <yaoq@vmware.com>

address comments UPDATE TODO

9cde14b

Signed-off-by: Qiyue Yao <yaoq@vmware.com>

address comments UPDATE change

3493743

Signed-off-by: Qiyue Yao <yaoq@vmware.com>

qiyueyao force-pushed the log-k8s-drop branch from 72ad389 to 3493743 Compare August 8, 2022 22:36

race condition comments

6b8a815

Signed-off-by: Qiyue Yao <yaoq@vmware.com>

qiyueyao mentioned this pull request Aug 9, 2022

Fix race conditions in NetworkPolicyController #4028

Merged

salv-orlando reviewed Aug 9, 2022

View reviewed changes

tnqn reviewed Aug 9, 2022

View reviewed changes

tnqn approved these changes Aug 9, 2022

View reviewed changes

tnqn merged commit 3454520 into antrea-io:main Aug 9, 2022

tnqn mentioned this pull request Aug 9, 2022

netpolicy logs should also log drops from standard k8s netpol rules #3640

Closed

qiyueyao mentioned this pull request Aug 10, 2022

[Follow Up] Audit Logging support K8s Networkpolicy #4099

Merged

qiyueyao deleted the log-k8s-drop branch August 10, 2022 22:18

qiyueyao added the action/backport Indicates a PR that requires backports. label Sep 3, 2022

qiyueyao restored the log-k8s-drop branch March 29, 2023 00:17

qiyueyao deleted the log-k8s-drop branch March 29, 2023 00:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Audit Logging support K8s Networkpolicy #4047

Audit Logging support K8s Networkpolicy #4047

qiyueyao commented Jul 21, 2022 •

edited

Loading

codecov bot commented Jul 21, 2022 •

edited

Loading

qiyueyao commented Aug 4, 2022

tnqn Aug 4, 2022

tnqn Aug 4, 2022

tnqn commented Aug 5, 2022

qiyueyao commented Aug 6, 2022 •

edited

Loading

tnqn commented Aug 8, 2022

GraysonWu commented Aug 8, 2022

qiyueyao commented Aug 9, 2022

salv-orlando Aug 9, 2022

tnqn Aug 9, 2022

jianjuns Aug 9, 2022

jianjuns Aug 9, 2022

qiyueyao Aug 10, 2022

salv-orlando Aug 9, 2022

qiyueyao Aug 9, 2022

tnqn Aug 5, 2022

tnqn Aug 5, 2022

qiyueyao Aug 10, 2022

tnqn Aug 9, 2022

tnqn commented Aug 9, 2022

tnqn left a comment •

edited

Loading

tnqn commented Aug 9, 2022

Audit Logging support K8s Networkpolicy #4047

Audit Logging support K8s Networkpolicy #4047

Conversation

qiyueyao commented Jul 21, 2022 • edited Loading

codecov bot commented Jul 21, 2022 • edited Loading

Codecov Report

qiyueyao commented Aug 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn commented Aug 5, 2022

qiyueyao commented Aug 6, 2022 • edited Loading

tnqn commented Aug 8, 2022

GraysonWu commented Aug 8, 2022

qiyueyao commented Aug 9, 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

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 commented Aug 9, 2022

tnqn left a comment • edited Loading

Choose a reason for hiding this comment

tnqn commented Aug 9, 2022

qiyueyao commented Jul 21, 2022 •

edited

Loading

codecov bot commented Jul 21, 2022 •

edited

Loading

qiyueyao commented Aug 6, 2022 •

edited

Loading

tnqn left a comment •

edited

Loading