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

Jump to bottom

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

Merged
merged 3 commits into from
Apr 12, 2024
Merged

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

merged 3 commits into from
Apr 12, 2024

Conversation

gcs278
Copy link
Contributor

@gcs278 gcs278 commented Jul 3, 2023 •
edited
Loading

Add the externaldns-operator-aws-assume-role enhancement to add support for sharing a hosted zone between two AWS accounts using assume role.

Update the aws-cross-account-dns-zone.md enhancement to include references to externaldns-operator-aws-assume-role as well as clean up some invalid references and add alternative on ExternalID.

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

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 3, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented Jul 3, 2023 •
edited by openshift-ci bot
Loading

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

In response to this:

Update the aws-cross-account-dns-zone.md enhancement to include details on how we are updating the External DNS Operator to support cross account DNS record creation in AWS shard VPCs.

Epic: 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 gcs278 force-pushed the external-dns-shared-vpc branch 3 times, most recently from d9cb4d8 to bcd9273 Compare July 3, 2023 21:30
gcs278
gcs278 commented Jul 5, 2023
View reviewed changes
Copy link
Contributor Author

@gcs278 gcs278 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 a quick review @Miciah

enhancements/installer/aws-cross-account-dns-zone.md Outdated Show resolved Hide resolved
enhancements/installer/aws-cross-account-dns-zone.md Outdated Show resolved Hide resolved
enhancements/installer/aws-cross-account-dns-zone.md Outdated Show resolved Hide resolved
enhancements/installer/aws-cross-account-dns-zone.md Outdated Show resolved Hide resolved
@Miciah
Copy link
Contributor

Miciah commented Jul 12, 2023

/assign

@candita
Copy link
Contributor

candita commented Jul 12, 2023

/assign

candita
candita reviewed Jul 13, 2023
View reviewed changes
enhancements/installer/aws-cross-account-dns-zone.md
Copy link
Contributor

@candita candita Jul 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Did you address anything that would necessitate removing the text about the open questions in L142 and L165? If not, just leave them there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't address anything, but the lines needed to be cleaned up as they don't make sense anymore.

For The name of the field is discussed further in open questions. and Please see Open Questions for further discussion of the install config., there are no open questions, so these didn't make sense.

CC: @patrickdillon was the original author

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll let him review this change then. I'll just review the stuff in the new EP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll ping him for a review.

candita
candita reviewed Jul 13, 2023
View reviewed changes
enhancements/installer/aws-cross-account-dns-zone.md Outdated

