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.

Sign up for GitHub

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
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions modules/kubernetes-addons/external-dns/data.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

Here is the PR: aws-ia/terraform-aws-eks-blueprints-addons#72

))
actions = [
"route53:ChangeResourceRecordSets",
"route53:ListResourceRecordSets",
]
actions = ["route53:ChangeResourceRecordSets"]
}

statement {
effect = "Allow"
resources = ["*"]
actions = ["route53:ListHostedZones"]
actions = [
"route53:ListHostedZones",
"route53:ListResourceRecordSets",
]
}
}