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

Jump to bottom

Add label filtering (-l) for describe commands #2934

Merged
merged 5 commits into from
Apr 18, 2024
Merged

Add label filtering (-l) for describe commands #2934

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7857cfb
Add label filtering (-l) for describe commands
deszhou Apr 3, 2024
3c7b3e5
fix nits
deszhou Apr 4, 2024
bc3262e
fix test
deszhou Apr 10, 2024
f69b125
refactor test
deszhou Apr 17, 2024
c7c406d
move tests to discoverer_test.go
deszhou Apr 18, 2024
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
47 changes: 43 additions & 4 deletions gwctl/cmd/describe.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,13 @@ import (
"sigs.k8s.io/gateway-api/gwctl/pkg/utils"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
)

func NewDescribeCommand() *cobra.Command {
var namespaceFlag string
var allNamespacesFlag bool
var labelSelector string

cmd := &cobra.Command{
Use: "describe {policies|httproutes|gateways|gatewayclasses|backends|namespace} RESOURCE_NAME",
Expand All @@ -45,6 +47,7 @@ func NewDescribeCommand() *cobra.Command {
}
cmd.Flags().StringVarP(&namespaceFlag, "namespace", "n", "default", "")
cmd.Flags().BoolVarP(&allNamespacesFlag, "all-namespaces", "A", false, "If present, list requested resources from all namespaces.")
cmd.Flags().StringVarP(&labelSelector, "selector", "l", "", "Selector (label query) to filter on, supports '=', '==', and '!='.(e.g. -l key1=value1,key2=value2). Matching objects must satisfy all of the specified label constraints.")

return cmd
}
Expand All @@ -64,6 +67,12 @@ func runDescribe(cmd *cobra.Command, args []string, params *utils.CmdParams) {
os.Exit(1)
}

labelSelector, err := cmd.Flags().GetString("selector")
if err != nil {
fmt.Fprintf(os.Stderr, "failed to read flag \"selector\": %v\n", err)
os.Exit(1)
}

if allNs {
ns = metav1.NamespaceAll
}
Expand Down Expand Up @@ -97,7 +106,15 @@ func runDescribe(cmd *cobra.Command, args []string, params *utils.CmdParams) {
policiesPrinter.PrintDescribeView(policyList)

case "httproute", "httproutes":
filter := resourcediscovery.Filter{Namespace: ns}
selector, err := labels.Parse(labelSelector)
if err != nil {
fmt.Fprintf(os.Stderr, "Unable to find resources that match the label selector \"%s\": %v\n", labelSelector, err)
os.Exit(1)
}
Comment on lines +109 to +113
Copy link
Contributor

@yashvardhan-kukreja yashvardhan-kukreja Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I believe this entire section can be taken outside the switch (say at Line 90) to reduce this code's repeatability across all the case-es

Copy link
Member Author

Choose a reason for hiding this comment

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

If placed on line 90, gwctl will first check the label and then the resources. I think the label logic in gwctl should align with kubectl

➜  kubectl get httproutess -l dsdsa.ds-
error: the server doesn't have a resource type "httproutess"
  • Current:
➜  gwctl get httproutess -l dsdasa.ds-
Unrecognized RESOURCE_TYPE
  • After:
➜  gwctl get httproutss -l dsdsm,ds--
Unable to find resources that match the label selector "dsdsm,ds--": unable to parse requirement: <nil>: Invalid value: "ds--": name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName',  or 'my.name',  or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]')

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the label logic in gwctl should align with kubectl

I get your point though I'm not sure if that's worthwhile.

As a user, I don't care (I never even noticed haha) the order of this validation (resource_type -> label OR label -> resource_type)

As a developer, I "personally" wouldn't find this reason strong enough to have this level of code repeatability across so many cases.
Moreover, this would add up to the cognitive load of copy-ing the same code again in case in the future a new set of resource types are introduced under gwctl.

Rest, this is just my personal pov.

I'd leave this to yours and @gauravkghildiyal 's judgement :)

Cheers!

Copy link
Member

Choose a reason for hiding this comment

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

Yep we do have code-duplication in a couple of places at present that needs to be improved. (Sorry I may be responsible for many of those) @yeedove thanks for an eye towards such subtle details, that's really nice!

Is it not feasible to have a function like parseLabelSelectorOrExit() and still keep the error evaluation order as a middle ground? If that's not possible, I feel like it's fair game if we do get rid of the code duplication here as that may not affect the user experience drastically. (Thanks @yashvardhan-kukreja!)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the change Dennis!

