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

fix: Update ExternalDNS policy to support multiple Route 53 zones #1496

Merged
merged 1 commit into from
Mar 20, 2023

Conversation

adonskikh
Copy link
Contributor

@adonskikh adonskikh commented Mar 15, 2023

What does this PR do?

If you have multiple Route 53 zones on the AWS account, then externalDNS will fail trying to list the resource record sets for other hosted zones (not the one used by your cluster) and won't be able to update even the entry for the zone it has access to.
The issue has appeared after this commit by @rodrigobersa : 8421a3b#diff-3972dd5c24a63dda22823ec8ba870f9c566791eeffef91eaa6d6c382fd3fd9d3L97
So this pull request basically reverts the policy body to the earlier version.

Motivation

More

  • Yes, I have tested the PR using my local account setup
  • Yes, I have updated the docs for this feature
  • Yes, I ran pre-commit run -a with this PR

For Moderators

  • E2E Test successfully complete before merge?

Additional Notes

I've created a new EKS cluster using my fork and the policy was created correctly:

{
    "Statement": [
        {
            "Action": "route53:ChangeResourceRecordSets",
            "Effect": "Allow",
            "Resource": "arn:aws:route53:::hostedzone/xxxxxxx",
            "Sid": ""
        },
        {
            "Action": [
                "route53:ListResourceRecordSets",
                "route53:ListHostedZones"
            ],
            "Effect": "Allow",
            "Resource": "*",
            "Sid": ""
        }
    ],
    "Version": "2012-10-17"
}

@adonskikh adonskikh requested a review from a team as a code owner March 15, 2023 14:07
@adonskikh adonskikh changed the title fix: Update ExternalDNS policy to support multiple Route 53 zones (#1474) fix: Update ExternalDNS policy to not fail in AWS accounts with multiple Route 53 zones (#1474) Mar 15, 2023
@@ -5,15 +5,15 @@ data "aws_iam_policy_document" "external_dns_iam_policy_document" {
[data.aws_route53_zone.selected.arn],
var.route53_zone_arns
Copy link
Contributor

Choose a reason for hiding this comment

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

provided the hosted zones are passed in via this variable, the solution works as intended.

please open any new issues in the new addons module https://github.com/aws-ia/terraform-aws-eks-blueprints-addons

Copy link
Contributor Author

@adonskikh adonskikh Mar 20, 2023

Choose a reason for hiding this comment

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

@bryantbiggs It can be used as a temporary workaround, but it won't work as a long-term solution.
Let's say I have two hosted zones (private hostedzone/1111 and public hostedzone/2222). I want my cluster to use hostedzone/1111 only, because it is a cluster for internal services and these services shouldn't be accessible outside of my cloud.
So ideally I'd like to configure ExternalDNS this way:

  external_dns_route53_zone_arns = [
    "arn:aws:route53:::hostedzone/1111"
  ]

But if I do so, ExternalDNS won't work and will be failing with the following error:

time="2023-03-20T11:40:27Z" level=error msg="failed to list resource records sets for zone /hostedzone/2222: AccessDenied: User: arn:aws:sts::*:assumed-role/my-cluster-external-dns-sa-irsa/12345 is not authorized to perform: route53:ListResourceRecordSets on resource: arn:aws:route53:::hostedzone/2222 because no identity-based policy allows the route53:ListResourceRecordSets action\n\tstatus code: 403"

As a result, I have to add all zones from my account to external_dns_route53_zone_arns variable to make it work, even though my cluster shouldn't have access to them:

  external_dns_route53_zone_arns = [
    "arn:aws:route53:::hostedzone/1111",
    "arn:aws:route53:::hostedzone/2222"
  ]

And the resulting policy is the following:

{
    "Statement": [
        {
            "Action": [
                "route53:ListResourceRecordSets",
                "route53:ChangeResourceRecordSets"
            ],
            "Effect": "Allow",
            "Resource": [
                "arn:aws:route53:::hostedzone/1111",
                "arn:aws:route53:::hostedzone/2222"
            ],
            "Sid": ""
        },
        {
            "Action": "route53:ListHostedZones",
            "Effect": "Allow",
            "Resource": "*",
            "Sid": ""
        }
    ],
    "Version": "2012-10-17"
}

So my cluster now has ChangeResourceRecordSets permission for zone 2222 too, and that's not what I planned initially.
Ok, let's say that it is not critical for us and we can live with it, but what happens if we add a third zone (id=3333) after the cluster is created? Right, ExternalDNS will start failing again because there is no ListResourceRecordSets permission for zone 3333 in the policy, so we'll have to update the policies for all our clusters.

That's why I still think that ListResourceRecordSets should be granted to "*" resource, like it was done in the previous versions of Blueprints:

        {
            "Action": [
                "route53:ListResourceRecordSets",
                "route53:ListHostedZones"
            ],
            "Effect": "Allow",
            "Resource": "*",
            "Sid": ""
        }

Restricting ListResourceRecordSets permission to specific zone IDs makes no difference for accounts with a single hosted zone (there is just one zone, so having * and 1111 is technically equivalent there), but it breaks everything for environments with multiple R53 zones.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, yes you are correct - I see this here as well https://github.com/kubernetes-sigs/external-dns/blob/64d6bbbbc2aac9e233b07afc6a95ecb4666da023/docs/tutorials/aws.md?plain=1#L29-L36

let me re-open and merge. Would you mind adding this to the new addon module as well via a PR? https://github.com/aws-ia/terraform-aws-eks-blueprints-addons

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll create a PR there soon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bryantbiggs bryantbiggs reopened this Mar 20, 2023
@bryantbiggs bryantbiggs temporarily deployed to EKS Blueprints Test March 20, 2023 15:39 — with GitHub Actions Inactive
@bryantbiggs bryantbiggs changed the title fix: Update ExternalDNS policy to not fail in AWS accounts with multiple Route 53 zones (#1474) fix: Update ExternalDNS policy to support multiple Route 53 zones Mar 20, 2023
@bryantbiggs bryantbiggs merged commit 09ed9f4 into aws-ia:main Mar 20, 2023
gminiba pushed a commit to gminiba/terraform-aws-eks-blueprints that referenced this pull request Apr 3, 2023
…ws-ia#1496)

Co-authored-by: Artem Donskikh <artem.donskikh@dataart.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants