gwctl: Introduce a `--for` flag to filter results related to a specified resource. #3068

gauravkghildiyal · 2024-05-09T18:39:57Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

This PR achieves two broad things:

Add --for flag for filtering resources based on an associated resource.

For example:
- gwctl get backends --for gateway/ns2/my-gateway
- gwctl get gateways --for httproute/ns1/my-httproute
- gwctl get gateways --for gatewayclass/foo-gatewayclass
- gwctl get policies --for service/foo-namespace/my-svc
Remove duplication from cli setup of Get and Describe commands. This inturn
allows both commands to share feataures/flags (when applicable) without the
need to manually configure them independently for each of them

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

gwctl allows filtering results to only those related to the specified resource (using the `--for` flag)

k8s-ci-robot · 2024-05-09T18:40:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gauravkghildiyal

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

robscott

Thanks for all the great work on this @gauravkghildiyal!

robscott · 2024-05-15T05:13:59Z

gwctl/cmd/root.go

@@ -26,31 +25,45 @@ import (
 	"github.com/spf13/cobra"
 	"k8s.io/klog/v2"

-	"sigs.k8s.io/gateway-api/gwctl/pkg/common"
-	"sigs.k8s.io/gateway-api/gwctl/pkg/policymanager"
 	cmdutils "sigs.k8s.io/gateway-api/gwctl/pkg/utils"


Nit: Package name doesn't really match path - maybe the scope of the package expanded over time?

Nice catch. Changed to use the default name.

robscott · 2024-05-15T05:15:40Z

gwctl/pkg/utils/types.go

+type Factory interface {
+	K8sClients() (*common.K8sClients, error)
+	PolicyManager() (*policymanager.PolicyManager, error)
 }


Why does this need to be an interface? Are you expecting other implementations of the interface to be added in the future? (Not saying this is necessarily bad, just trying to understand the use case).

Nice question. Added an internal comment for future context as well. Happy to explain it differently if needed.

// INTERNAL COMMENT: // - The reason for an interface here is to be able to inject this dependency // during unit tests. // - The reason for the "factory" pattern is to delay the construction of the // objects for when the commands get run (as opposed to when the commands // are registered in Cobra). This is required because during registration of // the commands, we will need to inject this dependency, but we cannot // construct the dependency because it depends on some flag values which are // only known during the runtime of the command.

robscott · 2024-05-15T05:19:26Z

gwctl/pkg/resourcediscovery/discoverer.go

+					Namespace: httpRouteNode.HTTPRoute.GetNamespace(),
+				}
+				var referenceAccepted bool
+				for _, referenceGrantNode := range backendNode.ReferenceGrants {


It would probably be useful to have some basic scalability tests - let's say 100 of each kind of resources with different levels of connections between to help keep some basic understanding of the impact each additional change like this adds. Not a blocker for this PR, but could be useful as a follow up issue for someone to tackle.

Yes of course you have the right thoughts/concerns. I certainly know that computing all such relations (including all the other ones) will definitely take some time. I have a few thoughts about how this user experience will look like. Will share / discuss separately.

robscott · 2024-05-15T05:19:59Z

gwctl/pkg/resourcediscovery/discoverer.go

+				// Backend does not exist in the resourceModel (maybe because of some
+				// error), so skip processing this backend.


Seems like some debug logging could be useful here.

Sure added one log.

The verbosity is a bit higher since a relevant error (with more precise details) for this specific case should have been logged in the previous looping construct i.e. Step 1

robscott · 2024-05-15T05:25:02Z

gwctl/cmd/root.go

+	rootCmd.AddCommand(NewSubCommand(factory, os.Stdout, commandNameGet))
+	rootCmd.AddCommand(NewSubCommand(factory, os.Stdout, commandNameDescribe))


This is a very neat bit of cleanup and refactoring (along with the new subcommand file), nicely done!

robscott · 2024-05-15T05:26:44Z

gwctl/cmd/root.go

+		if kubeConfigPath == "" {
+			kubeConfigPath = os.Getenv("KUBECONFIG")
+			if kubeConfigPath == "" {
+				kubeConfigPath = path.Join(os.Getenv("HOME"), ".kube/config")


I'm rusty here, but I thought if you left kubeconfig path empty when initializing clients, they'd already choose a sane default?

Oh yeah I gave it a try and from what I understand, that part is supposed to work when the process is running within a kubernetes clusters (so it can assume certain environment variables being set and so on).

Though that's a good callout. Added an action item to #2761 to figure out what the best approach here is.

gauravkghildiyal

Thanks for the review, @robscott

gauravkghildiyal · 2024-05-15T05:44:25Z

gwctl/cmd/root.go

@@ -26,31 +25,45 @@ import (
 	"github.com/spf13/cobra"
 	"k8s.io/klog/v2"

-	"sigs.k8s.io/gateway-api/gwctl/pkg/common"
-	"sigs.k8s.io/gateway-api/gwctl/pkg/policymanager"
 	cmdutils "sigs.k8s.io/gateway-api/gwctl/pkg/utils"


Nice catch. Changed to use the default name.

gauravkghildiyal · 2024-05-15T05:46:26Z

gwctl/pkg/resourcediscovery/discoverer.go

+					Namespace: httpRouteNode.HTTPRoute.GetNamespace(),
+				}
+				var referenceAccepted bool
+				for _, referenceGrantNode := range backendNode.ReferenceGrants {


Yes of course you have the right thoughts/concerns. I certainly know that computing all such relations (including all the other ones) will definitely take some time. I have a few thoughts about how this user experience will look like. Will share / discuss separately.

gauravkghildiyal · 2024-05-15T05:53:39Z

gwctl/pkg/resourcediscovery/discoverer.go

+				// Backend does not exist in the resourceModel (maybe because of some
+				// error), so skip processing this backend.


Sure added one log.

The verbosity is a bit higher since a relevant error (with more precise details) for this specific case should have been logged in the previous looping construct i.e. Step 1

gauravkghildiyal · 2024-05-15T06:23:41Z

gwctl/pkg/utils/types.go

+type Factory interface {
+	K8sClients() (*common.K8sClients, error)
+	PolicyManager() (*policymanager.PolicyManager, error)
 }


Nice question. Added an internal comment for future context as well. Happy to explain it differently if needed.

// INTERNAL COMMENT: // - The reason for an interface here is to be able to inject this dependency // during unit tests. // - The reason for the "factory" pattern is to delay the construction of the // objects for when the commands get run (as opposed to when the commands // are registered in Cobra). This is required because during registration of // the commands, we will need to inject this dependency, but we cannot // construct the dependency because it depends on some flag values which are // only known during the runtime of the command.

gauravkghildiyal · 2024-05-15T06:37:21Z

gwctl/cmd/root.go

+		if kubeConfigPath == "" {
+			kubeConfigPath = os.Getenv("KUBECONFIG")
+			if kubeConfigPath == "" {
+				kubeConfigPath = path.Join(os.Getenv("HOME"), ".kube/config")


Oh yeah I gave it a try and from what I understand, that part is supposed to work when the process is running within a kubernetes clusters (so it can assume certain environment variables being set and so on).

Though that's a good callout. Added an action item to #2761 to figure out what the best approach here is.

…y (-v) flag. Get rid of global flag variables like kubeConfigPath.

discoverBackendsForHTTPRoutes().

For example: * gwctl get backends --for gateway/ns2/my-gateway * gwctl get gateways --for httproute/ns1/my-httproute * gwctl get gateways --for gatewayclass/foo-gatewayclass

…ses() and discoverBackendsForHTTPRoutes().

gauravkghildiyal · 2024-05-16T21:32:21Z

(Rebased)

…resource.

robscott · 2024-05-16T23:56:03Z

Thanks @gauravkghildiyal!

/lgtm

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. area/gwctl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 9, 2024

k8s-ci-robot requested review from robscott and youngnick May 9, 2024 18:40

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 9, 2024

gauravkghildiyal force-pushed the cli-cleanup branch from 8d423fb to 3ff810b Compare May 9, 2024 18:50

robscott reviewed May 15, 2024

View reviewed changes

gauravkghildiyal commented May 15, 2024

View reviewed changes

gauravkghildiyal added 7 commits May 16, 2024 14:16

Remove unnecessary flags exposed by klog and only expose the verbosit…

a3d6996

…y (-v) flag. Get rid of global flag variables like kubeConfigPath.

Remove duplication in Get and Describe commands.

47ba51e

Add missing discovery functions discoverGatewaysForGatewayClasses() and

a6bcd89

discoverBackendsForHTTPRoutes().

Add --for flag for filtering resources based on an associated resource.

5b8ea2c

For example: * gwctl get backends --for gateway/ns2/my-gateway * gwctl get gateways --for httproute/ns1/my-httproute * gwctl get gateways --for gatewayclass/foo-gatewayclass

fixup! Remove duplication in Get and Describe commands.

1dbeb7c

fixup! Add missing discovery functions discoverGatewaysForGatewayClas…

5dc192d

…ses() and discoverBackendsForHTTPRoutes().

fixup! Remove duplication in Get and Describe commands.

158bca4

gauravkghildiyal force-pushed the cli-cleanup branch from a20a9cd to 611b912 Compare May 16, 2024 21:32

fixup! Add --for flag for filtering resources based on an associated …

2092ffd

…resource.

gauravkghildiyal force-pushed the cli-cleanup branch from 611b912 to 2092ffd Compare May 16, 2024 22:31

k8s-ci-robot assigned robscott May 16, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 16, 2024

k8s-ci-robot merged commit a296435 into kubernetes-sigs:main May 16, 2024
8 checks passed

gauravkghildiyal deleted the cli-cleanup branch May 17, 2024 04:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gwctl: Introduce a `--for` flag to filter results related to a specified resource. #3068

gwctl: Introduce a `--for` flag to filter results related to a specified resource. #3068

gauravkghildiyal commented May 9, 2024

k8s-ci-robot commented May 9, 2024

robscott left a comment

robscott May 15, 2024

gauravkghildiyal May 15, 2024

robscott May 15, 2024

gauravkghildiyal May 15, 2024 •

edited

robscott May 15, 2024

gauravkghildiyal May 15, 2024

robscott May 15, 2024

gauravkghildiyal May 15, 2024

robscott May 15, 2024

robscott May 15, 2024

gauravkghildiyal May 15, 2024

gauravkghildiyal left a comment

gauravkghildiyal May 15, 2024

gauravkghildiyal May 15, 2024

gauravkghildiyal May 15, 2024

gauravkghildiyal May 15, 2024 •

edited

gauravkghildiyal May 15, 2024

gauravkghildiyal commented May 16, 2024

robscott commented May 16, 2024

		// Backend does not exist in the resourceModel (maybe because of some
		// error), so skip processing this backend.

		rootCmd.AddCommand(NewSubCommand(factory, os.Stdout, commandNameGet))
		rootCmd.AddCommand(NewSubCommand(factory, os.Stdout, commandNameDescribe))

gwctl: Introduce a --for flag to filter results related to a specified resource. #3068

gwctl: Introduce a --for flag to filter results related to a specified resource. #3068

Conversation

gauravkghildiyal commented May 9, 2024

k8s-ci-robot commented May 9, 2024

robscott left a 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

gauravkghildiyal May 15, 2024 • edited

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

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

gauravkghildiyal May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gauravkghildiyal commented May 16, 2024

robscott commented May 16, 2024

gwctl: Introduce a `--for` flag to filter results related to a specified resource. #3068

gwctl: Introduce a `--for` flag to filter results related to a specified resource. #3068

gauravkghildiyal May 15, 2024 •

edited

gauravkghildiyal May 15, 2024 •

edited