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

deszhou · 2024-04-03T18:48:05Z

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?:

Add label filtering (-l) for describe commands

➜  gwctl describe httproute -l app=backend
Name: backend
Namespace: default
Hostnames:
- www.example.com
ParentRefs:
- group: gateway.networking.k8s.io
  kind: Gateway
  name: eg
EffectivePolicies:
  default/eg: {}

➜  gwctl describe ns -l kubernetes.io/metadata.name=envoy-gateway-system
Name: envoy-gateway-system
Annotations:
  kubectl.kubernetes.io/last-applied-configuration: |
    {"apiVersion":"v1","kind":"Namespace","metadata":{"annotations":{},"name":"envoy-gateway-system"}}
Labels:
  kubernetes.io/metadata.name: envoy-gateway-system
  name: envoy-gateway-system
Status: Active

k8s-ci-robot · 2024-04-03T18:48:15Z

Hi @yeedove. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

gwctl/cmd/describe.go

yashvardhan-kukreja · 2024-04-04T05:26:39Z

gwctl/cmd/describe.go

+		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)
+		}


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

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]')

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!

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!)

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?

yashvardhan-kukreja · 2024-04-04T05:27:26Z

gwctl/cmd/describe.go

+		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)
+		}


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

yashvardhan-kukreja · 2024-04-04T05:27:37Z

gwctl/cmd/describe.go

+		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)
+		}


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

gwctl/cmd/describe.go

yashvardhan-kukreja · 2024-04-04T05:34:55Z

Looks like some great work @yeedove !
Hi @gauravkghildiyal , if you don't mind, I've left some comments which I thought would be helpful here :)

gauravkghildiyal · 2024-04-10T01:58:16Z

/assign
/cc @mlavacca

gauravkghildiyal · 2024-04-10T01:58:39Z

/ok-to-test

gauravkghildiyal

Sorry for the delay @yeedove (I was away for a while)

Left some thoughts about the testing. Sorry I know I was fine with the LabelSelector tests for the Get command previously -- with more of such tests, I started feeling that it may not be the best idea.

gauravkghildiyal · 2024-04-17T07:20:55Z

gwctl/cmd/describe.go

+		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)
+		}


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!)

gauravkghildiyal · 2024-04-17T07:31:12Z

gwctl/pkg/printer/gatewayclasses_test.go

@@ -296,3 +296,102 @@ foo-com-internal-gateway-class  foo-com-internal-gateway-class/controller  True
 		t.Errorf("Unexpected diff\ngot=\n%v\nwant=\n%v\ndiff (-want +got)=\n%v", got, want, diff)
 	}
 }
+
+// TestGatewayClassesPrinterDescribe_LabelSelector Tests label selector filtering for GatewayClasses in 'describe' command.
+func TestGatewayClassesPrinterDescribe_LabelSelector(t *testing.T) {


Thanks for the thorough tests @yeedove!

I was actually thinking about this part and I feel that maybe testing LabelSelector is something which is better handled through unit tests within discoverer_test.go which will test functions like fetchGatewayClass (i.e. TestFetchGatewayClass). Idea being, I really want unit tests to be focused on functionality provided by an independent package.

The problem with LabelSelector tests right now is that they aren't just testing LabelSelector but are also testing how exactly the entire GatewayClass object gets printed (which is something that is already covered by the normal test functions like TestGatewayClassesPrinter_PrintDescribeView)

(I know that tests like TestGatewayClassesPrinter_PrintDescribeView are also not close to perfect since they too are indirectly testing the resourcediscovery package by relying on DiscoverResourcesForGatewayClass instead of actually constructing the entire ResourceModel itself. This is something that I would like to correct in the future if possible)

Within discoverer_test.go, these tests would only check that "given this label selector was provided, did I receive the correct GatewayClass names?"

Additionally, we could then also get rid of the LabelSelector tests for the gwctl get.

I recommend finalizing the label functionality for get and describe commands before optimization to avoid potential issues. For instance, the get backend -l command needs to display REFERRED BY ROUTES to these backends, but the relationship between HTTPRoutes and Services is not affected by labels, requiring additional handling. There may be better way :)

In creating unit tests for describe -l, I found that the tests were overly dependent on the command’s output. As we plan to Expand output for gwctl describe resources, updating tests for every change in output is not good idea. I will focus on correctly verifying GatewayClass names instead.

