-
Notifications
You must be signed in to change notification settings - Fork 366
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
Add Log Label to Audit Logging #4748
Conversation
8fc5240
to
5b76b48
Compare
docs/antrea-network-policy.md
Outdated
The rules are logged in the following format: | ||
**logSetting** and **enableLogging**: Antrea-native policy ingress or egress rules | ||
can be audited by enabling its logging field. `logSetting` and `enableLogging` | ||
cannot be used at the same time. When `logSetting` is provided and `enabled` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we deprecating enableLogging
in some feature release? It's worth mentioning that 1. since v1.x enableLogging
became available 2. We will be deprecating enableLogging
for logSetting
and hence they should not be set at the same time 3. logSetting.enabled: true == enableLogging: true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose enableLogging
will be deprecated after logSetting
is accepted by users. Updated the doc, welcome any more suggestion to make this explanation clearer.
6755648
to
4ab1c3c
Compare
Antrea v2.0 related question: should introduction of |
docs/antrea-network-policy.md
Outdated
`enabled` must be specified, if `enabled` is true and `logLabel` is provided, the | ||
label will be displayed in the log, otherwise it defaults to empty value. | ||
|
||
The [first example](#acnp-with-stand-alone-selectors) policy logs all traffic that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, we should change all "enableLogging" to "logSetting", as the former is to be deprecated. And why we even need so many logging examples? We can just keep one with "ACNP with log setting". Not necessary to talk about all possible values at all. People can understand. @Dyanngg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced other "enableLogging" to "logSettings" but just "enabled: true" without log labels. Only using "logLabel" in this section. Reduced the explanations.
In commit message, "crd" -> "CRDs". |
pkg/apis/crd/v1alpha1/types.go
Outdated
// This field can't be used with LogSettings, will be deprecated. | ||
// +optional | ||
EnableLogging bool `json:"enableLogging"` | ||
// LogSettings is used to specify logging behaviors of the rules when matched. | ||
// This field can't be used with EnableLogging. | ||
// +optional | ||
LogSettings *RuleLogSettings `json:"logSetting,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we really need to make a new struct and deprecate the old field, considering:
- Could we export the value to flow record for visbility as well, which adds one dimension that can be used to aggregate flows? Then the field doesn't need to be specific to log, and doesn't really need logging to be enabled.
- Similar API in products like NSX doesn't make the field specific to logging as well.
Perhaps adding a new field of string could fullfil the requirement and be more extensible when we want to leverage the value in other features like flow visibility, and can avoid the extra effort to deprecate the old field:
// Label is user-defined arbitrary string which will be printed in the NetworkPolicy
// logs and exported in flow records.
Label string `json:"label,omitempty"`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did not think about this. If we believe there is a use case to add the label in the flow records, I would agree with what @tnqn suggested (I guess we can still call it "LogLabel").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to not making it specific to audit logs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure that makes sense, didn't consider these use cases. Updating the CRD.
@@ -383,6 +383,9 @@ spec: | |||
type: string | |||
enableLogging: | |||
type: boolean | |||
logLabel: | |||
type: string | |||
pattern: "^.{0,32}$" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it just limits the length, it could use maxLength: 32
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, why did we choose 32 characters?
Maybe we could use 63 (same as K8s labels, which corresponds to the max length of a DNS label)? Are special characters generally supported for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
32 characters was used according to the issue description, I'm not sure was there a specific reason @edwardbadboy ? If not, I'll change that to 63.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, I updated the restriction to follow K8s valid label values.
63 or less, begin and end with an alphanumeric character, could contain dashes (-), underscores (_), dots (.), and alphanumerics between.
@@ -228,14 +231,14 @@ func getNetworkPolicyInfo(pktIn *ofctrl.PacketIn, c *Controller, ob *logInfo) er | |||
if err != nil { | |||
return fmt.Errorf("received error while unloading conjunction id from reg: %v", err) | |||
} | |||
ob.npRef, ob.ofPriority, ob.ruleName = c.ofClient.GetPolicyInfoFromConjunction(conjID) | |||
ob.npRef, ob.ofPriority, ob.ruleName, ob.logLabel = c.ofClient.GetPolicyInfoFromConjunction(conjID) | |||
ob.logLabel = strings.Replace(ob.logLabel, " ", "-", -1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's going to do the conversion, we should make it clear in doc, or we shouldn't even allow space in the label?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it still needed?
2a21841
to
3278d6c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, two minor comments
@@ -228,14 +231,14 @@ func getNetworkPolicyInfo(pktIn *ofctrl.PacketIn, c *Controller, ob *logInfo) er | |||
if err != nil { | |||
return fmt.Errorf("received error while unloading conjunction id from reg: %v", err) | |||
} | |||
ob.npRef, ob.ofPriority, ob.ruleName = c.ofClient.GetPolicyInfoFromConjunction(conjID) | |||
ob.npRef, ob.ofPriority, ob.ruleName, ob.logLabel = c.ofClient.GetPolicyInfoFromConjunction(conjID) | |||
ob.logLabel = strings.Replace(ob.logLabel, " ", "-", -1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it still needed?
e06a11a
to
f4e19e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/test-all |
/skip-conformance which failed because service IP overlapping with the testbed network, tracked by #4981 |
@qiyueyao could you rebase the PR? It conflicts with a commit that was just merged. |
Antrea native policies support firewall rule logs. Adding a rule label to the log allows convenient post-processing of the logs like grepping the same log labels. This feature adds a field "logLabel" to the CRDs, which will be printed in the logs and exported to flow records. "logLabel" defaults to empty value. Fixes antrea-io#4652 Signed-off-by: Qiyue Yao <yaoq@vmware.com>
@@ -253,7 +253,6 @@ spec: | |||
- protocol: TCP | |||
port: 5978 | |||
name: DropToThirdParty | |||
enableLogging: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why we removed these lines in sample yamls? Was it because we planned to deprecate it? Since it is no longer the case, it might be beneficial to leave some rules with enableLogging: true
across sample yamls, so that when users simply copy-paste these they benefit from better debuggability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline, removing to avoid users encountering meter limit for other feature, and we have a separate example for enableLogging
and logLabel
.
/test-all |
/skip-conformance |
Antrea native policies support firewall rule logs. Adding a rule label to the log allows convenient post-processing of the logs like grepping the same log labels. This feature adds a field "logLabel" to the CRDs, which will be printed in the logs and exported to flow records. "logLabel" defaults to empty value. Fixes antrea-io#4652 Signed-off-by: Qiyue Yao <yaoq@vmware.com>
Fixes #4652
This feature adds a field "logLabel" to ACNP and ANP CRDs. "logLabel" defaults to empty value, and limits to 32 chars, any space will be replaced by
-
to avoid mess in the logs.