-
Notifications
You must be signed in to change notification settings - Fork 359
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 Versioned API and Antctl for NetworkPolicyEvaluation (effective policy rule prediction) #5740
Conversation
9bf6c0f
to
e859616
Compare
|
||
// HandleFunc creates a http.HandlerFunc which uses an AgentNetworkPolicyInfoQuerier | ||
// to query network policy rules in current agent. | ||
func HandleFunc(eq networkpolicy.EndpointQuerier) http.HandlerFunc { |
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 was just curious as to whether some thought was given regarding implementing all this logic in antctl, rather than in the controller API server? I imagine that in theory antctl query networkpolicyanalysis
and antctl query endpoint
could share a single API (with extra information compared to what /endpoints
provide today) and that the rule comparison / ordering could happen in antctl instead of in the server. If however, we think that there is a need for a dedicated API providing the end result (e.g., because it needs to be consumed by some other component), then that would explain why we need a dedicated API.
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.
Yes we do intend to have this API consumed by some downstream components
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.
Since this tends to be a public API and not only consumed by antctl, should we make a well-defined, structured, and versioned resource API? The API looks helpful to me and could perhaps be extended to test Pod-to-IP as well.
If it's a non-resource API like the current one, it may be hard to evolve and handle compatibility with its consumers.
I think it could be an API under controlplane API like the group membership APIs. There are quite some examples in Kubernetes APIs similar to this, used as ephemeral queries, like AdmissionReview
, SubjectAccessReview
, TokenReview
, TokenRequest
, CertificateSigningRequest
.
There will be plenty of benefits by making it a resource API:
- It can be exposed via APIService so any consumers running out of the cluster (including antctl) can access it publicly.
- Generated client code makes it easier to consume, instead of rewriting the parser code in every client.
- It's versioned so easier to support different versions of clients.
- The request and response can be better structured when extending it.
For example, the data type in my mind:
type NetworkPolicyAccessReview struct {
metav1.TypeMeta
Request *NetworkPolicyAccessRequest
Response *NetworkPolicyAccessResponse
}
type Entity struct {
Namespace string
Pod string
// It can be added when we can support IP check.
IP string
}
type NetworkPolicyAccessRequest struct {
Source Entity
Destination Entity
// It can be added when we can support port level check.
Protocol Protocol
SourcePort int
DestinationPort int
}
type NetworkPolicyAccessResponse struct {
// The expected action.
Action Action
// The reference of the effective NetworkPolicy.
NetworkPolicy NetworkPolicyReference
Direction cpv1beta.Direction
RuleIndex int
// The content of the effective rule. Type is runtime.Object because it can be different types.
Rule runtime.Object
}
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.
Changed to resourced API, and updated antctl call to it.
pkg/antctl/antctl.go
Outdated
transformedResponse: reflect.TypeOf(controllernetworkpolicy.EndpointQueryResponse{}), | ||
transformedResponse: reflect.TypeOf(endpointServer.EndpointQueryResponse{}), | ||
}, | ||
{use: "networkpolicyanalysis", |
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.
Should we name it "effectivepolicyrule"?
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 implementing this comment #5740 (comment) , so perhaps this will be "networkpolicyaccess"?
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 assume that comment is about making the API definition generic? But the antctl command here is just for a single operation of returning the effective rule, or you plan to extend the command to support generic policy queries later?
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.
Named the API networkpolicyaccessreview
and named the antctl command effectivepolicyrule
.
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.
Let us hear what @antoninbas and @tnqn may suggest too.
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.
Sorry, I used NetworkPolicyAccessReview
in #5740 (comment) just as an example and didn't think too much about the naming. Now I feel it sounds different from what it actually represents. How about naming it NetworkPolicyEnforcement
, which may also be used as the cmd name, i.e. antctl query networkpolicyenforcement
?
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.
Since the api&cmd doesn't enforce the current configs, just queries them, perhaps enforcement
would be misleading? How about NetworkPolicyEffectiveRule
, NetworkPolicyPrimeRule
, NetworkPolicyEvaluation
, NetworkPolicyAnalysis
?
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.
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.
NetworkPolicyEvaluation
sounds good to me too.
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.
Renamed the types.
886c1a2
to
71e6128
Compare
return ns, pod | ||
} | ||
|
||
func NewNetworkPolicyEvaluation(args map[string]string) (runtime.Object, error) { |
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 thought this package was meant for output transforms. For parameter transform (which is a new concept), maybe it should be in a separate package?
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.
Created a new package paramter
under antctl
.
pkg/apiserver/registry/networkpolicy/networkpolicyevaluation/rest.go
Outdated
Show resolved
Hide resolved
pkg/apiserver/registry/networkpolicy/networkpolicyevaluation/rest.go
Outdated
Show resolved
Hide resolved
47108e1
to
319cab6
Compare
return ns, pod | ||
} | ||
|
||
func NewNetworkPolicyEvaluation(args map[string]string) (runtime.Object, error) { |
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.
should it be moved to transform/networkpolicy like the type of the response? otherwise this file would include all parameters of discrete commands.
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.
The function was moved here as a new package parameter
based on this comment, if I understood it correctly.
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.
We should probably find a better package name / location in a future PR though
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, do you have any suggestion or insight? To include it inside transform or refactor some of the existing structs?
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.
Thinking about it some more, maybe we could keep it in transform/networkpolicy
, but as a separate file to clearly mark the difference? Maybe we could have transform/networkpolicy/request.go
and ransform/networkpolicy/response.go
?
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.
Sounds good, will open a PR for this.
@@ -214,6 +216,7 @@ func installAPIGroup(s *APIServer, c completedConfig) error { | |||
cpv1beta2Storage["appliedtogroups"] = appliedToGroupStorage | |||
cpv1beta2Storage["networkpolicies"] = networkPolicyStorage | |||
cpv1beta2Storage["networkpolicies/status"] = networkPolicyStatusStorage | |||
cpv1beta2Storage["networkpolicyevaluation"] = networkPolicyEvaluationStorage |
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.
should we use "networkpolicies/evaluation" as the path to indicate their relationship? Like "pods/log", "pods/attach", "pods/exec" APIs?
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 did some test and found that, since NetworkPolicyEvaluation
is currently a resource, to use networkpolicies/evaluation
we have to make evaluation a subresource of networkpolicies
. But different from status&scale, this NetworkPolicyEvaluation
is not affiliated to a particular networkpolicy at input, so I could not provide a networkpolicy name for the subresource. I feel like this might have to stay as a separate resource like appliedtogroups
?
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.
makes sense
Depends on #5989 |
03bc071
to
78aaba6
Compare
@@ -214,6 +216,7 @@ func installAPIGroup(s *APIServer, c completedConfig) error { | |||
cpv1beta2Storage["appliedtogroups"] = appliedToGroupStorage | |||
cpv1beta2Storage["networkpolicies"] = networkPolicyStorage | |||
cpv1beta2Storage["networkpolicies/status"] = networkPolicyStatusStorage | |||
cpv1beta2Storage["networkpolicyevaluation"] = networkPolicyEvaluationStorage |
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.
makes sense
if len(commonRules) > 0 { | ||
commonRule = commonRules[0] | ||
// filter Antrea-native policy rules with Pass action | ||
// if pass rule currently has the highest precedence, skip the remaining rules | ||
// until the next K8s rule or Baseline rule, or return the pass rule otherwise | ||
isPass := func(ruleInfo *controlplane.NetworkPolicyRule) bool { | ||
return ruleInfo.Action != nil && *ruleInfo.Action == crdv1beta1.RuleActionPass | ||
} | ||
if isPass(commonRule.Rule) { | ||
for _, rule := range commonRules[1:] { | ||
if rule.Policy.SourceRef.Type == controlplane.K8sNetworkPolicy || | ||
(rule.Policy.TierPriority != nil && *rule.Policy.TierPriority == BaselineTierPriority && !isPass(rule.Rule)) { | ||
commonRule = rule | ||
break | ||
} | ||
} | ||
} | ||
} | ||
return |
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 the following equivalent?
for _, rule := range commonRules {
if rule.Policy.SourceRef.Type == controlplane.K8sNetworkPolicy ||
*rule.Policy.TierPriority == BaselineTierPriority ||
rule.Rule.Action == nil || *rule.Rule.Action != crdv1beta1.RuleActionPass) {
return rule
}
}
return nil
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.
Seems not, we discussed the cases when 1) Pass was the first rule, but there are no satisfied rules found later, then the first Pass rule should be returned. 2) If Pass was the first rule, we need to skip all ACNP/ANNP rules until a K8s rule or Baseline rule appears, looks like the above solution returns the next non-pass rule.
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.
makes sense
if len(commonRules) > 0 { | ||
commonRule = commonRules[0] | ||
// filter Antrea-native policy rules with Pass action | ||
// if pass rule currently has the highest precedence, skip the remaining rules | ||
// until the next K8s rule or Baseline rule, or return the pass rule otherwise | ||
isPass := func(ruleInfo *controlplane.NetworkPolicyRule) bool { | ||
return ruleInfo.Action != nil && *ruleInfo.Action == crdv1beta1.RuleActionPass | ||
} | ||
if isPass(commonRule.Rule) { | ||
for _, rule := range commonRules[1:] { | ||
if rule.Policy.SourceRef.Type == controlplane.K8sNetworkPolicy || | ||
(rule.Policy.TierPriority != nil && *rule.Policy.TierPriority == BaselineTierPriority && !isPass(rule.Rule)) { | ||
commonRule = rule | ||
break | ||
} | ||
} | ||
} | ||
} | ||
return |
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.
makes sense
Adds a versioned API and antctl query for NetworkPolicy evaluation that returns the predicted effective NetworkPolicy rule, which affects traffic from ns1/pod1 to ns2/pod2. Signed-off-by: Qiyue Yao <yaoq@vmware.com>
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
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, thanks for addressing all the comments on this giant PR
@jianjuns @antoninbas do you have other comments? |
@tnqn I took another quick look, LGTM |
/test-all |
Ignoring the following tests:
|
antctl query networkpolicyevaluation -S ns1/pod1 -D ns2/pod2
curl -d "@<test json file>" -H "Content-Type: application/json" -X POST <k8s-apiserver>:8001/apis/controlplane.antrea.io/v1beta2/networkpolicyevaluation
Adds the above APIs and antctl queriers that returns the predicted effective NetworkPolicy rule, which affects traffic from
ns1/pod1
tons2/pod2
. The solution picks the highest priority rule that satisfies the query.