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

[release-4.13] NE-1341: Add support for AWS shared VPC in another account #966

Merged

Conversation

gcs278
Copy link
Contributor

@gcs278 gcs278 commented Jul 20, 2023

Add support for configuring DNS records in AWS Route 53 using a separate account for the private hosted zone.

This PR cherry-picks 7251aaa (from #928, the original implementation) and 047bd98 (from #951, a bug fix for https://issues.redhat.com/browse/OCPBUGS-14998).

How I generated this backport:

  1. Created a new commit to vendor openshift/api backport [release-4.13] OCPSTRAT-659: DNS: Add awsPrivateHostedZoneRole api#1529
    • Modified go.mod to use github.com/openshift/api v0.0.0-20230803134339-2d9b46419536
    • go mod tidy
    • go mod vendor
  2. git cherry-pick 7251aaa9a30f3759c2731d061207d2f63cd945de (7251aaa)
  3. Resolved merge conflicts:
    • pkg/manifests/bindata.go: Regenerated
    • pkg/operator/controller/dns/controller.go: Removed PrivateHostedZoneAWSEnabled from config structure for the dns controller. Removed associated check for r.config.PrivateHostedZoneAWSEnabled
    • pkg/operator/operator.go: 7251aaa had feature gate logic. We are not backporting the feature gate so I did not integrate any feature gate logic in the merge.
  4. Completed cherry-pick with add, git cherry-pick --continue, etc...
  5. git cherry-pick 047bd986587a923029586a2497ec99d0a5a242a0 (047bd98)
    • No merge conflicts

@gcs278 gcs278 force-pushed the release-4.13-shared-vpc branch from 9dac29d to ee6dcce Compare July 20, 2023 21:17
@gcs278 gcs278 changed the title [release-4.13] Add support for AWS shared VPC in another account [release-4.13] NE-1341: Add support for AWS shared VPC in another account Jul 25, 2023
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jul 25, 2023

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

In response to this:

Add support for configuring DNS records in AWS Route 53 using a separate account for the private hosted zone.

This PR cherry-picks 7251aaa (from #928, the original implementation) and 047bd98 (from #951, a bug fix for https://issues.redhat.com/browse/OCPBUGS-14998).

How I generated this backport:

  1. git cherry-pick 7251aaa9a30f3759c2731d061207d2f63cd945de (7251aaa)
  2. Resolved merge conflicts:
    • pkg/manifests/bindata.go: Regenerated
    • pkg/operator/controller/dns/controller.go: Removed PrivateHostedZoneAWSEnabled from config structure for the dns controller. Removed associated check for r.config.PrivateHostedZoneAWSEnabled
    • pkg/operator/operator.go: 7251aaa had feature gate logic. We are not backporting the feature gate so I did not integrate any feature gate logic in the merge.
  3. Completed cherry-pick with add, git cherry-pick --continue, etc...
  4. Created a new commit to vendor openshift/api backport [release-4.13] OCPSTRAT-659: DNS: Add awsPrivateHostedZoneRole api#1529
    • Modified go.mod to use github.com/patrickdillon/api v0.0.0-20230717195958-74684214e356
    • go mod tidy
    • go mod vendor
  5. git cherry-pick 047bd986587a923029586a2497ec99d0a5a242a0 (047bd98)
    • No merge conflicts

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 openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 25, 2023
@melvinjoseph86
Copy link

/test e2e-aws-operator

@Miciah
Copy link
Contributor

Miciah commented Aug 1, 2023

The order of the commits looks funny:

% git --no-pager log1 --no-decorate --reverse origin/release-4.13..gcs278/release-4.13-shared-vpc --
77943a4d7 Add support for AWS shared VPC in another account
f5eaf5ec9 Bump openshift/api for PrivateHostedZoneAWS
ee6dccea5 OCPBUGS-14998: Only use RoleARN for Route53 API

Going from oldest to newest, the bump should be first, then the implementation, and then the bug fix. Otherwise the changes look fine.
/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 1, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miciah

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 1, 2023
@gcs278 gcs278 force-pushed the release-4.13-shared-vpc branch from ee6dcce to a0e5846 Compare August 1, 2023 17:30
@gcs278
Copy link
Contributor Author

gcs278 commented Aug 1, 2023

The order of the commits looks funny

@Miciah Good catch, that makes sense so the implementation commit isn't broken.

https://github.com/openshift/cluster-ingress-operator/compare/ee6dccea564293606159ff6854cb084072d1d199..a0e58466f0ed23fee3e3334a15559133a21eb98b reorders the commits.

@Miciah
Copy link
Contributor

Miciah commented Aug 1, 2023

Looks good; we just need the API backport to merge now so that the bump commit can have a proper ref.

@melvinjoseph86
Copy link

/retest-required

@melvinjoseph86
Copy link

/test e2e-aws-operator

@gcs278
Copy link
Contributor Author

gcs278 commented Aug 2, 2023

/assign @Miciah

@gcs278
Copy link
Contributor Author

gcs278 commented Aug 2, 2023

/hold
Should be on hold as it's still using Patrick's unofficial API branch and not ready to merge. @patrickdillon may be recreating this PR with the official API merge depending on timing.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 2, 2023
gcs278 and others added 3 commits August 3, 2023 12:27
Bump to github.com/openshift/api@2d9b4641953634dd191455e2399189b57a099a08
to get the new "PrivateHostedZoneAWS" feature gate
and the DNS.spec.platform.aws.privateZoneIAMRole API field
to allow configuring a private hosted zone in a shared VPC.

* go.mod: Bump openshift/api.
* go.sum:
* vendor/*: Regenerate.
Add support for configuring DNS records in AWS Route 53 using a separate
account for the private hosted zone.

This commit resolves NE-1294.

https://issues.redhat.com/browse/NE-1294

* manifests/00-ingress-credentials-request.yaml: Add "sts:AssumeRole" to
the CredentialsRequest for AWS.
* pkg/manifests/bindata.go: Regenerate.
* pkg/dns/aws/dns.go (Config): Add a RoleARN field.
(NewProvider): If config.RoleARN is set, use it to configure the AWS client
using the specified role.
* pkg/dns/split/dns.go: New file.  Define a DNS provider implementation
that wraps two other DNS providers, using one of them to publish records to
the public zone and the other to publish records to the private zone.
(Provider): New type.  Store the private and public DNS providers, as well
as the private zone so that the Ensure, Delete, and Replace methods can use
it to determine whether they are publishing to the public zone or to the
private zone.
(NewProvider): New function.  Return a split DNS provider.
(Ensure, Delete, Replace): New methods.  Implement the dns.Provider
interface by calling the respective methods on the wrapped private and
public DNS providers.
* pkg/dns/split/dns_test.go (TestSplitDNSProvider): Verify that the split
DNS provider correctly dispatches to the private or public DNS provider as
appropriate, using fakeProvider.
(fakeProvider): New type.  Define a fake named DNS provider that records
its name when invoked.
(Ensure, Delete, Replace): New methods for fakeProvider to record
invocations and implement the dns.Provider interface.
(newFakeProvider): New function.  Return a fake provider.
* pkg/operator/controller/dns/controller.go (Config): Add a
"PrivateHostedZoneAWSEnabled" field to indicate whether the
"PrivateHostedZoneAWS" feature gate is enabled.
(createDNSProvider): Use the new split DNS provider and the AWS DNS
provider's new RoleARN configuration option to configure separate DNS
providers for public and private zones when a role ARN for the private zone
is specified in the cluster infrastructure config if the
"PrivateHostedZoneAWS" feature gate is enabled.
* pkg/operator/operator.go (New): Check the "PrivateHostedZoneAWS" feature
gate and specify it in the DNS controller config.
To support Shared VPC, we split the DNS client into public and private
providers, the private using the RoleARN (Account A) and the public
using the default (Account B). However, the RoleARN only provides API
access for Account A's Route53 service, not the ability to describe
Account B's ELBs. This fix isolates the RoleARN to only be used with
Route53 API services.

`pkg/dns/aws/dns.go`: Create a separate Route53 session object that
uses the RoleARN when provided.
@Miciah Miciah force-pushed the release-4.13-shared-vpc branch from a0e5846 to 1c6bac8 Compare August 3, 2023 16:28
@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 Aug 3, 2023
@patrickdillon
Copy link
Contributor

@patrickdillon may be recreating this PR with the official API merge depending on timing.

https://github.com/openshift/cluster-ingress-operator/compare/a0e58466f0ed23fee3e3334a15559133a21eb98b..1c6bac83c205164bb7e18d9715359f7c6818742e bumps to openshift/api@2d9b464 since openshift/api#1529 merged. /hold cancel /test all

Looks like miciah was able to push directly to your remote and keep the original PR. Nice! Thanks!

@Miciah
Copy link
Contributor

Miciah commented Aug 5, 2023

/lgtm

This change adds sts:AssumeRole to the CredentialsRequest and otherwise does not change the operator's behavior unless the new spec.platform.aws.privateZoneIAMRole option in the cluster DNS config is specified.
/label backport-risk-assessed

e2e-aws-operator failed because must-gather failed.
/test e2e-aws-operator

@openshift-ci openshift-ci bot added the backport-risk-assessed Indicates a PR to a release branch has been evaluated and considered safe to accept. label Aug 5, 2023
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 5, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 5, 2023

@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.

@melvinjoseph86
Copy link

/label cherry-pick-approved

@openshift-ci openshift-ci bot added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Aug 6, 2023
@mrunalp mrunalp added the jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. label Aug 7, 2023
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Aug 7, 2023

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

Retaining the jira/valid-bug label as it was manually added.

In response to this:

Add support for configuring DNS records in AWS Route 53 using a separate account for the private hosted zone.

This PR cherry-picks 7251aaa (from #928, the original implementation) and 047bd98 (from #951, a bug fix for https://issues.redhat.com/browse/OCPBUGS-14998).

How I generated this backport:

  1. git cherry-pick 7251aaa9a30f3759c2731d061207d2f63cd945de (7251aaa)
  2. Resolved merge conflicts:
    • pkg/manifests/bindata.go: Regenerated
    • pkg/operator/controller/dns/controller.go: Removed PrivateHostedZoneAWSEnabled from config structure for the dns controller. Removed associated check for r.config.PrivateHostedZoneAWSEnabled
    • pkg/operator/operator.go: 7251aaa had feature gate logic. We are not backporting the feature gate so I did not integrate any feature gate logic in the merge.
  3. Completed cherry-pick with add, git cherry-pick --continue, etc...
  4. Created a new commit to vendor openshift/api backport [release-4.13] OCPSTRAT-659: DNS: Add awsPrivateHostedZoneRole api#1529
    • Modified go.mod to use github.com/openshift/api v0.0.0-20230803134339-2d9b46419536
    • go mod tidy
    • go mod vendor
  5. git cherry-pick 047bd986587a923029586a2497ec99d0a5a242a0 (047bd98)
    • No merge conflicts

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-merge-robot openshift-merge-robot merged commit 78a28b2 into openshift:release-4.13 Aug 7, 2023
openshift-merge-robot added a commit that referenced this pull request Sep 7, 2023
[release-4.12] NE-1372: Add support for AWS shared VPC in another account #966
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. backport-risk-assessed Indicates a PR to a release branch has been evaluated and considered safe to accept. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants