diff --git a/cmd/crank/beta/trace/internal/printer/default.go b/cmd/crank/beta/trace/internal/printer/default.go index 0c506495c11..5b5dba5fcd9 100644 --- a/cmd/crank/beta/trace/internal/printer/default.go +++ b/cmd/crank/beta/trace/internal/printer/default.go @@ -31,9 +31,9 @@ import ( ) const ( - errFmtCannotWriteHeader = "cannot write header: %s" - errFmtCannotWriteRow = "cannot write row: %s" - errFmtCannotFlushTabWriter = "cannot flush tab writer: %s" + errWriteHeader = "cannot write header" + errWriteRow = "cannot write row" + errFlushTabWriter = "cannot flush tab writer" ) // DefaultPrinter defines the DefaultPrinter configuration @@ -71,7 +71,7 @@ func (p *DefaultPrinter) Print(w io.Writer, root *resource.Resource) error { status: "STATUS", } if _, err := fmt.Fprintln(tw, headers.String()); err != nil { - return errors.Errorf(errFmtCannotWriteHeader, err) + return errors.Wrap(err, errWriteHeader) } type queueItem struct { @@ -81,30 +81,35 @@ func (p *DefaultPrinter) Print(w io.Writer, root *resource.Resource) error { prefix string } - // Initialize LIFO queue with root element + // Initialize LIFO queue with root element to traverse the tree depth-first, + // enqueuing children in reverse order so that they are dequeued in the + // right order w.r.t. the way they are defined by the resources. queue := []*queueItem{{resource: root}} for len(queue) > 0 { - // Dequeue first element var item *queueItem l := len(queue) - item, queue = queue[l-1], queue[:l-1] + item, queue = queue[l-1], queue[:l-1] // Pop the last element - // Choose the right prefix + // Build the name of the current node, prepending the required prefix to + // show the tree structure name := strings.Builder{} - - childPrefix := item.prefix - if item.depth > 0 { - if item.isLast { - name.WriteString(item.prefix + "└─ ") - childPrefix += " " - } else { - name.WriteString(item.prefix + "├─ ") - childPrefix += "│ " - } + childPrefix := item.prefix // Inherited prefix for all the children of the current node + switch { + case item.depth == 0: + // We don't need a prefix for the root, nor a custom + // prefix for its children + case item.isLast: + name.WriteString(item.prefix + "└─ ") + childPrefix += " " + default: + name.WriteString(item.prefix + "├─ ") + childPrefix += "│ " } name.WriteString(fmt.Sprintf("%s/%s", item.resource.Unstructured.GetKind(), item.resource.Unstructured.GetName())) + + // Append the namespace if it's not empty if item.resource.Unstructured.GetNamespace() != "" { name.WriteString(fmt.Sprintf(" (%s)", item.resource.Unstructured.GetNamespace())) } @@ -117,46 +122,61 @@ func (p *DefaultPrinter) Print(w io.Writer, root *resource.Resource) error { status: status, } if _, err := fmt.Fprintln(tw, row.String()); err != nil { - return errors.Errorf(errFmtCannotWriteRow, err) + return errors.Wrap(err, errWriteRow) } - // Enqueue the children of the current node + // Enqueue the children of the current node in reverse order to ensure + // that they are dequeued from the LIFO queue in the same order w.r.t. + // the way they are defined by the resources. for idx := len(item.resource.Children) - 1; idx >= 0; idx-- { isLast := idx == len(item.resource.Children)-1 queue = append(queue, &queueItem{resource: item.resource.Children[idx], depth: item.depth + 1, isLast: isLast, prefix: childPrefix}) } } + if err := tw.Flush(); err != nil { - return errors.Errorf(errFmtCannotFlushTabWriter, err) + return errors.Wrap(err, errFlushTabWriter) } return nil } // getResourceStatus returns the status of the resource. -func getResourceStatus(r *resource.Resource, wide bool) (ready string, synced string, status string) { //nolint:gocyclo // it's just a switch +func getResourceStatus(r *resource.Resource, wide bool) (ready string, synced string, status string) { readyCond := r.GetCondition(xpv1.TypeReady) syncedCond := r.GetCondition(xpv1.TypeSynced) var m string switch { case r.Error != nil: + // if there is an error we want to show it status = "Error" m = r.Error.Error() case readyCond.Status == corev1.ConditionTrue && syncedCond.Status == corev1.ConditionTrue: + // if both are true we want to show the ready reason only status = string(readyCond.Reason) + + // The following cases are for when one of the conditions is false, + // prioritizing readiness over synced in case of issues. case readyCond.Status == corev1.ConditionFalse: status = string(readyCond.Reason) m = readyCond.Message case syncedCond.Status == corev1.ConditionFalse: status = string(syncedCond.Reason) m = syncedCond.Message - case syncedCond.Status != corev1.ConditionTrue && readyCond.Status != corev1.ConditionTrue: + + default: + // both are unknown or unset, let's try showing the ready reason, probably empty status = string(readyCond.Reason) m = readyCond.Message } + + // Crop the message to the last 64 characters if it's too long and we are + // not in wide mode if !wide && len(m) > 64 { m = "..." + m[len(m)-64:] } + + // Append the message to the status if it's not empty if m != "" { status = fmt.Sprintf("%s: %s", status, m) } diff --git a/cmd/crank/beta/trace/internal/printer/default_test.go b/cmd/crank/beta/trace/internal/printer/default_test.go index 404293c0bd4..dda4005b587 100644 --- a/cmd/crank/beta/trace/internal/printer/default_test.go +++ b/cmd/crank/beta/trace/internal/printer/default_test.go @@ -52,14 +52,15 @@ func TestDefaultPrinter(t *testing.T) { want: want{ // Note: Use spaces instead of tabs for intendation output: ` -NAME READY SYNCED STATUS -ObjectStorage/test-resource (default) True True -└─ XObjectStorage/test-resource-hash (default) True True - ├─ Bucket/test-resource-bucket-hash (default) True True - │ ├─ User/test-resource-child-1-bucket-hash (default) False True SomethingWrongHappened: Error with bucket child 1 - │ └─ User/test-resource-child-2-bucket-hash (default) False True SomethingWrongHappened: Error with bucket child 2 - │ └─ User/test-resource-child-2-1-bucket-hash (default) False True SomethingWrongHappened: Error with bucket child 2-1 - └─ User/test-resource-user-hash (default) True True +NAME READY SYNCED STATUS +ObjectStorage/test-resource (default) True True +└─ XObjectStorage/test-resource-hash True True + ├─ Bucket/test-resource-bucket-hash True True + │ ├─ User/test-resource-child-1-bucket-hash False True SomethingWrongHappened: Error with bucket child 1 + │ ├─ User/test-resource-child-mid-bucket-hash True True AllGood + │ └─ User/test-resource-child-2-bucket-hash False True SomethingWrongHappened: Error with bucket child 2 + │ └─ User/test-resource-child-2-1-bucket-hash False True SomethingWrongHappened: Error with bucket child 2-1 + └─ User/test-resource-user-hash True True `, err: nil, }, diff --git a/cmd/crank/beta/trace/internal/printer/dot_test.go b/cmd/crank/beta/trace/internal/printer/dot_test.go index ef643392a1b..b174d9299d1 100644 --- a/cmd/crank/beta/trace/internal/printer/dot_test.go +++ b/cmd/crank/beta/trace/internal/printer/dot_test.go @@ -37,18 +37,20 @@ func TestPrintDotGraph(t *testing.T) { dotString: `graph { n1[label="Name: ObjectStorage/test-resource\nApiVersion: test.cloud/v1alpha1\nNamespace: default\nReady: True\nSynced: True\n",penwidth="2"]; - n2[label="Name: XObjectStorage/test-resource-hash\nApiVersion: test.cloud/v1alpha1\nNamespace: default\nReady: True\nSynced: True\n",penwidth="2"]; - n3[label="Name: Bucket/test-resource-bucket-hash\nApiVersion: test.cloud/v1alpha1\nNamespace: default\nReady: True\nSynced: True\n",penwidth="2"]; - n4[label="Name: User/test-resource-user-hash\nApiVersion: test.cloud/v1alpha1\nNamespace: default\nReady: True\nSynced: True\n",penwidth="2"]; - n5[label="Name: User/test-resource-child-1-bucket-hash\nApiVersion: test.cloud/v1alpha1\nNamespace: default\nReady: False\nSynced: True\n",penwidth="2"]; - n6[label="Name: User/test-resource-child-2-bucket-hash\nApiVersion: test.cloud/v1alpha1\nNamespace: default\nReady: False\nSynced: True\n",penwidth="2"]; - n7[label="Name: User/test-resource-child-2-1-bucket-hash\nApiVersion: test.cloud/v1alpha1\nNamespace: default\nReady: False\nSynced: True\n",penwidth="2"]; + n2[label="Name: XObjectStorage/test-resource-hash\nApiVersion: test.cloud/v1alpha1\nReady: True\nSynced: True\n",penwidth="2"]; + n3[label="Name: Bucket/test-resource-bucket-hash\nApiVersion: test.cloud/v1alpha1\nReady: True\nSynced: True\n",penwidth="2"]; + n4[label="Name: User/test-resource-user-hash\nApiVersion: test.cloud/v1alpha1\nReady: True\nSynced: True\n",penwidth="2"]; + n5[label="Name: User/test-resource-child-1-bucket-hash\nApiVersion: test.cloud/v1alpha1\nReady: False\nSynced: True\n",penwidth="2"]; + n6[label="Name: User/test-resource-child-mid-bucket-hash\nApiVersion: test.cloud/v1alpha1\nReady: True\nSynced: True\n",penwidth="2"]; + n7[label="Name: User/test-resource-child-2-bucket-hash\nApiVersion: test.cloud/v1alpha1\nReady: False\nSynced: True\n",penwidth="2"]; + n8[label="Name: User/test-resource-child-2-1-bucket-hash\nApiVersion: test.cloud/v1alpha1\nReady: False\nSynced: True\n",penwidth="2"]; n1--n2; n2--n3; n2--n4; n3--n5; n3--n6; - n6--n7; + n3--n7; + n7--n8; } `, diff --git a/cmd/crank/beta/trace/internal/printer/json_test.go b/cmd/crank/beta/trace/internal/printer/json_test.go index 1aba0d3285e..2bcd8657ef0 100644 --- a/cmd/crank/beta/trace/internal/printer/json_test.go +++ b/cmd/crank/beta/trace/internal/printer/json_test.go @@ -82,7 +82,7 @@ func TestJSONPrinter(t *testing.T) { "kind": "XObjectStorage", "metadata": { "name": "test-resource-hash", - "namespace": "default" + "namespace": "" }, "status": { "conditions": [ @@ -106,7 +106,7 @@ func TestJSONPrinter(t *testing.T) { "kind": "Bucket", "metadata": { "name": "test-resource-bucket-hash", - "namespace": "default" + "namespace": "" }, "status": { "conditions": [ @@ -130,7 +130,7 @@ func TestJSONPrinter(t *testing.T) { "kind": "User", "metadata": { "name": "test-resource-child-1-bucket-hash", - "namespace": "default" + "namespace": "" }, "status": { "conditions": [ @@ -149,13 +149,37 @@ func TestJSONPrinter(t *testing.T) { } } }, + { + "object": { + "apiVersion": "test.cloud/v1alpha1", + "kind": "User", + "metadata": { + "name": "test-resource-child-mid-bucket-hash", + "namespace": "" + }, + "status": { + "conditions": [ + { + "status": "True", + "type": "Synced" + }, + { + "type": "Ready", + "status": "True", + "lastTransitionTime": null, + "reason": "AllGood" + } + ] + } + } + }, { "object": { "apiVersion": "test.cloud/v1alpha1", "kind": "User", "metadata": { "name": "test-resource-child-2-bucket-hash", - "namespace": "default" + "namespace": "" }, "status": { "conditions": [ @@ -180,7 +204,7 @@ func TestJSONPrinter(t *testing.T) { "kind": "User", "metadata": { "name": "test-resource-child-2-1-bucket-hash", - "namespace": "default" + "namespace": "" }, "status": { "conditions": [ @@ -209,7 +233,7 @@ func TestJSONPrinter(t *testing.T) { "kind": "User", "metadata": { "name": "test-resource-user-hash", - "namespace": "default" + "namespace": "" }, "status": { "conditions": [ diff --git a/cmd/crank/beta/trace/internal/printer/printer_test.go b/cmd/crank/beta/trace/internal/printer/printer_test.go index 2d09dd9a416..89bbbe7a46b 100644 --- a/cmd/crank/beta/trace/internal/printer/printer_test.go +++ b/cmd/crank/beta/trace/internal/printer/printer_test.go @@ -25,14 +25,14 @@ import ( ) // Returns an unstructured that has basic fields set to be used by other tests. -func DummyManifest(kind, name, syncedStatus string, readyCond *xpv1.Condition) unstructured.Unstructured { +func DummyManifest(kind, name, namespace, syncedStatus string, readyCond *xpv1.Condition) unstructured.Unstructured { m := unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "test.cloud/v1alpha1", "kind": kind, "metadata": map[string]interface{}{ "name": name, - "namespace": "default", + "namespace": namespace, }, "status": map[string]interface{}{ "conditions": []interface{}{ @@ -51,25 +51,25 @@ func DummyManifest(kind, name, syncedStatus string, readyCond *xpv1.Condition) u func GetComplexResource() *resource.Resource { return &resource.Resource{ - Unstructured: DummyManifest("ObjectStorage", "test-resource", "True", &xpv1.Condition{ + Unstructured: DummyManifest("ObjectStorage", "test-resource", "default", "True", &xpv1.Condition{ Type: "Ready", Status: "True", }), Children: []*resource.Resource{ { - Unstructured: DummyManifest("XObjectStorage", "test-resource-hash", "True", &xpv1.Condition{ + Unstructured: DummyManifest("XObjectStorage", "test-resource-hash", "", "True", &xpv1.Condition{ Type: "Ready", Status: "True", }), Children: []*resource.Resource{ { - Unstructured: DummyManifest("Bucket", "test-resource-bucket-hash", "True", &xpv1.Condition{ + Unstructured: DummyManifest("Bucket", "test-resource-bucket-hash", "", "True", &xpv1.Condition{ Type: "Ready", Status: "True", }), Children: []*resource.Resource{ { - Unstructured: DummyManifest("User", "test-resource-child-1-bucket-hash", "True", &xpv1.Condition{ + Unstructured: DummyManifest("User", "test-resource-child-1-bucket-hash", "", "True", &xpv1.Condition{ Type: "Ready", Status: "False", Reason: "SomethingWrongHappened", @@ -77,7 +77,14 @@ func GetComplexResource() *resource.Resource { }), }, { - Unstructured: DummyManifest("User", "test-resource-child-2-bucket-hash", "True", &xpv1.Condition{ + Unstructured: DummyManifest("User", "test-resource-child-mid-bucket-hash", "", "True", &xpv1.Condition{ + Type: "Ready", + Status: "True", + Reason: "AllGood", + }), + }, + { + Unstructured: DummyManifest("User", "test-resource-child-2-bucket-hash", "", "True", &xpv1.Condition{ Type: "Ready", Reason: "SomethingWrongHappened", Status: "False", @@ -85,7 +92,7 @@ func GetComplexResource() *resource.Resource { }), Children: []*resource.Resource{ { - Unstructured: DummyManifest("User", "test-resource-child-2-1-bucket-hash", "True", &xpv1.Condition{ + Unstructured: DummyManifest("User", "test-resource-child-2-1-bucket-hash", "", "True", &xpv1.Condition{ Type: "Ready", Reason: "SomethingWrongHappened", Status: "False", @@ -97,7 +104,7 @@ func GetComplexResource() *resource.Resource { }, }, { - Unstructured: DummyManifest("User", "test-resource-user-hash", "True", &xpv1.Condition{ + Unstructured: DummyManifest("User", "test-resource-user-hash", "", "True", &xpv1.Condition{ Type: "Ready", Status: "True", }), diff --git a/cmd/crank/beta/trace/internal/resource/client.go b/cmd/crank/beta/trace/internal/resource/client.go index b3b02380e94..e5f3cc96b72 100644 --- a/cmd/crank/beta/trace/internal/resource/client.go +++ b/cmd/crank/beta/trace/internal/resource/client.go @@ -35,10 +35,11 @@ import ( ) const ( - errCouldntGetRootResource = "couldn't get root resource" - errCouldntGetChildResource = "couldn't get child resource" - errCouldntGetResource = "couldn't get resource" + errGetRootResource = "couldn't get root resource" + errGetChildResource = "couldn't get child resource" + errGetResource = "couldn't get resource" errFmtResourceTypeNotFound = "the server doesn't have a resource type %q" + errGetChildrenRefs = "couldn't get children refs" ) // Client to get a Resource with all its children. @@ -49,12 +50,38 @@ type Client struct { rmapper meta.RESTMapper } +// ClientOption is a functional option for a Client. +type ClientOption func(*Client) + +// WithConnectionSecrets is a functional option that sets the client to get secrets to the desired value. +func WithConnectionSecrets(v bool) ClientOption { + return func(c *Client) { + c.getConnectionSecrets = v + } +} + +// NewClient returns a new Client. +func NewClient(in client.Client, rmapper meta.RESTMapper, opts ...ClientOption) (*Client, error) { + uClient := xpunstructured.NewClient(in) + + c := &Client{ + client: uClient, + rmapper: rmapper, + } + + for _, o := range opts { + o(c) + } + + return c, nil +} + // GetResourceTree returns the requested Resource and all its children. func (kc *Client) GetResourceTree(ctx context.Context, rootRef *v1.ObjectReference) (*Resource, error) { // Get the root resource root, err := kc.getResource(ctx, rootRef) if err != nil { - return nil, errors.Wrap(err, errCouldntGetRootResource) + return nil, errors.Wrap(err, errGetRootResource) } // breadth-first search of children @@ -66,12 +93,12 @@ func (kc *Client) GetResourceTree(ctx context.Context, rootRef *v1.ObjectReferen refs := getResourceChildrenRefs(res, kc.getConnectionSecrets) if err != nil { - return nil, errors.Wrap(err, errCouldntGetRootResource) + return nil, errors.Wrap(err, errGetChildrenRefs) } for i := range refs { child, err := kc.getResource(ctx, &refs[i]) if err != nil { - return nil, errors.Wrap(err, errCouldntGetChildResource) + return nil, errors.Wrap(err, errGetChildResource) } res.Children = append(res.Children, child) queue = append(queue, child) // Enqueue child @@ -85,6 +112,7 @@ func (kc *Client) GetResourceTree(ctx context.Context, rootRef *v1.ObjectReferen func (kc *Client) getResource(ctx context.Context, ref *v1.ObjectReference) (*Resource, error) { result := unstructured.Unstructured{} result.SetGroupVersionKind(ref.GroupVersionKind()) + err := kc.client.Get(ctx, xpmeta.NamespacedNameOf(ref), &result) if kerrors.IsNotFound(err) { result.SetName(ref.Name) @@ -92,7 +120,7 @@ func (kc *Client) getResource(ctx context.Context, ref *v1.ObjectReference) (*Re return &Resource{Unstructured: result, Error: err}, nil } if err != nil { - return nil, errors.Wrap(err, errCouldntGetResource) + return nil, errors.Wrap(err, errGetResource) } return &Resource{Unstructured: result}, nil @@ -102,7 +130,7 @@ func (kc *Client) getResource(ctx context.Context, ref *v1.ObjectReference) (*Re // Resource, assuming it's a Crossplane resource, XR or XRC. func getResourceChildrenRefs(r *Resource, getConnectionSecrets bool) []v1.ObjectReference { obj := r.Unstructured - // collect owner references + // collect object references for the var refs []v1.ObjectReference xrc := claim.Unstructured{Unstructured: obj} @@ -122,7 +150,7 @@ func getResourceChildrenRefs(r *Resource, getConnectionSecrets bool) []v1.Object return refs } - // handle write connection secrets for both XR and XRC + // Handle write connection secrets for both XR and XRC xrSecretRef := xr.GetWriteConnectionSecretToReference() if xrSecretRef != nil { ref := v1.ObjectReference{ @@ -182,29 +210,3 @@ func (kc *Client) MappingFor(resourceOrKindArg string) (*meta.RESTMapping, error } return mapping, nil } - -// ClientOption is a functional option for a Client. -type ClientOption func(*Client) - -// WithConnectionSecrets is a functional option that sets the client to get secrets to the desired value. -func WithConnectionSecrets(v bool) ClientOption { - return func(c *Client) { - c.getConnectionSecrets = v - } -} - -// NewClient returns a new Client. -func NewClient(in client.Client, rmapper meta.RESTMapper, opts ...ClientOption) (*Client, error) { - uClient := xpunstructured.NewClient(in) - - c := &Client{ - client: uClient, - rmapper: rmapper, - } - - for _, o := range opts { - o(c) - } - - return c, nil -} diff --git a/cmd/crank/beta/trace/internal/resource/client_test.go b/cmd/crank/beta/trace/internal/resource/client_test.go index d44788077b2..666e0d3602b 100644 --- a/cmd/crank/beta/trace/internal/resource/client_test.go +++ b/cmd/crank/beta/trace/internal/resource/client_test.go @@ -92,7 +92,7 @@ func TestGetResourceChildrenRefs(t *testing.T) { args args want want }{ - "XRCWithChildrenMR": { + "XRCWithChildrenXR": { reason: "Should return the XR child for an XRC.", args: args{ resource: &Resource{ @@ -113,8 +113,8 @@ func TestGetResourceChildrenRefs(t *testing.T) { }, }, }, - "XRCWithChildrenXR": { - reason: "Should return a list of children refs for an XR.", + "XRCWithChildren": { + reason: "Should return the list of children refs for an XR.", args: args{ resource: &Resource{ Unstructured: *buildXR(withXRRefs(v1.ObjectReference{ @@ -133,6 +133,7 @@ func TestGetResourceChildrenRefs(t *testing.T) { APIVersion: "example2.com/v1", Kind: "XRC", Name: "xrc-1", + Namespace: "ns-1", }, )), }, @@ -158,11 +159,12 @@ func TestGetResourceChildrenRefs(t *testing.T) { APIVersion: "example2.com/v1", Kind: "XRC", Name: "xrc-1", + Namespace: "ns-1", }, }, }, }, - "XRCWithChildrenMRAndSecret": { + "XRCWithChildrenXRandConnectionSecretEnabled": { reason: "Should return the XR child and writeConnectionSecret ref for an XRC.", args: args{ witSecrets: true, @@ -192,8 +194,8 @@ func TestGetResourceChildrenRefs(t *testing.T) { }, }, }, - "XRCWithChildrenMRAndSecretDisabled": { - reason: "Should return the XR child and writeConnectionSecret ref for an XRC.", + "XRCWithChildrenXRandConnectionSecretDisabled": { + reason: "Should return the XR child, but no writeConnectionSecret, ref for an XRC.", args: args{ witSecrets: false, resource: &Resource{ diff --git a/cmd/crank/beta/trace/trace.go b/cmd/crank/beta/trace/trace.go index b7429fd66f9..4cc53858e33 100644 --- a/cmd/crank/beta/trace/trace.go +++ b/cmd/crank/beta/trace/trace.go @@ -91,7 +91,7 @@ Examples: // Run runs the trace command. func (c *Cmd) Run(k *kong.Context, logger logging.Logger) error { //nolint:gocyclo // TODO(phisco): refactor - logger = logger.WithValues("Resource", c.Resource) + logger = logger.WithValues("Resource", c.Resource, "Name", c.Name) // Init new printer p, err := printer.New(c.Output) @@ -143,6 +143,7 @@ func (c *Cmd) Run(k *kong.Context, logger logging.Logger) error { //nolint:gocyc Name: c.Name, } if mapping.Scope.Name() == meta.RESTScopeNameNamespace && c.Namespace != "" { + logger.Debug("Requested resource is namespaced", "namespace", c.Namespace) rootRef.Namespace = c.Namespace } logger.Debug("Getting resource tree", "rootRef", rootRef.String())