Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 5 commits into from
Apr 18, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7857cfb
Add label filtering (-l) for describe commands
deszhou Apr 3, 2024
3c7b3e5
fix nits
deszhou Apr 4, 2024
bc3262e
fix test
deszhou Apr 10, 2024
f69b125
refactor test
deszhou Apr 17, 2024
c7c406d
move tests to discoverer_test.go
deszhou Apr 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 44 additions & 4 deletions gwctl/cmd/describe.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,13 @@ import (
"sigs.k8s.io/gateway-api/gwctl/pkg/utils"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
)

func NewDescribeCommand() *cobra.Command {
var namespaceFlag string
var allNamespacesFlag bool
var labelSelector string

cmd := &cobra.Command{
Use: "describe {policies|httproutes|gateways|gatewayclasses|backends|namespace} RESOURCE_NAME",
Expand All @@ -45,6 +47,7 @@ func NewDescribeCommand() *cobra.Command {
}
cmd.Flags().StringVarP(&namespaceFlag, "namespace", "n", "default", "")
cmd.Flags().BoolVarP(&allNamespacesFlag, "all-namespaces", "A", false, "If present, list requested resources from all namespaces.")
cmd.Flags().StringVarP(&labelSelector, "selector", "l", "", "Label selector.")
deszhou marked this conversation as resolved.
Show resolved Hide resolved

return cmd
}
Expand All @@ -64,6 +67,12 @@ func runDescribe(cmd *cobra.Command, args []string, params *utils.CmdParams) {
os.Exit(1)
}

labelSelector, err := cmd.Flags().GetString("selector")
if err != nil {
fmt.Fprintf(os.Stderr, "failed to read flag \"selector\": %v\n", err)
os.Exit(1)
}

if allNs {
ns = metav1.NamespaceAll
}
Expand Down Expand Up @@ -97,7 +106,15 @@ func runDescribe(cmd *cobra.Command, args []string, params *utils.CmdParams) {
policiesPrinter.PrintDescribeView(policyList)

case "httproute", "httproutes":
filter := resourcediscovery.Filter{Namespace: ns}
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)
}
Comment on lines +109 to +113
Copy link
Contributor

@yashvardhan-kukreja yashvardhan-kukreja Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

filter := resourcediscovery.Filter{
Namespace: ns,
Labels: selector,
}
if len(args) > 1 {
filter.Name = args[1]
}
Expand All @@ -109,7 +126,15 @@ func runDescribe(cmd *cobra.Command, args []string, params *utils.CmdParams) {
httpRoutesPrinter.PrintDescribeView(resourceModel)

case "gateway", "gateways":
filter := resourcediscovery.Filter{Namespace: ns}
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)
}
Comment on lines +129 to +133
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

filter := resourcediscovery.Filter{
Namespace: ns,
Labels: selector,
}
if len(args) > 1 {
filter.Name = args[1]
}
Expand All @@ -121,7 +146,15 @@ func runDescribe(cmd *cobra.Command, args []string, params *utils.CmdParams) {
gwPrinter.PrintDescribeView(resourceModel)

case "gatewayclass", "gatewayclasses":
filter := resourcediscovery.Filter{}
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)
}
filter := resourcediscovery.Filter{
Namespace: ns,
Labels: selector,
}
deszhou marked this conversation as resolved.
Show resolved Hide resolved
if len(args) > 1 {
filter.Name = args[1]
}
Expand All @@ -147,7 +180,14 @@ func runDescribe(cmd *cobra.Command, args []string, params *utils.CmdParams) {
backendsPrinter.PrintDescribeView(resourceModel)

case "namespace", "namespaces", "ns":
filter := resourcediscovery.Filter{}
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)
}
Comment on lines +182 to +186
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

filter := resourcediscovery.Filter{
Labels: selector,
}
if len(args) > 1 {
filter.Name = args[1]
}
Expand Down
99 changes: 99 additions & 0 deletions gwctl/pkg/printer/gatewayclasses_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@deszhou deszhou Apr 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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.

fakeClock := testingclock.NewFakeClock(time.Now())

gatewayClass := func(name string, labels map[string]string) *gatewayv1.GatewayClass {
return &gatewayv1.GatewayClass{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Labels: labels,
CreationTimestamp: metav1.Time{
Time: fakeClock.Now().Add(-365 * 24 * time.Hour),
},
},
Spec: gatewayv1.GatewayClassSpec{
ControllerName: gatewayv1.GatewayController(name + "/controller"),
Description: common.PtrTo("random"),
},
Status: gatewayv1.GatewayClassStatus{
Conditions: []metav1.Condition{
{
Type: "Accepted",
Status: metav1.ConditionTrue,
},
},
},
}
}
objects := []runtime.Object{
&apiextensionsv1.CustomResourceDefinition{
ObjectMeta: metav1.ObjectMeta{
Name: "healthcheckpolicies.foo.com",
Labels: map[string]string{
gatewayv1alpha2.PolicyLabelKey: "true",
},
},
Spec: apiextensionsv1.CustomResourceDefinitionSpec{
Group: "foo.com",
Versions: []apiextensionsv1.CustomResourceDefinitionVersion{{Name: "v1"}},
Names: apiextensionsv1.CustomResourceDefinitionNames{
Plural: "healthcheckpolicies",
Kind: "HealthCheckPolicy",
},
},
},
&unstructured.Unstructured{
Object: map[string]interface{}{
"apiVersion": "foo.com/v1",
"kind": "HealthCheckPolicy",
"metadata": map[string]interface{}{
"name": "policy-name",
},
"spec": map[string]interface{}{
"targetRef": map[string]interface{}{
"group": "gateway.networking.k8s.io",
"kind": "GatewayClass",
"name": "foo-com-internal-gateway-class",
},
},
},
},
gatewayClass("foo-com-external-gateway-class", map[string]string{"app": "foo"}),
gatewayClass("foo-com-internal-gateway-class", map[string]string{"app": "foo", "env": "internal"}),
}
params := utils.MustParamsForTest(t, common.MustClientsForTest(t, objects...))
discoverer := resourcediscovery.Discoverer{
K8sClients: params.K8sClients,
PolicyManager: params.PolicyManager,
}
labelSelector := "env=internal"
selector, err := labels.Parse(labelSelector)
if err != nil {
t.Errorf("Unable to find resources that match the label selector \"%s\": %v\n", labelSelector, err)
}
resourceModel, err := discoverer.DiscoverResourcesForGatewayClass(resourcediscovery.Filter{Labels: selector})
if err != nil {
t.Fatalf("Failed to construct resourceModel: %v", resourceModel)
}

