Add kubectl plugin #3779

alexkursell · 2019-02-19T20:44:57Z

What this PR does / why we need it: This PR adds a kubectl plugin, kubectl ingress-nginx, that makes it easier to debug ingress-nginx. In the future, this plugin should also get the ability to add/remove/configure ingress-nginx deployments in a cluster (see https://gist.github.com/aledbf/db25dbc723fc7d71345a9657abfd766d). Right now, it just includes debugging functionality.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #3755

Current features

kubectl ingress-nginx backends, which basically can query the output of /dbg backends inside ingress-nginx pods
kubectl ingress-nginx general, kubectl ingress-nginx conf, which do the same but for /dbg general and /dbg conf
kubectl ingress-nginx ingress which tries to output a table similar to kubectl get ingress, but with richer information including hostnames, paths, whether or not a rule is protected by TLS, and backend name.
kubectl ingress-nginx info Pretty sparse right now, just prints the internal and external addresses of the ingress-nginx deployment.

Packaging

The plugin binaries + krew manifest can be built using make build-plugin. Right now it builds for Linux, MacOS, and Windows, although I've only tested it on MacOS. Distributing the plugin would look something like this:

Before an ingress-nginx release, run make build-plugin and commit the generated cmd/plugin/release directory.
Add the binary tarballs to the github release (such as https://github.com/kubernetes/ingress-nginx/releases/tag/nginx-0.22.0)
Make the release.
Make a PR in https://github.com/GoogleContainerTools/krew-index/tree/master/plugins with the new generated ingress-nginx.yaml manifest

To-do

Right now, cloud authentication (like with GCP, AWS, etc.) is broken. It works when using the latest master versions of the various kubernetes dependencies, but not with the 1.13.3 versions we have vendored now. I'm still trying to fix this. (edit: fixed by Update mergo dependency #3793)
Give the plugin the ability to actually manage ingress-nginx deployments as in https://gist.github.com/aledbf/db25dbc723fc7d71345a9657abfd766d. Although this may not be necessary for an MVP (edit: punting to a future PR)
Documentation (edit: punting to a future PR)
Handle ingress-nginx deployments that have names other than ingress-nginx (edit: punting to a future PR)
Any other suggested improvements
Testing (edit: punting to a future PR)

cc: @ElvinEfendi @aledbf

aledbf · 2019-02-19T20:52:10Z

@alexkursell thank you for working on this!

aledbf · 2019-02-19T22:36:48Z

cmd/plugin/request/request.go

+
+const (
+	ingressPodName     = "nginx-ingress-controller"
+	ingressServiceName = "nginxcontrib-ingress"


ingressServiceName = ingress-nginx

Ah, true. I was testing on a cluster that changed the name of the service. I wonder if there is a way to "detect" an ingress-nginx deployment or service regardless of the name? Or maybe just adding a config flag would be better. Probably just adding a flag considering multiple ingress-nginx deployments or services can exist at a time.

Assume the default ingress-nginx/use the labels https://github.com/kubernetes/ingress-nginx/blob/master/deploy/provider/cloud-generic.yaml#L13
Those labels could be used to find the service/s and pod/s

aledbf · 2019-02-19T22:53:51Z

@alexkursell can we dump the lua tables? (sticky session state for instance)

alexkursell · 2019-02-19T23:06:18Z

@alexkursell can we dump the lua tables? (sticky session state for instance)

That is on my list, along with dynamic cert info. I just have to add endpoints and a /dbg subcommand first. I'll add those to this PR as well when I get the time.

ElvinEfendi · 2019-02-20T04:42:53Z

cmd/plugin/main.go

+			return nil
+		},
+	}
+	backendsCmd.Flags().String("pod", "", "Query a particular ingress-nginx pod")


What happens when we don't specify this?

oh nevermind, https://github.com/kubernetes/ingress-nginx/pull/3779/files#diff-99cbd32a1c30f61ec2f0f0b744cf1f35R55

alexkursell · 2019-02-20T18:52:57Z

@alexkursell can we dump the lua tables? (sticky session state for instance)

That is on my list, along with dynamic cert info. I just have to add endpoints and a /dbg subcommand first. I'll add those to this PR as well when I get the time.

So regarding dumping lua tables, the only ones that are shared between all workers are the configuration_data and certificate_data tables. configuration_data is already exposed by /dbg backends and /dbg general, and I'll be adding certs shortly. However, things like load balancing state are stored per worker, and aren't accessible as shared dictionaries.

aledbf · 2019-02-20T18:55:04Z

However, things like load balancing state are stored per worker, and aren't accessible as shared dictionaries.

@ElvinEfendi any idea how can we dump this? (this could help with some of the issues with affinity)

aledbf · 2019-02-20T18:57:51Z

@alexkursell @ElvinEfendi can we think of adding redis to store some of the states as an option? (like the affinity). Also the rate limiting could benefit from this to have a global state and not per ingress controller pod

ElvinEfendi · 2019-02-20T23:01:57Z

@aledbf I suggest we stick to existing minimal debugging features provided by dbg executable and ship the plugin first. We can then iterate and add more commands.

can we think of adding redis to store some of the states as an option?

Please no redis :) It's a great advantage of ingress-nginx to not rely on any other storage component. What kind of states are you looking to get i.e in the case of session affinity debugging?

