Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add BearerToken Defaults for Kubernetes Plugins #6356

Merged

Conversation

rawkode
Copy link
Contributor

@rawkode rawkode commented Sep 5, 2019

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:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

@danielnelson
Copy link
Contributor

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 "").

@rawkode
Copy link
Contributor Author

rawkode commented Sep 5, 2019

Thanks for the prompt reply, @danielnelson. We could update the default, provided I change the code to accept BearerTokenString as precedent, only resolving to this value when no other option is available?

Or am I missing something there too?

@danielnelson
Copy link
Contributor

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.

@rawkode
Copy link
Contributor Author

rawkode commented Sep 5, 2019

Good point. What if we leave the defaults of both at "" and if both are empty, we inject the in-cluster configuration?

Alternatively, what if we provide in-cluster = true that overrides both, and handles BearerToken and url?

@rawkode
Copy link
Contributor Author

rawkode commented Sep 5, 2019

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.

insecure_skip_verify can't be forced, but in secure environments we can provide a zero config Kubernetes experience; which is my goal.

@danielnelson
Copy link
Contributor

What if we leave the defaults of both at "" and if both are empty, we inject the in-cluster configuration?

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.

Alternatively, what if we provide in-cluster = true that overrides both

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.

@rawkode rawkode force-pushed the feature/kubernetes-defaults branch 3 times, most recently from a4b5d81 to f5550e3 Compare October 18, 2019 13:10
@@ -176,6 +190,7 @@ func init() {
return &KubernetesInventory{
ResponseTimeout: internal.Duration{Duration: time.Second * 5},
Namespace: "default",
BearerToken: "/run/secrets/kubernetes.io/serviceaccount/token",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rawkode rawkode force-pushed the feature/kubernetes-defaults branch 2 times, most recently from f128183 to d48f839 Compare October 25, 2019 15:24
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 danielnelson added this to the 1.13.0 milestone Nov 6, 2019
@danielnelson danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Nov 6, 2019
@danielnelson danielnelson merged commit 284c7fc into influxdata:master Nov 6, 2019
@rawkode rawkode deleted the feature/kubernetes-defaults branch November 6, 2019 22:19
athoune pushed a commit to bearstech/telegraf that referenced this pull request Apr 17, 2020
idohalevi pushed a commit to idohalevi/telegraf that referenced this pull request Sep 29, 2020
arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants