feat(aws): use AWS profiles using .credentials file #3973

roehrijn · 2023-10-05T11:40:53Z

Description

This PR introduces a new command line flag --aws-profile which can be used multiple times and allows to make use of AWS profiles in .credentials files (see also Configuration and credential file settings).
Furthermore the PR allows to use multiple of these profiles at once in a way that it queries for available Route53 zones using credentials of each profile and later distinguishes between the profiles in order to make changes with proper credentials to the right Route53 zone.

We at Mercedes-Benz are running a fork with those changes now for several months in production.

Checklist

Unit tests updated
End user documentation updated

_{Jan Roehrich jan.roehrich@mercedes-benz.com, Mercedes-Benz Tech Innovation GmbH, legal info/Impressum}

k8s-ci-robot · 2023-10-05T11:41:01Z

Welcome @roehrijn!

It looks like this is your first PR to kubernetes-sigs/external-dns 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/external-dns has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2023-10-05T11:41:02Z

Hi @roehrijn. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2023-10-05T11:41:12Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign johngmyers for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tobiasgiese · 2023-10-05T14:15:05Z

/ok-to-test

johngmyers · 2023-10-08T23:54:23Z

What is the value of having this complexity versus running an external-dns deployment per profile?

johngmyers · 2023-10-08T23:54:46Z

/retest

roehrijn · 2023-10-13T06:16:44Z

Hi @johngmyers,
we run almost 1000 Kubernetes clusters which are concurrently accessing our Route53 zones through external-dns. To cope with Route53 throttling we need to distribute our zones over several AWS accounts which leads us to 6 profiles, currently. During some migration situations there might even be 9 profiles.
Thus, the answer to your question about value has two dimensions. First, we have the complexity of running and managing up to 9 external-dns instances on up to 1000 clusters. On the other hand it is also about resource consumption. In the setup we use we need to request at least 60MB of memory per instance in order to work properly, in some situations a bit more. 60MB * 6 instances * 1000 clusters = a lot of additional EC2 costs.

mloiseleur · 2023-10-31T13:30:43Z

AWS profile is a feature of aws config file used in many aws compatible tools.
Do you think you can rebase and finish this PR ?

roehrijn · 2023-11-13T09:52:51Z

Hi @mloiseleur, yes I'm going to rebase soon. Just had some other important topics on the table over the last view weeks.

roehrijn · 2023-11-14T09:04:36Z

Hi @mloiseleur, I rebased the PR. Do you see a chance to get this aspect merged? If yes, I would also take the effort to adapt the documentation accordingly.

mloiseleur · 2023-12-23T17:13:40Z

@roehrijn As soon as you finish and rebase this PR, I don't see why not ?

Note: It surely could help too to remove the WIP in the title.

szuecs · 2024-01-11T11:18:15Z

