-
Notifications
You must be signed in to change notification settings - Fork 488
feat(fluent-bit): Add automountServiceAccountToken override at pod level #633
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Greg Smith <[email protected]>
|
@stevehipwell This is the same change (#632) you were reviewing but with a cleaner commit history. Sorry for the churn/confusion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gsmith-sas after further thought I think the existing value should be deprecated and replaced by new values that are clearer in their intent. The pattern I've suggested is backwards compatible, and while this chart is pre-release I think it's worth using a deprecation pattern.
|
@stevehipwell Thanks for reviewing the PR and providing your feedback. However, I was a little confused after reading your comments and want to clarify things before making any changes. I believe a high-level summary of your suggestions is the following:
But, if so, I was also confused by a couple of comments in your suggested changes to values.yaml:
Assuming I am correct in what your overall intent is, I will also need to make changes to the Pod template as well: adding support for And, finally, assuming I have things straight, I think that means the following things need to be included in the
Sorry for the lengthy comment but I thought it would be best to sort this out before pushing changes. Thank you again for taking time to review this PR. Cheers, |
|
@gsmith-sas the intent is as follows. The existing value controlling both the sa and the pod is deprecated and is deprioritized; if it is set then it will only apply where the new more specific values haven't been set. The new values are specific to the sa and the pod and override the deprecated value. It's probably easiest to comment on iterative changes, otherwise the cognitive load to cross reference non-inline comments with the code is much too high. |
|
@stevehipwell Thank you for the clarification. I believe I have addressed all of your concerns in my latest commit. |
Signed-off-by: Greg Smith <[email protected]>
15f03e7 to
f291287
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the changes, but I think we still need another change.
Co-authored-by: Steve Hipwell <[email protected]> Signed-off-by: Greg Smith <[email protected]>
Co-authored-by: Steve Hipwell <[email protected]> Signed-off-by: Greg Smith <[email protected]>
|
@stevehipwell Thanks for the (re)review and suggested code modifications. I have incorporated your suggestions which I think address all outstanding concerns. If so, I think this PR should be good to go. Cheers, Greg |
Signed-off-by: Greg Smith <[email protected]>
Signed-off-by: Greg Smith <[email protected]>
Signed-off-by: Greg Smith <[email protected]>
|
@stevehipwell I just noticed there were some merge conflicts due to other PRs being merged in recently. I've now taken care of those conflicts, so we should be good to go. Cheers, Greg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @gsmith-sas. I just tested this locally and spotted one more thing to fix and then we can get this merged.
Co-authored-by: Steve Hipwell <[email protected]> Signed-off-by: Greg Smith <[email protected]>
|
@stevehipwell I accepted your change (rather than re-implementing it) and I still run into a failure when trying to deploy. I apologize for including the following lengthy chunks of console session output, but I wanted to be transparent about my testing process. In this first snippet, you can see I've done the following:
To confirm the issue was related to the most recent change, I then did the following:
Based on all of that, I think I should revert the most recent suggested change. But if these same test-cases are working for you, we'll need to figure out where I'm going wrong or what is different in our two test environments that may be causing these problems. Regards, |
|
@stevehipwell Have you had a chance to validate (or not) the two use-cases I identified where the proposed change seems to result in errors? I'll be out of office next week but hope we can finalize this PR when I return. Regards, Greg |
|
@gsmith-sas if you move the auto mount code in the pod template below the service account line and then remove the trailing |
NOTE: This is the same change as PR #632. I abandoned that PR since it looks like I may have messed up the commit history while rebasing.
This change allows users to override the
automountServiceAccountTokensetting at the pod level. The Kubernetes API allows this property to be set at two levels:ServiceAccount level - when set at the ServiceAccount level, the setting impacts all pods associated with the ServiceAccount. The default value for this property is "true" meaning that the API token of the ServiceAccount is automatically mounted to any pod associated with the ServiceAccount.
Pod level - when set at the Pod level, the setting only impacts that specific pod. When set at both levels, the Pod-level setting overrides the ServiceAccount level.
As described in PR #629, many sites require that this property be disabled for security reasons since the default value of "true" makes API tokens available by default. And, some security scans will flag this default behavior as potentially problematic. The change in PR #629 allows users to disable this setting and, when disabled, disables it at both the ServiceAccount and Pod levels.
However, there are situations where it is appropriate to disable this setting at the ServiceAccount level but enable it at the Pod level. For example, when configuring Fluent Bit to collect Kubernetes Events, access to the API tokens is required. The solution is to disable the setting at the ServiceAccount level (which PR#629 now permits) but enable it at the Pod level (which PR#629 does not permit). This PR is designed to allow users to enable this.
In this PR, I chose to name the new property
automountServiceAccountTokeneven though that is very similr to the new property added in PR #629 (which addedserviceAccount.automountServiceAccountToken). While this is potentially confusing, I think aligning the names of the Helm chart values with the properties they impact in the underlying Kubernetes resources is critical. This naming is consistent with other Helm charts I have reviewed (e.g. the Grafana Helm chart that support setting this property at both the ServiceAccount and Pod levels. I have also attempted to minimize confusion by adding comments immediately before both properties in thevalues.yamlfile explaining the scope of each property.