ElvinEfendi · 2019-02-20T23:04:17Z

kubectl ingress-nginx ingress which tries to output a table similar to kubectl get ingress, but with richer information including hostnames, paths, whether or not a rule is protected by TLS, and backend name

how's this different that kubectl describe ingress?

ElvinEfendi · 2019-02-20T23:07:30Z

@alexkursell can we ship the first version without any of the TODOs you mentioned?

Right now, cloud authentication (like with GCP, AWS, etc.) is broken. It works when using the latest master versions of the various kubernetes dependencies, but not with the 1.13.3 versions we have vendored now. I'm still trying to fix this.

So if I pull your branch and build and install the plugin as you documented it won't work? What's the actual issue, maybe @aledbf can help with?

aledbf · 2019-02-20T23:10:27Z

So if I pull your branch and build and install the plugin as you documented it won't work? What's the actual issue, maybe @aledbf can help with?

krew install --manifest ingress-nginx.yaml --archive kubectl-ingress_nginx-linux-amd64.tar.gz worked for me

aledbf · 2019-02-20T23:14:55Z

kubectl ingress-nginx ingresses --all-namespaces

how's this different that kubectl describe ingress?

We get more information:

NAMESPACE        INGRESS NAME   HOST+PATH                  ADDRESSES                       TLS   SERVICE   PORT
api              rocket-pi     rocket-science.io/push  XXXXXX.us-west-2.elb.amazonaws.com   NO    api-api 3000

aledbf · 2019-02-20T23:16:24Z

What kind of states are you looking to get i.e in the case of session affinity debugging?

This is not related to debugging itself but an additional (optional) feature. Right now when we use rate limits or session affinity that only applies per pod, not to all the replicas in the deployment

ElvinEfendi · 2019-02-20T23:48:26Z

Right now when we use rate limits or session affinity that only applies per pod, not to all the replicas in the deployment

I see, it's a bit offtopic for this PR, but session affinity does not care about state because it's implemented using consistent hashing. So every Nginx replicas will behave the same. As to rate limit, yeah that's per replica/pod. I feel like this would be a great fit to be implemented as a plugin.

alexkursell · 2019-02-21T15:38:29Z

@aledbf Right now, the plugin fails to authenticate on gcp clusters with the error ingresses.extensions is forbidden: User "system:anonymous" cannot list ingresses.extensions in the namespace "default". This error does not occur when using the latest master versions of the various kubernetes dependencies, only when using the ingress-nginx vendored dependencies. I've looked and it seems like the authentication plugin isn't managing to add the gcp credentials to the parsed settings. Have you seen anything like this error before?

@alexkursell can we ship the first version without any of the TODOs you mentioned?

@ElvinEfendi I'd like to resolve the above issue, as right now the plugin is totally useless to anyone who uses gcp. As for e2e-tests and documentation, I suppose it's up to you and @aledbf how complete we want to be in this regard before merging. But in terms of the extra features listed there, we can definitely leave them to another PR.

aledbf · 2019-02-21T15:59:31Z

Right now, the plugin fails to authenticate on gcp clusters with the error ingresses.extensions is forbidden: User "system:anonymous" cannot list ingresses.extensions in the namespace "default"

Please check https://github.com/kubernetes/client-go/tree/master/examples#auth-plugins

aledbf · 2019-02-21T16:11:32Z

@alexkursell @ElvinEfendi I am fine with the state of this PR as the first version.
That said, I would not open a PR in krew (yet) and just provide the command to install the plugin from the next github release.

ElvinEfendi · 2019-02-21T18:27:22Z

That said, I would not open a PR in krew (yet) and just provide the command to install the plugin from the next github release.

Sounds like a plan! That'll give us a chance to polish the plugin and make it more useful for large audience.

alexkursell · 2019-02-25T20:20:02Z

I think that this is ready for merge unless @ElvinEfendi or @aledbf think there is something else that needs to be done.

ElvinEfendi · 2019-02-25T20:35:35Z

build/build-plugin.sh

+
+
+
+


Can you delete these redundant lines?

fixed. I should probably look into some linting tool that detects trailing newlines in text files.

ElvinEfendi · 2019-02-25T20:35:46Z

cmd/plugin/ingress-nginx.yaml.tmpl

