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.

Sign up for GitHub

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

[Followup] Fix action field in NetworkPolicyEvaluation for KNP #6217

Merged
merged 1 commit into from
Apr 26, 2024
Merged

[Followup] Fix action field in NetworkPolicyEvaluation for KNP #6217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions pkg/antctl/transform/networkpolicy/response.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,14 +187,18 @@ func (r EvaluationResponse) GetTableHeader() []string {

func (r EvaluationResponse) GetTableRow(_ int) []string {
if r.NetworkPolicyEvaluation != nil && r.Response != nil {
// Handle K8s NPs empty action field. "Allow" corresponds to a K8s NP
// allow action, and "Isolate" corresponds to a drop action because of the
// default isolation model. Otherwise, display the action field content.
action := "Allow"
action := ""
Copy link
Member

Choose a reason for hiding this comment

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

I don't get why it shouldn't be assigned with "Allow". Aren't all K8s explicit rules "allow" rules?

Copy link
Contributor Author

@qiyueyao qiyueyao Apr 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I should have updated the comment as well.
For now, K8s explicit rules will go to the Action != nil case with their default allow action, and manual inserted isolation rules will go to math.MaxInt32. Based on current implementation, the only case left over is Action = nil, (which will never happen as long as Antrea controller is functioning, so it's not default value for K8s explicit rules), so perhaps defaulting to "" is more suitable.

Copy link
Contributor

Choose a reason for hiding this comment

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

For the sake of our understanding, could you point to the code that ensures that the action is set to Allow K8s NP explicit rules? Do we have a unit test that validates this behavior server-side?

Copy link
Contributor Author

@qiyueyao qiyueyao Apr 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The action for K8s rules are set in controller networkpolicy, then controller endpoint querier and evaluation. Also after discussion, we thought action field should not be set for isolation since it was not an actual rule (being changed in this PR).
It is covered by networkpolicy UT, but not endpoint_querier UT (changing in this PR)

if r.Response.Rule.Action != nil {
action = string(*r.Response.Rule.Action)
} else if r.Response.RuleIndex == math.MaxInt32 {
// Responses from endpoint query with original rules will always have
// valid action fields, except for the synthetic isolation rules,
// identified by a MaxInt32 rule index. "Isolate" corresponds to
// a drop action because of the default isolation model of K8s NPs.
action = "Isolate"
} else {
// Should not be possible.
action = "Unknown"
}
qiyueyao marked this conversation as resolved.
Show resolved Hide resolved
return []string{
r.Response.NetworkPolicy.Name,
Expand Down
17 changes: 15 additions & 2 deletions pkg/antctl/transform/networkpolicy/response_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ func TestEvaluationResponseTransform(t *testing.T) {
assert.Equal(t, []string{"NAME", "NAMESPACE", "POLICY-TYPE", "RULE-INDEX", "DIRECTION", "ACTION"}, test.GetTableHeader())
assert.False(t, test.SortRows())
assert.Equal(t, []string{"", "", "", "", "", ""}, test.GetTableRow(32))
testDropAction := crdv1beta1.RuleActionDrop
testDropAction, testAllowAction := crdv1beta1.RuleActionDrop, crdv1beta1.RuleActionAllow

tests := []struct {
name string
Expand All @@ -198,7 +198,7 @@ func TestEvaluationResponseTransform(t *testing.T) {
Name: "testK8s",
},
RuleIndex: 10,
Rule: cpv1beta.RuleRef{Direction: cpv1beta.DirectionIn},
Rule: cpv1beta.RuleRef{Direction: cpv1beta.DirectionIn, Action: &testAllowAction},
},
expectedOutput: []string{"testK8s", "ns", "K8sNetworkPolicy", "10", "In", "Allow"},
},
Expand Down Expand Up @@ -228,6 +228,19 @@ func TestEvaluationResponseTransform(t *testing.T) {
},
expectedOutput: []string{"testK8s", "ns", "K8sNetworkPolicy", fmt.Sprint(math.MaxInt32), "In", "Isolate"},
},
{
name: "Unknown action in response",
testResponse: &cpv1beta.NetworkPolicyEvaluationResponse{
NetworkPolicy: cpv1beta.NetworkPolicyReference{
Type: cpv1beta.AntreaNetworkPolicy,
Namespace: "ns",
Name: "testError",
},
RuleIndex: 10,
Rule: cpv1beta.RuleRef{Direction: cpv1beta.DirectionIn},
},
expectedOutput: []string{"testError", "ns", "AntreaNetworkPolicy", "10", "In", "Unknown"},
},
}

for _, tt := range tests {
Expand Down
6 changes: 3 additions & 3 deletions pkg/controller/networkpolicy/endpoint_querier.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ func (eq *EndpointQuerierImpl) QueryNetworkPolicyRules(namespace, podName string
appliedToGroupKeys := groups[appliedToGroupType]
// We iterate over all AppliedToGroups (same for AddressGroups below). This is acceptable
// since this implementation only supports user queries (in particular through antctl) and
// should resturn within a reasonable amount of time. We experimented with adding Pod
// should return within a reasonable amount of time. We experimented with adding Pod
// Indexers to the AppliedToGroup and AddressGroup stores, but we felt that this use case
// did not justify the memory overhead. If we can find another use for the Indexers as part
// of the NetworkPolicy Controller implementation, we may consider adding them back.
Expand Down Expand Up @@ -192,10 +192,10 @@ func processEndpointAppliedRules(appliedPolicies []*antreatypes.NetworkPolicy, i
for _, rule := range internalPolicy.Rules {
if rule.Direction == controlplane.DirectionIn && !isSourceEndpoint {
isolationRules = append(isolationRules, &antreatypes.RuleInfo{Policy: internalPolicy, Index: math.MaxInt32,
Rule: &controlplane.NetworkPolicyRule{Direction: rule.Direction, Name: rule.Name, Action: rule.Action}})
Rule: &controlplane.NetworkPolicyRule{Direction: rule.Direction, Name: rule.Name}})
} else if rule.Direction == controlplane.DirectionOut && isSourceEndpoint {
isolationRules = append(isolationRules, &antreatypes.RuleInfo{Policy: internalPolicy, Index: math.MaxInt32,
Rule: &controlplane.NetworkPolicyRule{Direction: rule.Direction, Name: rule.Name, Action: rule.Action}})
Rule: &controlplane.NetworkPolicyRule{Direction: rule.Direction, Name: rule.Name}})
}
}
}
Expand Down
33 changes: 16 additions & 17 deletions pkg/controller/networkpolicy/endpoint_querier_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,10 +264,10 @@ func TestQueryNetworkPolicyRules(t *testing.T) {
{SourceRef: &policyRef},
},
EndpointAsIngressSrcRules: []*antreatypes.RuleInfo{
{Policy: &antreatypes.NetworkPolicy{SourceRef: &policyRef}, Rule: &controlplane.NetworkPolicyRule{Direction: controlplane.DirectionIn}},
{Policy: &antreatypes.NetworkPolicy{SourceRef: &policyRef}, Rule: &controlplane.NetworkPolicyRule{Direction: controlplane.DirectionIn, Action: &allowAction}},
},
EndpointAsEgressDstRules: []*antreatypes.RuleInfo{
{Policy: &antreatypes.NetworkPolicy{SourceRef: &policyRef}, Rule: &controlplane.NetworkPolicyRule{Direction: controlplane.DirectionOut}},
{Policy: &antreatypes.NetworkPolicy{SourceRef: &policyRef}, Rule: &controlplane.NetworkPolicyRule{Direction: controlplane.DirectionOut, Action: &allowAction}},
},
},
},
Expand All @@ -284,10 +284,10 @@ func TestQueryNetworkPolicyRules(t *testing.T) {
{SourceRef: &policyRef1},
},
EndpointAsIngressSrcRules: []*antreatypes.RuleInfo{
{Policy: &antreatypes.NetworkPolicy{SourceRef: &policyRef}, Rule: &controlplane.NetworkPolicyRule{Direction: controlplane.DirectionIn}},
{Policy: &antreatypes.NetworkPolicy{SourceRef: &policyRef}, Rule: &controlplane.NetworkPolicyRule{Direction: controlplane.DirectionIn, Action: &allowAction}},
},
EndpointAsEgressDstRules: []*antreatypes.RuleInfo{
{Policy: &antreatypes.NetworkPolicy{SourceRef: &policyRef}, Rule: &controlplane.NetworkPolicyRule{Direction: controlplane.DirectionOut}},
{Policy: &antreatypes.NetworkPolicy{SourceRef: &policyRef}, Rule: &controlplane.NetworkPolicyRule{Direction: controlplane.DirectionOut, Action: &allowAction}},
},
},
},
Expand All @@ -303,7 +303,7 @@ func TestQueryNetworkPolicyRules(t *testing.T) {
{SourceRef: &policyRef2},
},
EndpointAsIngressSrcRules: []*antreatypes.RuleInfo{
{Policy: &antreatypes.NetworkPolicy{SourceRef: &policyRef2}, Index: 1, Rule: &controlplane.NetworkPolicyRule{Direction: controlplane.DirectionIn}},
{Policy: &antreatypes.NetworkPolicy{SourceRef: &policyRef2}, Index: 1, Rule: &controlplane.NetworkPolicyRule{Direction: controlplane.DirectionIn, Action: &allowAction}},
},
},
},
Expand All @@ -313,6 +313,7 @@ func TestQueryNetworkPolicyRules(t *testing.T) {
assert.Equal(t, len(expectedRules), len(responseRules))
for idx := range expectedRules {
assert.EqualValues(t, expectedRules[idx].Rule.Direction, responseRules[idx].Rule.Direction)
assert.EqualValues(t, expectedRules[idx].Rule.Action, responseRules[idx].Rule.Action)
assert.Equal(t, expectedRules[idx].Index, responseRules[idx].Index)
assert.Equal(t, expectedRules[idx].Policy.SourceRef, responseRules[idx].Policy.SourceRef)
}
Expand Down Expand Up @@ -377,9 +378,7 @@ func TestQueryNetworkPolicyEvaluation(t *testing.T) {
Direction: direction,
Name: fmt.Sprintf("Policy%sRule%d", policyUID, i),
Priority: int32(i),
}
if action != nil {
rules[i].Action = action
Action: action,
}
}
return []*antreatypes.NetworkPolicy{{
Expand Down Expand Up @@ -497,35 +496,35 @@ func TestQueryNetworkPolicyEvaluation(t *testing.T) {
request: accessRequest,
mockQueryResponse: []mockResponse{
{response: generateResponse(1, generatePolicies(uid1, controlplane.AntreaNetworkPolicy, controlplane.DirectionOut, ptr.To(crdv1beta1.BaselineTierPriority), nil, 1, &allowAction),
generateRuleInfo(generatePolicies(uid2, controlplane.K8sNetworkPolicy, controlplane.DirectionIn, nil, &defaultPriority, 1, nil)[0]))},
{response: generateResponse(2, generatePolicies(uid2, controlplane.K8sNetworkPolicy, controlplane.DirectionIn, nil, &defaultPriority, 1, nil),
generateRuleInfo(generatePolicies(uid2, controlplane.K8sNetworkPolicy, controlplane.DirectionIn, nil, &defaultPriority, 1, &allowAction)[0]))},
{response: generateResponse(2, generatePolicies(uid2, controlplane.K8sNetworkPolicy, controlplane.DirectionIn, nil, &defaultPriority, 1, &allowAction),
generateRuleInfo(generatePolicies(uid1, controlplane.AntreaNetworkPolicy, controlplane.DirectionOut, ptr.To(crdv1beta1.BaselineTierPriority), nil, 1, &allowAction)[0]))},
},
expectedResult: &controlplane.NetworkPolicyEvaluationResponse{
NetworkPolicy: controlplane.NetworkPolicyReference{Type: controlplane.K8sNetworkPolicy, Namespace: namespace, Name: "Policy222", UID: uid2},
RuleIndex: 0,
Rule: controlplane.RuleRef{Direction: controlplane.DirectionIn, Name: "Policy222Rule0"},
Rule: controlplane.RuleRef{Direction: controlplane.DirectionIn, Name: "Policy222Rule0", Action: &allowAction},
},
},
{
name: "KNP and default isolation",
request: accessRequest,
mockQueryResponse: []mockResponse{
{response: generateResponse(1, generatePolicies(uid1, controlplane.K8sNetworkPolicy, controlplane.DirectionOut, nil, &defaultPriority, 1, nil), nil)},
{response: generateResponse(2, generatePolicies(uid2, controlplane.K8sNetworkPolicy, controlplane.DirectionIn, nil, &defaultPriority, 1, nil),
generateRuleInfo(generatePolicies(uid1, controlplane.K8sNetworkPolicy, controlplane.DirectionOut, nil, &defaultPriority, 1, nil)[0]))},
{response: generateResponse(1, generatePolicies(uid1, controlplane.K8sNetworkPolicy, controlplane.DirectionOut, nil, &defaultPriority, 1, &allowAction), nil)},
{response: generateResponse(2, generatePolicies(uid2, controlplane.K8sNetworkPolicy, controlplane.DirectionIn, nil, &defaultPriority, 1, &allowAction),
generateRuleInfo(generatePolicies(uid1, controlplane.K8sNetworkPolicy, controlplane.DirectionOut, nil, &defaultPriority, 1, &allowAction)[0]))},
},
expectedResult: &controlplane.NetworkPolicyEvaluationResponse{
NetworkPolicy: controlplane.NetworkPolicyReference{Type: controlplane.K8sNetworkPolicy, Namespace: namespace, Name: "Policy111", UID: uid1},
RuleIndex: 0,
Rule: controlplane.RuleRef{Direction: controlplane.DirectionOut, Name: "Policy111Rule0"},
Rule: controlplane.RuleRef{Direction: controlplane.DirectionOut, Name: "Policy111Rule0", Action: &allowAction},
},
},
{
name: "KNP egress default isolation",
request: accessRequest,
mockQueryResponse: []mockResponse{
{response: generateResponse(1, generatePolicies(uid1, controlplane.K8sNetworkPolicy, controlplane.DirectionOut, nil, &defaultPriority, 1, nil), nil)},
{response: generateResponse(1, generatePolicies(uid1, controlplane.K8sNetworkPolicy, controlplane.DirectionOut, nil, &defaultPriority, 1, &allowAction), nil)},
{response: generateResponse(2, nil, nil)},
},
expectedResult: &controlplane.NetworkPolicyEvaluationResponse{
Expand All @@ -539,7 +538,7 @@ func TestQueryNetworkPolicyEvaluation(t *testing.T) {
request: accessRequest,
mockQueryResponse: []mockResponse{
{response: generateResponse(1, nil, nil)},
{response: generateResponse(2, generatePolicies(uid2, controlplane.K8sNetworkPolicy, controlplane.DirectionIn, nil, &defaultPriority, 1, nil), nil)},
{response: generateResponse(2, generatePolicies(uid2, controlplane.K8sNetworkPolicy, controlplane.DirectionIn, nil, &defaultPriority, 1, &allowAction), nil)},
},
expectedResult: &controlplane.NetworkPolicyEvaluationResponse{
NetworkPolicy: controlplane.NetworkPolicyReference{Type: controlplane.K8sNetworkPolicy, Namespace: namespace, Name: "Policy222", UID: uid2},
Expand Down
Loading