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

Optional AWS Route53 region #7287

Merged

Conversation

wallrj
Copy link
Member

@wallrj wallrj commented Sep 19, 2024

When you install cert-manager on EKS and use IAM Roles for Service Accounts or Pod Identity,
an AWS mutating webhook will add AWS_REGION and AWS_DEFAULT_REGION environment variables to the controller Pod. There's no need to specify the region in the Issuer config so it should not be a required field.

📖 Read Grant Kubernetes workloads access to AWS using Kubernetes Service Accounts to learn more about these two mechanisms.

AWS Route53 is a global service and does not have regional endpoints.
The AWS SDK for Go V2 uses the region (whether supplied or detected from environment variables) as a hint,
with which to compute the AWS partition domain name (i.e. route53.amazonaws.com / amazonaws.com.cn / route53.us-gov.amazonaws.com) and the credential scope (us-east-1 / cn-northwest-1 / us-gov-west-1).
Here is the metadata that the SDK uses for Route53.

  • Updated the webhook validation tests, to demonstrate that all the Route53 fields are now optional.
  • Removed the webhook validation for region, which was being used if the user supplied an empty string for region.
  • OpenAPI validation was also being used, if the region field was omitted from the supplied JSON.
  • So I marked annotated the field with omitempty and +optional and regenerated the CRDS.

Fixes: #6175
Fixes: cert-manager/website#56

/kind bug

The Route53 DNS01 solver of the ACME Issuer can now detect the AWS region from the `AWS_REGION` and `AWS_DEFAULT_REGION` environment variables, which is set by the IAM for Service Accounts (IRSA) webhook and by the Pod Identity webhook. 
The `issuer.spec.acme.solvers.dns01.route53.region` field is now optional.
The API documentation of the `region` field has been updated to explain when and how the region value is used.

Testing

API Documentation

$ kubectl explain issuer.spec.acme.solvers.dns01.route53.region
GROUP:      cert-manager.io
KIND:       Issuer
VERSION:    v1

FIELD: region <string>