main.go

 	if cfg.Provider == "aws" || cfg.Provider == "aws-sd" || cfg.Registry == "dynamodb" {
-		awsSession, err = aws.NewSession(
+		if len(cfg.AWSProfiles) == 0 || (len(cfg.AWSProfiles) == 1 && cfg.AWSProfiles[0] == "") {


Here is a longer code change without test.
Maybe better to also have in our provider/aws package a function that creates the session instead of putting it in to main(). main() is not good testable.

Hi @szuecs, I extracted two functions, one to create sessions per profile and one to create a default session. I hope this is what you've intended. Anyway I stumbled over two aspects:

I think the code of creating sessions over creating AWS clients to create an AWS provider can be further streamlined to remove complexity from main. I think this can all be integrated in aws.NewAWSProvider(). I'm eager to work on that but I guess its better to do that in a dedicated PR. WDYT?

I didn't add tests for the newly created functions. IMHO they're hard to unit test due to their strong dependencies to AWS SDK packages and behavior. However, I was wondering if you ever considered making use of LocalStack for testing the AWS provider? Same here, I would be eager to take care about that.

As far as I know AWS SDK packages have a test part for mocking so you could test that at least you can instantiate a mock session, not sure if it is really helpful but a thing we could do.
I have no strong opinion on having aws.NewAWSProvider(aws.Options{..}) or something like that.

I could not find any suitable mocking option for AWS sessions in AWS SDK v1, what is still in use here. However, it turned out that in most cases session creation facilities in AWS SDK don't talk to any remote endpoint. So I was able to test the profile loading behavior from a credential file as well as the default behavior from before this PR. What is unfortunately not possible to test in this way is the role assumption options. They require connectivity to AWS' STS service.

roehrijn · 2024-03-06T11:18:22Z

@mloiseleur @szuecs, please let me know if there is something left to do in order to get this merged. I would be very happy to avoid further rebases if possible, because it is a lot of work to validate function and valid documentation after each rebase. I'm eager to deal with missing aspects, however it would kindly ask to get to know them all together in order to avoid extra work.

mloiseleur · 2024-03-12T16:04:04Z

I understand. I think we are at the end.
/lgtm
/assign @szuecs for final review

mloiseleur · 2024-03-12T16:04:50Z

/retitle feat(aws): use AWS profiles using .credentials file

szuecs · 2024-04-24T10:06:21Z

provider/aws/aws.go

+			if errors.As(err, &awsErr) {
+				switch awsErr.Code() {
+				case "AccessDenied":
+					log.Warnf("Skipping AWS profile %q due to missing permission: %v", profile, awsErr.Message())


Why this is only a warning and not fatal?
This does not seem to be a temporary issue, but requires attention so we should stop the process.

please see answer on next comment. I think both have same scope, aren't they?

szuecs · 2024-04-24T10:07:04Z

provider/aws/aws.go

+					log.Warnf("Skipping AWS profile %q due to missing permission: %v", profile, awsErr.Message())
+					continue
+				case "InvalidClientTokenId", "ExpiredToken", "SignatureDoesNotMatch":
+					log.Warnf("Skipping AWS profile %q due to credential issues: %v", profile, awsErr.Message())


Why this is only a warning and not fatal?
This does not seem to be a temporary issue, but requires attention so we should stop the process.

At least I expect this to be an error instead of warning.

Hi @szuecs, thanks for the review.

In our use-case where we handle many AWS profiles with one external-dns instance, it occasionally happens that one profile is not ready due to eventual consistency topics during resource provisioning. However, the other profiles are working well in those situations. That's why we don't want the process to be killed. Especially because we heavily depend on caching in order not to face the Route53 throttling thresholds. Thats why I implemented it as a warning.

However, I understand other views on that. Changing this two cases to log an error instead of a warning would be completely fine for me. Additionally I see two more possibilities to handle that:

In order to keep the former semantics I could log the warnings (or errors) and add fatal behavior in case none of the given profiles is working.

I can introduce a behavioral switch as command line argument in order to let the user decide between fatal and graceful behavior (don't like that option much TBH).

WDYT?

szuecs · 2024-04-24T10:07:57Z

provider/aws/aws.go

+					log.Warnf("Skipping AWS profile %q due to credential issues: %v", profile, awsErr.Message())
+					continue
+				default:
+					// noting to do here. Falling through to general error handling


so you will wrap and ignore rest of the errors by SoftError, right?

I would not say ignoring them, but yes, they're handled by NewSoftError(...)

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 5, 2023

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 5, 2023

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 5, 2023

k8s-ci-robot requested review from johngmyers and szuecs October 5, 2023 11:41

roehrijn force-pushed the roehrijn/aws-profiles branch from c7ad1ea to 2815f48 Compare October 5, 2023 11:59

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 5, 2023

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 5, 2023

roehrijn force-pushed the roehrijn/aws-profiles branch from 42283a6 to 9e0e725 Compare October 5, 2023 19:47

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2023

roehrijn force-pushed the roehrijn/aws-profiles branch from 9e0e725 to 92b6089 Compare November 14, 2023 08:12

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 14, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 22, 2023

roehrijn force-pushed the roehrijn/aws-profiles branch from 92b6089 to 5ba3201 Compare December 29, 2023 09:45

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 29, 2023

roehrijn changed the title ~~[WIP] Allow usage of (multiple) profiles using .credentials file in AWS provider~~ Allow usage of (multiple) profiles using .credentials file in AWS provider Dec 29, 2023

szuecs reviewed Jan 11, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 15, 2024

roehrijn force-pushed the roehrijn/aws-profiles branch from c0993a8 to 7dd022f Compare January 28, 2024 14:34

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 28, 2024

roehrijn force-pushed the roehrijn/aws-profiles branch 2 times, most recently from d9206cf to 5f4a13c Compare January 28, 2024 15:04

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 9, 2024

roehrijn added 8 commits March 5, 2024 23:06

Allow usage of (multiple) AWS profiles using .credentials file

7fb25f4

Fix linter issues

a3426d6

Rebase adaptions

38621aa

add AWS profile aspect to documentation

d298517

fixing mloiseleur's findings

7fb0996

fixing mloiseleur's findings

3ac5a8e

fixing szuecs's findings

2059367

rebase

fd74c25

roehrijn force-pushed the roehrijn/aws-profiles branch from 5f4a13c to fd74c25 Compare March 5, 2024 22:27

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 5, 2024

roehrijn force-pushed the roehrijn/aws-profiles branch 2 times, most recently from 80af5cc to 11e098f Compare March 6, 2024 09:09

add test for newSession()

d3fb0eb

roehrijn force-pushed the roehrijn/aws-profiles branch from 11e098f to d3fb0eb Compare March 6, 2024 09:38

k8s-ci-robot assigned mloiseleur Mar 12, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 12, 2024

k8s-ci-robot changed the title ~~Allow usage of (multiple) profiles using .credentials file in AWS provider~~ feat(aws): use AWS profiles using .credentials file Mar 12, 2024

szuecs reviewed Apr 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(aws): use AWS profiles using .credentials file #3973

feat(aws): use AWS profiles using .credentials file #3973

roehrijn commented Oct 5, 2023 •

edited

k8s-ci-robot commented Oct 5, 2023

k8s-ci-robot commented Oct 5, 2023

k8s-ci-robot commented Oct 5, 2023

tobiasgiese commented Oct 5, 2023

johngmyers commented Oct 8, 2023

johngmyers commented Oct 8, 2023

roehrijn commented Oct 13, 2023 •

edited

mloiseleur commented Oct 31, 2023

roehrijn commented Nov 13, 2023

roehrijn commented Nov 14, 2023

mloiseleur commented Dec 23, 2023

szuecs Jan 11, 2024

roehrijn Jan 28, 2024 •

edited

szuecs Feb 13, 2024

roehrijn Mar 6, 2024

roehrijn commented Mar 6, 2024

mloiseleur commented Mar 12, 2024

mloiseleur commented Mar 12, 2024

szuecs Apr 24, 2024

roehrijn Apr 25, 2024

szuecs Apr 24, 2024

roehrijn Apr 25, 2024

szuecs Apr 24, 2024

roehrijn Apr 25, 2024

feat(aws): use AWS profiles using .credentials file #3973

Are you sure you want to change the base?

feat(aws): use AWS profiles using .credentials file #3973

Conversation

roehrijn commented Oct 5, 2023 • edited

k8s-ci-robot commented Oct 5, 2023

k8s-ci-robot commented Oct 5, 2023

k8s-ci-robot commented Oct 5, 2023

tobiasgiese commented Oct 5, 2023

johngmyers commented Oct 8, 2023

johngmyers commented Oct 8, 2023

roehrijn commented Oct 13, 2023 • edited

mloiseleur commented Oct 31, 2023

roehrijn commented Nov 13, 2023

roehrijn commented Nov 14, 2023

mloiseleur commented Dec 23, 2023

Choose a reason for hiding this comment

roehrijn Jan 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roehrijn commented Mar 6, 2024

mloiseleur commented Mar 12, 2024

mloiseleur commented Mar 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roehrijn commented Oct 5, 2023 •

edited

roehrijn commented Oct 13, 2023 •

edited

roehrijn Jan 28, 2024 •

edited