Prior to this enhancement, the [External DNS Operator API](https://github.com/openshift/external-dns-operator/blob/main/api)
did not allow users to configure an AWS role ARN field. As a result, there wasn't a supported way to use the External
DNS Operator to create DNS records in another AWS account within a shared VPC.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any built-in installer support ? I thought ExternalDNS was a day-two installation option, meaning none of it is handled by the OpenShift installer. How does this new functionality integrate with OpenShift Installer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, ExternalDNS is a day-two operation, installed as a addon operator.

It doesn't integrate with the installer for this feature. It effectively provides the same feature as the installer and Ingress Operator implement together: support for creating a DNS record in an another zone.

gcs278
gcs278 commented Jul 14, 2023
View reviewed changes
Copy link
Contributor Author

@gcs278 gcs278 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 the review.

enhancements/installer/aws-cross-account-dns-zone.md Outdated Show resolved Hide resolved
enhancements/installer/aws-cross-account-dns-zone.md Outdated Show resolved Hide resolved
enhancements/installer/aws-cross-account-dns-zone.md Outdated Show resolved Hide resolved
enhancements/installer/aws-cross-account-dns-zone.md Outdated

Prior to this enhancement, the [External DNS Operator API](https://github.com/openshift/external-dns-operator/blob/main/api)
did not allow users to configure an AWS role ARN field. As a result, there wasn't a supported way to use the External
DNS Operator to create DNS records in another AWS account within a shared VPC.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, ExternalDNS is a day-two operation, installed as a addon operator.

It doesn't integrate with the installer for this feature. It effectively provides the same feature as the installer and Ingress Operator implement together: support for creating a DNS record in an another zone.

alebedev87
alebedev87 reviewed Jul 17, 2023
View reviewed changes
Copy link
Contributor

@alebedev87 alebedev87 left a comment

Choose a reason for hiding this comment

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

Some clarifications asked.

enhancements/installer/aws-cross-account-dns-zone.md Outdated Show resolved Hide resolved
enhancements/installer/aws-cross-account-dns-zone.md Outdated Show resolved Hide resolved
enhancements/installer/aws-cross-account-dns-zone.md Show resolved Hide resolved
gcs278
gcs278 commented Jul 19, 2023
View reviewed changes
Copy link
Contributor Author

@gcs278 gcs278 left a comment

Choose a reason for hiding this comment

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

Forgot to post this on Monday when I pushed up my fixes.

enhancements/installer/aws-cross-account-dns-zone.md Outdated Show resolved Hide resolved
enhancements/installer/aws-cross-account-dns-zone.md Outdated Show resolved Hide resolved
enhancements/installer/aws-cross-account-dns-zone.md Show resolved Hide resolved
@gcs278 gcs278 mentioned this pull request Jul 19, 2023
Merged
alebedev87
alebedev87 reviewed Jul 19, 2023
View reviewed changes
Copy link
Contributor

@alebedev87 alebedev87 left a comment

Choose a reason for hiding this comment

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

A couple of comments about passing Credentials to optional.

enhancements/installer/aws-cross-account-dns-zone.md Outdated Show resolved Hide resolved
enhancements/installer/aws-cross-account-dns-zone.md Outdated Show resolved Hide resolved
@gcs278
Copy link
Contributor Author

gcs278 commented Jan 2, 2024

/reopen

@openshift-ci openshift-ci bot reopened this Jan 2, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented Jan 2, 2024 •
edited by openshift-ci bot
Loading

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

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

Add the externaldns-operator-aws-assume-role enhancement to add support for sharing a hosted zone between two AWS accounts using assume role.

Update the aws-cross-account-dns-zone.md enhancement to include references to externaldns-operator-aws-assume-role as well as clean up some invalid references and add alternative on ExternalID.

Epic: 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.

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jan 2, 2024

@gcs278: Reopened this PR.

In response to this:

/reopen

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
Copy link
Contributor Author

gcs278 commented Jan 4, 2024

/remove-lifecycle rotten

@openshift-ci openshift-ci bot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Jan 4, 2024
@openshift-bot
Copy link

Inactive enhancement proposals go stale after 28d of inactivity.

See https://github.com/openshift/enhancements#life-cycle for details.

Mark the proposal as fresh by commenting /remove-lifecycle stale.
Stale proposals rot after an additional 7d of inactivity and eventually close.
Exclude this proposal from closing by commenting /lifecycle frozen.

If this proposal is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 2, 2024
@openshift-bot
Copy link

Stale enhancement proposals rot after 7d of inactivity.

See https://github.com/openshift/enhancements#life-cycle for details.

Mark the proposal as fresh by commenting /remove-lifecycle rotten.
Rotten proposals close after an additional 7d of inactivity.
Exclude this proposal from closing by commenting /lifecycle frozen.

If this proposal is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

@openshift-ci openshift-ci bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 9, 2024
@dhellmann
Copy link
Contributor

#1555 is changing the enhancement template in a way that will cause the header check in the linter job to fail for existing PRs. If this PR is merged within the development period for 4.16 you may override the linter if the only failures are caused by issues with the headers (please make sure the markdown formatting is correct). If this PR is not merged before 4.16 development closes, please update the enhancement to conform to the new template.

@Miciah
Copy link
Contributor

Miciah commented Feb 13, 2024

/remove-lifecycle rotten

@openshift-ci openshift-ci bot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Feb 13, 2024
@openshift-bot
Copy link

Inactive enhancement proposals go stale after 28d of inactivity.

See https://github.com/openshift/enhancements#life-cycle for details.

Mark the proposal as fresh by commenting /remove-lifecycle stale.
Stale proposals rot after an additional 7d of inactivity and eventually close.
Exclude this proposal from closing by commenting /lifecycle frozen.

If this proposal is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 13, 2024
@Miciah
Copy link
Contributor

Miciah commented Mar 19, 2024

/remove-lifecycle stale

@openshift-ci openshift-ci bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 19, 2024
@alebedev87
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 10, 2024
@alebedev87 alebedev87 mentioned this pull request Apr 10, 2024
Open
@candita
Copy link
Contributor

candita commented Apr 11, 2024

Discussed with team, this is good to go.
/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 11, 2024
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD eb17554 and 2 for PR HEAD daec8f5 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD b99a80d and 1 for PR HEAD daec8f5 in total

@Miciah
Copy link
Contributor

Miciah commented Apr 12, 2024 •
edited
Loading

ci/prow/markdownlint failed with the following errors:

Checking enhancements/dns/externaldns-operator-aws-assume-role.md
enhancements/dns/externaldns-operator-aws-assume-role.md missing "### Topology Considerations"
enhancements/dns/externaldns-operator-aws-assume-role.md missing "#### Hypershift / Hosted Control Planes"
enhancements/dns/externaldns-operator-aws-assume-role.md missing "#### Standalone Clusters"
enhancements/dns/externaldns-operator-aws-assume-role.md missing "#### Single-node Deployments or MicroShift"
enhancements/dns/externaldns-operator-aws-assume-role.md missing "## Test Plan"
enhancements/dns/externaldns-operator-aws-assume-role.md missing "## Graduation Criteria"
enhancements/dns/externaldns-operator-aws-assume-role.md missing "### Dev Preview -> Tech Preview"
enhancements/dns/externaldns-operator-aws-assume-role.md missing "### Tech Preview -> GA"
enhancements/dns/externaldns-operator-aws-assume-role.md missing "### Removing a deprecated feature"
enhancements/dns/externaldns-operator-aws-assume-role.md missing "## Upgrade / Downgrade Strategy"
enhancements/dns/externaldns-operator-aws-assume-role.md missing "## Version Skew Strategy"
enhancements/dns/externaldns-operator-aws-assume-role.md missing "## Operational Aspects of API Extensions"
enhancements/dns/externaldns-operator-aws-assume-role.md missing "## Support Procedures"

Overriding per #1436 (comment):

If this PR is merged within the development period for 4.16 you may override the linter if the only failures are caused by issues with the headers (please make sure the markdown formatting is correct).

/override ci/prow/markdownlint

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Apr 12, 2024

@Miciah: Overrode contexts on behalf of Miciah: ci/prow/markdownlint

In response to this:

ci/prow/markdownlint failed with the following errors:

Checking enhancements/dns/externaldns-operator-aws-assume-role.md
enhancements/dns/externaldns-operator-aws-assume-role.md missing "### Topology Considerations"
enhancements/dns/externaldns-operator-aws-assume-role.md missing "#### Hypershift / Hosted Control Planes"
enhancements/dns/externaldns-operator-aws-assume-role.md missing "#### Standalone Clusters"
enhancements/dns/externaldns-operator-aws-assume-role.md missing "#### Single-node Deployments or MicroShift"
enhancements/dns/externaldns-operator-aws-assume-role.md missing "## Test Plan"
enhancements/dns/externaldns-operator-aws-assume-role.md missing "## Graduation Criteria"
enhancements/dns/externaldns-operator-aws-assume-role.md missing "### Dev Preview -> Tech Preview"
enhancements/dns/externaldns-operator-aws-assume-role.md missing "### Tech Preview -> GA"
enhancements/dns/externaldns-operator-aws-assume-role.md missing "### Removing a deprecated feature"
enhancements/dns/externaldns-operator-aws-assume-role.md missing "## Upgrade / Downgrade Strategy"
enhancements/dns/externaldns-operator-aws-assume-role.md missing "## Version Skew Strategy"
enhancements/dns/externaldns-operator-aws-assume-role.md missing "## Operational Aspects of API Extensions"
enhancements/dns/externaldns-operator-aws-assume-role.md missing "## Support Procedures"

Overriding per #1436 (comment):

If this PR is merged within the development period for 4.16 you may override the linter if the only failures are caused by issues with the headers (please make sure the markdown formatting is correct).

/override ci/prow/markdownlint

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 OpenShift CI
Copy link
Contributor

openshift-ci bot commented Apr 12, 2024

@gcs278: all tests passed!

Full PR test history. Your PR dashboard.

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.

@openshift-merge-bot openshift-merge-bot bot merged commit e7d490a into openshift:master Apr 12, 2024
2 checks passed
@yuqi-zhang yuqi-zhang mentioned this pull request Apr 23, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Miciah Miciah Miciah left review comments

@candita candita candita left review comments

@alebedev87 alebedev87 alebedev87 left review comments

@lmzuccarelli lmzuccarelli Awaiting requested review from lmzuccarelli

@spadgett spadgett Awaiting requested review from spadgett

Assignees

@Miciah Miciah

@candita candita

@patrickdillon patrickdillon

@alebedev87 alebedev87

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@gcs278 @openshift-ci-robot @Miciah @candita @openshift-bot @alebedev87 @patrickdillon @melvinjoseph86 @dhellmann