gcp := &GatewayClassesPrinter{
Out: params.Out,
Clock: fakeClock,
}
gcp.PrintDescribeView(resourceModel)

got := params.Out.(*bytes.Buffer).String()
want := `
Name: foo-com-internal-gateway-class
ControllerName: foo-com-internal-gateway-class/controller
Description: random
DirectlyAttachedPolicies:
- Group: foo.com
Kind: HealthCheckPolicy
Name: policy-name
`
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)
}
}
128 changes: 128 additions & 0 deletions gwctl/pkg/printer/gateways_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,3 +449,131 @@ gateway-2 gatewayclass-1 192.168.100.5 8080 False 5d
t.Errorf("Unexpected diff\ngot=\n%v\nwant=\n%v\ndiff (-want +got)=\n%v", got, want, diff)
}
}

// TestGatewaysPrinterDescribe_LabelSelector tests label selector filtering for Gateways in 'describe' command.
func TestGatewaysPrinterDescribe_LabelSelector(t *testing.T) {
fakeClock := testingclock.NewFakeClock(time.Now())
gateway := func(name string, labels map[string]string) *gatewayv1.Gateway {
return &gatewayv1.Gateway{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Labels: labels,
CreationTimestamp: metav1.Time{
Time: fakeClock.Now().Add(-5 * 24 * time.Hour),
},
},
Spec: gatewayv1.GatewaySpec{
GatewayClassName: "gatewayclass-1",
Listeners: []gatewayv1.Listener{
{
Name: "http-8080",
Protocol: gatewayv1.HTTPProtocolType,
Port: gatewayv1.PortNumber(8080),
},
},
},
Status: gatewayv1.GatewayStatus{
Addresses: []gatewayv1.GatewayStatusAddress{
{
Value: "192.168.100.5",
},
},
Conditions: []metav1.Condition{
{
Type: "Programmed",
Status: "False",
},
},
},
}
}

objects := []runtime.Object{
&gatewayv1.GatewayClass{
ObjectMeta: metav1.ObjectMeta{
Name: "gatewayclass-1",
},
Spec: gatewayv1.GatewayClassSpec{
ControllerName: "example.net/gateway-controller",
Description: common.PtrTo("random"),
},
},

&apiextensionsv1.CustomResourceDefinition{
ObjectMeta: metav1.ObjectMeta{
Name: "healthcheckpolicies.foo.com",
Labels: map[string]string{
gatewayv1alpha2.PolicyLabelKey: "inherited",
},
},
Spec: apiextensionsv1.CustomResourceDefinitionSpec{
Scope: apiextensionsv1.ClusterScoped,
Group: "foo.com",
Versions: []apiextensionsv1.CustomResourceDefinitionVersion{{Name: "v1"}},
Names: apiextensionsv1.CustomResourceDefinitionNames{
Plural: "healthcheckpolicies",
Kind: "HealthCheckPolicy",
},
},
},
&unstructured.Unstructured{
Object: map[string]interface{}{
"apiVersion": "foo.com/v1",
"kind": "HealthCheckPolicy",
"metadata": map[string]interface{}{
"name": "health-check-gatewayclass",
},
"spec": map[string]interface{}{
"override": map[string]interface{}{
"key1": "value-parent-1",
},
"default": map[string]interface{}{
"key2": "value-parent-2",
},
"targetRef": map[string]interface{}{
"group": "gateway.networking.k8s.io",
"kind": "GatewayClass",
"name": "gatewayclass-1",
},
},
},
},
gateway("gateway-1", map[string]string{"app": "foo"}),
gateway("gateway-2", map[string]string{"app": "foo", "env": "internal"}),
}

params := utils.MustParamsForTest(t, common.MustClientsForTest(t, objects...))
discoverer := resourcediscovery.Discoverer{
K8sClients: params.K8sClients,
PolicyManager: params.PolicyManager,
}
labelSelector := "env=internal"
selector, err := labels.Parse(labelSelector)
if err != nil {
t.Errorf("Unable to find resources that match the label selector \"%s\": %v\n", labelSelector, err)
}
resourceModel, err := discoverer.DiscoverResourcesForGateway(resourcediscovery.Filter{Labels: selector})
if err != nil {
t.Fatalf("Failed to construct resourceModel: %v", resourceModel)
}

nsp := &GatewaysPrinter{
Out: params.Out,
Clock: fakeClock,
}
nsp.PrintDescribeView(resourceModel)

got := params.Out.(*bytes.Buffer).String()
want := `
Name: gateway-2
GatewayClass: gatewayclass-1
EffectivePolicies:
HealthCheckPolicy.foo.com:
key1: value-parent-1
key2: value-parent-2
`

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