Trace cli in go #15

turkenh · 2019-10-21T13:29:34Z

To do:

Test and adapt "Simple Resource Class Selection" Simple Resource Class Selection one pager crossplane#926
Better identifier, UID missing if object does not exist
Adapt bootstrap.sh to include trace
Ready/Pending/Missing states to highlight what is wrong
Unit tests
Test different scenarios and edge case handling
Refactor: better naming, using constants, comments etc
Polish graph output

Build & Install:

$ make build
$ make install

Trace a kubernetesapplication:

$ kubectl crossplane trace kubernetesapplication <name> -n <namespace>

Graph output:

# Graphviz needs to be installed first, e.g. brew install graphviz
$  kubectl crossplane trace kubernetesapplication <name> -n <namespace> -o dot |dot -Tpng > /tmp/output.png&& open /tmp/output.png

suskin

@turkenh I like that there's an early draft for early review and comments! A couple questions.

Would you be able to use more upbound-style commit messages? Here's the formula I've been using:

category: one-line summary of change

This section down here is longer, and is an explanation
of why the change is being made, or why it is happening
in the way it is being done. The goal of this section is to
help people get into the mindset of the original author, so
that they can keep making changes in the way that the
original author would be, if that's what they want to do.

commit signature

What testing has been done?

bin/kubectl-crossplane-trace

suskin · 2019-10-21T17:56:23Z

bin/kubectl-crossplane-trace

+
+check_help "${1}"
+
+crossplane-cli trace "${@}"


Where does crossplane-cli come from?

This will be the built binary of go code.
e.g.

go build -o /bin/crossplane-cli cmd/main.go

suskin · 2019-10-21T17:56:50Z

bin/kubectl-crossplane-trace

@@ -0,0 +1,22 @@
+#!/usr/bin/env bash


What is the benefit of having a bash wrapper around another binary?

As you know, kubectl plugins require an exact name matching the command/subcommand.
This way, we can use a single binary produced from the go code for different subcommands (currently only for trace but later we can consider adding more).

turkenh · 2019-10-21T19:37:44Z

Would you be able to use more upbound-style commit messages? Here's the formula I've been using

Sure. I'll most probably, overwrite the commit history here with some sort of "Initial commit" for trace cli go project since the initial commits include experimental changes and I didn't follow well scoped commits.

What testing has been done?

I am mainly testing with wordpress stack example both happy path and also by intentionally breaking some stuff.

First commit adding the golang project for trace cli Signed-off-by: Hasan Turken <turkenh@gmail.com>

We need the path of the fields for accessing the data in unstructured object. This commit defines and use some varaibles for this purpose. Signed-off-by: Hasan Turken <turkenh@gmail.com>

Now the cred secret used by a provider is also included into the trace. Signed-off-by: Hasan Turken <turkenh@gmail.com>

When there are two kinds from different groups, it was not possible to indicate which one is expected to be traced. e.g. if both "providers.gcp.crossplane.io" and "providers.aws.crossplane.io", kubectl get providers simply selects one of them. This commit adds support for types (or kinds) with group just like kubectl can do. example: kubectl crossplane trace providers.gcp.crossplane.io gcp-provider -n gcp Signed-off-by: Hasan Turken <turkenh@gmail.com>

This commit introduces a new method (with unittests) on trace.crossplane interface (yet to be consumed by printer) which returns ObjectDetails struct so that printers can access and print details with their format. Previously, details of crossplane objects was formatted strings which means embedding formatting logic into non related objects. Signed-off-by: Hasan Turken <turkenh@gmail.com>

With this commit, simple printer uses ObjectDetails returned by crossplane objects and prints details with a common go template. Signed-off-by: Hasan Turken <turkenh@gmail.com>

Signed-off-by: Hasan Turken <turkenh@gmail.com>

Removes the wrapper bash file which was intented to make future subcommands available from single binary. Because it is not necessary right now and we can consider again once we introduce new subcommands. Signed-off-by: Hasan Turken <turkenh@gmail.com>

Released artifacts will be platform specific .tar.gz files including bin directory with all existing bash scripts plus kubectl-crossplane-trace binary built from go code. ./bootstrap.sh will fetch platform specific artifact and extract to appropriate directory as before. For backward compatibility, if RELEASE is set to master, then bootstrap will behave as before but missing trace. Signed-off-by: Hasan Turken <turkenh@gmail.com>

Each crossplane object now has an IsReady method indicating its readiness. This information is also made avilable on simpleprinter with a sign (ready/notReady/missing) and on graph with colors (red missing, orange notReady). This could help to immediately identify what can be wrong. Signed-off-by: Hasan Turken <turkenh@gmail.com>

