CLI: Introduced `identity` command to fetch tls-certificates for a pod by jimil749 · Pull Request #5408 · linkerd/linkerd2

jimil749 · 2020-12-18T16:44:02Z

CLI: Introduced identity command to fetch tls-certificates for a pod (#4459)

Modified and added a new cli command, which initiates a sni-tls session to the proxy's admin port and returns the certificate.

Usage:

linkerd identity pod/<pod-name> : fetches certificate from the specified pod
linkerd identity -n emojivoto : fetches certificate from all pods in the emojivoto namespace
linkerd identity -l app=svc/emoji : fetches certificate from all pods with label app=svc/emoji

Currently, I am not a 100% sure of the way I am outputting the certificates, I'll change that if needed, but overall these commands do fetch the required certificates.

Screenshot:

I would be happy to work upon any changes suggested

adleong · 2020-12-21T19:59:26Z

This is so awesome. 😱

Can't wait to test this out

jimil749 · 2020-12-24T18:24:59Z

@adleong, did you get the chance to review/test this?

cpretzer · 2020-12-24T18:48:03Z

@jimil749 I had a chance to run some basic tests and this looks good! Thanks for your hard work.

One thing that I noticed is that the command doesn't write any output when Linkerd isn't installed in the cluster.

I had a new kind cluster running without Linkerd installed, and when I ran the linkerd identity ... command, the command returned with no output or indication that anything happened.

In contrast, the linkerd stat deploy and linkerd edges deploy commands have this output when the Linkerd control plane is not deployed:

$ linkerd stat deploy
Cannot find Linkerd: configmaps "linkerd-config" not found
Validate the install with: linkerd check

What do you think about adding a check to make sure that linkerd is installed?

jimil749 · 2020-12-25T05:36:34Z

@cpretzer, thanks for the feedback, I'll do the needful. :)

Signed-off-by: Jimil Desai <jimildesai42@gmail.com>

linkerd#4459) Modified and added a new cli command, which initiates a sni-tls session to the proxy's admin port and returns the certificate. Usage: - linkerd identity pod/<pod-name> : fetches certificate from the specified pod - linkerd identity -n emojivoto : fetches certificate from all pods in the emojivoto namespace - linkerd identity -l app=svc/emoji : fetches certificate from all pods with label app=svc/emoji Signed-off-by: Jimil Desai <jimildesai42@gmail.com>

Signed-off-by: Jimil Desai <jimildesai42@gmail.com>

cpretzer · 2020-12-25T05:42:56Z

👍

jimil749 · 2020-12-25T12:17:22Z

@cpretzer I think the reason we aren't getting any output is because we do not have the proxy sidecar container injected. The metrics command behaves the same way as identity (returns with no output or any indication)! I am not using the control plane APIs to fetch the certs, I am simply establishing a sni-tls connection to the proxy's admin port to do so. The check for whether the control plane is installed or not is done while building a new public API client (in case of stat and edges), which is not required in my case. I am currently not a 100% sure of how to add the checks. I think checking whether the service is meshed i.e the proxy exists would be a better check.

cpretzer · 2020-12-25T23:29:32Z

@jimil749 I see what you're saying. The edges and stats commands use the checkPublicAPIClientOrExit function

Perhaps the correct approach is similar to the one used by the uninstall command, which checks whether the linkerd namespace exists and exits if it does not.

Signed-off-by: Jimil Desai <jimildesai42@gmail.com>

jimil749 · 2020-12-27T16:21:06Z

@cpretzer, Sorry for the delayed response, but I've added the check for the linkerd control plane but I wasn't able to test the command entirely, because I am having an error in installing the control plane in my cluster after fetching the latest commit, which I think is because of this, I am getting the same error as in this comment. Not a 100% sure, but is that the reason the tests are failing?

cpretzer · 2020-12-28T18:32:05Z

hi @jimil749 thanks for adding the check for the namespace. We're working on getting the build fixed, and once it is, we can run the tests again.

adleong

I played with this a little bit and it's pretty great.

I like the linkerd identity (pod-name) mode, though I think we should drop the resource type or at least make it optional since only pods are supported. i.e. instead of linkerd identity pod/linkerd-destination-6887bb8f49-h8qvr it should be linkerd identity linkerd-destination-6887bb8f49-h8qvr

I also like the -l selector mode.

I found the third mode where it prints all certificates in a namespace to be confusing and I think we can remove that.

I also looked into certificate formatting a bit more and found https://github.com/smallstep/certinfo. I think we should use this library for formatting the certificates.

jimil749 · 2021-01-05T09:37:06Z

I like the linkerd identity (pod-name) mode, though I think we should drop the resource type or at least make it optional since only pods are supported. i.e. instead of linkerd identity pod/linkerd-destination-6887bb8f49-h8qvr it should be linkerd identity linkerd-destination-6887bb8f49-h8qvr

Yeah, makes sense. Will do the necessary changes!

I found the third mode where it prints all certificates in a namespace to be confusing and I think we can remove that.

The reason to do was this comment, where it is mentioned that a command like linkerd -n ns identity can be used to get certificates for all pods in the namespace. I will remove the command if it is not required as such

I also looked into certificate formatting a bit more and found https://github.com/smallstep/certinfo. I think we should use this library for formatting the certificates.

I will take a look at the library. 👍

Summary of changes: - Fetching the certificates in series rather than parallel. - URL trim using `strings.Trim` for `tls.Dial()` - Dropped the resource type from `linkerd identity` command - Removed the `linkerd -n ns` subcommand, which used to fetch certificates for all pods in the provided ns - Used `grantae/certinfo` library to print the certificates in a format similar to OpenSSL - Removed RootCAs field from tls.Dial Signed-off-by: Jimil Desai <jimildesai42@gmail.com>

jimil749 · 2021-01-05T17:44:46Z

@adleong, I've addressed the changes proposed, here's the summary of the changes:

Fetching the certificates in series rather than parallel.
URL trim using strings.Trim for tls.Dial() (instead of extracting the portno from the URL)
Dropped the resource type from linkerd identity command
Removed the linkerd -n ns subcommand, which used to fetch certificates for all pods in the provided ns
Used grantae/certinfo library to print the certificates in a format similar to OpenSSL
Removed RootCAs field from tls.Dial

Signed-off-by: Jimil Desai <jimildesai42@gmail.com>

adleong

This is so awesome! I'm really excited for this to land. 😻

- Added check for un-meshed pod - Print pod name for multiple certificates - Removed control-plane check and PEM encoding Signed-off-by: Jimil Desai <jimildesai42@gmail.com>

jimil749 · 2021-01-05T20:08:01Z

@adleong, I made the changes as suggested. But I was not sure of what exact error message to print out when no certificates are returned. Maybe you could suggest a better message to output?

kleimkuhler

@jimil749 This is super cool thanks for getting this change up!

I've left a few smaller comments focused around formatting help text and such.

There are a few comments though that are more significant. Let me know if you have any questions about those I'd be glad to clarify a little more!

kleimkuhler · 2021-01-06T15:34:23Z

+	if len(arg) > 0 {
+		pod, err := clientset.CoreV1().Pods(namespace).Get(ctx, arg[0], metav1.GetOptions{})
+		if err != nil {
+			return nil, err
+		}
+		return []corev1.Pod{*pod}, nil
+	}


So I run the command linkerd identity -n linkerd linkerd-identity-xyz linkerd-destination-xyz.

My expectation for the output would be two certificate outputs, but with only getting the pod arg[0] I only see one.

I think if there len(arg) > 0, then we would want to loop through arg (whether it's 1 or greater) and return that slice of pods.

Oh alright. To be honest, I had not thought about such a situation, I was under the impression that we just want to fetch cert for a single pod. I will make the required changes! :)

kleimkuhler · 2021-01-06T15:57:51Z

+	for _, c := range pod.Spec.Containers {
+		for _, p := range c.Ports {
+			if p.Name == portName {
+				containers = append(containers, c)
+			}
+		}
+	}


We expect the admin port to be on a linkerd-proxy container right? Can we loop through the containers until we have a container name that matches linkerd-proxy (k8s.ProxyContainerName) and then loop through the ports?

Also, once we have the port we're looking for we can stop looping through both the containers and then ports. We have something similar to this that you can look at here!

adleong

Thanks for updating, @jimil749!

Signed-off-by: Jimil Desai <jimildesai42@gmail.com>

jimil749 · 2021-01-06T19:00:33Z

@kleimkuhler, I've made the required changes!

kleimkuhler

This looks great! I have one more suggestion to just make sure errors are printing as expected.

After that it should be good. Sorry I did not catch this in the previous review!

Signed-off-by: Jimil Desai <jimildesai42@gmail.com>

kleimkuhler · 2021-01-06T20:12:14Z

Thanks for addressing all these reviews @jimil749!

jimil749 requested a review from a team as a code owner December 18, 2020 16:44

jimil749 force-pushed the new-branch branch from 13aeb60 to 87a0299 Compare December 18, 2020 17:29

jimil749 added 5 commits December 25, 2020 11:09

identity cert: for a single pod

39a44a6

Signed-off-by: Jimil Desai <jimildesai42@gmail.com>

refractored the code a bit

e93d449

Signed-off-by: Jimil Desai <jimildesai42@gmail.com>

identity command

1251dd8

Signed-off-by: Jimil Desai <jimildesai42@gmail.com>

fixed lint error

c80810c

Signed-off-by: Jimil Desai <jimildesai42@gmail.com>

check for linkerd control plane

cfa1985

Signed-off-by: Jimil Desai <jimildesai42@gmail.com>

jimil749 force-pushed the new-branch branch from 1086b74 to cfa1985 Compare December 26, 2020 16:36

adleong reviewed Jan 4, 2021

View reviewed changes

Comment thread cli/cmd/identity.go Outdated

Comment thread cli/cmd/identity.go Outdated

Comment thread cli/cmd/identity.go Outdated

removed extra argument from getCertificate

4a10334

Signed-off-by: Jimil Desai <jimildesai42@gmail.com>

adleong reviewed Jan 5, 2021

View reviewed changes

Comment thread cli/cmd/identity.go Outdated

Comment thread cli/cmd/identity.go Outdated

Comment thread cli/cmd/identity.go Outdated

Comment thread cli/cmd/identity.go

Refractored the code

ed9aafb

- Added check for un-meshed pod - Print pod name for multiple certificates - Removed control-plane check and PEM encoding Signed-off-by: Jimil Desai <jimildesai42@gmail.com>

kleimkuhler reviewed Jan 6, 2021

View reviewed changes

adleong reviewed Jan 6, 2021

View reviewed changes

Comment thread cli/cmd/identity.go Outdated

adleong approved these changes Jan 6, 2021

View reviewed changes

jimil749 added 2 commits January 6, 2021 23:52

code refractor

73a8e55

Signed-off-by: Jimil Desai <jimildesai42@gmail.com>

fix lint

15680b1

Signed-off-by: Jimil Desai <jimildesai42@gmail.com>

kleimkuhler reviewed Jan 6, 2021

View reviewed changes

Comment thread cli/cmd/identity.go Outdated

Comment thread cli/cmd/identity.go Outdated

error formatting

dd76b56

Signed-off-by: Jimil Desai <jimildesai42@gmail.com>

kleimkuhler approved these changes Jan 6, 2021

View reviewed changes

kleimkuhler merged commit bce3547 into linkerd:main Jan 6, 2021

jimil749 deleted the new-branch branch January 8, 2021 17:37

Pothulapati mentioned this pull request Mar 17, 2021

cli: identity cert <pod-selection> #4459

Closed

Uh oh!

Conversation

jimil749 commented Dec 18, 2020

Uh oh!

adleong commented Dec 21, 2020

Uh oh!

jimil749 commented Dec 24, 2020

Uh oh!

cpretzer commented Dec 24, 2020

Uh oh!

jimil749 commented Dec 25, 2020

Uh oh!

cpretzer commented Dec 25, 2020

Uh oh!

jimil749 commented Dec 25, 2020

Uh oh!

cpretzer commented Dec 25, 2020

Uh oh!

jimil749 commented Dec 27, 2020

Uh oh!

cpretzer commented Dec 28, 2020

Uh oh!

adleong left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jimil749 commented Jan 5, 2021

Uh oh!

jimil749 commented Jan 5, 2021

Uh oh!

adleong left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jimil749 commented Jan 5, 2021

Uh oh!

kleimkuhler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kleimkuhler Jan 6, 2021

Choose a reason for hiding this comment

Uh oh!

jimil749 Jan 6, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kleimkuhler Jan 6, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

adleong left a comment

Choose a reason for hiding this comment

Uh oh!

jimil749 commented Jan 6, 2021

Uh oh!

kleimkuhler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kleimkuhler commented Jan 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!