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

NE-1294: Add support for AWS shared VPC in another account #928

Conversation

Miciah
Copy link
Contributor

@Miciah Miciah commented May 6, 2023

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

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

This PR is a draft because the Infrastructure.status.platformStatus.aws.privateHostedZoneRole API field doesn't exist yet and this is completely untested.

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

openshift-ci-robot commented May 6, 2023

@Miciah: This pull request references NE-1294 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.

  • 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 (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.

This PR is a draft because the Infrastructure.status.platformStatus.aws.privateHostedZoneRole API field doesn't exist yet and this is completely untested.

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 bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 6, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 6, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@Miciah Miciah force-pushed the NE-1294-add-support-for-AWS-shared-VPC-in-another-account branch 2 times, most recently from 682d60b to e336dd1 Compare May 18, 2023 20:58
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 18, 2023
@Miciah Miciah force-pushed the NE-1294-add-support-for-AWS-shared-VPC-in-another-account branch from e336dd1 to 584d8fd Compare May 22, 2023 20:17
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 22, 2023
@Miciah Miciah force-pushed the NE-1294-add-support-for-AWS-shared-VPC-in-another-account branch from 584d8fd to 064d847 Compare May 22, 2023 20:21
@Miciah
Copy link
Contributor Author

Miciah commented May 22, 2023

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented May 22, 2023

@Miciah: This pull request references NE-1294 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.

  • 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 SharedVPCEnabled field to indicate whether the "SharedVPC" 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 "SharedVPC" feature gate is enabled.
  • pkg/operator/operator.go (New): Check the "SharedVPC" feature gate and specify it in the DNS controller config.

This PR is a draft because the Infrastructure.status.platformStatus.aws.privateHostedZoneRole API field doesn't exist yet and this is completely untested.

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.

@Miciah Miciah force-pushed the NE-1294-add-support-for-AWS-shared-VPC-in-another-account branch from 064d847 to c064b43 Compare May 23, 2023 14:01
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented May 23, 2023

@Miciah: This pull request references NE-1294 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.

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

This PR is a draft because the Infrastructure.status.platformStatus.aws.privateHostedZoneRole API field doesn't exist yet and this is completely untested.

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.

@Miciah
Copy link
Contributor Author

Miciah commented May 23, 2023

https://github.com/openshift/cluster-ingress-operator/compare/064d8477b16e8551802a8fb3025bfe65eca5c8e2..c064b43a8dafbb43d5e240e3a875ddfa4d85e857 bumps the vendored openshift/api to get the actual PrivateHostedZoneAWS feature gate that was added in openshift/api#1465. This bump also makes the spec.platform field of the DNS config a pointer value.

@gcs278
Copy link
Contributor

gcs278 commented May 23, 2023

I'm still keeping an eye on this, thanks for the responsiveness, updates look good to me

@Miciah Miciah force-pushed the NE-1294-add-support-for-AWS-shared-VPC-in-another-account branch from c064b43 to 1bee175 Compare May 30, 2023 18:21
@Miciah
Copy link
Contributor Author

Miciah commented May 30, 2023

@Miciah Miciah force-pushed the NE-1294-add-support-for-AWS-shared-VPC-in-another-account branch from 1bee175 to 112d591 Compare May 31, 2023 00:20
@Miciah
Copy link
Contributor Author

Miciah commented May 31, 2023

@Miciah Miciah marked this pull request as ready for review May 31, 2023 00:22
@openshift-ci 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 May 31, 2023
@openshift-ci openshift-ci bot requested review from alebedev87 and miheer May 31, 2023 00:23
@romfreiman
Copy link

/test ?

@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 31, 2023

@romfreiman: The following commands are available to trigger required jobs:

  • /test e2e-aws-operator
  • /test e2e-aws-ovn
  • /test e2e-aws-ovn-serial
  • /test e2e-aws-ovn-upgrade
  • /test e2e-azure-operator
  • /test e2e-gcp-operator
  • /test e2e-hypershift
  • /test images
  • /test unit
  • /test verify

The following commands are available to trigger optional jobs:

  • /test e2e-aws-ovn-single-node
  • /test e2e-azure-ovn
  • /test e2e-gcp-ovn

Use /test all to run all jobs.

In response to this:

/test ?

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.

@romfreiman
Copy link

/test e2e-aws-ovn-upgrade

@romfreiman
Copy link

/test e2e-hypershift

@gcs278
Copy link
Contributor

gcs278 commented May 31, 2023

Unrelated hypershift issues:

NAME      VERSION   AVAILABLE   PROGRESSING   SINCE   STATUS
version             False       True          23m     Unable to apply 4.13.0-0.nightly-2023-01-17-084735: some cluster operators are not available

@lihongan
Copy link
Contributor

lihongan commented Jun 6, 2023

/label qe-approved

With Installer QE's helping, we got a cluster and tested the PR, it looks good.

$ oc get co/ingress
NAME      VERSION                                                AVAILABLE   PROGRESSING   DEGRADED   SINCE   MESSAGE
ingress   4.14.0-0.test-2023-06-05-024830-ci-ln-xf681l2-latest   True        False         False      19h     

$ oc get dns.config cluster -oyaml | yq .spec
baseDomain: yunjiang-xvpc1.qe.devcluster.openshift.com
platform:
  aws:
    privateZoneIAMRole: arn:aws:iam::123456:role/yunjiang-rol1
  type: AWS
privateZone:
  id: XYZZZZ
publicZone:
  id: ABC

### creating more ingresscontrollers and all work
$ oc -n openshift-ingress-operator get ingresscontroller
NAME      AGE
default   19h
extlb     6h16m
lb-int    50m

note: the test results is from a Passthrough mode cluster. We're still seeing some issues on STS mode cluster but looks like configuration issue and will not block this PR to be merged.

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Jun 6, 2023
@melvinjoseph86
Copy link

/jira-refresh

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 6, 2023
Bump to github.com/openshift/api@5c5196d9f4af26e7a404d5f101f7305036cf19a7
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.
@Miciah Miciah force-pushed the NE-1294-add-support-for-AWS-shared-VPC-in-another-account branch from 1eecdd9 to dbf1626 Compare June 6, 2023 13:13
@Miciah
Copy link
Contributor Author

Miciah commented Jun 6, 2023

Rebased for #906.

@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 6, 2023
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 6, 2023
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.
@Miciah Miciah force-pushed the NE-1294-add-support-for-AWS-shared-VPC-in-another-account branch from dbf1626 to 7251aaa Compare June 6, 2023 16:53
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jun 6, 2023

@Miciah: This pull request references NE-1294 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.

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

This PR is a draft because the Infrastructure.status.platformStatus.aws.privateHostedZoneRole API field doesn't exist yet and this is completely untested.

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.

@Miciah
Copy link
Contributor Author

Miciah commented Jun 6, 2023

https://github.com/openshift/cluster-ingress-operator/compare/dbf16265f1718bc51476d36e75e709bdd8991b00..7251aaa9a30f3759c2731d061207d2f63cd945de adds sts:AssumeRole to the AWS CredentialsRequest.

@gcs278
Copy link
Contributor

gcs278 commented Jun 6, 2023

sts:AssumeRole update looks good to me - just based on the slack thread
/lgtm
/approved

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 6, 2023
@gcs278
Copy link
Contributor

gcs278 commented Jun 7, 2023

TestAWSELBConnectionIdleTimeout known failure
/test e2e-aws-operator
TestInternalLoadBalancer known failure
/test e2e-gcp-operator
Same test-node-pool failure I've been seeing:
/test e2e-hypershift

@gcs278
Copy link
Contributor

gcs278 commented Jun 7, 2023

Prometheus [apigroup:image.openshift.io] when installed on the cluster should report telemetry
/test e2e-gcp-ovn

@Miciah
Copy link
Contributor Author

Miciah commented Jun 7, 2023

e2e-aws-operator failed on TestAWSELBConnectionIdleTimeout, which is a known issue (OCPBUGS-13810) with a medium flake-rate (search.ci reports a 44% impact over the past 24 hours as of this writing).
/test e2e-aws-operator

e2e-gcp-operator failed because TestUnmanagedDNSToManagedDNSInternalIngressController and TestScopeChange failed, which is a known issue (OCPBUGS-13106) with a medium flake-rate (search.ci reports a 50% impact over the past 24 hours as of this writing).
/test e2e-gcp-operator

e2e-hypershift failed because TestNodePool/NodePool_Tests_Group/TestNodePoolReplaceUpgrade failed. We don't know how to diagnose e2e-hypershift failures, but we have seen a lot of flakiness in general with this CI job.
/test e2e-hypershift

@Miciah
Copy link
Contributor Author

Miciah commented Jun 7, 2023

Regarding the e2e-hypershift failure, search.ci reports rather a lot of failures for TestNodePool/NodePool_Tests_Group/TestNodePoolReplaceUpgrade over the past 24 hours:

pull-ci-openshift-cluster-ingress-operator-master-e2e-hypershift (all) - 10 runs, 80% failed, 38% of failures match = 30% impact

pull-ci-openshift-hypershift-main-e2e-aws (all) - 20 runs, 25% failed, 20% of failures match = 5% impact

pull-ci-openshift-machine-config-operator-master-e2e-hypershift (all) - 12 runs, 83% failed, 10% of failures match = 8% impact

pull-ci-openshift-cluster-network-operator-master-e2e-hypershift-ovn (all) - 3 runs, 100% failed, 67% of failures match = 67% impact

@Miciah
Copy link
Contributor Author

Miciah commented Jun 8, 2023

e2e-hypershift failed on TestNodePool/NodePool_Tests_Group/TestNodePoolReplaceUpgrade again.
@enxebre, can you help diagnose these failures?
/test e2e-hypershift

@Miciah
Copy link
Contributor Author

Miciah commented Jun 8, 2023

e2e-hypershift also failed on #927 with the same test failure, and e2e-hypershift had previously passed on #927.

@Miciah
Copy link
Contributor Author

Miciah commented Jun 9, 2023

/test e2e-hypershift
now that openshift/hypershift#2665 has merged.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 9, 2023

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

@deepsm007
Copy link

/label jira/valid-bug

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. docs-approved Signifies that Docs has signed off on this PR 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. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.