Load all auth plugins which makes the tool able to authenticate with cloud providers. /~https://github.com/kubernetes/client-go/tree/master/examples#auth-plugins Signed-off-by: Hasan Turken <turkenh@gmail.com>

Signed-off-by: Hasan Turken <turkenh@gmail.com>

Adds unit tests for fetchObj and BuildGraph in graphbuilder.go Also includes some related fixing and refactorings. Signed-off-by: Hasan Turken <turkenh@gmail.com>

Signed-off-by: Hasan Turken <turkenh@gmail.com>

turkenh · 2019-10-23T13:28:02Z

@suskin I still have something to do, but believe that PR is ready for review.

It would be great if you could have a chance to test on your side as well.

With v0.4 Portable classes being removed and resource classes, providers and managed resources are being changed as cluster scoped. This commitadds anticipated compatibility changes by still trying to keep it backward compatible with v0.3 Signed-off-by: Hasan Turken <turkenh@gmail.com>

suskin

Awesome - thanks @turkenh ! Since it's a larger pull request it's a little harder to do an in-depth review quickly, but it looks pretty good to me. I just had some minor questions and suggestions.

I'm going to approve this since we're in different timezones, but I'd like us to at a minimum address the comments about testing (add make test, verify that installation from a release has been tested) and usability before we merge the changes.

For posterity, I asked @turkenh in a separate conversation whether we would be able to use more of the code from kubectl - for example, for the kubectl get commands. He said that he looked, but that he wasn't able to find anything which was easily reusable. We both agreed that we'd like to revisit that in the future at some point, but that we're okay with doing a quick thing for now so we can start using the tool.

suskin · 2019-10-23T23:51:42Z

bootstrap.sh

@@ -10,13 +10,23 @@ if [[ -z "${RELEASE}" ]]; then
  RELEASE=master
 fi

+PLATFORM="linux"


@turkenh what platform are you running? I'm testing this on darwin.

I'm also using darwin, but just tested on a debian instance and working fine.

suskin · 2019-10-23T23:52:56Z

Makefile

 install:
-	ln -si $(abspath bin/kubectl-crossplane-stack-*) $(INSTALL_DIR)/
+	ln -si $(abspath bin/kubectl-crossplane-*) $(INSTALL_DIR)/
 .PHONY: install

 uninstall:


Is the .DS_Store file below needed? Should we remove it and .gitignore it?

No, definitely not needed. Deleted and added to .gitignore.

suskin · 2019-10-23T23:53:13Z

bootstrap.sh

+  curl -sL -o "${PREFIX}"/bin/kubectl-crossplane-stack-generate_install https://raw.githubusercontent.com/crossplaneio/crossplane-cli/"${RELEASE}"/bin/kubectl-crossplane-stack-generate_install >/dev/null
+  curl -sL -o "${PREFIX}"/bin/kubectl-crossplane-stack-list https://raw.githubusercontent.com/crossplaneio/crossplane-cli/"${RELEASE}"/bin/kubectl-crossplane-stack-list >/dev/null
+else
+  curl -sL /~https://github.com/crossplaneio/crossplane-cli/releases/download/"${RELEASE}"/crossplane-cli-"${RELEASE}"-"${PLATFORM}".tar.gz | tar xz --strip 1 -C "${PREFIX}"/bin


Has this been tested on linux and darwin?

Yes, works fine on both. Tested with my test releases: /~https://github.com/turkenh/crossplane-cli/releases/tag/v0.1.2-test

We need to follow the naming convention here with our releases.

crossplane-cli-"${RELEASE}"-"${PLATFORM}".tar.gz

suskin · 2019-10-23T23:56:26Z

bootstrap.sh

-curl -sL -o "${PREFIX}"/bin/kubectl-crossplane-stack-generate_install https://raw.githubusercontent.com/crossplaneio/crossplane-cli/"${RELEASE}"/bin/kubectl-crossplane-stack-generate_install >/dev/null
-curl -sL -o "${PREFIX}"/bin/kubectl-crossplane-stack-list https://raw.githubusercontent.com/crossplaneio/crossplane-cli/"${RELEASE}"/bin/kubectl-crossplane-stack-list >/dev/null
+if [[ "${RELEASE}" == "master" ]]; then
+  echo "trace subcommand will not be available from master, RELEASE must be set to a released version."


I have been taking the approach of sending progress and error messages to stderr instead; let's redirect this: >&2

Also, could we put some sort of attention-grabbing prefix at the beginning of the message, and add an example of a released version? For example:

