From 543f1d583f5008e07a679da248c48905979d3051 Mon Sep 17 00:00:00 2001 From: Yang Ding Date: Wed, 18 Nov 2020 14:00:05 -0800 Subject: [PATCH] Address comments and move sort logic to the client side --- docs/antctl.md | 16 ++--- docs/antrea-network-policy.md | 5 +- .../handlers/networkpolicy/handler.go | 62 +----------------- pkg/antctl/antctl.go | 7 +- pkg/antctl/command_definition.go | 18 +++-- pkg/antctl/command_definition_test.go | 6 +- .../transform/addressgroup/transform.go | 9 +-- .../transform/appliedtogroup/transform.go | 9 +-- .../transform/controllerinfo/transform.go | 2 +- .../transform/networkpolicy/transform.go | 65 +++++++++++++++++-- pkg/antctl/transform/utils.go | 8 +-- pkg/antctl/transform/version/transform.go | 4 +- 12 files changed, 108 insertions(+), 103 deletions(-) diff --git a/docs/antctl.md b/docs/antctl.md index 72a1695402c..22d598def02 100644 --- a/docs/antctl.md +++ b/docs/antctl.md @@ -168,14 +168,7 @@ antctl get appliedtogroup [name] [-o yaml] antctl get addressgroup [name] [-o yaml] ``` -Antrea Agent additionally supports printing NetworkPolicies applied to a -specified local Pod using this `antctl` command: - -```bash -antctl get networkpolicy -p pod -n namespace -``` - -Antrea Agent also supports `sort-by=effectivePriority` option, which can be used to +NetworkPolicy also supports `sort-by=effectivePriority` option, which can be used to view the effective order in which the NetworkPolicies are evaluated. Antrea-native NetworkPolicy ordering is documented [here]( antrea-network-policy.md#antrea-native-policy-ordering-based-on-priorities). @@ -184,6 +177,13 @@ antrea-network-policy.md#antrea-native-policy-ordering-based-on-priorities). antctl get networkpolicy --sort-by=effectivePriority ``` +Antrea Agent additionally supports printing NetworkPolicies applied to a +specified local Pod using this `antctl` command: + +```bash +antctl get networkpolicy -p pod -n namespace +``` + #### Mapping endpoints to NetworkPolicies `antctl` supports mapping a specific Pod to the NetworkPolicies which "select" diff --git a/docs/antrea-network-policy.md b/docs/antrea-network-policy.md index 20a172a2412..228ba74b79c 100644 --- a/docs/antrea-network-policy.md +++ b/docs/antrea-network-policy.md @@ -503,8 +503,9 @@ policy rules match, the packet is then enforced for rules created for K8s NP. If the packet still does not match any rule for K8s NP, it will then be evaluated against policies created in the "baseline" Tier. -The [antctl command](antctl.md#networkPolicy-commands) with 'sort-by' flag can be used -to check the order of policy enforcemnet on a specific Node. An example output will look like +The [antctl command](antctl.md#networkPolicy-commands) with 'sort-by=effectivePriority' +flag can be used to check the order of policy enforcemnet. +An example output will look like the following: ```text antctl get netpol --sort-by=effectivePriority diff --git a/pkg/agent/apiserver/handlers/networkpolicy/handler.go b/pkg/agent/apiserver/handlers/networkpolicy/handler.go index b9bcac63dad..083bc092d14 100644 --- a/pkg/agent/apiserver/handlers/networkpolicy/handler.go +++ b/pkg/agent/apiserver/handlers/networkpolicy/handler.go @@ -19,12 +19,10 @@ import ( "fmt" "net/http" "net/url" - "sort" "strings" agentquerier "github.com/vmware-tanzu/antrea/pkg/agent/querier" cpv1beta "github.com/vmware-tanzu/antrea/pkg/apis/controlplane/v1beta2" - "github.com/vmware-tanzu/antrea/pkg/controller/networkpolicy" "github.com/vmware-tanzu/antrea/pkg/querier" ) @@ -32,7 +30,7 @@ import ( // to query network policy rules in current agent. func HandleFunc(aq agentquerier.AgentQuerier) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - npFilter, err := parseURLQuery(r.URL.Query()) + npFilter, err := newFilterFromURLQuery(r.URL.Query()) if err != nil { http.Error(w, err.Error(), http.StatusBadRequest) return @@ -50,12 +48,7 @@ func HandleFunc(aq agentquerier.AgentQuerier) http.HandlerFunc { } else { nps = npq.GetNetworkPolicies(npFilter) } - npSorter := &NPSorter{ - networkPolicies: nps, - sortBy: r.URL.Query().Get("sort-by"), - } - sort.Sort(npSorter) - obj = cpv1beta.NetworkPolicyList{Items: npSorter.networkPolicies} + obj = cpv1beta.NetworkPolicyList{Items: nps} if err := json.NewEncoder(w).Encode(obj); err != nil { http.Error(w, "Failed to encode response: "+err.Error(), http.StatusInternalServerError) @@ -63,51 +56,6 @@ func HandleFunc(aq agentquerier.AgentQuerier) http.HandlerFunc { } } -var ( - sortByEffectivePriority = "effectivePriority" - // Compute a tierPriority value in between the application tier and the baseline tier, - // which can be used to sort policies by tier. - effectiveTierPriorityK8sNP = (networkpolicy.DefaultTierPriority + networkpolicy.BaselineTierPriority) / 2 -) - -type NPSorter struct { - networkPolicies []cpv1beta.NetworkPolicy - sortBy string -} - -func (nps *NPSorter) Len() int { return len(nps.networkPolicies) } -func (nps *NPSorter) Swap(i, j int) { - nps.networkPolicies[i], nps.networkPolicies[j] = nps.networkPolicies[j], nps.networkPolicies[i] -} -func (nps *NPSorter) Less(i, j int) bool { - switch nps.sortBy { - case sortByEffectivePriority: - var ti, tj int32 - if nps.networkPolicies[i].TierPriority == nil { - ti = effectiveTierPriorityK8sNP - } else { - ti = *nps.networkPolicies[i].TierPriority - } - if nps.networkPolicies[j].TierPriority == nil { - tj = effectiveTierPriorityK8sNP - } else { - tj = *nps.networkPolicies[j].TierPriority - } - pi, pj := nps.networkPolicies[i].Priority, nps.networkPolicies[j].Priority - if ti != tj { - return ti < tj - } - if pi != nil && pj != nil && *pi != *pj { - return *pi < *pj - } - fallthrough - default: - // Do not need a tie-breaker here since NetworkPolicy names are set as UID - // of the source policy and will be unique. - return nps.networkPolicies[i].Name < nps.networkPolicies[j].Name - } -} - // From user shorthand input to cpv1beta1.NetworkPolicyType var mapToNetworkPolicyType = map[string]cpv1beta.NetworkPolicyType{ "NP": cpv1beta.K8sNetworkPolicy, @@ -117,7 +65,7 @@ var mapToNetworkPolicyType = map[string]cpv1beta.NetworkPolicyType{ } // Create a Network Policy Filter from URL Query -func parseURLQuery(query url.Values) (*querier.NetworkPolicyQueryFilter, error) { +func newFilterFromURLQuery(query url.Values) (*querier.NetworkPolicyQueryFilter, error) { namespace := query.Get("namespace") pod := query.Get("pod") if pod != "" && namespace == "" { @@ -136,10 +84,6 @@ func parseURLQuery(query url.Values) (*querier.NetworkPolicyQueryFilter, error) return nil, fmt.Errorf("with a name, none of the other fields can be set") } - sortBy := query.Get("sort-by") - if sortBy != "" && sortBy != sortByEffectivePriority { - return nil, fmt.Errorf("unsupported sort-by option. Supported value is %s", sortByEffectivePriority) - } return &querier.NetworkPolicyQueryFilter{ Name: name, SourceName: source, diff --git a/pkg/antctl/antctl.go b/pkg/antctl/antctl.go index 2ed1402536d..4767e0e76b4 100644 --- a/pkg/antctl/antctl.go +++ b/pkg/antctl/antctl.go @@ -109,7 +109,7 @@ var CommandList = &commandList{ $ antctl get networkpolicy 6001549b-ba63-4752-8267-30f52b4332db Get the list of all control plane NetworkPolicies $ antctl get networkpolicy - Get the list of all control plane NetworkPolicies, sorted by the order in which the policies are evaluated (supported by agent only) + Get the list of all control plane NetworkPolicies, sorted by the order in which the policies are evaluated. $ antctl get networkpolicy --sort-by=effectivePriority Get the control plane NetworkPolicy with a specific source (supported by agent only) $ antctl get networkpolicy -S allow-http -n ns1 @@ -155,11 +155,6 @@ var CommandList = &commandList{ usage: "Get NetworkPolicies with specific type. Type means the type of its source network policy: K8sNP, ACNP, ANP", shorthand: "T", }, - { - name: "sort-by", - usage: "Get NetworkPolicies in specific order. Current supported value is effectivePriority. If not specified, by default results are sorted by name.", - shorthand: "O", - }, }, outputType: multiple, }, diff --git a/pkg/antctl/command_definition.go b/pkg/antctl/command_definition.go index c77c9699170..cd4ad700693 100644 --- a/pkg/antctl/command_definition.go +++ b/pkg/antctl/command_definition.go @@ -127,6 +127,14 @@ func (e *resourceEndpoint) flags() []flagInfo { usage: "Filter the resource by namespace", }) } + if e.groupVersionResource == &v1beta2.NetworkPolicyVersionResource { + flags = append(flags, flagInfo{ + name: "sort-by", + shorthand: "b", + defaultValue: "", + usage: "Get NetworkPolicies in specific order. Current supported value is effectivePriority.", + }) + } return flags } @@ -151,7 +159,7 @@ type endpoint struct { // addonTransform is used to transform or update the response data received // from the handler, it must returns an interface which has same type as // TransformedResponse. - addonTransform func(reader io.Reader, single bool) (interface{}, error) + addonTransform func(reader io.Reader, single bool, opts map[string]string) (interface{}, error) } // flagInfo represents a command-line flag that can be provided when invoking an antctl command. @@ -200,7 +208,7 @@ func (cd *commandDefinition) namespaced() bool { return false } -func (cd *commandDefinition) getAddonTransform() func(reader io.Reader, single bool) (interface{}, error) { +func (cd *commandDefinition) getAddonTransform() func(reader io.Reader, single bool, opts map[string]string) (interface{}, error) { if runtime.Mode == runtime.ModeAgent && cd.agentEndpoint != nil { return cd.agentEndpoint.addonTransform } else if runtime.Mode == runtime.ModeController && cd.controllerEndpoint != nil { @@ -681,7 +689,7 @@ func (cd *commandDefinition) tableOutput(obj interface{}, writer io.Writer) erro // format. If the AddonTransform is set, it will use the function to transform // the data first. It will try to output the resp in the format ft specified after // doing transform. -func (cd *commandDefinition) output(resp io.Reader, writer io.Writer, ft formatterType, single bool) (err error) { +func (cd *commandDefinition) output(resp io.Reader, writer io.Writer, ft formatterType, single bool, args map[string]string) (err error) { var obj interface{} addonTransform := cd.getAddonTransform() @@ -695,7 +703,7 @@ func (cd *commandDefinition) output(resp io.Reader, writer io.Writer, ft formatt return fmt.Errorf("error when decoding response %v: %w", resp, err) } } else { - obj, err = addonTransform(resp, single) + obj, err = addonTransform(resp, single, args) if err != nil { return fmt.Errorf("error when doing local transform: %w", err) } @@ -780,7 +788,7 @@ func (cd *commandDefinition) newCommandRunE(c *client) func(*cobra.Command, []st return err } isSingle := cd.getEndpoint().OutputType() != multiple && (cd.getEndpoint().OutputType() == single || argGet) - return cd.output(resp, os.Stdout, formatterType(outputFormat), isSingle) + return cd.output(resp, os.Stdout, formatterType(outputFormat), isSingle, argMap) } } diff --git a/pkg/antctl/command_definition_test.go b/pkg/antctl/command_definition_test.go index 3fd027b78ea..58570a98ec0 100644 --- a/pkg/antctl/command_definition_test.go +++ b/pkg/antctl/command_definition_test.go @@ -304,7 +304,7 @@ func TestFormat(t *testing.T) { for _, tc := range []struct { name string single bool - transform func(reader io.Reader, single bool) (interface{}, error) + transform func(reader io.Reader, single bool, opts map[string]string) (interface{}, error) rawResponseData interface{} responseStruct reflect.Type expected string @@ -328,7 +328,7 @@ func TestFormat(t *testing.T) { { name: "StructureData-Transform-Single-Yaml", single: true, - transform: func(reader io.Reader, single bool) (i interface{}, err error) { + transform: func(reader io.Reader, single bool, opts map[string]string) (i interface{}, err error) { foo := &Foobar{} err = json.NewDecoder(reader).Decode(foo) return &struct{ Bar string }{Bar: foo.Foo}, err @@ -363,7 +363,7 @@ func TestFormat(t *testing.T) { responseData, err := json.Marshal(tc.rawResponseData) assert.Nil(t, err) var outputBuf bytes.Buffer - err = opt.output(bytes.NewBuffer(responseData), &outputBuf, tc.formatter, tc.single) + err = opt.output(bytes.NewBuffer(responseData), &outputBuf, tc.formatter, tc.single, map[string]string{}) assert.Nil(t, err) assert.Equal(t, tc.expected, outputBuf.String()) }) diff --git a/pkg/antctl/transform/addressgroup/transform.go b/pkg/antctl/transform/addressgroup/transform.go index 31fdbfae1c5..a34929e67a6 100644 --- a/pkg/antctl/transform/addressgroup/transform.go +++ b/pkg/antctl/transform/addressgroup/transform.go @@ -28,18 +28,18 @@ type Response struct { Pods []common.GroupMember `json:"pods,omitempty"` } -func listTransform(l interface{}) (interface{}, error) { +func listTransform(l interface{}, opts map[string]string) (interface{}, error) { groups := l.(*cpv1beta.AddressGroupList) result := []interface{}{} for i := range groups.Items { item := groups.Items[i] - o, _ := objectTransform(&item) + o, _ := objectTransform(&item, opts) result = append(result, o.(Response)) } return result, nil } -func objectTransform(o interface{}) (interface{}, error) { +func objectTransform(o interface{}, _ map[string]string) (interface{}, error) { group := o.(*cpv1beta.AddressGroup) var pods []common.GroupMember for _, pod := range group.GroupMembers { @@ -48,12 +48,13 @@ func objectTransform(o interface{}) (interface{}, error) { return Response{Name: group.Name, Pods: pods}, nil } -func Transform(reader io.Reader, single bool) (interface{}, error) { +func Transform(reader io.Reader, single bool, opts map[string]string) (interface{}, error) { return transform.GenericFactory( reflect.TypeOf(cpv1beta.AddressGroup{}), reflect.TypeOf(cpv1beta.AddressGroupList{}), objectTransform, listTransform, + opts, )(reader, single) } diff --git a/pkg/antctl/transform/appliedtogroup/transform.go b/pkg/antctl/transform/appliedtogroup/transform.go index 33d9201dc2c..d4298ad9ef8 100644 --- a/pkg/antctl/transform/appliedtogroup/transform.go +++ b/pkg/antctl/transform/appliedtogroup/transform.go @@ -28,18 +28,18 @@ type Response struct { Pods []common.GroupMember `json:"pods,omitempty"` } -func listTransform(l interface{}) (interface{}, error) { +func listTransform(l interface{}, opts map[string]string) (interface{}, error) { groups := l.(*cpv1beta.AppliedToGroupList) result := []Response{} for i := range groups.Items { group := groups.Items[i] - o, _ := objectTransform(&group) + o, _ := objectTransform(&group, opts) result = append(result, o.(Response)) } return result, nil } -func objectTransform(o interface{}) (interface{}, error) { +func objectTransform(o interface{}, _ map[string]string) (interface{}, error) { group := o.(*cpv1beta.AppliedToGroup) var pods []common.GroupMember for _, pod := range group.GroupMembers { @@ -48,12 +48,13 @@ func objectTransform(o interface{}) (interface{}, error) { return Response{Name: group.GetName(), Pods: pods}, nil } -func Transform(reader io.Reader, single bool) (interface{}, error) { +func Transform(reader io.Reader, single bool, opts map[string]string) (interface{}, error) { return transform.GenericFactory( reflect.TypeOf(cpv1beta.AppliedToGroup{}), reflect.TypeOf(cpv1beta.AppliedToGroupList{}), objectTransform, listTransform, + opts, )(reader, single) } diff --git a/pkg/antctl/transform/controllerinfo/transform.go b/pkg/antctl/transform/controllerinfo/transform.go index d11594fb775..a5ebca49328 100644 --- a/pkg/antctl/transform/controllerinfo/transform.go +++ b/pkg/antctl/transform/controllerinfo/transform.go @@ -38,7 +38,7 @@ type Response struct { ControllerConditions []clusterinfo.ControllerCondition `json:"controllerConditions,omitempty"` // Controller condition contains types like ControllerHealthy } -func Transform(reader io.Reader, _ bool) (interface{}, error) { +func Transform(reader io.Reader, _ bool, _ map[string]string) (interface{}, error) { b, err := ioutil.ReadAll(reader) if err != nil { return nil, err diff --git a/pkg/antctl/transform/networkpolicy/transform.go b/pkg/antctl/transform/networkpolicy/transform.go index 4519199d293..134f57037f6 100644 --- a/pkg/antctl/transform/networkpolicy/transform.go +++ b/pkg/antctl/transform/networkpolicy/transform.go @@ -17,40 +17,95 @@ package networkpolicy import ( "io" "reflect" + "sort" "strconv" "github.com/vmware-tanzu/antrea/pkg/antctl/transform" "github.com/vmware-tanzu/antrea/pkg/antctl/transform/common" cpv1beta "github.com/vmware-tanzu/antrea/pkg/apis/controlplane/v1beta2" + "github.com/vmware-tanzu/antrea/pkg/controller/networkpolicy" ) type Response struct { *cpv1beta.NetworkPolicy } -func objectTransform(o interface{}) (interface{}, error) { +func objectTransform(o interface{}, _ map[string]string) (interface{}, error) { return Response{o.(*cpv1beta.NetworkPolicy)}, nil } -func listTransform(l interface{}) (interface{}, error) { +func listTransform(l interface{}, opts map[string]string) (interface{}, error) { policyList := l.(*cpv1beta.NetworkPolicyList) + sortBy := "" + if sb, ok := opts["sort-by"]; ok { + sortBy = sb + } + npSorter := &NPSorter{ + networkPolicies: policyList.Items, + sortBy: sortBy, + } + sort.Sort(npSorter) result := make([]Response, 0, len(policyList.Items)) - for i := range policyList.Items { - o, _ := objectTransform(&policyList.Items[i]) + for i := range npSorter.networkPolicies { + o, _ := objectTransform(&npSorter.networkPolicies[i], opts) result = append(result, o.(Response)) } return result, nil } -func Transform(reader io.Reader, single bool) (interface{}, error) { +func Transform(reader io.Reader, single bool, opts map[string]string) (interface{}, error) { return transform.GenericFactory( reflect.TypeOf(cpv1beta.NetworkPolicy{}), reflect.TypeOf(cpv1beta.NetworkPolicyList{}), objectTransform, listTransform, + opts, )(reader, single) } +var ( + sortByEffectivePriority = "effectivePriority" + // Compute a tierPriority value in between the application tier and the baseline tier, + // which can be used to sort all policies by tier. + effectiveTierPriorityK8sNP = (networkpolicy.DefaultTierPriority + networkpolicy.BaselineTierPriority) / 2 +) + +type NPSorter struct { + networkPolicies []cpv1beta.NetworkPolicy + sortBy string +} + +func (nps *NPSorter) Len() int { return len(nps.networkPolicies) } +func (nps *NPSorter) Swap(i, j int) { nps.networkPolicies[i], nps.networkPolicies[j] = nps.networkPolicies[j], nps.networkPolicies[i] } +func (nps *NPSorter) Less(i, j int) bool { + switch nps.sortBy { + case sortByEffectivePriority: + var ti, tj int32 + if nps.networkPolicies[i].TierPriority == nil { + ti = effectiveTierPriorityK8sNP + } else { + ti = *nps.networkPolicies[i].TierPriority + } + if nps.networkPolicies[j].TierPriority == nil { + tj = effectiveTierPriorityK8sNP + } else { + tj = *nps.networkPolicies[j].TierPriority + } + pi, pj := nps.networkPolicies[i].Priority, nps.networkPolicies[j].Priority + if ti != tj { + return ti < tj + } + if pi != nil && pj != nil && *pi != *pj { + return *pi < *pj + } + fallthrough + default: + // Do not need a tie-breaker here since NetworkPolicy names are set as UID + // of the source policy and will be unique. + return nps.networkPolicies[i].Name < nps.networkPolicies[j].Name + } +} + func priorityToString(p interface{}) string { if reflect.ValueOf(p).IsNil() { return "" diff --git a/pkg/antctl/transform/utils.go b/pkg/antctl/transform/utils.go index 215fb49e7ef..fa023f17fe6 100644 --- a/pkg/antctl/transform/utils.go +++ b/pkg/antctl/transform/utils.go @@ -20,10 +20,10 @@ import ( "reflect" ) -type unary func(interface{}) (interface{}, error) +type unary func(interface{}, map[string]string) (interface{}, error) type FuncType func(reader io.Reader, single bool) (interface{}, error) -func GenericFactory(objType, listType reflect.Type, objTransform, listTransform unary) FuncType { +func GenericFactory(objType, listType reflect.Type, objTransform, listTransform unary, opts map[string]string) FuncType { return func(reader io.Reader, single bool) (interface{}, error) { var refType reflect.Type if single { @@ -36,9 +36,9 @@ func GenericFactory(objType, listType reflect.Type, objTransform, listTransform return nil, err } if single && objTransform != nil { - return objTransform(refVal.Interface()) + return objTransform(refVal.Interface(), opts) } else if !single && listTransform != nil { - return listTransform(refVal.Interface()) + return listTransform(refVal.Interface(), opts) } return refVal.Interface(), nil } diff --git a/pkg/antctl/transform/version/transform.go b/pkg/antctl/transform/version/transform.go index efc44aadfeb..bd4b839ede3 100644 --- a/pkg/antctl/transform/version/transform.go +++ b/pkg/antctl/transform/version/transform.go @@ -35,7 +35,7 @@ type Response struct { // AgentVersion is the AddonTransform for the version command. This function // will try to parse the response as a AgentVersionResponse and then populate // it with the version of antctl to a transformedVersionResponse object. -func AgentTransform(reader io.Reader, _ bool) (interface{}, error) { +func AgentTransform(reader io.Reader, _ bool, _ map[string]string) (interface{}, error) { b, err := ioutil.ReadAll(reader) if err != nil { return nil, err @@ -60,7 +60,7 @@ func AgentTransform(reader io.Reader, _ bool) (interface{}, error) { return resp, nil } -func ControllerTransform(reader io.Reader, _ bool) (interface{}, error) { +func ControllerTransform(reader io.Reader, _ bool, _ map[string]string) (interface{}, error) { b, err := ioutil.ReadAll(reader) if err != nil { return nil, err