Refactor gwctl: restructure codebase, add new commands, and enable extensions #3244

gauravkghildiyal · 2024-08-04T22:19:19Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

Apologies for the large size of this PR, but it represents a significant step forward in making gwctl more usable and extensible. The updated README.md provides a comprehensive overview of all the latest capabilities.

Introduce new command structure and new commands (Highlights: analyze, apply, delete)
Implement extensions and validation (think of this like k8s webhooks that operate on a provided input and return validations / modifications)
Improve topology visualization and graph generation.
Enabled CRUD operations for non-Gateway resources.
Better error handling and reporting details to the user

Next steps:

Provide a contributions guide to facilitate adding more validations and extensions.
Expand unit and integration test coverage. I have added some basic framework for integration tests.

Does this PR introduce a user-facing change?:

Add features analyze, apply, delete and graphing to gwctl.

/area gwctl

k8s-ci-robot · 2024-08-04T22:23:30Z

[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

gauravkghildiyal · 2024-08-04T22:25:40Z

/cc @robscott

mlavacca

Thanks for all the work on this, @gauravkghildiyal!

This PR looks massive and I honestly find the review of such a big piece of code very hard. Since in the PR description you outlined what the PR is about (this is great description) I wonder if it is possible to split this PR into multiple smaller PRs, focused on a specific area of improvement/refactoring. Do you think it's feasible? It'd help a lot the review :)

…te the graphical modeling of resource relationships.

…Gateway API.

…ges. These will augment information into the Graph model.

modelling.

…aml, describe, etc. formats

gauravkghildiyal · 2024-08-14T05:58:26Z

Thanks for all the work on this, @gauravkghildiyal!

This PR looks massive and I honestly find the review of such a big piece of code very hard. Since in the PR description you outlined what the PR is about (this is great description) I wonder if it is possible to split this PR into multiple smaller PRs, focused on a specific area of improvement/refactoring. Do you think it's feasible? It'd help a lot the review :)

Thanks for taking the time to help with this @mlavacca. I totally feel the pain of reviewing a PR like this. I've started with splitting this into individual commits, ordered in a manner to make things easier to review.

Given the number of issues this PR tackles, as I was developing the code, I realized that splitting it into completely independent, atomic PRs would have significantly impacted development time. Obviously, there are tradeoffs to that decision which appear in the form of not-so-easy reviews. I understand that a comprehensive review may be challenging within a reasonable timeframe. I propose we prioritize identifying any obvious "red flags" and rely on the integration tests to verify the overall behaviour. We can continue to improve the codebase iteratively in future PRs.

robscott

Thanks @gauravkghildiyal! Partial review from last week, will try to get back to this soon.

robscott · 2024-08-15T05:13:27Z

gwctl/cmd/analyze/analyze.go

+	fmt.Fprintf(o.IOStreams.Out, "\n")
+
+	// Step 1: Parse the files and extract the objects from the files.
+	infos, err := o.factory.NewBuilder().


Nit: "infos" feels like an overly vague name here, is there something a bit more specific that would work here?

robscott · 2024-08-15T05:14:08Z

gwctl/cmd/analyze/analyze.go

+	graph, err := topology.NewBuilder(common.NewDefaultGroupKindFetcher(o.factory, common.WithAdditionalResources(sources))).
+		StartFrom(sources).
+		UseRelationships(topologygw.AllRelations).
+		WithMaxDepth(4).


This seems like something that should be configurable?

robscott · 2024-08-15T05:14:37Z

gwctl/cmd/analyze/analyze.go