echo "NOTICE: the trace command is not available from master. RELEASE must be set to a released version (such as v0.2.0). See /~https://github.com/crossplaneio/crossplane-cli/releases for the full list of releases." >&2

Both makes sense. Done.

suskin · 2019-10-23T23:57:57Z

bootstrap.sh

@@ -10,13 +10,23 @@ if [[ -z "${RELEASE}" ]]; then
  RELEASE=master
 fi

+PLATFORM="linux"
+if [[ "$OSTYPE" == "darwin"* ]]; then


Style nitpick: could we change this to use curly braces for consistency? So something more like "${OSTYPE}". I like using curly braces for clarity for scripts. In general, I like to use the long forms of syntax in scripts, since they're usually clearer.

suskin · 2019-10-24T00:08:11Z

pkg/crossplane/application.go

+)
+
+type Application struct {
+	u *unstructured.Unstructured


In general I like descriptive variable names over single-letter names, but if this is the go idiom for untyped and anonymous variables like this, I'm okay with it.

Go idiom likes short names but not sure this applies here.
Changed it as u -> instance which is already something that I used somewhere else in the code.

suskin · 2019-10-24T00:10:01Z

pkg/crossplane/application.go

+	return o.GetStatus() == stateAppSubmitted
+}
+
+func (o *Application) GetRelated(filterByLabel func(metav1.GroupVersionKind, string, string) ([]unstructured.Unstructured, error)) ([]*unstructured.Unstructured, error) {


Something to keep in mind for the future is that it'd be great if we could add support for more types without having to recompile the trace binary. We don't have to have a solution now, but it's something to think about.

Yes, that's a good point.
This is something that I also considered in the context of making this tool as a "general purpose" tracing tool as you mentioned in the crossplane/crossplane#744. This could be possible with some sort of configuration file which describes the types and their relations with other types.

suskin · 2019-10-24T00:12:26Z

pkg/crossplane/application_test.go

+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+)
+
+func TestApplication_GetObjectDetails(t *testing.T) {


Awesome! Could we also add a make test target for running the unit tests?

suskin · 2019-10-24T00:33:37Z

pkg/crossplane/application.go

+}
+
+func (o *Application) GetStatus() string {
+	return getNestedString(o.u.Object, fieldsStatusState...)


I usually prefer that the imports for shared functionality are explicit so they're easier to follow for readers who are not familiar with the codebase, but this is something we can probably reexamine in the future.

Well, if you mean getNestedString this is not coming from an import. it is a helper in the same package.
Otherwise, I agree with your comment in general.

suskin · 2019-10-24T00:39:11Z

pkg/trace/graphprinter.go

+}
+
+// Print prints graph definition in Graphviz dot format for a given root node.
+func (p *GraphPrinter) Print(nodes []*Node) error {


suskin · 2019-10-24T00:52:58Z

I forgot to mention, but I did some integration testing on my side as well. I used it to inspect the statuses for a mysqlinstance and a kubernetesapplication which I created as part of the Stacks guide. And I used it to watch the kubernetesapplication become ready over time. It was way better than doing those things without the tool!

As another note, we'll want to update some of the documentation - in this repo and in crossplane - to recommend that we use trace. The documentation will need to be updated before we consider the trace work to be complete.

Adds .DS_Store (a mac os system file) and produced binary to .gitignore. Signed-off-by: Hasan Turken <turkenh@gmail.com>

Attention grabbing message for installations from master and curly braces for variables. Signed-off-by: Hasan Turken <turkenh@gmail.com>

Changed as "instance" from "u" Signed-off-by: Hasan Turken <turkenh@gmail.com>

Signed-off-by: Hasan Turken <turkenh@gmail.com>

turkenh · 2019-10-24T09:11:25Z

@suskin Thanks a lot for reviewing and testing!
All of your comments should be resolved right now.

For posterity, I asked @turkenh in a separate conversation whether we would be able to use more of the code from kubectl - for example, for the kubectl get commands. He said that he looked, but that he wasn't able to find anything which was easily reusable. We both agreed that we'd like to revisit that in the future at some point, but that we're okay with doing a quick thing for now so we can start using the tool.

Most related parts from kubectl get was the ones for printing, since for interacting with api-server I used client-go dynamic client which was pretty good. I checked /~https://github.com/kubernetes/cli-runtime which could be helpful, but as @suskin mentioned, I found adding my own simple printer as more easy and flexible. I still agree that this should be something to revisit in future.

I forgot to mention, but I did some integration testing on my side as well. I used it to inspect the statuses for a mysqlinstance and a kubernetesapplication which I created as part of the Stacks guide. And I used it to watch the kubernetesapplication become ready over time. It was way better than doing those things without the tool!

Thanks a lot!
Which cloud provider were you using? I always used GCP and now looking for a way to test with others. Another this that needs to be test is the recent changes happening around simple resource class selection.

Adapt crossplane object to work with both v0.3 and v0.4 Signed-off-by: Hasan Turken <turkenh@gmail.com>

Signed-off-by: Hasan Turken <turkenh@gmail.com>

Previously overview was printed first and details second. But for long outputs, user first sees the bottom of details as console slided down. With this commit, overview will be what user sees first. Signed-off-by: Hasan Turken <turkenh@gmail.com>

With this change kubeconfig is selected with the same logic as kubectl. --kubeconfig flag overwrites all env KUBECONFIG selected otherwise default path (~/.kube/config) in homedir is none set Signed-off-by: Hasan Turken <turkenh@gmail.com>

suskin · 2019-10-24T17:37:53Z

Which cloud provider were you using?

I was using GCP, and I was also using Crossplane 0.3 instead of master

suskin · 2019-10-24T18:25:50Z

bootstrap.sh

@@ -10,13 +10,23 @@ if [[ -z "${RELEASE}" ]]; then
  RELEASE=master
 fi

+PLATFORM="linux"


suskin · 2019-10-24T18:29:01Z

pkg/trace/graphbuilder.go

+	err = g.fetchObj(root)
+	if kerrors.IsNotFound(err) {
+		return root, nil, errors.New(
+			fmt.Sprintf("Object to trace is not found: \"%s\" \"%s\" in namespace \"%s\"", groupRes, name, namespace))


Grammar nitpick: Object to trace not found or Object to trace was not found would be better. I would normally go with the Object to trace not found form

suskin · 2019-10-24T18:32:04Z

pkg/trace/graphbuilder_test.go

+				resource:          "kubernetescluster",
+			},
+			want: want{
+				err: errors.NewNotFound(schema.ParseGroupResource("kubernetesclusters.compute.crossplane.io"), "test"),


When I ran the unit tests, I got this:

--- FAIL: TestKubeGraphBuilder_BuildGraph (0.00s) --- FAIL: TestKubeGraphBuilder_BuildGraph/NotExist (0.00s) graphbuilder_test.go:306: g.BuildGraph(...): -want error, +got error: interface{}( - e`kubernetesclusters.compute.crossplane.io "test" not found`, + e`Object to trace is not found: "kubernetescluster" "test" in namespace "testnamespace"`, )

Fixed. I should get used to running make test before pushing :)

haha : ) - ideally we'll also set up some automated builds so we don't need to always remember

