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

Add antctl query for policy conjunction ID #6470

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Dyanngg
Copy link
Contributor

@Dyanngg Dyanngg commented Jun 20, 2024

The antctl query will be available in Antrea agents. It returns the OpenFlow conjunction ID(s) assigned for the queried policy's rules on that specific Node.
Note that if a policy does not apply to any Pods on that Node, this antctl command will return a http NotFound since the Antrea agent will not have information of that policy.
In addition, this PR also corrects the deprecated ANP type flag in antctl get networkpolicy queries to point to the new AdminNetworkPolicy type.

The antctl query will be available in Antrea agents. It returns the
OpenFlow conjunction ID(s) assigned for the queried policy's rules
on that specific Node. Note that if a policy does not apply to any
Pods on that Node, this antctl command will return a http NotFound
since the Antrea agent will not have information of that policy.

Signed-off-by: Dyanngg <dingyang@vmware.com>

// HandleFunc creates a http.HandlerFunc which uses an AgentNetworkPolicyInfoQuerier
// to query network policy rules in current agent.
func HandleFunc(aq agentquerier.AgentQuerier) http.HandlerFunc {
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't it make sense to add this information (list of conj IDs) to the existing networkpolicy handler instead of defining a new HTTP endpoint? We can always enrich it if we need to support a few additional query parameters? Likewise, maybe we could use the existing antctl command and have a flag to display conjunction IDs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As of now the response type of the antctl get networkpolicy command is basically controlplane.NetworkPolicy since it's a shared endpoint between controller and agent. As a result it might be a bit tricky to add a conjunction ID field to the Response type, as for the controller it is this antctl command is a resourceEndpoint command https://github.com/antrea-io/antrea/blob/main/pkg/antctl/antctl.go#L227

Copy link
Contributor

Choose a reason for hiding this comment

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

There are some differences already in the Agent command compared to the Controller command, and the handlers are different, but I think I see your point: we still use the same response type for both handlers, and it may not be a good idea to change that.

Is there any information that would be useful for debugging on the Agent-side besides conjunction IDs? I am wondering if we should have a more generic (but still Agent-specific) endpoint / command, not one named specifically "policyconjunctions". For example, would it make sense to also include the ruleID in the response, or anything else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ummm if you are talking about the ID of the rule stored in agent, usually it does not carry information that we can use during debugging. It is a hash that's computed based on all fields of a rule and not really relatable to any controlplane rule info.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opened #6487 which fixed some old antctl ovs-dump command that would essentially show conjunction ID information (buried in the flows). However, it feels to me that it's still quite different from the command provided in this PR: the ovs-dump returns the datapath state, which, in times of troubleshooting, might return empty flows based on the match keys stored. Wondering if this PR is still warranted @tnqn

Comment on lines -62 to +65
"NP": cpv1beta.K8sNetworkPolicy,
"K8SNP": cpv1beta.K8sNetworkPolicy,
"ACNP": cpv1beta.AntreaClusterNetworkPolicy,
"ANNP": cpv1beta.AntreaNetworkPolicy,
"ANP": cpv1beta.AntreaNetworkPolicy,
"ANP": cpv1beta.AdminNetworkPolicy,
Copy link
Contributor

Choose a reason for hiding this comment

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

we should move changes to this file to a separate PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #6487

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants