Add dapr annotate command

[jjcollinge]

Description

Add a command to automatically inject the dapr annotations into a Kubernetes resource configuration as a low friction way to add dapr to existing resources.

This implementation is inspired by the linkerd CLI but has different behavior and less features.

Currently the command is just a little helper to work with the sidecar-injector (only adds annotations) - in theory we could pull out some of the sidecar patching logic and use this as a way to create static manifests for deployment that won't need the sidecar-injector but that is beyond the scope of this PR.

Usage

Add dapr annotations into a Kubernetes configuration. Supported platforms: Kubernetes

Usage:
  dapr annotate [flags] CONFIG-FILE

Examples:

# Inject the first deployment found in the input
kubectl get deploy -l app=node -o yaml | dapr annotate - | kubectl apply -f -

# Inject multiple deployments by name in a chain
kubectl get deploy -o yaml | dapr annotate -r nodeapp - | dapr annotate -r pythonapp | kubectl apply -f -

# Inject deployment in a specific namespace from file or directory by name
dapr annotate -r nodeapp -n namespace mydeployment.yml | kubectl apply -f -

# Inject deployment from url by name
dapr annotate -r nodeapp --log-level debug https://raw.githubusercontent.com/dapr/quickstarts/master/hello-kubernetes/deploy/node.yaml | kubectl apply -f -

----------------------------------------
WARNING: If an app id is not provided, we will generate one using the format '<namespace>-<kind>-<name>'.
----------------------------------------


Flags:
      --api-token-secret string         The secret to use for the API token
  -a, --app-id string                   The app id to annotate
      --app-max-concurrency int         The maximum number of concurrent requests to allow (default -1)
  -p, --app-port int                    The port to expose the app on (default -1)
      --app-protocol string             The protocol to use for the app
      --app-ssl                         Enable SSL for the app
      --app-token-secret string         The secret to use for the app token
  -c, --config string                   The config file to inject
      --cpu-limit string                The CPU limit to set
      --cpu-request string              The CPU request to set
      --dapr-image string               The image to use for the dapr sidecar container
      --enable-debug                    Enable debug
      --enable-metrics                  Enable metrics
      --enable-profile                  Enable profiling
      --env string                      Environment variables to set (key value pairs, comma separated)
      --graceful-shutdown-seconds int   The number of seconds to wait for the app to shutdown (default -1)
  -h, --help                            help for inject
      --http-stream-request-body        Enable streaming request body for HTTP
      --listen-addresses string         The addresses to listen on
      --liveness-probe-delay int        The delay to use for the liveness probe (default -1)
      --liveness-probe-period int       The period to use for the liveness probe (default -1)
      --liveness-probe-threshold int    The threshold to use for the liveness probe (default -1)
      --liveness-probe-timeout int      The timeout to use for the liveness probe (default -1)
      --log-level string                The log level to use
      --max-request-body-size int       The maximum request body size to use (default -1)
      --memory-limit string             The memory limit to set
      --memory-request string           The memory request to set
      --metrics-port int                The port to expose the metrics on (default -1)
  -n, --namespace string                The namespace the resource target is in (can only be set if --resource is also set)
      --readiness-probe-delay int       The delay to use for the readiness probe (default -1)
      --readiness-probe-period int      The period to use for the readiness probe (default -1)
      --readiness-probe-threshold int   The threshold to use for the readiness probe (default -1)
      --readiness-probe-timeout int     The timeout to use for the readiness probe (default -1)
  -r, --resource string                 The resource to target for injection

Global Flags:
      --log-as-json   Log output in JSON format

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

This implementation is based on the linkerd implementation although has different behavior and only adds annotations.

Please reference the issue this PR will close: #870

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

• Code compiles correctly
• Created/updated tests
• Extended the documentation

[codecov]

Codecov Report

Merging #873 (9d71021) into master (8e9c675) will increase coverage by 7.59%.
The diff coverage is 75.39%.