Yes you are right that the label selection (for the time being) is only for the immediate resource being fetched (like gwctl describe httproutes -l mylabel=myhttproute is only supposed to filter the HTTPRoutes, and not extend the filtering to resources associated with the HTTPRoute, like Gateways and Backends)

From your present commit, the function under test for the tests happens to be DiscoverResourcesForGatewayClass() (and equivalent). Given this, can we please move them to the discoverer_test.go file within the resourcediscovery package?

filter := resourcediscovery.Filter{
Namespace: ns,
Labels: selector,
}
if len(args) > 1 {
filter.Name = args[1]
}
Expand All @@ -109,7 +126,15 @@ func runDescribe(cmd *cobra.Command, args []string, params *utils.CmdParams) {
httpRoutesPrinter.PrintDescribeView(resourceModel)

case "gateway", "gateways":
filter := resourcediscovery.Filter{Namespace: ns}
selector, err := labels.Parse(labelSelector)
if err != nil {
fmt.Fprintf(os.Stderr, "Unable to find resources that match the label selector \"%s\": %v\n", labelSelector, err)
os.Exit(1)
}
Comment on lines +129 to +133
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as https://github.com/kubernetes-sigs/gateway-api/pull/2934/files#r1550940269

filter := resourcediscovery.Filter{
Namespace: ns,
Labels: selector,
}
if len(args) > 1 {
filter.Name = args[1]
}
Expand All @@ -121,7 +146,14 @@ func runDescribe(cmd *cobra.Command, args []string, params *utils.CmdParams) {
gwPrinter.PrintDescribeView(resourceModel)

case "gatewayclass", "gatewayclasses":
filter := resourcediscovery.Filter{}
selector, err := labels.Parse(labelSelector)
if err != nil {
fmt.Fprintf(os.Stderr, "Unable to find resources that match the label selector \"%s\": %v\n", labelSelector, err)
os.Exit(1)
}
filter := resourcediscovery.Filter{
Labels: selector,
}
if len(args) > 1 {
filter.Name = args[1]
}
Expand All @@ -147,7 +179,14 @@ func runDescribe(cmd *cobra.Command, args []string, params *utils.CmdParams) {
backendsPrinter.PrintDescribeView(resourceModel)

case "namespace", "namespaces", "ns":
filter := resourcediscovery.Filter{}
selector, err := labels.Parse(labelSelector)
if err != nil {
fmt.Fprintf(os.Stderr, "Unable to find resources that match the label selector \"%s\": %v\n", labelSelector, err)
os.Exit(1)
}
Comment on lines +182 to +186
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as https://github.com/kubernetes-sigs/gateway-api/pull/2934/files#r1550940269

filter := resourcediscovery.Filter{
Labels: selector,
}
if len(args) > 1 {
filter.Name = args[1]
}
Expand Down
2 changes: 1 addition & 1 deletion gwctl/cmd/get.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func NewGetCommand() *cobra.Command {
}
cmd.Flags().StringVarP(&namespaceFlag, "namespace", "n", "default", "")
cmd.Flags().BoolVarP(&allNamespacesFlag, "all-namespaces", "A", false, "If present, list requested resources from all namespaces.")
cmd.Flags().StringVarP(&labelSelector, "selector", "l", "", "Label selector.")
cmd.Flags().StringVarP(&labelSelector, "selector", "l", "", "Selector (label query) to filter on, supports '=', '==', and '!='.(e.g. -l key1=value1,key2=value2). Matching objects must satisfy all of the specified label constraints.")

return cmd
}
Expand Down
2 changes: 2 additions & 0 deletions gwctl/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ require (
github.com/evanphx/json-patch v5.9.0+incompatible
github.com/google/go-cmp v0.6.0
github.com/spf13/cobra v1.8.0
github.com/stretchr/testify v1.9.0
k8s.io/api v0.29.3
k8s.io/apiextensions-apiserver v0.29.3
k8s.io/apimachinery v0.29.3
Expand Down Expand Up @@ -41,6 +42,7 @@ require (
github.com/modern-go/reflect2 v1.0.2 // indirect
github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect
github.com/pkg/errors v0.9.1 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/spf13/pflag v1.0.5 // indirect
golang.org/x/net v0.20.0 // indirect
golang.org/x/oauth2 v0.16.0 // indirect
Expand Down
20 changes: 7 additions & 13 deletions gwctl/pkg/printer/gatewayclasses_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"time"

"github.com/google/go-cmp/cmp"
"github.com/stretchr/testify/assert"

apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -236,7 +237,7 @@ ControllerName: example.net/gateway-controller
}
}

// TestGatewayClassesPrinter_LabelSelector Tests label selector filtering for GatewayClasses in 'get' command.
// TestGatewayClassesPrinter_LabelSelector Tests label selector filtering for GatewayClasses.
func TestGatewayClassesPrinter_LabelSelector(t *testing.T) {
fakeClock := testingclock.NewFakeClock(time.Now())

Expand Down Expand Up @@ -281,18 +282,11 @@ func TestGatewayClassesPrinter_LabelSelector(t *testing.T) {
t.Fatalf("Failed to construct resourceModel: %v", resourceModel)
}

gcp := &GatewayClassesPrinter{
Out: params.Out,
Clock: fakeClock,
expectedGatewayClassNames := []string{"foo-com-internal-gateway-class"}
gatewayClassNames := make([]string, 0, len(resourceModel.GatewayClasses))
for _, gatewayClassNode := range resourceModel.GatewayClasses {
gatewayClassNames = append(gatewayClassNames, gatewayClassNode.GatewayClass.GetName())
}
gcp.Print(resourceModel)

got := params.Out.(*bytes.Buffer).String()
want := `
NAME CONTROLLER ACCEPTED AGE
foo-com-internal-gateway-class foo-com-internal-gateway-class/controller True 365d
`
if diff := cmp.Diff(common.YamlString(want), common.YamlString(got), common.YamlStringTransformer); diff != "" {
t.Errorf("Unexpected diff\ngot=\n%v\nwant=\n%v\ndiff (-want +got)=\n%v", got, want, diff)
}
assert.Equal(t, expectedGatewayClassNames, gatewayClassNames, "Expected GatewayClass name does not match the found one")
Copy link
Member

Choose a reason for hiding this comment

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

Non blocking: Let's try to avoid a new dependency if possible. Instead, let's not shy away from writing something like:

	if diff := cmp.Diff(expectedGatewayClassNames, gatewayClassNames); diff != "" {
		t.Errorf("DiscoverResourcesForGatewayClass(%+v) returned unexpected diff (-want, +got):\n%v", filter, diff)
	}

(Personally, I try to avoid assertion libraries but wiser people than me do infact use it so can't really complain and don't want to enforce)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, done!

}
22 changes: 8 additions & 14 deletions gwctl/pkg/printer/gateways_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import (
"time"

"github.com/google/go-cmp/cmp"
"github.com/stretchr/testify/assert"

apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
Expand Down Expand Up @@ -366,7 +368,7 @@ EffectivePolicies:
}
}

// TestGatewaysPrinter_LabelSelector tests label selector filtering for Gateways in 'get' command.
// TestGatewaysPrinter_LabelSelector tests label selector filtering for Gateways.
func TestGatewaysPrinter_LabelSelector(t *testing.T) {
fakeClock := testingclock.NewFakeClock(time.Now())
gateway := func(name string, labels map[string]string) *gatewayv1.Gateway {
Expand Down Expand Up @@ -433,19 +435,11 @@ func TestGatewaysPrinter_LabelSelector(t *testing.T) {
t.Fatalf("Failed to construct resourceModel: %v", resourceModel)
}

gp := &GatewaysPrinter{
Out: params.Out,
Clock: fakeClock,
expectedGatewayNames := []string{"gateway-2"}
gatewayNames := make([]string, 0, len(resourceModel.Gateways))
for _, gatewayNode := range resourceModel.Gateways {
gatewayNames = append(gatewayNames, gatewayNode.Gateway.GetName())
}
gp.Print(resourceModel)

got := params.Out.(*bytes.Buffer).String()
want := `
NAME CLASS ADDRESSES PORTS PROGRAMMED AGE
gateway-2 gatewayclass-1 192.168.100.5 8080 False 5d
`

if diff := cmp.Diff(common.YamlString(want), common.YamlString(got), common.YamlStringTransformer); diff != "" {
t.Errorf("Unexpected diff\ngot=\n%v\nwant=\n%v\ndiff (-want +got)=\n%v", got, want, diff)
}
assert.Equal(t, expectedGatewayNames, gatewayNames, "Expected Gateway name does not match the found one")
}
27 changes: 10 additions & 17 deletions gwctl/pkg/printer/httproutes_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,9 @@ import (
"testing"
"time"

gatewayv1 "sigs.k8s.io/gateway-api/apis/v1"
gatewayv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2"

"github.com/google/go-cmp/cmp"
"github.com/stretchr/testify/assert"

corev1 "k8s.io/api/core/v1"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -33,6 +32,8 @@ import (
"k8s.io/apimachinery/pkg/runtime"
testingclock "k8s.io/utils/clock/testing"

gatewayv1 "sigs.k8s.io/gateway-api/apis/v1"
gatewayv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2"
"sigs.k8s.io/gateway-api/gwctl/pkg/common"
"sigs.k8s.io/gateway-api/gwctl/pkg/resourcediscovery"
"sigs.k8s.io/gateway-api/gwctl/pkg/utils"
Expand Down Expand Up @@ -437,7 +438,7 @@ EffectivePolicies:
}
}

// TestHTTPRoutesPrinter_LabelSelector tests label selector filtering for HTTPRoute in 'get' command.
// TestHTTPRoutesPrinter_LabelSelector tests label selector filtering for HTTPRoute.
func TestHTTPRoutesPrinter_LabelSelector(t *testing.T) {
fakeClock := testingclock.NewFakeClock(time.Now())
httpRoute := func(name string, labels map[string]string) *gatewayv1.HTTPRoute {
Expand Down Expand Up @@ -503,19 +504,11 @@ func TestHTTPRoutesPrinter_LabelSelector(t *testing.T) {
t.Fatalf("Failed to discover resources: %v", err)
}

hp := &HTTPRoutesPrinter{
Out: params.Out,
Clock: fakeClock,
expectedHTTPRouteNames := []string{"httproute-2"}
HTTPRouteNames := make([]string, 0, len(resourceModel.HTTPRoutes))
for _, HTTPRouteNode := range resourceModel.HTTPRoutes {
HTTPRouteNames = append(HTTPRouteNames, HTTPRouteNode.HTTPRoute.GetName())
}
hp.Print(resourceModel)

got := params.Out.(*bytes.Buffer).String()
want := `
NAMESPACE NAME HOSTNAMES PARENT REFS AGE
default httproute-2 example.com 1 24h

`
if diff := cmp.Diff(common.YamlString(want), common.YamlString(got), common.YamlStringTransformer); diff != "" {
t.Errorf("Unexpected diff\ngot=\n%v\nwant=\n%v\ndiff (-want +got)=\n%v", got, want, diff)
}
assert.Equal(t, expectedHTTPRouteNames, HTTPRouteNames, "Expected HTTPRoute name does not match the found one")
}
21 changes: 7 additions & 14 deletions gwctl/pkg/printer/namespace_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"time"

"github.com/google/go-cmp/cmp"
"github.com/stretchr/testify/assert"

corev1 "k8s.io/api/core/v1"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
Expand Down Expand Up @@ -259,7 +260,7 @@ DirectlyAttachedPolicies:
}
}

// TestNamespacesPrinter_LabelSelector tests label selector filtering for Namespaces in 'get' command.
// TestNamespacesPrinter_LabelSelector tests label selector filtering for Namespaces.
func TestNamespacesPrinter_LabelSelector(t *testing.T) {
fakeClock := testingclock.NewFakeClock(time.Now())
namespace := func(name string, labels map[string]string) *corev1.Namespace {
Expand Down Expand Up @@ -297,19 +298,11 @@ func TestNamespacesPrinter_LabelSelector(t *testing.T) {
t.Fatalf("Failed to construct resourceModel: %v", resourceModel)
}

nsp := &NamespacesPrinter{
Out: params.Out,
Clock: fakeClock,
expectedNamespaceNames := []string{"namespace-2"}
namespaceNames := make([]string, 0, len(resourceModel.Namespaces))
for _, namespaceNode := range resourceModel.Namespaces {
namespaceNames = append(namespaceNames, namespaceNode.Namespace.Name)
}
nsp.Print(resourceModel)

got := params.Out.(*bytes.Buffer).String()
want := `
NAME STATUS AGE
namespace-2 Active 46d
`

if diff := cmp.Diff(common.YamlString(want), common.YamlString(got), common.YamlStringTransformer); diff != "" {
t.Errorf("Unexpected diff\ngot=\n%v\nwant=\n%v\ndiff (-want +got)=\n%v", got, want, diff)
}
assert.Equal(t, expectedNamespaceNames, namespaceNames, "Expected Namespace name does not match the found one")
}