Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Port Number to Audit Logging #3277

Merged
merged 1 commit into from Feb 23, 2022
Merged

Add Port Number to Audit Logging #3277

merged 1 commit into from Feb 23, 2022

Conversation

qiyueyao
Copy link
Contributor

@qiyueyao qiyueyao commented Feb 1, 2022 •
edited

Adding port number to audit logging to solve issue #3168

Change:

  • Added and updated port number to log, documentation, unit tests and e2e tests.
  • Refactored logPacket in packetin file, replaced the old parse packet function for consistency, removed corresponding UT.
  • Added UT to pkg/ovs/openflow/ofctrl_packetin_test.go.

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

@codecov-commenter
Copy link

codecov-commenter commented Feb 1, 2022 •
edited

Codecov Report

Merging #3277 (1cae13d) into main (e23cf3b) will increase coverage by 0.00%.
The diff coverage is 74.19%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #3277   +/-   ##
=======================================
  Coverage   60.12%   60.12%           
=======================================
  Files         331      331           
  Lines       28444    28434   -10     
=======================================
- Hits        17102    17097    -5     
+ Misses       9483     9477    -6     
- Partials     1859     1860    +1
Flag Coverage Δ
kind-e2e-tests 47.91% <74.19%> (-0.04%) ⬇️
unit-tests 41.93% <3.22%> (+0.07%) ⬆️

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

Impacted Files Coverage Δ
pkg/agent/controller/networkpolicy/packetin.go 69.07% <ø> (+1.91%) ⬆️
...kg/agent/controller/networkpolicy/audit_logging.go 87.80% <74.19%> (-8.35%) ⬇️
pkg/agent/nodeportlocal/k8s/annotations.go 83.87% <0.00%> (-16.13%) ⬇️
...g/controller/networkpolicy/store/appliedtogroup.go 86.36% <0.00%> (-3.04%) ⬇️
pkg/agent/nodeportlocal/k8s/npl_controller.go 58.70% <0.00%> (-2.05%) ⬇️
pkg/agent/flowexporter/exporter/exporter.go 81.29% <0.00%> (-1.94%) ⬇️
pkg/controller/externalippool/controller.go 85.62% <0.00%> (-1.88%) ⬇️
...gent/controller/noderoute/node_route_controller.go 54.91% <0.00%> (-1.10%) ⬇️
pkg/ovs/openflow/ofctrl_packetout.go 79.60% <0.00%> (-0.79%) ⬇️
pkg/agent/cniserver/pod_configuration.go 53.69% <0.00%> (-0.78%) ⬇️
... and 10 more

@qiyueyao qiyueyao added the area/network-policy Issues or PRs related to network policies. label Feb 1, 2022
@qiyueyao qiyueyao added this to the Antrea v1.6 release milestone Feb 1, 2022
Dyanngg
Dyanngg reviewed Feb 2, 2022
View reviewed changes
pkg/agent/controller/networkpolicy/packetin.go
@@ -107,80 +105,6 @@ func getInfoInReg(regMatch *ofctrl.MatchField, rng *openflow13.NXRange) (uint32,
return regValue.Data, nil
}

// getNetworkPolicyInfo fills in tableName, npName, ofPriority, disposition of logInfo ob.
func getNetworkPolicyInfo(pktIn *ofctrl.PacketIn, c *Controller, ob *logInfo) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

qq: why is this UT no longer needed? Is it because it is covered in some other testcases?

Copy link
Contributor Author

@qiyueyao qiyueyao Feb 2, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the only usage of getPacketInfo was audit logging, a new function ParsePacketIn was later implemented in pkg/ovs/openflow/ofctrl_packetin.go that covered the exact functionality and was more widely used.
Deleted the getPacketInfo function and thus the UT.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess my question is then if the same test deleted here is already implemented for the UT of ParsePacketIn

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it was not implemented. Adding some similar UT for ParsePacketIn and other funcs in ofctrl_packetin.go.

Dyanngg
Dyanngg previously approved these changes Feb 8, 2022
View reviewed changes
Copy link
Contributor

@Dyanngg Dyanngg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

tnqn
tnqn reviewed Feb 8, 2022
View reviewed changes
docs/antrea-network-policy.md Outdated Show resolved Hide resolved
pkg/agent/controller/networkpolicy/audit_logging_test.go Outdated Show resolved Hide resolved
pkg/ovs/openflow/ofctrl_packetin_test.go Outdated Show resolved Hide resolved
pkg/ovs/openflow/ofctrl_packetin_test.go Outdated Show resolved Hide resolved
test/e2e/antreapolicy_test.go Outdated Show resolved Hide resolved
pkg/agent/controller/networkpolicy/audit_logging.go Show resolved Hide resolved
tnqn
tnqn previously approved these changes Feb 15, 2022
View reviewed changes
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, one minor comment

pkg/agent/controller/networkpolicy/audit_logging.go Show resolved Hide resolved
pkg/ovs/openflow/ofctrl_packetin_test.go Outdated Show resolved Hide resolved
@qiyueyao
Add port for logging
df2cd24 
Signed-off-by: Qiyue Yao <yaoq@vmware.com>

add UT

Signed-off-by: Qiyue Yao <yaoq@vmware.com>
@qiyueyao
Copy link
Contributor Author

/test-all

tnqn
tnqn approved these changes Feb 23, 2022
View reviewed changes
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tnqn
Copy link
Member

tnqn commented Feb 23, 2022

/test-e2e
/test-integration

@tnqn
Copy link
Member

tnqn commented Feb 23, 2022

/skip-e2e which failed on unrelated case TestIPSec/testIPSecTunnelConnectivity.

@tnqn tnqn added the action/release-note Indicates a PR that should be included in release notes. label Feb 23, 2022
@tnqn tnqn merged commit ba5043a into antrea-io:main Feb 23, 2022
@tnqn tnqn mentioned this pull request Feb 23, 2022
Closed
@qiyueyao qiyueyao deleted the log-port branch February 23, 2022 20:20
bangqipropel pushed a commit to bangqipropel/antrea that referenced this pull request Mar 2, 2022
@qiyueyao
Add port for logging (antrea-io#3277)
1cc8380 
Signed-off-by: Qiyue Yao <yaoq@vmware.com>
@qiyueyao qiyueyao added the action/backport Indicates a PR that requires backports. label Sep 3, 2022
@qiyueyao qiyueyao mentioned this pull request Sep 7, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tnqn tnqn tnqn approved these changes

@antoninbas antoninbas Awaiting requested review from antoninbas

@GraysonWu GraysonWu Awaiting requested review from GraysonWu

@Dyanngg Dyanngg Awaiting requested review from Dyanngg

Assignees
No one assigned
Labels
action/backport Indicates a PR that requires backports. action/release-note Indicates a PR that should be included in release notes. area/network-policy Issues or PRs related to network policies.
Projects
None yet
Milestone
Antrea v1.6 release
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@qiyueyao @codecov-commenter @tnqn @Dyanngg