Previously it was shown as tabbed but on AWS and Azure resources value does not fit into colums which breaks layout Signed-off-by: Hasan Turken <turkenh@gmail.com>

From "Object to trace is not found" to "Object to trace not found" as per review comments. Signed-off-by: Hasan Turken <turkenh@gmail.com>

Full object reference from resource class to provider is not available. Only name of the provider there. To resolve the provider, there was a workaround in the code, which sets apiVersion of the provider as the apiVersion of the resource class. This assumption does not hold with v1beta1 versions recently introduced in stack-gcp. This commit fixes by setting group instead of apiVersion. Signed-off-by: Hasan Turken <turkenh@gmail.com>

suskin

Thanks @turkenh ! This looks good to me. I believe we'll need to do a couple more things to get a release up, but we can work on it after this pull request is merged.

turkenh force-pushed the trace-cli-in-go branch from 5296fed to d0db46e Compare October 21, 2019 13:32

suskin reviewed Oct 21, 2019

View reviewed changes

turkenh force-pushed the trace-cli-in-go branch from d0db46e to 27a07a5 Compare October 21, 2019 20:17

trace: add crossplane-cli go project

a8d41bf

First commit adding the golang project for trace cli Signed-off-by: Hasan Turken <turkenh@gmail.com>

turkenh force-pushed the trace-cli-in-go branch from e2560ae to a8d41bf Compare October 22, 2019 06:57

turkenh added 3 commits October 22, 2019 10:59

trace: refactor - define vars for accessing fields

654a173

We need the path of the fields for accessing the data in unstructured object. This commit defines and use some varaibles for this purpose. Signed-off-by: Hasan Turken <turkenh@gmail.com>

trace: add relation from provider to credsecret

4e80117

Now the cred secret used by a provider is also included into the trace. Signed-off-by: Hasan Turken <turkenh@gmail.com>

turkenh force-pushed the trace-cli-in-go branch from b1f9ebe to 7f56405 Compare October 22, 2019 08:38

