feat: add option to assume-role in route53 #1917

moolen · 2019-07-23T13:25:37Z

What this PR does / why we need it:
This PR adds the field role to the route53 dns provider. It allows users to specify a role per route53 provider which lets them manage zones in other accounts. see discussion at #1274

Which issue this PR fixes: fixes #1274

Special notes for your reviewer:

There is bunch of documentation missing regarding route53 (see #1750). This PR will add a minimum-viable documentation only for this specific field.

The assume-role logic itself is trivial but i really wanted a way to test the behavior (hence the indirection using type sessionProvider).

Release note:

Add option to assume role in Route53 provider

jetstack-bot · 2019-07-23T13:25:52Z

Hi @moolen. Thanks for your PR.

I'm waiting for a jetstack or cert-manager 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.

cyrus-mc · 2019-07-24T17:27:04Z

@moolen I see this hasn't been assigned yet. Where are you in the development? Hoping to get this merged as this is a feature that I require.

Thanks

moolen · 2019-07-24T17:29:01Z

I'm waiting for feedback. I missed the part with /assigning someone 😅

/assign @kragniz

cyrus-mc · 2019-07-24T17:35:15Z

I'm waiting for feedback. I missed the part with /assigning someone 😅

/assign @kragniz

No problem. That is why I pinged you. I missed that in my PR as well.

I am going to build your branch and test.

cyrus-mc · 2019-07-24T22:47:50Z

@moolen

Test successfully.

moolen · 2019-07-25T07:40:22Z

thanks for your effort! <3

munnerz · 2019-07-25T08:51:00Z

Thanks for testing @cyrus-mc 😄

I'm going to take a look over the implementation for this today. I'm personally not so well versed in AWS APIs, so I'm going to defer API review to someone else who is.. 🙄.

/cc @simonswine
/cc @JoshVanL
/ok-to-test

moolen · 2019-07-25T09:20:44Z

/test pull-cert-manager-e2e-v1-13 (could not resolve host github.com)

simonswine

Looks good in general 👍 , a few questions about the details are inline

simonswine · 2019-07-25T09:47:09Z

docs/tasks/issuers/setup-acme/dns01/route53.rst

@@ -22,6 +22,8 @@ Amazon Route53
             secretAccessKeySecretRef:
               name: prod-route53-credentials-secret
               key: secret-access-key
+             # you may specify a role to assume
+             role: arn:aws:iam::XXXXXXXXXXXX:role/cert-manager


I think a full blown example would be great here (Including IAM Role configuration, AssumePolicy and so on)

Also pointing out the two options using Instance Profile + Assume Role (useAmbientCerds=false) or AccessKey + SecretKey + AssumeRole. (useAmbientCerds=true)

Yep, are these to auth methods mutually exclusive? If you could update comments on API types to explain this too, we may want to consider restructuring how auth is defined if so to make it fit better.. but let's first document what's what so we are all on the same page 😄

simonswine · 2019-07-25T09:47:30Z

pkg/issuer/acme/dns/route53/route53.go

+		klog.V(5).Infof("assuming role: %s", d.Role)
+		stsSvc := d.StsProvider(sess)
+		result, err := stsSvc.AssumeRole(&sts.AssumeRoleInput{
+			DurationSeconds: aws.Int64(900),


Not too sure how long we use that GetSession(). A validation might take longer than 900 seconds and we might not cleanup properly.

It also might be the case that we always get a new session on every reconcile (@munnerz to confirm)

It also might be the case that we always get a new session on every reconcile (@munnerz to confirm)

We do currently get a new session on each reconcile (although we don't always obtain a session on every reconcile, i.e. we only get one if we need to Present or CleanUp the record, so you shouldn't see 10s or hundreds of sessions being created for a single challenge)

A validation might take longer than 900 seconds and we might not cleanup properly.

Good point! 1h is the sdk default and should be enough.

moolen · 2019-07-25T12:46:43Z

@simonswine & @munnerz i added docs for auth, policies, api spec and changed the DefaultSessionDuration. PTAL.

moolen · 2019-07-29T14:52:31Z

ping @simonswine @munnerz

munnerz · 2019-08-02T14:18:50Z

Thanks for this 😄

/lgtm
/approve

jetstack-bot · 2019-08-02T14:18:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: moolen, munnerz

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:

~~OWNERS~~ [munnerz]

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

munnerz · 2019-08-02T14:19:13Z

Would you mind also editing your original comment and adding a short (one-line) release note for this change?

retest-bot · 2019-08-02T15:03:42Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

retest-bot · 2019-08-02T15:43:44Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

retest-bot · 2019-08-02T16:23:42Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

munnerz · 2019-08-02T16:36:14Z

Can you run bazel run //hack:update-crds and commit the changes? 😄

/lgtm cancel

Signed-off-by: Moritz Johner <beller.moritz@googlemail.com>

moolen · 2019-08-03T15:45:32Z

i rebased, updated the CRDs and squashed the commits. that should do it 🤞

*edit
everything green 🎉 @munnerz

munnerz · 2019-08-08T19:03:30Z

/lgtm

jetstack-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Jul 23, 2019

jetstack-bot requested review from munnerz and vdesjardins July 23, 2019 13:25

jetstack-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 23, 2019

moolen force-pushed the feat/route53-assume-role branch from bcd9200 to b542eb0 Compare July 23, 2019 13:26

jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Jul 23, 2019

munnerz mentioned this pull request Jul 24, 2019

Added support for AWS credentials through assume_role #1925

Closed

jetstack-bot assigned kragniz Jul 24, 2019

jetstack-bot requested review from JoshVanL and simonswine July 25, 2019 08:51

jetstack-bot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 25, 2019

simonswine reviewed Jul 25, 2019

View reviewed changes

jetstack-bot added dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. and removed dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. labels Jul 25, 2019

jetstack-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 25, 2019

moolen force-pushed the feat/route53-assume-role branch from c7114cc to e6ac3d7 Compare July 25, 2019 12:12

jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Jul 25, 2019

jetstack-bot assigned munnerz Aug 2, 2019

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 2, 2019

jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 2, 2019

jetstack-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 Aug 2, 2019

jetstack-bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 2, 2019

feat: add option to assume-role in route53

5915872

Signed-off-by: Moritz Johner <beller.moritz@googlemail.com>

moolen force-pushed the feat/route53-assume-role branch from 6664746 to 5915872 Compare August 3, 2019 15:38

jetstack-bot added the area/deploy Indicates a PR modifies deployment configuration label Aug 3, 2019

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 8, 2019

jetstack-bot merged commit 36a888a into cert-manager:master Aug 8, 2019

munnerz mentioned this pull request Oct 3, 2019

Make Route53 dns01 work with IAM roles for service accounts #2147

Closed

TBBle mentioned this pull request Apr 23, 2020

Route53: document use of "region" field cert-manager/website#56

Open

munnerz mentioned this pull request Apr 24, 2020

Pass only role name and not full ARN for kube2iam #2768

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add option to assume-role in route53 #1917

feat: add option to assume-role in route53 #1917

moolen commented Jul 23, 2019 •

edited

Loading

jetstack-bot commented Jul 23, 2019

cyrus-mc commented Jul 24, 2019

moolen commented Jul 24, 2019

cyrus-mc commented Jul 24, 2019

cyrus-mc commented Jul 24, 2019

moolen commented Jul 25, 2019

munnerz commented Jul 25, 2019

moolen commented Jul 25, 2019

simonswine left a comment

simonswine Jul 25, 2019

munnerz Jul 25, 2019

simonswine Jul 25, 2019

munnerz Jul 25, 2019

moolen Jul 25, 2019

moolen commented Jul 25, 2019

moolen commented Jul 29, 2019

munnerz commented Aug 2, 2019

jetstack-bot commented Aug 2, 2019

munnerz commented Aug 2, 2019

retest-bot commented Aug 2, 2019

retest-bot commented Aug 2, 2019

retest-bot commented Aug 2, 2019

munnerz commented Aug 2, 2019

moolen commented Aug 3, 2019 •

edited

Loading

munnerz commented Aug 8, 2019

feat: add option to assume-role in route53 #1917

feat: add option to assume-role in route53 #1917

Conversation

moolen commented Jul 23, 2019 • edited Loading

jetstack-bot commented Jul 23, 2019

cyrus-mc commented Jul 24, 2019

moolen commented Jul 24, 2019

cyrus-mc commented Jul 24, 2019

cyrus-mc commented Jul 24, 2019

moolen commented Jul 25, 2019

munnerz commented Jul 25, 2019

moolen commented Jul 25, 2019

simonswine left a comment

Choose a reason for hiding this comment

simonswine Jul 25, 2019

Choose a reason for hiding this comment

munnerz Jul 25, 2019

Choose a reason for hiding this comment

simonswine Jul 25, 2019

Choose a reason for hiding this comment

munnerz Jul 25, 2019

Choose a reason for hiding this comment

moolen Jul 25, 2019

Choose a reason for hiding this comment

moolen commented Jul 25, 2019

moolen commented Jul 29, 2019

munnerz commented Aug 2, 2019

jetstack-bot commented Aug 2, 2019

munnerz commented Aug 2, 2019

retest-bot commented Aug 2, 2019

retest-bot commented Aug 2, 2019

retest-bot commented Aug 2, 2019

munnerz commented Aug 2, 2019

moolen commented Aug 3, 2019 • edited Loading

munnerz commented Aug 8, 2019

moolen commented Jul 23, 2019 •

edited

Loading

moolen commented Aug 3, 2019 •

edited

Loading