Skip to content

Commit

Permalink
Fix action field in NetworkPolicyEvaluation for KNP
Browse files Browse the repository at this point in the history 
When processing Kubernetes NetworkPolicy, a default action
Allow is assigned in Antrea controller. This was not handled
corretly in NetworkPolicyEvaluation action field, for default
isolation model. As it will undeterminately display action.

This PR fixes the issue, by indicating the rule as default
isolation with nil action field, and then assigning Isolate
to the response in NetworkPolicyEvaluation.

Signed-off-by: Qiyue Yao <yaoq@vmware.com>
  • Loading branch information
@qiyueyao
qiyueyao committed Apr 25, 2024
1 parent 42e7cfa commit 7fb8e1e
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 26 deletions.
12 changes: 8 additions & 4 deletions pkg/antctl/transform/networkpolicy/response.go
View file
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 := ""
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"
}
return []string{
r.Response.NetworkPolicy.Name,
Expand Down
17 changes: 15 additions & 2 deletions pkg/antctl/transform/networkpolicy/response_test.go
View file
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
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
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

0 comments on commit 7fb8e1e

Please sign in to comment.