DESCRIPTION:
    Override the AWS region.

    Route53 is a global service and does not have regional endpoints but the
    region specified here (or via environment variables) is used as a hint to
    help compute the correct AWS credential scope and partition when it
    connects to Route53. See:
    - [Amazon Route 53 endpoints and
    quotas](https://docs.aws.amazon.com/general/latest/gr/r53.html)
    - [Global
    services](https://docs.aws.amazon.com/whitepapers/latest/aws-fault-isolation-boundaries/global-services.html)

    Region is not needed if you use [IAM Roles for Service Accounts
    (IRSA)](https://docs.aws.amazon.com/eks/latest/userguide/iam-roles-for-service-accounts.html).
    Instead an AWS_REGION environment variable is added to the cert-manager
    controller Pod by:
    [Amazon EKS Pod Identity
    Webhook](https://github.com/aws/amazon-eks-pod-identity-webhook),

    Region is not needed if you use [EKS Pod
    Identities](https://docs.aws.amazon.com/eks/latest/userguide/pod-identities.html).
    Instead an AWS_REGION environment variable is added to the cert-manager
    controller Pod by:
    [Amazon EKS Pod Identity
    Agent](https://github.com/aws/eks-pod-identity-agent),

    Region is not used for computing STS regional endpoints.
    If you configure the `role` field, cert-manager will always use the
    global STS endpoint to make AssumeRole and AssumeRoleWithWebIdentity
    requests.

    Region is ignored if ambient credentials mode is disabled, by
    `--cluster-issuer-ambient-credentials` or `--isssuer-ambient-credentials`
    controller flags.

Remove webhook validation for Route53 region

Signed-off-by: Richard Wall <[email protected]>
@cert-manager-prow cert-manager-prow bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. area/testing Issues relating to testing needs-kind Indicates a PR lacks a `kind/foo` label and requires one. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 19, 2024
@wallrj wallrj force-pushed the optional-aws-route53-region branch from b96b8a9 to 9378c8e Compare September 19, 2024 14:27
@cert-manager-prow cert-manager-prow bot added area/api Indicates a PR directly modifies the 'pkg/apis' directory area/deploy Indicates a PR modifies deployment configuration labels Sep 19, 2024
Region string `json:"region"`
// Override the AWS region.
// The region is used to compute the STS endpoint and it is used in API
// request signatures.
Copy link
Member

Choose a reason for hiding this comment

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

Do you know what would happen if the signature has the wrong region?

Copy link
Member Author

Choose a reason for hiding this comment

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

I misunderstood. My latest understanding is that the region given here or from AWS_REGION is used as a hint to select one of two credential scope regions.
I've updated the comment and added some links to AWS documentation.
See also:

@wallrj wallrj force-pushed the optional-aws-route53-region branch from 9378c8e to 398c9e3 Compare September 19, 2024 16:41
@cert-manager-prow cert-manager-prow bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Sep 20, 2024
@wallrj wallrj changed the title WIP: Optional aws route53 region WIP: Optional AWS Route53 region Sep 20, 2024
@wallrj wallrj changed the title WIP: Optional AWS Route53 region Optional AWS Route53 region Sep 20, 2024
@cert-manager-prow cert-manager-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 20, 2024
@wallrj wallrj requested a review from inteon September 20, 2024 10:06
Copy link
Member Author

Choose a reason for hiding this comment

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

Region string `json:"region"`
// Override the AWS region.
// The region is used to compute the STS endpoint and it is used in API
// request signatures.
Copy link
Member Author

Choose a reason for hiding this comment

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

I misunderstood. My latest understanding is that the region given here or from AWS_REGION is used as a hint to select one of two credential scope regions.
I've updated the comment and added some links to AWS documentation.
See also:

Comment on lines 624 to 626
// Region is ignored if ambient credentials mode is disabled, by
// `--cluster-issuer-ambient-credentials` or `--isssuer-ambient-credentials`
// controller flags.
Copy link
Member Author

@wallrj wallrj Sep 20, 2024

Choose a reason for hiding this comment

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

I wrote this wrong. Should be:

Suggested change
// Region is ignored if ambient credentials mode is disabled, by
// `--cluster-issuer-ambient-credentials` or `--isssuer-ambient-credentials`
// controller flags.
// Region is used unconditionally if ambient credentials mode is disabled, by
// `--cluster-issuer-ambient-credentials` or `--isssuer-ambient-credentials`
// controller flags.

Personally, I think this is a bug, but that is how it's coded.
Ambient mode should only concern the loading of ambient credentials,
not other metadata such as region.

@wallrj wallrj force-pushed the optional-aws-route53-region branch from 398c9e3 to 9de6aa6 Compare September 20, 2024 10:41
Copy link
Member

@inteon inteon left a comment

Choose a reason for hiding this comment

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

Thanks for investigating this. I'm 100% onboard with making the region field optional.
Might want to tune the comment a bit more in the future, but LGTM for this beta release.
/approve
/lgtm

@cert-manager-prow cert-manager-prow bot added the lgtm Indicates that a PR is ready to be merged. label Sep 20, 2024
@cert-manager-prow
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: inteon

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@cert-manager-prow cert-manager-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 20, 2024
@wallrj
Copy link
Member Author

wallrj commented Sep 20, 2024

/kind bug

@cert-manager-prow cert-manager-prow bot added kind/bug Categorizes issue or PR as related to a bug. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Sep 20, 2024
@cert-manager-prow cert-manager-prow bot merged commit 569f920 into cert-manager:master Sep 20, 2024
6 checks passed
@wallrj wallrj deleted the optional-aws-route53-region branch September 24, 2024 15:35
@wallrj
Copy link
Member Author

wallrj commented Sep 26, 2024

/kind feature

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/api Indicates a PR directly modifies the 'pkg/apis' directory area/deploy Indicates a PR modifies deployment configuration area/testing Issues relating to testing dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. kind/bug Categorizes issue or PR as related to a bug. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

region should be optional in a Route53 dns solver Route53: document use of "region" field
2 participants