@@            Coverage Diff             @@
##           master     #873      +/-   ##
==========================================
+ Coverage   21.66%   29.25%   +7.59%     
==========================================
  Files          33       35       +2     
  Lines        1925     2242     +317     
==========================================
+ Hits          417      656     +239     
- Misses       1453     1512      +59     
- Partials       55       74      +19     

Impacted Files	Coverage Δ
pkg/kubernetes/annotator.go	67.50% <67.50%> (ø)
pkg/kubernetes/annotator_config.go	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e9c675...9d71021. Read the comment docs.

[yaron2]

@jjcollinge the Dapr app-id annotation is required for Dapr - if I understand correctly, the current implementation will not set an app-id unless specified explicitly which will result in a failed deployment.

this raises the question of whether we require an app-id flag or default to a value (and if so, what would it be).

[jjcollinge]

@yaron2 yes you are correct. I see 3 plausible options:

1. if the appId is not provided and not present then error. This is a more clear behaviour for the user but does require additional effort on their behalf.

2. if the appId is not provided and not present then we generate a new one using a "unique" name generator similar to docker. This is lower friction for the user but will mean the appId is less meaningful i.e. nodeapp vs erratic_moose and unlike docker a Dapr app id is used by clients so should include some identifiable information.

3. If the appId is not provided and not present then use some data from the manifest such as metadata.name. This is probably the most intuitive, however, we could see conflicts if multiple resource kinds have the same name or multiple resources exist with the same name in different namespaces. So we'd have to prepend the kind and namespace e.g. <namespace>-<kind>-<name>, etc. This does leak some of the platform (namespace, kind) into the user's app.

I'd think option 3 will probably give the best user experience even if the appid is a little verbose and leaky as this command only works with K8s anyway. @yaron2 WDYT?

[jjcollinge]

@yaron2 I went ahead and implemented option 3 just to test it out. This testdata shows you what you'd expect from a Pod definition with an autogenerated app id. I also added the ability to target a specific resource in a specific namespace (in case you have duplicate names). However, this doesn't allow you to specify a specific kind of resource with a specific name in a specific namespace as I think it's pretty unlikely you have that level of conflict in your K8 manifests.

[yaron2]

@yaron2 I went ahead and implemented option 3 just to test it out. This testdata shows you what you'd expect from a Pod definition with an autogenerated app id. I also added the ability to target a specific resource in a specific namespace (in case you have duplicate names). However, this doesn't allow you to specify a specific kind of resource with a specific name in a specific namespace as I think it's pretty unlikely you have that level of conflict in your K8 manifests.

I think option 3 makes sense, but in documentation we should be explicit about providing an app-id, and if we default to the resource name we should clearly print out the app-id.

[jjcollinge]

@yaron2 I went ahead and implemented option 3 just to test it out. This testdata shows you what you'd expect from a Pod definition with an autogenerated app id. I also added the ability to target a specific resource in a specific namespace (in case you have duplicate names). However, this doesn't allow you to specify a specific kind of resource with a specific name in a specific namespace as I think it's pretty unlikely you have that level of conflict in your K8 manifests.

I think option 3 makes sense, but in documentation we should be explicit about providing an app-id, and if we default to the resource name we should clearly print out the app-id.

Yh that makes sense. Not sure we can print the generated app-id clearly as the command is intended to output to stdout the updated YAML. We could potentially write it to stderr but it wouldn't be clear to the user. Guess we'll just have to add it to the --help and the documentation as clearly as possible.

[jjcollinge]

@mukundansundar yep np, please assign me.

[mukundansundar]

@jjcollinge please rebase and fix conflicts ...

[jjcollinge]

@mukundansundar I'm not familiar with this error - any ideas?

[mukundansundar]

@jjcollinge I am also not familiar with this error ... Can you fix the go.mod file and rebase to master and see if the error occurs again?

[mukundansundar]

@yaron2 Can you review this PR?

[dapr-bot]

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[yaron2]

@jjcollinge please add usage instructions to README.md and we'll be good to merge. This is great work!