[gwctl] Expand the output of gwctl describe gatewayclass #2930

yashvardhan-kukreja · 2024-04-03T14:05:18Z

What type of PR is this?

/kind gep

What this PR does / why we need it:

This PR introduces an expansion in the output of gwctl describe gatewayclass/gatewayclasses <name> as per the expectation of this GEP.

Checkout the example in the last section.

Note on the "null" values of Labels/Annotations**

If you notice in the above output, the "empty" value of Labels is being parsed as "null" despite the GEP expecting it to be "" (analogous to how kubectl describe works with empty values).

The reason this happens in our situation but not with kubectl describe is that we're using the YAML unmarshaler directly over Labels which is of the type *map[string]string and it inherently deals with the zero-values of pointers (to a map in this case) by stringifying it to "null" instead of "".

On the other hand, kubectl describe doesn't seem to rely upon the YAML Unmarshaller at all [1]. It describe resources flexibly yet in the hard way giving itself the opportunity to realise the zero-ness of custom types like pointers, maps, etc. and spit out <NONE> for them instead.

What's the solution then?

As a user, I don't care much between "null" and "", so let's just continue with the existing implementation of "null".
We change the type from *map[string]string to map[string]string (without omitempty): This would print out {} instead of null - Not sure how would one be more favorable than the other though
We change the logic of GatewayClassesPrinter) PrintDescribeView[2] to not use YAML unmarshaller[3] at all. Instead it can use the Describer interface under kubectl. A very high-level skim across kubectl insinuates that we can use the GenericResourceDescriber[4] to help describing none-native (corev1, appsv1, etc.) types. This would help produce an output identical to kubectl describe gatewayclass <NAME> and this would've its own slight differences with the GEP, so not sure it would be worth the effort.

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?:

The output of `gwctl describe gatewayclass <NAME>` is enhanced further.

For example:
GatewayClass manifest applied

apiVersion: gateway.networking.k8s.io/v1
kind: GatewayClass
metadata:
  name: gatewayclass-observed-generation-bump
spec:
  controllerName: "example.net/gateway-controller"
  description: "old"

Previous output of gwctl describe gatewayclass gatewayclass-observed-generation-bump (Before this PR)

Name: gatewayclass-observed-generation-bump
ControllerName: example.net/gateway-controller
Description: old

New output of gwctl describe gatewayclass gatewayclass-observed-generation-bump (After this PR)

Name: gatewayclass-observed-generation-bump
Labels: null
Annotations:
  kubectl.kubernetes.io/last-applied-configuration: |
    {"apiVersion":"gateway.networking.k8s.io/v1","kind":"GatewayClass","metadata":{"annotations":{},"name":"gatewayclass-observed-generation-bump"},"spec":{"controllerName":"example.net/gateway-controller","description":"old"}}
APIVersion: gateway.networking.k8s.io/v1alpha2
Kind: GatewayClass
Metadata:
  creationTimestamp: "2024-04-03T12:23:58Z"
  generation: 1
  resourceVersion: "2428"
  uid: 2581bf37-7a76-4084-82b3-e26b5438f9e8
ControllerName: example.net/gateway-controller
Description: old
Status:
  conditions:
  - lastTransitionTime: "1970-01-01T00:00:00Z"
    message: Waiting for controller
    reason: Waiting
    status: Unknown
    type: Accepted

References:
[0] Cause of vendor-ed changes
[1] Not a single mention of yaml package in the imports of kubectl's describe backend
[2] PrintDescribeView method
[3] Usage of YAML Unmarshal
[4] GenericResourceDescriber

k8s-ci-robot · 2024-04-03T14:05:28Z

Welcome @yashvardhan-kukreja!

It looks like this is your first PR to kubernetes-sigs/gateway-api 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/gateway-api has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-04-03T14:05:28Z

Hi @yashvardhan-kukreja. 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.

robscott · 2024-04-04T00:10:40Z

