Add BearerToken Defaults for Kubernetes Plugins #6356
Conversation
|
We can't change the default for this value because it will break configs, but we can update the sample string to this value. Another possiblility is to change the sample config to have this option set to a non-default value (basically we uncomment it but the default remains ""). |
|
Thanks for the prompt reply, @danielnelson. We could update the default, provided I change the code to accept Or am I missing something there too? |
|
Switching the priority could cause problems if anyone has both set, but I think that is unlikely and something I would consider changng, but even if we swap the priority of the two bearer string options it would require users not using a bearer string to set both to the empty string to keep current behavior. |
|
Good point. What if we leave the defaults of both at Alternatively, what if we provide |
|
Just to explain my motives: Almost everyone using these plugins will be running in-cluster. We don't actually need to know the BearerToken or URL, as this is standard.
|
Technically this could be an issue if no bearer token is wanted but the file exists, but I think this is rare enough to be acceptable. If you do this only log at debug if the bearer token file is not found.
Adding more options won't help because you can't enable them without breaking config files. When a user upgrades to a newer Telegraf version their unmodified config can't change meaning or stop working. |
| @@ -176,6 +190,7 @@ func init() { | |||
| return &KubernetesInventory{ | |||
| ResponseTimeout: internal.Duration{Duration: time.Second * 5}, | |||
| Namespace: "default", | |||
| BearerToken: "/run/secrets/kubernetes.io/serviceaccount/token", | |||
There was a problem hiding this comment.
Won't this mean the BearerToken will always be set later during the checks?
I think what we will want to do is get rid of the priority system for the two bearer token option. If they are both set then on initialization it should be an error. Technically a breaking change, but I think minor enough impact to consider.
There was a problem hiding this comment.
Sorry, I thought I removed this line
| // Even neither are provided, use the default service account. | ||
| if ki.BearerToken == "" && ki.BearerTokenString == "" { | ||
| if _, err := os.Stat(defaultServiceAccountPath); err == nil { | ||
| ki.BearerToken = defaultServiceAccountPath |
There was a problem hiding this comment.
It would be best to just read the token file once, and base any decisions on if it is successful, instead of doing a stat and then reading later.
As we now allow for a func (ki *KubernetesInventory) Init() error { function, this would be a good place to put the checking and reading.
Due to Kubernetes RBAC policies, it's extremely rare and unlikely that anyone will run these plugins outside of the cluster. With that in-mind, we should use as many defaults as possible to remove configuration. All pods within Kubernetes have a ServiceAccount token mounted in to this location, which shouldn't need to be repeated each time.
danielnelson
added
the
feat
Due to Kubernetes RBAC policies, it's extremely rare and unlikely that
anyone will run these plugins outside of the cluster. With that in-mind,
we should use as many defaults as possible to remove configuration.
All pods within Kubernetes have a ServiceAccount token mounted in to this
location, which shouldn't need to be repeated each time.
Required for all PRs: