NE-1323: Add AWS RoleARN for Shared VPC support by gcs278 · Pull Request #195 · openshift/external-dns-operator

gcs278 · 2023-06-26T18:27:44Z

Adds the ability to specify a RoleARN to create Route 53 DNS records in a different AWS Account. Supports Shared VPC scenario.

The new API is propagated to the existing external-dns argument --aws-assume-role.

Epic Link: https://issues.redhat.com/browse/NE-1299
EP PR: openshift/enhancements#1436

alebedev87

I believe you need to add sts:AssumeRole to the AWS CredentilasRequest too.

alebedev87 · 2023-06-27T15:54:17Z

 	}

+	if b.externalDNS.Spec.AWSRoleARN != nil {
+		args = append(args, fmt.Sprintf("--aws-assume-role=%s", *b.externalDNS.Spec.AWSRoleARN))


I'm wondering whether we should expose the external ID as the API knob too. ExternalDNS has a dedicated flag for it. I see that we didn't bother with it in C-I-O.

That's a good point, I hadn't heard of the ExternalID until now. But, if we didn't bother with it in CIO, I wonder if we should bother with it here. I can point it out to the Shared VPC folks so they are aware something like this exists.

alebedev87 · 2023-06-27T15:58:39Z

@gcs278 : what are your thoughts about the testing of this feature?

openshift-ci-robot · 2023-06-30T21:02:11Z

@gcs278: This pull request references NE-1323 which is a valid jira issue.

Details

In response to this:

Added an API for AWSRoleArn and propagated to the argument --aws-assume-role.

Needs work, review, testing, enhancement, jira.

But I just confirmed this POC works with a real Shared-VPC cluster.

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.

openshift-ci-robot · 2023-07-07T20:49:52Z

@gcs278: This pull request references NE-1323 which is a valid jira issue.

Details

In response to this:

Adds the ability to specify a RoleARN to create Route 53 DNS records in a different AWS Account. Supports Shared VPC scenario.

The new API is propagated to the existing external-dns argument --aws-assume-role.

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.

openshift-ci-robot · 2023-07-07T20:50:08Z

@gcs278: This pull request references NE-1323 which is a valid jira issue.

Details

In response to this:

Adds the ability to specify a RoleARN to create Route 53 DNS records in a different AWS Account. Supports Shared VPC scenario.

The new API is propagated to the existing external-dns argument --aws-assume-role.

Epic Link: https://issues.redhat.com/browse/NE-1299

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.

gcs278 · 2023-07-07T21:01:54Z

@gcs278 : what are your thoughts about the testing of this feature?

I would like to E2E test it, but we need a E2E test job that stands up a shared VPC in Account A, and stands up a cluster with a role ARN in Account B. My assumption is that we need a new, seperate E2E job that does these things as prerequisites, and separately runs a subset of Shared VPC E2E tests.

I'm waiting on https://issues.redhat.com/browse/CORS-2650 to finish up, as this story will hopefully paving a way, creating items in the CI Step Registry that we could potentially leverage.

Do you have any thoughts?

gcs278 · 2023-07-17T20:43:38Z

@alebedev87 thanks for your time. Update:
https://github.com/openshift/external-dns-operator/compare/840c17c4d4ac43b3a7d0bcc51b5cf04b4fe5056a..2c7e1d8c69ab95cb7049fb54b0b8db89b4629edf

Moved CRD validation to webhook to use IsARN() from AWS SDK
Made spec.provider.aws.credentials optional in CRD
Added another webhook validation test for RoleARN in openshift platform
Fixed broken unit tests

gcs278

Thanks for the review @alebedev87

I'm working on E2E tests at the moment. I may add them to this PR if it's still open, but I don't think we have to hold this PR up for the E2E tests.

gcs278 · 2023-09-05T23:16:18Z

+    - '{{.Name}}.mydomain.net'
+```
+
+**Note**: Due to a limitation of the `v1beta1` API requiring the `credentials` field, OpenShift users will be required


Okay thanks. I'll leave what I have now, and possibly open another PR later with details on CredentialsRequest to keep this PR cleaner.

gcs278 · 2023-09-05T23:24:24Z

+					AWS: &ExternalDNSAWSProviderOptions{
+						AssumeRole: &ExternalDNSAWSAssumeRoleOptions{
+							ARN: pointer.String("arn:aws:iam::123456789012:role/foo"),
+						},
+					},


This test is for OpenShift, and the CRDs aren't involved, just the webhook. The webhook doesn't validate credentials for Openshift. The CRDs were the issue due to +required on the Credentials field.

The webhook works without issue, but use of the CRD would require an empty credentials field.

alebedev87 · 2023-09-06T07:57:50Z

I'm working on E2E tests at the moment. I may add them to this PR if it's still open, but I don't think we have to hold this PR up for the E2E tests.

Agree, we don't need to hold this PR. Do you plan to add e2e test in another PR then?

alebedev87 · 2023-09-08T21:48:54Z

/lgtm

gcs278 · 2023-09-08T23:58:32Z

/test verify

gcs278 · 2023-09-11T17:11:03Z

@alebedev87 forgot to run make manifests

alebedev87 · 2023-09-11T17:11:20Z

/lgtm

Adds the ability to specify a Role ARN to create Route 53 DNS records in a different AWS Account. Supports Shared VPC scenario. `api/v1beta1/externaldns_types.go`: Add AssumeRole API `api/v1beta1/webhook_test.go`: Add AssumeRole WebHook Validation test `api/v1beta1/zz_generated.deepcopy.go`: Generated `bundle/manifests/externaldns.olm.openshift.io_externaldnses.yaml`: Generated `config/crd/bases/externaldns.olm.openshift.io_externaldnses.yaml`: Generated `docs/usage.md`: Add section on AWS Assume Role `pkg/operator/controller/externaldns/credentials_request.go`: Add sts:AssumeRole to credential requests `pkg/operator/controller/externaldns/deployment_test.go`: Unit test `desiredExternalDNSDeployment` `pkg/operator/controller/externaldns/pod.go`: Add --aws-assume-role argument with AssumeRole ARN value

alebedev87 · 2023-09-11T21:35:33Z

/lgtm
/approve

openshift-ci · 2023-09-11T21:35:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alebedev87

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [alebedev87]

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

alebedev87 · 2023-09-12T08:19:51Z

Cluster installation failed.

/test e2e-gcp-operator

gcs278 · 2023-09-12T15:55:06Z

cluster install failure
/test e2e-gcp-operator

CFields651 · 2023-09-12T18:30:18Z

/label px-approved

melvinjoseph86 · 2023-09-13T16:35:56Z

/label qe-approved

gcs278 · 2023-09-13T16:57:43Z

Cluster install failed
/test e2e-gcp-operator

openshift-ci · 2023-09-13T18:10:54Z

@gcs278: all tests passed!

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

ahardin-rh · 2023-09-18T17:26:42Z

/label docs-approved

openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 26, 2023

openshift-ci Bot requested review from candita and lmzuccarelli June 26, 2023 18:28

alebedev87 reviewed Jun 27, 2023

View reviewed changes

gcs278 force-pushed the shared-vpc branch from 842551b to 9c8d207 Compare June 27, 2023 21:00

gcs278 commented Jun 27, 2023

View reviewed changes

Comment thread api/v1beta1/externaldns_types.go

gcs278 changed the title ~~WIP: Proof of concept for Shared VPC support~~ [WIP] NE-1323: Proof of concept for Shared VPC support Jun 30, 2023

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jun 30, 2023

gcs278 changed the title ~~[WIP] NE-1323: Proof of concept for Shared VPC support~~ [WIP] NE-1323: Shared VPC support Jun 30, 2023

gcs278 force-pushed the shared-vpc branch from 9c8d207 to 3f39d65 Compare July 7, 2023 20:41

gcs278 changed the title ~~[WIP] NE-1323: Shared VPC support~~ NE-1323: Add AWS RoleARN for Shared VPC support Jul 7, 2023

openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 7, 2023

gcs278 force-pushed the shared-vpc branch from 3f39d65 to 2ff7e48 Compare July 7, 2023 20:44

gcs278 commented Jul 7, 2023

View reviewed changes

Comment thread api/v1beta1/externaldns_types.go

Comment thread api/v1beta1/webhook_test.go

gcs278 force-pushed the shared-vpc branch from 2ff7e48 to d474491 Compare July 7, 2023 20:49

gcs278 force-pushed the shared-vpc branch from d474491 to abb7610 Compare July 7, 2023 20:53

gcs278 force-pushed the shared-vpc branch from abb7610 to 840c17c Compare July 7, 2023 21:08

gcs278 commented Jul 7, 2023

View reviewed changes

Comment thread pkg/operator/controller/externaldns/credentials_request.go

alebedev87 self-assigned this Jul 7, 2023

gcs278 force-pushed the shared-vpc branch 3 times, most recently from c3b0f26 to 2c7e1d8 Compare July 17, 2023 20:39

alebedev87 mentioned this pull request Jul 17, 2023

Support for IRSA for AWS Provider #190

Closed

gcs278 force-pushed the shared-vpc branch from 2c7e1d8 to b4e7cbc Compare July 17, 2023 22:54

gcs278 commented Sep 5, 2023

View reviewed changes

gcs278 mentioned this pull request Sep 7, 2023

NE-1325: External DNS Operator support for Shared VPCs openshift/enhancements#1436

Merged

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Sep 8, 2023

gcs278 mentioned this pull request Sep 8, 2023

NE-1324: E2E tests for Assume Role in Shared VPC Cluster #198

Merged

gcs278 force-pushed the shared-vpc branch from 42fe267 to b563c34 Compare September 11, 2023 17:10

openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 11, 2023

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Sep 11, 2023

gcs278 force-pushed the shared-vpc branch from b563c34 to d5ce581 Compare September 11, 2023 21:04

openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 11, 2023

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Sep 11, 2023

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 11, 2023

openshift-ci Bot added the px-approved Signifies that Product Support has signed off on this PR label Sep 12, 2023

openshift-ci Bot added the qe-approved Signifies that QE has signed off on this PR label Sep 13, 2023

openshift-ci Bot added the docs-approved Signifies that Docs has signed off on this PR label Sep 18, 2023

openshift-merge-robot merged commit 5e964d7 into openshift:main Sep 18, 2023

Uh oh!

Conversation

gcs278 commented Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alebedev87 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alebedev87 Jun 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gcs278 Jun 27, 2023

Choose a reason for hiding this comment

Uh oh!

gcs278 Jun 27, 2023

Choose a reason for hiding this comment

Uh oh!

alebedev87 commented Jun 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

openshift-ci-robot commented Jun 30, 2023 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

openshift-ci-robot commented Jul 7, 2023 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Jul 7, 2023 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gcs278 commented Jul 7, 2023

Uh oh!

Uh oh!

gcs278 commented Jul 17, 2023

Uh oh!

gcs278 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gcs278 Sep 5, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gcs278 Sep 5, 2023

Choose a reason for hiding this comment

Uh oh!

alebedev87 commented Sep 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alebedev87 commented Sep 8, 2023

Uh oh!

gcs278 commented Sep 8, 2023

Uh oh!

gcs278 commented Sep 11, 2023

Uh oh!

alebedev87 commented Sep 11, 2023

Uh oh!

alebedev87 commented Sep 11, 2023

Uh oh!

openshift-ci Bot commented Sep 11, 2023

Uh oh!

alebedev87 commented Sep 12, 2023

Uh oh!

gcs278 commented Sep 12, 2023

Uh oh!

CFields651 commented Sep 12, 2023

Uh oh!

melvinjoseph86 commented Sep 13, 2023

Uh oh!

gcs278 commented Sep 13, 2023

Uh oh!

openshift-ci Bot commented Sep 13, 2023

Uh oh!

ahardin-rh commented Sep 18, 2023

gcs278 commented Jun 26, 2023 •

edited

Loading

alebedev87 Jun 27, 2023 •

edited

Loading

alebedev87 commented Jun 27, 2023 •

edited

Loading

openshift-ci-robot commented Jun 30, 2023 •

edited by openshift-ci Bot

Loading

openshift-ci-robot commented Jul 7, 2023 •

edited by openshift-ci Bot

Loading

openshift-ci-robot commented Jul 7, 2023 •

edited by openshift-ci Bot

Loading

alebedev87 commented Sep 6, 2023 •

edited

Loading