Thanks for the work on this @yashvardhan-kukreja!

@dprotaso I think you've got the most context on the applyconfigurations you just added, does this change to vendor look correct to you?

gauravkghildiyal · 2024-04-04T00:16:03Z

Thanks for the work on this @yashvardhan-kukreja!

@dprotaso I think you've got the most context on the applyconfigurations you just added, does this change to vendor look correct to you?

We had previously decided to not commit vendor changes to gwctl sub folder (similar to how the parent folder does not have vendor). I think we can still continue with that?

If we do decide to make vendor changes, it would be nice to do them in an independent PR for the first time.

CC: @yashvardhan-kukreja

gauravkghildiyal · 2024-04-04T00:18:31Z

/assign
/remove-label kind/gep
/retitle [gwctl] Expand the output of gwctl describe gatewayclass

k8s-ci-robot · 2024-04-04T00:18:33Z

@gauravkghildiyal: The label(s) /remove-label kind/gep cannot be applied. These labels are supported: api-review, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, team/katacoda, refactor. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

In response to this:

/assign
/remove-label kind/gep
/retitle [gwctl] Expand the output of gwctl describe gatewayclass

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.

gauravkghildiyal · 2024-04-04T00:19:47Z

/remove-kind gep

yashvardhan-kukreja · 2024-04-04T00:36:43Z

If we do decide to make vendor changes, it would be nice to do them in an independent PR for the first time.

CC: @yashvardhan-kukreja

So, the vendor-ed changes are only required to call these LOC so as to help resolving the right apiVersion and Kind.

I presume, for the time being, we can hardcode them as GatewayClass (for Kind) and gateway.networking.k8s.io/v1alpha2 (for ApiVersion)

This would help us get rid of this import hence, allowing us to get rid of vendor-ed changes as well.

wdyt

dprotaso · 2024-04-04T03:14:45Z

Yeah there's no reason to vendor the go deps in gwctl - you can remove that whole directory

yashvardhan-kukreja · 2024-04-04T09:19:04Z

Hi everyone, I got rid of vendor/ and added it to .gitignore as well

yashvardhan-kukreja · 2024-04-09T03:22:16Z

Hi @dprotaso @gauravkghildiyal , mind taking a look at this one?

Thanks!

yashvardhan-kukreja · 2024-04-10T08:36:03Z

Hi @mlavacca,
would you mind adding the ok-to-test label to this PR?

mlavacca

/ok-to-test

mlavacca · 2024-04-10T08:50:42Z

.gitignore

@@ -45,6 +45,7 @@ Session.vim
 /_tmp/
 *.un~
 .vagrant
+vendor


Suggested change

vendor

I added this intentionally to ensure that the contributors don't end up committing the vendor/ (it seemed like only a build-time req. anyway).

Should I still remove it from gitignore?

This need not block the PR, but we can likely skip this update in this PR, and if required send an independent one. Reason being, I actually don't recall why vendor was never added to .gitignore (although we infact don't commit vendor) -- this may just turn into another long discussion which may block this PR even longer.

Gotcha makes sense!

gwctl/pkg/printer/gatewayclasses.go

gwctl/pkg/utils/types.go

Signed-off-by: Yashvardhan Kukreja <yash.kukreja.98@gmail.com>

yashvardhan-kukreja · 2024-04-12T18:06:14Z

apologies for the ping @mlavacca : just in case you missed the above commits and messages.

Thank you

gauravkghildiyal

Thanks @yashvardhan-kukreja! (and sorry for the delay)

Mostly looks good, though one thing: The tabular structure of DirectlyAttachedPolicies field as mentioned in https://gateway-api.sigs.k8s.io/geps/gep-2722/ is actually intentional.

DirectlyAttachedPolicies:
  TYPE                   NAME
  ----                   ----
  TimeoutPolicy.bar.com  demo-timeout-policy-on-gatewayclass