+        os: windows
+        arch: amd64
+
+


ElvinEfendi · 2019-02-25T20:40:00Z

@alexkursell can you also add minimal documentation on how to install and use this?

Then squash the commits and we should be good to merge.

fejta-bot · 2019-02-25T20:45:24Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

ElvinEfendi · 2019-02-25T20:46:57Z

This PR may require API review.

I don't think it does - @aledbf is this safe to ignore?

aledbf · 2019-02-25T20:59:50Z

is this safe to ignore?

Yes (at least for now)

alexkursell · 2019-02-25T21:41:12Z

is this safe to ignore?

Yes (at least for now)

In that case I guess we're good to merge!

ElvinEfendi · 2019-02-25T22:08:38Z

/approved

I'll let @aledbf to lgtm.

ElvinEfendi · 2019-02-25T22:09:08Z

/approve

aledbf · 2019-02-26T01:39:17Z

/lgtm

k8s-ci-robot · 2019-02-26T01:39:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, alexkursell, ElvinEfendi

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:

~~OWNERS~~ [ElvinEfendi,aledbf]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vasrem · 2019-02-28T11:48:40Z

Nice work with this. I just wanted to put a comment here regarding the certs command because it took me sometime to find this out. It doesn't work if you don't have the flag --enable-dynamic-certificates set in the deployment.

aledbf · 2019-02-28T11:53:17Z

It doesn't work if you don't have the flag --enable-dynamic-certificates set in the deployment

Apologies for this but this is on purpose. Starting in 0.24 dynamic certificates will be the default, and the static mode is going to be removed in 0.25.

k8s-ci-robot requested review from aledbf and thockin February 19, 2019 20:45

aledbf reviewed Feb 19, 2019

View reviewed changes

ElvinEfendi reviewed Feb 20, 2019

View reviewed changes

alexkursell force-pushed the dbg-kubectl branch from 6a3df5d to 5f8a239 Compare February 20, 2019 20:17

This was referenced Feb 21, 2019

Update mergo dependency #3793

Merged

Add /dbg certs command #3799

Merged

aledbf added this to In Progress in 0.23.0 Feb 25, 2019

alexkursell force-pushed the dbg-kubectl branch 3 times, most recently from 38e2196 to 864a85e Compare February 25, 2019 19:08

alexkursell changed the title ~~[WIP] Add kubectl plugin~~ Add kubectl plugin Feb 25, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 25, 2019

ElvinEfendi reviewed Feb 25, 2019

View reviewed changes

Add kubectl plugin

9e424a4

alexkursell force-pushed the dbg-kubectl branch from ce772ab to 9e424a4 Compare February 25, 2019 20:54

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 25, 2019

k8s-ci-robot assigned aledbf Feb 26, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 26, 2019

k8s-ci-robot merged commit 2fcbcbc into kubernetes:master Feb 26, 2019

aledbf moved this from In Progress to done in 0.23.0 Feb 26, 2019

alexkursell deleted the dbg-kubectl branch February 26, 2019 15:02

Add kubectl plugin #3779

Add kubectl plugin #3779

Conversation

alexkursell commented Feb 19, 2019 • edited

Current features

Packaging

To-do

aledbf commented Feb 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aledbf Feb 19, 2019 • edited

Choose a reason for hiding this comment

aledbf commented Feb 19, 2019

alexkursell commented Feb 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexkursell commented Feb 20, 2019

aledbf commented Feb 20, 2019

aledbf commented Feb 20, 2019 • edited

ElvinEfendi commented Feb 20, 2019

ElvinEfendi commented Feb 20, 2019

ElvinEfendi commented Feb 20, 2019

aledbf commented Feb 20, 2019

aledbf commented Feb 20, 2019

aledbf commented Feb 20, 2019

ElvinEfendi commented Feb 20, 2019

alexkursell commented Feb 21, 2019 • edited

aledbf commented Feb 21, 2019

aledbf commented Feb 21, 2019

ElvinEfendi commented Feb 21, 2019

alexkursell commented Feb 25, 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

ElvinEfendi commented Feb 25, 2019

fejta-bot commented Feb 25, 2019

ElvinEfendi commented Feb 25, 2019

aledbf commented Feb 25, 2019

alexkursell commented Feb 25, 2019

ElvinEfendi commented Feb 25, 2019

ElvinEfendi commented Feb 25, 2019

aledbf commented Feb 26, 2019

k8s-ci-robot commented Feb 26, 2019

vasrem commented Feb 28, 2019

aledbf commented Feb 28, 2019

alexkursell commented Feb 19, 2019 •

edited

aledbf Feb 19, 2019 •

edited

aledbf commented Feb 20, 2019 •

edited

alexkursell commented Feb 21, 2019 •

edited