turkenh force-pushed the trace-cli-in-go branch from b345529 to 046f83a Compare October 22, 2019 22:28

turkenh added 4 commits October 23, 2019 01:30

trace: refactor - simple printer with common template

c29094a

With this commit, simple printer uses ObjectDetails returned by crossplane objects and prints details with a common go template. Signed-off-by: Hasan Turken <turkenh@gmail.com>

trace: add help message

1d33767

Signed-off-by: Hasan Turken <turkenh@gmail.com>

turkenh force-pushed the trace-cli-in-go branch from 046f83a to 3c381b6 Compare October 22, 2019 23:39

turkenh added 6 commits October 23, 2019 11:48

trace: load all auth plugins

b95f01f

Load all auth plugins which makes the tool able to authenticate with cloud providers. /~https://github.com/kubernetes/client-go/tree/master/examples#auth-plugins Signed-off-by: Hasan Turken <turkenh@gmail.com>

trace: add build and clean makefile targets

81ecca6

Signed-off-by: Hasan Turken <turkenh@gmail.com>

trace: add unit tests from graphbuilder

135a2c9

Adds unit tests for fetchObj and BuildGraph in graphbuilder.go Also includes some related fixing and refactorings. Signed-off-by: Hasan Turken <turkenh@gmail.com>

trace: unit test for build graph with one reference

3576de8

Signed-off-by: Hasan Turken <turkenh@gmail.com>

trace: add comments for exporteds in trace package

77db799

Signed-off-by: Hasan Turken <turkenh@gmail.com>

turkenh marked this pull request as ready for review October 23, 2019 13:20

turkenh requested a review from suskin October 23, 2019 13:24

suskin approved these changes Oct 24, 2019

View reviewed changes

add .gitignore

91ca5f1

Adds .DS_Store (a mac os system file) and produced binary to .gitignore. Signed-off-by: Hasan Turken <turkenh@gmail.com>

turkenh added 3 commits October 24, 2019 11:32

bootsrap: style and message improvements

fd757be

Attention grabbing message for installations from master and curly braces for variables. Signed-off-by: Hasan Turken <turkenh@gmail.com>

trace: rename unstructured variables to more descriptive

2b77868

Changed as "instance" from "u" Signed-off-by: Hasan Turken <turkenh@gmail.com>

makefile: add test target for go unit tests

387a2ea

Signed-off-by: Hasan Turken <turkenh@gmail.com>

turkenh added 4 commits October 24, 2019 15:39

trace: fixes for simple class selection

d945da1

Adapt crossplane object to work with both v0.3 and v0.4 Signed-off-by: Hasan Turken <turkenh@gmail.com>

trace: better error message in case object not found

0c735d3

Signed-off-by: Hasan Turken <turkenh@gmail.com>

trace: respect KUBECONFIG env var

b87f7b8

With this change kubeconfig is selected with the same logic as kubectl. --kubeconfig flag overwrites all env KUBECONFIG selected otherwise default path (~/.kube/config) in homedir is none set Signed-off-by: Hasan Turken <turkenh@gmail.com>

turkenh mentioned this pull request Oct 24, 2019

add initial one pager for relationship labels crossplane/crossplane#961

Merged

suskin reviewed Oct 24, 2019

View reviewed changes

suskin self-requested a review October 24, 2019 18:35

turkenh added 3 commits October 24, 2019 21:56

trace: print additional status details one per line

73e515f

Previously it was shown as tabbed but on AWS and Azure resources value does not fit into colums which breaks layout Signed-off-by: Hasan Turken <turkenh@gmail.com>

trace: change wording in error message and fix unit test

aabf8ae

From "Object to trace is not found" to "Object to trace not found" as per review comments. Signed-off-by: Hasan Turken <turkenh@gmail.com>

suskin approved these changes Oct 24, 2019

View reviewed changes

suskin self-assigned this Oct 24, 2019

suskin merged commit 1d93603 into crossplane:master Oct 24, 2019

Trace cli in go #15

Trace cli in go #15

Conversation

turkenh commented Oct 21, 2019 • edited Loading

suskin 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

turkenh commented Oct 21, 2019

turkenh commented Oct 23, 2019

suskin 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

turkenh Oct 24, 2019 • edited Loading

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

turkenh Oct 24, 2019 • edited Loading

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

suskin commented Oct 24, 2019

turkenh commented Oct 24, 2019

suskin commented Oct 24, 2019

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

suskin left a comment

Choose a reason for hiding this comment

turkenh commented Oct 21, 2019 •

edited

Loading

turkenh Oct 24, 2019 •

edited

Loading

turkenh Oct 24, 2019 •

edited

Loading