Ideally it would be nice to have it in the same PR, but if you want to send a separate PR for that, lets keep the issue open after this PR merges (and not use "Fixed" in the PR description)

gauravkghildiyal · 2024-04-17T06:10:47Z

gwctl/pkg/printer/gatewayclasses.go


+		// views ordered with respect to https://deploy-preview-2723--kubernetes-sigs-gateway-api.netlify.app/geps/gep-2722/#:~:text=gwctl%20describe%20gatewayclass%20foo%2Dcom%2Dexternal%2Dgateway%2Dclass


nit: You can either remove this comment, or change the link to a permanent one at https://gateway-api.sigs.k8s.io/geps/gep-2722/

gauravkghildiyal · 2024-04-17T06:18:58Z

gwctl/pkg/printer/gatewayclasses.go

+				Kind: kind,
+			},
+			{
+				Metadata: &metav1.ObjectMeta{


Lets follow a consistent approach as:

gateway-api/gwctl/pkg/printer/policies.go

Lines 176 to 180 in 877803e

metadata := crd.ObjectMeta.DeepCopy()

metadata.Labels = nil

metadata.Annotations = nil

metadata.Name = ""

metadata.Namespace = ""

where we remove duplicated fields from metadata and print the rest? (This would allow for possible future expansion of the fields within Metadata to be included by default)

Sure, sounds neat

gauravkghildiyal · 2024-04-17T06:21:49Z

.gitignore

@@ -45,6 +45,7 @@ Session.vim
 /_tmp/
 *.un~
 .vagrant
+vendor


This need not block the PR, but we can likely skip this update in this PR, and if required send an independent one. Reason being, I actually don't recall why vendor was never added to .gitignore (although we infact don't commit vendor) -- this may just turn into another long discussion which may block this PR even longer.

gauravkghildiyal · 2024-04-17T06:32:44Z

gwctl/pkg/resourcediscovery/discoverer.go

@@ -286,6 +287,11 @@ func fetchGatewayClasses(ctx context.Context, k8sClients *common.K8sClients, fil
 			return []gatewayv1.GatewayClass{}, err
 		}

+		// because api-server doesn't return TypeMeta in `gatewayClass`
+		gcApplyConfig := apisv1beta1.GatewayClass(gatewayClass.Name)


Ah nice catch! This may just be a bit more involved.

The thing is, because we construct the gatewayClass object with the v1 versioned struct (i.e. &gatewayv1.GatewayClass{}, client-go will fetch the GatewayClass for the v1 version. This means the correct APIVersion would correspond to the v1 package (and not apisv1beta1 = v1beta1):

gcApplyConfig := apisv1.GatewayClass(gatewayClass.Name)

Having said that, if we think about it a bit more, use of ApplyConfig isn't giving any real advantage here. How about something like this:

gatewayClass.APIVersion = gatewayv1.GroupVersion.String() gatewayClass.Kind = reflect.TypeOf(gatewayClass).Elem().Name()

(Infact, using gatewayClass.Kind = "GatewayClass" would also just have been good enough)

Ahh good point, will take care of this.

gauravkghildiyal · 2024-04-17T06:46:18Z

/label tide/merge-method-squash

Signed-off-by: Yashvardhan Kukreja <yash.kukreja.98@gmail.com>

yashvardhan-kukreja · 2024-04-17T07:13:42Z

Hi @gauravkghildiyal
Took care of all the comments you made. Feel free to resolve them if you're happy with the newer commit.

yashvardhan-kukreja · 2024-04-17T07:14:14Z

Mostly looks good, though one thing: The tabular structure of DirectlyAttachedPolicies field as mentioned in https://gateway-api.sigs.k8s.io/geps/gep-2722/ is actually intentional.

Makes sense though I noticed that the same concern is with other DescribeViews such as the ones corresponding to Gateway and HTTPRoute

So, I'd suggest a separate PR addressing all of these ones. What do you think?

gauravkghildiyal · 2024-04-17T07:51:02Z

gwctl/pkg/resourcediscovery/discoverer.go

@@ -286,6 +285,10 @@ func fetchGatewayClasses(ctx context.Context, k8sClients *common.K8sClients, fil
 			return []gatewayv1.GatewayClass{}, err
 		}

+		// because api-server doesn't return TypeMeta in `gatewayClass`


nit: kube-apiserver does infact return the TypeMeta (it always does)...this mishap happens when that object is decoded. We can likely add kubernetes/kubernetes#80609 as reference.

ohh yeah of course it does, my bad (else kubectl-like clients would never show type meta lol), I meant client-go I guess haha.

Anyway, let me change this comment to a ref to the above issue instead.

Thanks for pointing it out!

gauravkghildiyal

Mostly looks good, though one thing: The tabular structure of DirectlyAttachedPolicies field as mentioned in https://gateway-api.sigs.k8s.io/geps/gep-2722/ is actually intentional.

Makes sense though I noticed that the same concern is with other DescribeViews such as the ones corresponding to Gateway and HTTPRoute

So, I'd suggest a separate PR addressing all of these ones. What do you think?

Yep right! Sorry this is lack of clarity from my side that "Expand the output of XXX" is also expecting this change.

Sure lets tackle that in a separate PR for gatewayclass

Also, while you are editing the PR description to remove the "Fixed" keyword, FYI, we do infact want that particular format for the release note:

**Does this PR introduce a user-facing change?**:
<!--
If no, just write "NONE" in the release-note block below.
If yes, please enter a release note below:
-->
```release-note

```

This should resolve the do-not-merge/release-note-label-needed label

gauravkghildiyal · 2024-04-17T07:59:33Z

/approve

k8s-ci-robot · 2024-04-17T07:59:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gauravkghildiyal, yashvardhan-kukreja

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

yashvardhan-kukreja · 2024-04-17T09:16:51Z

Whoops, ptal now.
Thanks :)

Signed-off-by: Yashvardhan Kukreja <yash.kukreja.98@gmail.com>

gauravkghildiyal · 2024-04-17T16:55:26Z

@yashvardhan-kukreja sorry one last thing. Can you please keep the thing with the "release notes quote" a self contained statement -- even a one liner helps (in this case, it could simply just be the PR title itself, or equivalent) Thanks

yashvardhan-kukreja · 2024-04-18T12:48:39Z

Looks good @gauravkghildiyal ?

gauravkghildiyal · 2024-04-18T16:29:25Z

/lgtm

k8s-ci-robot added kind/gep PRs related to Gateway Enhancement Proposal(GEP) do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 3, 2024

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 3, 2024

k8s-ci-robot requested review from gauravkghildiyal and youngnick April 3, 2024 14:05

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/gwctl labels Apr 3, 2024

youngnick requested review from robscott and removed request for youngnick April 4, 2024 00:02

k8s-ci-robot assigned gauravkghildiyal Apr 4, 2024

k8s-ci-robot changed the title ~~GEP: Expand the output of gwctl describe gatewayclass~~ [gwctl] Expand the output of gwctl describe gatewayclass Apr 4, 2024

k8s-ci-robot added needs-kind Indicates a PR lacks a `kind/foo` label and requires one. and removed kind/gep PRs related to Gateway Enhancement Proposal(GEP) labels Apr 4, 2024

yashvardhan-kukreja force-pushed the issue-2796/gwctl-describe-gatewayclass branch from 48f6f5a to 01a2aa0 Compare April 4, 2024 05:13

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 4, 2024

mlavacca reviewed Apr 10, 2024

View reviewed changes

yashvardhan-kukreja force-pushed the issue-2796/gwctl-describe-gatewayclass branch from fc9524a to 65da0b6 Compare April 10, 2024 10:04

yashvardhan-kukreja added 7 commits April 11, 2024 08:45

feat: expand gwctl describe gatewayclass

de4dc33

Signed-off-by: Yashvardhan Kukreja <yash.kukreja.98@gmail.com>

chore: nits

006d912

Signed-off-by: Yashvardhan Kukreja <yash.kukreja.98@gmail.com>

fix: tests and deal with empty outputs gracefully

854895c

Signed-off-by: Yashvardhan Kukreja <yash.kukreja.98@gmail.com>

chore: introduce vendor to .gitignore

b85bafc

Signed-off-by: Yashvardhan Kukreja <yash.kukreja.98@gmail.com>

chore: nit cleanup

5cd2818

Signed-off-by: Yashvardhan Kukreja <yash.kukreja.98@gmail.com>

chore: use k8s.io/ptr for generic ptr ops

3065d57

Signed-off-by: Yashvardhan Kukreja <yash.kukreja.98@gmail.com>

fix: deps

bcffb8b

Signed-off-by: Yashvardhan Kukreja <yash.kukreja.98@gmail.com>

yashvardhan-kukreja force-pushed the issue-2796/gwctl-describe-gatewayclass branch from 65da0b6 to bcffb8b Compare April 11, 2024 12:47

gauravkghildiyal reviewed Apr 17, 2024

View reviewed changes

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Apr 17, 2024

chore: address comments

7be8284

Signed-off-by: Yashvardhan Kukreja <yash.kukreja.98@gmail.com>

gauravkghildiyal reviewed Apr 17, 2024

View reviewed changes

chore: nit

5420ee3

Signed-off-by: Yashvardhan Kukreja <yash.kukreja.98@gmail.com>

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

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


		// views ordered with respect to https://deploy-preview-2723--kubernetes-sigs-gateway-api.netlify.app/geps/gep-2722/#:~:text=gwctl%20describe%20gatewayclass%20foo%2Dcom%2Dexternal%2Dgateway%2Dclass

	metadata := crd.ObjectMeta.DeepCopy()
	metadata.Labels = nil
	metadata.Annotations = nil
	metadata.Name = ""
	metadata.Namespace = ""

[gwctl] Expand the output of gwctl describe gatewayclass #2930

[gwctl] Expand the output of gwctl describe gatewayclass #2930

Conversation

yashvardhan-kukreja commented Apr 3, 2024 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Note on the "null" values of Labels/Annotations**

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?:

k8s-ci-robot commented Apr 3, 2024

k8s-ci-robot commented Apr 3, 2024

robscott commented Apr 4, 2024

gauravkghildiyal commented Apr 4, 2024

gauravkghildiyal commented Apr 4, 2024

k8s-ci-robot commented Apr 4, 2024

gauravkghildiyal commented Apr 4, 2024

yashvardhan-kukreja commented Apr 4, 2024 • edited Loading

dprotaso commented Apr 4, 2024

yashvardhan-kukreja commented Apr 4, 2024

yashvardhan-kukreja commented Apr 9, 2024

yashvardhan-kukreja commented Apr 10, 2024

mlavacca 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

Choose a reason for hiding this comment

yashvardhan-kukreja commented Apr 12, 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

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 commented Apr 17, 2024

yashvardhan-kukreja commented Apr 17, 2024

yashvardhan-kukreja commented Apr 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gauravkghildiyal left a comment • edited Loading

Choose a reason for hiding this comment

gauravkghildiyal commented Apr 17, 2024

k8s-ci-robot commented Apr 17, 2024

yashvardhan-kukreja commented Apr 17, 2024

gauravkghildiyal commented Apr 17, 2024

yashvardhan-kukreja commented Apr 18, 2024

gauravkghildiyal commented Apr 18, 2024

yashvardhan-kukreja commented Apr 3, 2024 •

edited

Loading

yashvardhan-kukreja commented Apr 4, 2024 •

edited

Loading

gauravkghildiyal left a comment •

edited

Loading