+	}
+
+	policyManager := policymanager.New(common.NewDefaultGroupKindFetcher(o.factory, common.WithAdditionalResources(sources)))
+	if err := policyManager.Init(); err != nil { //nolint:govet


Why's the nolint needed here?

robscott · 2024-08-15T05:17:26Z

gwctl/cmd/analyze/analyze.go

+	topologygw "sigs.k8s.io/gateway-api/gwctl/pkg/topology/gateway"
+)
+
+func NewCmd(factory common.Factory, iostreams genericiooptions.IOStreams) *cobra.Command {


Somewhere in this file it would be useful to have a comment describing what the analyze command is trying to achieve.

robscott · 2024-08-15T05:19:55Z

gwctl/cmd/apply/apply.go

+			return fmt.Errorf("%v: %v", info.Source, err)
+		}
+
+		obj, err := helper.Patch(


When I see Patch, I assume this will only update existing options, where I'd expect apply to do an upsert

robscott · 2024-08-15T05:41:19Z

gwctl/pkg/flags/flags.go

+		case "gatewayclass", "gateawyclasses":
+			objRef.Group = gatewayv1.GroupVersion.Group
+			objRef.Kind = "GatewayClass"
+			objRef.Namespace = ""
+		case "gateway", "gateways":
+			objRef.Group = gatewayv1.GroupVersion.Group
+			objRef.Kind = "Gateway"
+		case "httproute", "httproutes":
+			objRef.Group = gatewayv1.GroupVersion.Group
+			objRef.Kind = "HTTPRoute"
+		case "service", "services":
+			objRef.Kind = "Service"


A comment describing why these are the only supported input types would be useful.

k8s-ci-robot · 2024-08-24T09:03:55Z

PR needs rebase.

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-sigs/prow repository.

k8s-ci-robot · 2024-09-10T20:23:55Z

@gauravkghildiyal: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-gateway-api-crds-validation-3	`41f8da8`	link	true	`/test pull-gateway-api-crds-validation-3`
pull-gateway-api-crds-validation-2	`41f8da8`	link	true	`/test pull-gateway-api-crds-validation-2`
pull-gateway-api-crds-validation-1	`41f8da8`	link	true	`/test pull-gateway-api-crds-validation-1`
pull-gateway-api-crds-validation-5	`41f8da8`	link	true	`/test pull-gateway-api-crds-validation-5`
pull-gateway-api-crds-validation-4	`41f8da8`	link	true	`/test pull-gateway-api-crds-validation-4`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-sigs/prow repository. I understand the commands that are listed here.

shaneutt · 2024-09-18T13:15:52Z

@gauravkghildiyal at this point should we close this and apply it to the new repository, since this is getting moved?

robscott · 2024-09-18T15:25:50Z

@gauravkghildiyal at this point should we close this and apply it to the new repository, since this is getting moved?

good point, yeah, I think this got missed

gauravkghildiyal · 2024-09-18T15:28:35Z

Thanks Shane, thanks Rob!

I'll archive any other open PRs and issues too, or suggest people reopen them on kubernetes-sigs/gwctl

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 labels Aug 4, 2024

k8s-ci-robot requested a review from mlavacca August 4, 2024 22:19

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 4, 2024

k8s-ci-robot requested a review from sunjayBhatia August 4, 2024 22:19

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 4, 2024

gauravkghildiyal force-pushed the gwctl-revamp branch from 81ff183 to bbe84d0 Compare August 4, 2024 22:23

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

k8s-ci-robot requested a review from robscott August 4, 2024 22:25

gauravkghildiyal mentioned this pull request Aug 4, 2024

Make gwctl work with Policies having multiple targetRefs #3217

Closed

gauravkghildiyal force-pushed the gwctl-revamp branch 2 times, most recently from 1e89830 to 8f0d1ca Compare August 5, 2024 03:22

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 7, 2024

gauravkghildiyal mentioned this pull request Aug 9, 2024

Coordinate development of initial gwctl feature set #2761

Closed

gauravkghildiyal force-pushed the gwctl-revamp branch from 8f0d1ca to 6840dc0 Compare August 9, 2024 20:35

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2024

mlavacca reviewed Aug 12, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 12, 2024

gauravkghildiyal added 9 commits August 13, 2024 22:34

Update README for gwctl to match the latest set of capabilities.

8ac4d2a

Add a topology package offering reusable graph structures to facilita…

b217e26

…te the graphical modeling of resource relationships.

Add topology/gateway package to model relationships specifically for …

a903ba8

…Gateway API.

Add a generic extensions package with various specific extenion packa…

999be12

…ges. These will augment information into the Graph model.

Refactor the existing get and describe commands to work with the newer

f037c91

modelling.

Add the gwctl apply command

1d1da0e

Add the gwctl delete command

413275c

Add the gwctl analyze command

02c5618

Expose all gwctl sub-commands through the top-level cobra command

1e5dd46

gauravkghildiyal added 6 commits August 13, 2024 22:36

Refactor printer package with better abstractions for various json, y…

567272d

…aml, describe, etc. formats

Add basic framework for integration test.

f164ab2

Refactor policymanager and common package

90d13fa

Update Makefile and embed version and build-commit into the binary

e6cb5f3

Delete obsolete packages for resourcediscovery and related utils.

fce48b9

go mod tidy

41f8da8

gauravkghildiyal force-pushed the gwctl-revamp branch from 6840dc0 to 41f8da8 Compare August 14, 2024 05:58

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 14, 2024

robscott reviewed Aug 19, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 24, 2024

shaneutt added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 18, 2024

robscott closed this Sep 18, 2024

This was referenced Sep 20, 2024

Initialize repository with commits from gateway-api kubernetes-sigs/gwctl#2

Closed

Initialize repository with commits from gateway-api kubernetes-sigs/gwctl#4

Closed

Initialize repository with commits from gateway-api kubernetes-sigs/gwctl#5

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor gwctl: restructure codebase, add new commands, and enable extensions #3244

Refactor gwctl: restructure codebase, add new commands, and enable extensions #3244

gauravkghildiyal commented Aug 4, 2024 •

edited

Loading

k8s-ci-robot commented Aug 4, 2024

gauravkghildiyal commented Aug 4, 2024

mlavacca left a comment

gauravkghildiyal commented Aug 14, 2024

robscott left a comment

robscott Aug 15, 2024

robscott Aug 15, 2024

robscott Aug 15, 2024

robscott Aug 15, 2024

robscott Aug 15, 2024

robscott Aug 15, 2024

k8s-ci-robot commented Aug 24, 2024

k8s-ci-robot commented Sep 10, 2024

shaneutt commented Sep 18, 2024

robscott commented Sep 18, 2024

gauravkghildiyal commented Sep 18, 2024

Refactor gwctl: restructure codebase, add new commands, and enable extensions #3244

Refactor gwctl: restructure codebase, add new commands, and enable extensions #3244

Conversation

gauravkghildiyal commented Aug 4, 2024 • edited Loading

k8s-ci-robot commented Aug 4, 2024

gauravkghildiyal commented Aug 4, 2024

mlavacca left a comment

Choose a reason for hiding this comment

gauravkghildiyal commented Aug 14, 2024

robscott left a comment

Choose a reason for hiding this comment

robscott Aug 15, 2024

Choose a reason for hiding this comment

robscott Aug 15, 2024

Choose a reason for hiding this comment

robscott Aug 15, 2024

Choose a reason for hiding this comment

robscott Aug 15, 2024

Choose a reason for hiding this comment

robscott Aug 15, 2024

Choose a reason for hiding this comment

robscott Aug 15, 2024

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 24, 2024

k8s-ci-robot commented Sep 10, 2024

shaneutt commented Sep 18, 2024

robscott commented Sep 18, 2024

gauravkghildiyal commented Sep 18, 2024

gauravkghildiyal commented Aug 4, 2024 •

edited

Loading