-
Notifications
You must be signed in to change notification settings - Fork 113
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
[Egress providers - S3 store] Add support for service accounts when running dotnet monitor in kubernetes #6626
[Egress providers - S3 store] Add support for service accounts when running dotnet monitor in kubernetes #6626
Conversation
…entication via Open ID connect is possible It is enough for the project to be referenced for this to work, and there is no reference to it in the code base, because it's existence allows the AWS SDK to work
@dotnet-policy-service agree company="Gearset" |
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 looking into this and making this PR with the detailed explanation!
Can you also update this file in this PR? We need to explicitly configuring signing information for 3rd party files:
dotnet-monitor/eng/Signing.props
Lines 2 to 5 in a0927a5
<ItemGroup> | |
<FileSignInfo Include="AWSSDK.Core.dll" CertificateName="3PartySHA2" /> | |
<FileSignInfo Include="AWSSDK.S3.dll" CertificateName="3PartySHA2" /> | |
<FileSignInfo Include="Newtonsoft.Json.dll" CertificateName="3PartySHA2" /> |
e.g. add a new line with:
<FileSignInfo Include="AWSSDK.SecurityToken.dll" CertificateName="3PartySHA2" />
Spell checker flagged that Kubernetes should have an uppercase K and to use the American spelling of utilize Co-authored-by: Justin Anderson <jander-msft@users.noreply.github.com>
@schmittjoseph done in c3c0a5f, thanks! Anything further I need to do with the dependabot so that this new package gets updated correctly? |
Nope, everything looks good in terms of dependabot! We pull our public dependencies from the .NET org's nuget.org mirror(https://dev.azure.com/dnceng/public/_artifacts/feed/dotnet-public) which has a slightly older version of |
/backport to release/8.x |
Started backporting to release/8.x: https://github.com/dotnet/dotnet-monitor/actions/runs/9116375732 |
…unning dotnet monitor in kubernetes (#6626) * Reference AWSSDK.SecurityToken in the S3 Storage project so that authentication via Open ID connect is possible It is enough for the project to be referenced for this to work, and there is no reference to it in the code base, because it's existence allows the AWS SDK to work * Documentation: add docs to cover the use of service accounts for S3 * Update documentation in line with spell checker Spell checker flagged that Kubernetes should have an uppercase K and to use the American spelling of utilize Co-authored-by: Justin Anderson <jander-msft@users.noreply.github.com> * PR Feedback: Add AWSSDK.SecurityToken.dll to the Signing.props file --------- Co-authored-by: Justin Anderson <jander-msft@users.noreply.github.com>
/backport to release/8.0 |
Started backporting to release/8.0: https://github.com/dotnet/dotnet-monitor/actions/runs/9116382467 |
…unning dotnet monitor in kubernetes (#6626) * Reference AWSSDK.SecurityToken in the S3 Storage project so that authentication via Open ID connect is possible It is enough for the project to be referenced for this to work, and there is no reference to it in the code base, because it's existence allows the AWS SDK to work * Documentation: add docs to cover the use of service accounts for S3 * Update documentation in line with spell checker Spell checker flagged that Kubernetes should have an uppercase K and to use the American spelling of utilize Co-authored-by: Justin Anderson <jander-msft@users.noreply.github.com> * PR Feedback: Add AWSSDK.SecurityToken.dll to the Signing.props file --------- Co-authored-by: Justin Anderson <jander-msft@users.noreply.github.com>
Summary
Reason for change
As it stands, the S3 egress provider does not currently support the use of service accounts when running dotnet monitor in kubernetes.
This is one of the two recommended ways to give pods permission to access AWS resources via roles. This tends to be recommended over long lived access keys as it removes the need for regular rotation of credentials etc.
Details of change made
The full documentation for this lives here on AWS's documentation site. It uses OpenID Connect to do authentication and is natively supported in the existing C# SDK which is already used. However, there is a note when talking about SDK support for .NET that says:
This is correlated to my experience trying service accounts on my own kubernetes cluster, as I tried to use service accounts and received the error:
This happens because the S3 Storage extension already uses
FallbackCredentialsFactory.GetCredentials
, which does all the authentication work automaticallydotnet-monitor/src/Extensions/S3Storage/S3Storage.cs
Line 71 in cca1dad
(note that the different ways it can be authenticated are best shown in the AWS SDK code itself, which shows it supports Web Identity Credentials (the thing needed for service accounts) as well as credentials from environment variables)
However, that function isn't possible unless
AWSSDK.SecurityToken
is available at runtime. Therefore, this PR adds a dependency to the S3 project to bring inAWSSDK.SecurityToken
Places I was unsure
I wasn't sure how the dependabot setup works. AFAICT it relies on variables defined in the
Versions
file, but I couldn't quite figure out how to get that linked up to actually work so any guidance on that would be much appreciatedI also very much welcome any feedback on the documentation, as I wasn't quite sure on tone of voice etc that made sense for this change
I haven't created an issue for this as it was such a small change, however, if you would rather I do so I would be happy to!
Release Notes Entry
Support added for kubernetes service accounts when using the S3 storage egress provider