gauravkghildiyal · 2024-04-17T18:38:56Z

gwctl/cmd/describe.go

+		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)
+		}


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?

gauravkghildiyal · 2024-04-17T19:00:32Z

gwctl/pkg/printer/gatewayclasses_test.go

-	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")


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)

deszhou · 2024-04-18T18:16:02Z

/retest

gauravkghildiyal · 2024-04-18T21:26:14Z

/lgtm
/approve
/label tide/merge-method-squash

Thanks @yeedove!

k8s-ci-robot · 2024-04-18T21:26:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gauravkghildiyal, yeedove

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~gwctl/OWNERS~~ [gauravkghildiyal]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Add label filtering (-l) for describe commands

7857cfb

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 3, 2024

k8s-ci-robot requested review from gauravkghildiyal and shaneutt April 3, 2024 18:48

k8s-ci-robot added area/gwctl needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 3, 2024

shaneutt removed their request for review April 3, 2024 19:22

yashvardhan-kukreja reviewed Apr 4, 2024

View reviewed changes

gwctl/cmd/describe.go Outdated Show resolved Hide resolved

yashvardhan-kukreja reviewed Apr 4, 2024

View reviewed changes

gwctl/cmd/describe.go Outdated Show resolved Hide resolved

fix nits

3c7b3e5

k8s-ci-robot assigned gauravkghildiyal Apr 10, 2024

k8s-ci-robot requested a review from mlavacca April 10, 2024 01:58

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 10, 2024

fix test

bc3262e

gauravkghildiyal reviewed Apr 17, 2024

View reviewed changes

refactor test

f69b125

deszhou requested a review from gauravkghildiyal April 17, 2024 11:44

gauravkghildiyal reviewed Apr 17, 2024

View reviewed changes

move tests to discoverer_test.go

c7c406d

k8s-ci-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 18, 2024

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 18, 2024

k8s-ci-robot added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Apr 18, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 18, 2024

k8s-ci-robot merged commit 6b93e6e into kubernetes-sigs:main Apr 18, 2024
8 checks passed

gauravkghildiyal mentioned this pull request Apr 30, 2024

Support JSON and YAML output format in get commands (-o json) #2940

Merged

deszhou mentioned this pull request Jun 6, 2024

REQUEST: New membership for <yeedove> kubernetes/org#5004

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

deszhou commented Apr 3, 2024

k8s-ci-robot commented Apr 3, 2024

yashvardhan-kukreja Apr 4, 2024 •

edited

Loading

deszhou Apr 4, 2024

yashvardhan-kukreja Apr 4, 2024

gauravkghildiyal Apr 17, 2024

gauravkghildiyal Apr 17, 2024

yashvardhan-kukreja Apr 4, 2024

yashvardhan-kukreja Apr 4, 2024

yashvardhan-kukreja commented Apr 4, 2024 •

edited

Loading

gauravkghildiyal commented Apr 10, 2024

gauravkghildiyal commented Apr 10, 2024

gauravkghildiyal left a comment

gauravkghildiyal Apr 17, 2024

gauravkghildiyal Apr 17, 2024

deszhou Apr 17, 2024 •

edited

Loading

gauravkghildiyal Apr 17, 2024

gauravkghildiyal Apr 17, 2024

deszhou Apr 18, 2024

deszhou commented Apr 18, 2024

gauravkghildiyal commented Apr 18, 2024

k8s-ci-robot commented Apr 18, 2024

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

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

Conversation

deszhou commented Apr 3, 2024

k8s-ci-robot commented Apr 3, 2024

yashvardhan-kukreja Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yashvardhan-kukreja commented Apr 4, 2024 • edited Loading

gauravkghildiyal commented Apr 10, 2024

gauravkghildiyal commented Apr 10, 2024

gauravkghildiyal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deszhou Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deszhou commented Apr 18, 2024

gauravkghildiyal commented Apr 18, 2024

k8s-ci-robot commented Apr 18, 2024

yashvardhan-kukreja Apr 4, 2024 •

edited

Loading

yashvardhan-kukreja commented Apr 4, 2024 •

edited

Loading

deszhou Apr 17, 2024 •

edited

Loading