Add gwctl to gateway-api repo

[gauravkghildiyal]

What type of PR is this?
/kind feature

What this PR does / why we need it:
Import gwctl packages from https://github.com/gauravkghildiyal/gwctl

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

Add gwctl command-line tool which provides a better way to view and manage policies (https://gateway-api.sigs.k8s.io/geps/gep-713/#kubectl-plugin)

[gauravkghildiyal]

/retest

[robscott]

This is very cool! Thanks for all the work on this @gauravkghildiyal! There's a lot here, but took a high level pass and had some nits + questions.

[robscott]

This is fine for now, but we'll likely want to update this to point to a Gateway API release as soon as we have RC1 out.

[gauravkghildiyal]

Acknowledged. Yes sure that that seems good. Will update as we make progress on that front in upcoming PRs.

[robscott]

Probably missing something obvious, but why do we need this?

[gauravkghildiyal]

No that's a good question. I should move the comment from within the function to outside the function. (Will update)

This is just a "good to have" thing which helps provide neat diffs during test. Idea being, split long blobs of yaml text by each line. When the diff is generated, it will clearly point out diffs per line (as oppposed to saying that: "somethings different in this large string, now go find what the difference is")

[howardjohn]

You may consider naming the binary `kubectl-<something>` so its a "kubectl plugin"

…

On Mon, Sep 25, 2023 at 6:21 PM Rob Scott ***@***.***> wrote: ***@***.**** commented on this pull request. This is very cool! Thanks for all the work on this @gauravkghildiyal <https://github.com/gauravkghildiyal>! There's a lot here, but took a high level pass and had some nits + questions. ------------------------------ In gwctl/README.md <#2428 (comment)> : > @@ -0,0 +1,137 @@ +# gwctl + +gwctl is a tool that improves the usability of the Gateway API by providing a better way to view and manage policies ([GEP-713](https://gateway-api.sigs.k8s.io/geps/gep-713)). The aim is to make it available as a standalone binary, a kubectl plugin, and a library. + +gwctl allows you to view all Gateway API policy types that are present in a cluster, as well as all "policy bindings" in a namespace (or across all namespaces). It also shows you the attached policies when you view any Gateway resource (like HTTPRoute, Gateway, GatewayClass, etc.) + +gwctl uses the `gateway.networking.k8s.io/policy=true` <http://gateway.networking.k8s.io/policy=true> label to identify Policy CRDs (https://gateway-api.sigs.k8s.io/geps/gep-713/#kubectl-plugin) + +Please note that gwctl is <b>still under development and may have bugs</b>. There may be changes at various places, including the command-line interface, the output format, and the supported features. ⬇️ Suggested change -Please note that gwctl is <b>still under development and may have bugs</b>. There may be changes at various places, including the command-line interface, the output format, and the supported features. +Please note that gwctl is still considered an [experimental feature of Gateway API](https://gateway-api.sigs.k8s.io/concepts/versioning/#release-channels-eg-experimental-standard). While we iterate on the early stages of this tool, bugs and incompatible changes will be more likely. ------------------------------ In gwctl/README.md <#2428 (comment)> : > +```bash +# Clone the gwctl repository +git clone https://github.com/kubernetes-sigs/gateway-api.git + +# Go to the gwctl directory +cd gateway-api + +# Ensure vendor depedencies +go mod tidy +go mod vendor + +# Build the gwctl binary +go build -o bin/gwctl gwctl/cmd/main.go + +# Add binary to PATH +export PATH=./bin:${PATH} + +# Start using! +gwctl help +``` This is fine for now, but we'll likely want to update this to point to a Gateway API release as soon as we have RC1 out. ------------------------------ In gwctl/pkg/cmd/describe.go <#2428 (comment)> : > + case "backend", "backends": + var backendsList []unstructured.Unstructured I think this may be a little more abstract than necessary at this point. It may be simplest to just deal with known and common types like Services to start. ------------------------------ In gwctl/pkg/common/consts.go <#2428 (comment)> : > +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package common + +const ( + GatewayPolicyLabelKey = "gateway.networking.k8s.io/policy" Nit: This might actually belong in https://github.com/kubernetes-sigs/gateway-api/blob/main/apis/v1alpha2/policy_types.go . ------------------------------ In gwctl/pkg/common/helpers.go <#2428 (comment)> : > +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package common + +import ( + "strings" + + "github.com/google/go-cmp/cmp" +) + +type YamlString string + +var YamlStringTransformer = cmp.Transformer("YamlLines", func(s YamlString) []string { Probably missing something obvious, but why do we need this? ------------------------------ In gwctl/pkg/policymanager/manager.go <#2428 (comment)> : > + policy, err := PolicyFromUnstrucutred(unstrucutredPolicy, p.policyCRDs) + if err != nil { + return err + } + p.policies[unstrucutredPolicy.GetNamespace()+"/"+unstrucutredPolicy.GetName()] = policy This may benefit from a separate function like p.AddPolicy(unstructuredPolicy) ------------------------------ In gwctl/pkg/policymanager/manager.go <#2428 (comment)> : > +type Policy struct { + u unstructured.Unstructured + targetRef ObjRef + // Indicates whether the policy is supposed to be "inherited" (as opposed to + // "direct"). + inherited bool +} + +type ObjRef struct { + Group string `json:",omitempty"` + Kind string `json:",omitempty"` + Name string `json:",omitempty"` + Namespace string `json:",omitempty"` +} + +func PolicyFromUnstrucutred(u unstructured.Unstructured, policyCRDs map[PolicyCrdID]PolicyCRD) (Policy, error) { ⬇️ Suggested change -func PolicyFromUnstrucutred(u unstructured.Unstructured, policyCRDs map[PolicyCrdID]PolicyCRD) (Policy, error) { +func PolicyFromUnstructured(u unstructured.Unstructured, policyCRDs map[PolicyCrdID]PolicyCRD) (Policy, error) { ------------------------------ In gwctl/pkg/resources/backends/backends.go <#2428 (comment)> : > + } + } + + return filteredHTTPRoutes, nil +} + +type describeView struct { + Group string `json:",omitempty"` + Kind string `json:",omitempty"` + Name string `json:",omitempty"` + Namespace string `json:",omitempty"` + DirectlyAttachedPolicies []policymanager.ObjRef `json:",omitempty"` + EffectivePolicies map[string]map[policymanager.PolicyCrdID]policymanager.Policy `json:",omitempty"` +} + +func PrintDescribeView(ctx context.Context, params *types.Params, backendsList []unstructured.Unstructured) { Non-blocking nit: To simplify the use of this code as a library it may be best to separate code into different packages. For example, one package or set of packages intended to be used as a lib, and a separate package that is more unique to gwctl inputs and outputs (like this describe view here). I'm not really sure what the ideal code structure is here yet, so this may be worth filing a follow up issue for more discussion once this one merges. ------------------------------ In gwctl/pkg/resources/policies/policies.go <#2428 (comment)> : > + + "sigs.k8s.io/yaml" + + "sigs.k8s.io/gateway-api/gwctl/pkg/policymanager" + "sigs.k8s.io/gateway-api/gwctl/pkg/types" +) + +func Print(params *types.Params, policies []policymanager.Policy) { + sort.Slice(policies, func(i, j int) bool { + a := fmt.Sprintf("%v/%v", policies[i].Unstructured().GetNamespace(), policies[i].Unstructured().GetName()) + b := fmt.Sprintf("%v/%v", policies[j].Unstructured().GetNamespace(), policies[j].Unstructured().GetName()) + return a < b + }) + + tw := tabwriter.NewWriter(params.Out, 0, 0, 2, ' ', 0) + row := []string{"POLICYNAME", "POLICYKIND", "TARGETNAME", "TARGETKIND"} ⬇️ Suggested change - row := []string{"POLICYNAME", "POLICYKIND", "TARGETNAME", "TARGETKIND"} + row := []string{"POLICY NAME", "POLICY KIND", "TARGET NAME", "TARGET KIND"} ------------------------------ In gwctl/pkg/resources/policies/policies.go <#2428 (comment)> : > + policy.TargetRef().Kind, + } + tw.Write([]byte(strings.Join(row, "\t") + "\n")) + } + tw.Flush() +} + +func PrintCRDs(params *types.Params, policyCRDs []policymanager.PolicyCRD) { + sort.Slice(policyCRDs, func(i, j int) bool { + a := fmt.Sprintf("%v/%v", policyCRDs[i].CRD().GetNamespace(), policyCRDs[i].CRD().GetName()) + b := fmt.Sprintf("%v/%v", policyCRDs[j].CRD().GetNamespace(), policyCRDs[j].CRD().GetName()) + return a < b + }) + + tw := tabwriter.NewWriter(params.Out, 0, 0, 2, ' ', 0) + row := []string{"CRD_NAME", "CRD_GROUP", "CRD_KIND", "CRD_INHERITED", "CRD_SCOPE"} ⬇️ Suggested change - row := []string{"CRD_NAME", "CRD_GROUP", "CRD_KIND", "CRD_INHERITED", "CRD_SCOPE"} + row := []string{"NAME", "GROUP", "KIND", "POLICY TYPE", "SCOPE"} ------------------------------ In gwctl/pkg/resources/policies/policies.go <#2428 (comment)> : > + sort.Slice(policyCRDs, func(i, j int) bool { + a := fmt.Sprintf("%v/%v", policyCRDs[i].CRD().GetNamespace(), policyCRDs[i].CRD().GetName()) + b := fmt.Sprintf("%v/%v", policyCRDs[j].CRD().GetNamespace(), policyCRDs[j].CRD().GetName()) + return a < b + }) + + tw := tabwriter.NewWriter(params.Out, 0, 0, 2, ' ', 0) + row := []string{"CRD_NAME", "CRD_GROUP", "CRD_KIND", "CRD_INHERITED", "CRD_SCOPE"} + tw.Write([]byte(strings.Join(row, "\t") + "\n")) + + for _, policyCRD := range policyCRDs { + row := []string{ + policyCRD.CRD().Name, + policyCRD.CRD().Spec.Group, + policyCRD.CRD().Spec.Names.Kind, + fmt.Sprintf("%v", policyCRD.IsInherited()), Would recommend using a string value here of "Inherited" or "Direct" instead ------------------------------ In gwctl/pkg/types/params.go <#2428 (comment)> : > + +import ( + "bytes" + "context" + "io" + "testing" + + "k8s.io/client-go/discovery" + "k8s.io/client-go/dynamic" + "sigs.k8s.io/controller-runtime/pkg/client" + + "sigs.k8s.io/gateway-api/gwctl/pkg/common" + "sigs.k8s.io/gateway-api/gwctl/pkg/policymanager" +) + +type Params struct { Nit: I'm not really sure what the best name for this is, but Params doesn't seem quite right. It seems like this is a mix of 3 k8s clients, an internal data structure, and an io writer. Are there other ways we can split this up? How broadly is this type used? Are there common combinations that only need a subset of what's in here? — Reply to this email directly, view it on GitHub <#2428 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXMX7GD3ECFS6XMDSNTX4IUX7ANCNFSM6AAAAAA5GPQQ7U> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[robscott]

You may consider naming the binary kubectl-<something> so its a "kubectl plugin"

Maybe kubectl-gw then?

[robscott]

Thanks for all the work on this @gauravkghildiyal! I took some time to test this out today and have some high level non-blocking feedback.

1. I'd love to see this be a drop-in replacement for kubectl when reading (get/describe) Gateway API resources, so ideally this would be a superset of kubectl capabilities. With that said, if we end up having to recreate too many things, that's also not ideal. My main point of confusion here is that we're using get as a command, but it's not currently a drop-in replacement for kubectl get. If we wanted to get there, I think we'd need the following:
   • Support for all resources for both get and describe commands
   • Show at least the same set of columns, ideally more
   • Support for yaml and json output
   • Maybe support for more advanced features like label selection, etc
   • The longer this list gets, the less I'm sure about the overall goal 😄
2. It would be great to more gracefully handle errors. For example, if one or more Gateway API CRDs are not found, we could just tell the user they need to install the CRD(s) that are missing.
3. Similar to 1, but potentially more achievable in the short term, it would be great to include more information in our describe output. Right now the primary value seems to be policy connections, but it would be useful if it could include that + much of the other useful information included by kubectl describe.

I don't think any of these are blockers, but before we go too much further with this, it would probably be helpful to sketch out a user guide for the full set of functionality we want this to be able to accomplish, and then we can split up the work into smaller pieces so it can grow over time from this foundation.

I'm going to go ahead and approve what's here, because it works and looks like a good foundation, but would be very interested in a vision for how we want to build on top of this going forward.

/approve

[gauravkghildiyal]

Thanks a lot for the review and great constructive feedback @robscott. Everything that you highlighted is definitely something that I can start addressing ASAP.

I agree that kubectl doesn't natively provide an extension mechanism in a way in which we can reuse it for certain things which ends up with us having to duplicate things. I will create a user guide on what I think we can/should support so others can start contributing as well.

I had been working on some improvements on the "usage as a library" aspects which I'll send separately.

(Rebasing to resolve merge conflict)

[gauravkghildiyal]

/retest

[shaneutt]

Awesome, thanks for adding this!

I think adding it here in the repo is a good idea, as it will keep things in sync.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gauravkghildiyal, robscott, shaneutt

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:

• OWNERS [robscott,shaneutt]

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

[youngnick]

yeah, I really like this to get us started, but I think we should spend a little time post-GA and talk about what we want this to go - tbh the more I think about it, the more I think we may be better off with two similar but distinct tools - gwctl which is a pretty straight kubectl replacement for get and describe, and a separate gw kubectl plugin that does the same things but with a slightly different interface. Typing kubectl gw get all or something seems slightly more readable than kubectl gwctl get all.

/lgtm