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

Enable route53_zone_association #463

Merged
merged 4 commits into from
Jun 22, 2023

Conversation

ytsarev
Copy link
Collaborator

@ytsarev ytsarev commented Jan 24, 2023

Description of your changes

Signed-off-by: Yury Tsarev [email protected]

I have:

  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Locally e2e, and full cross-account uptest run, see #463 (comment)

@ytsarev
Copy link
Collaborator Author

ytsarev commented Feb 2, 2023

Together with @sergenyalcin and @ulucinar we decided to defer adding the association resource because:

  • From XRM perspective, we cannot avoid adding the inlined vpc as, to the best of our knowledge, it’s the only way to provision a “private” Route53 zone.

  • The association resource is discouraged and this is explicitly stated in the Terraform docs. What happens, if some point in time, they deprecate it (maybe after implementing the allegedly missing functionality somewhere else). This may or may not happen of course.

  • Because we anyways need to add the inline vpc (remember the private Zone constraint), we can always later add the association resource if a customer comes and tell us that the inline vpc functionality cannot cover their use case. But then, we may need to consider extending upjet with support for adding (for instance) the “ignore_changes” lifecycle meta-argument. Currently, if we add the association resource, we cannot implement what Terraform suggests its users to do.

@patrickleet
Copy link

I need this resource to implement cross-account vpc associations with a zone - it's not possible without this API as the zone needs to exist for the authorization, but can't exist without the authorization as is

@ytsarev
Copy link
Collaborator Author

ytsarev commented Jun 8, 2023

@patrickleet thanks for the feedback, reopening. The PR will need some rebase though.

@ytsarev ytsarev reopened this Jun 8, 2023
@mmuthukrishnan
Copy link

We are looking for the same feature and it will be very helpful for cross account associations of VPC.
@ytsarev

@mmuthukrishnan
Copy link

mmuthukrishnan commented Jun 16, 2023

@sergenyalcin could you please review the changes. much needed though.
We are planning to have our master control plane controlling clusters in diff aws accounts requiring this cross account VPC associations

@haarchri
Copy link
Member

we need this feature as well

@ytsarev
Copy link
Collaborator Author

ytsarev commented Jun 19, 2023

/test-examples="examples/route53/zoneassociation.yaml"

* Addition to crossplane-contrib#456
* Dedicate `ZoneAssociation` for more complex ZoneToVPC associations for
  a complex cases
* According to investigation documented at crossplane-contrib#456 (comment)
  we will still need inline `vpc` field to instantiate private Zone
first
* crossplane-contrib#456 should be merged first so we can ehance `ZoneAssociation` example

Signed-off-by: Yury Tsarev <[email protected]>
@ytsarev
Copy link
Collaborator Author

ytsarev commented Jun 19, 2023

/test-examples="examples/route53/zoneassociation.yaml"

forProvider:
name: example.com
region: us-west-1
vpc:
Copy link
Member

Choose a reason for hiding this comment

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

@ytsarev in terraform documentation we can find the following:

Terraform provides both this standalone Zone VPC Association resource and exclusive VPC associations defined in-line in the [aws_route53_zone resource](https://registry.terraform.io/providers/hashicorp/aws/2.54.0/docs/resources/route53_zone) via vpc configuration blocks. At this time, you cannot use those in-line VPC associations in conjunction with this resource and the same zone ID otherwise it will cause a perpetual difference in plan output.

so i think we need for the primary vpc also an ZoneAssociation and in Zone resource we need to skip lateinit for vpc field ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@haarchri thanks a lot for highlighting this one.

This is generated example so I left it as it is.

The crafted/tested example below does not use vpc spec in the Zone
https://github.com/upbound/provider-aws/pull/463/files/febb5711405ed06d4826007feffe3d802a940fd5#diff-0241fdef09239ca9bc31c3f588de9c70b3c894cee80a27fbcc1c5e9b3087b6d8R29-R32 , only in the ZoneAssociation

Copy link
Member

Choose a reason for hiding this comment

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

but i think we need to skip lateinit for vpc field in Zone resource - otherwise the provider will fill it or ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm, not sure, it was good in my local tests without it. Let's wait for uptest-after-half-an-year-rebase :D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I confirm it's tricky :D crafting the example and possible PR extension

@ytsarev
Copy link
Collaborator Author

ytsarev commented Jun 19, 2023

/test-examples="examples/route53/zoneassociation.yaml"

To test as close cross-account scenario as possible

Signed-off-by: Yury Tsarev <[email protected]>
@ytsarev
Copy link
Collaborator Author

ytsarev commented Jun 19, 2023

/test-examples="examples/route53/zoneassociation.yaml"

@ytsarev
Copy link
Collaborator Author

ytsarev commented Jun 19, 2023

the uptest for examples/route53/zoneassociation.yaml is green locally, the failure above is related to restricted access of the test environment(sorting it out).

@haarchri was also so kind to test this change in his cross-account environment to get the full confidence that the implementation works.

@ytsarev
Copy link
Collaborator Author

ytsarev commented Jun 20, 2023

/test-examples="examples/route53/zoneassociation.yaml"

@ytsarev
Copy link
Collaborator Author

ytsarev commented Jun 20, 2023

/test-examples="examples/route53/zoneassociation.yaml"

2 similar comments
@ytsarev
Copy link
Collaborator Author

ytsarev commented Jun 21, 2023

/test-examples="examples/route53/zoneassociation.yaml"

@ytsarev
Copy link
Collaborator Author

ytsarev commented Jun 21, 2023

/test-examples="examples/route53/zoneassociation.yaml"

Otherwise full cross-account testing of ZoneAssociation will fail with

```
is not authorized to perform: route53:AssociateVPCWithHostedZone on resource: arn:aws:route53:::hostedzone/ZXXX because no resource-based policy allows the route53:AssociateVPCWithHostedZone action
```

Signed-off-by: Yury Tsarev <[email protected]>
@ytsarev
Copy link
Collaborator Author

ytsarev commented Jun 21, 2023

/test-examples="examples/route53/zoneassociation.yaml"

1 similar comment
@ytsarev
Copy link
Collaborator Author

ytsarev commented Jun 21, 2023

/test-examples="examples/route53/zoneassociation.yaml"

@ytsarev
Copy link
Collaborator Author

ytsarev commented Jun 21, 2023

Finally, all green with full cross-account uptest setup :)

@haarchri VPCAssociationAuthorization is the key for cross-account setup, without it ZoneAssociation fails with

is not authorized to perform: route53:AssociateVPCWithHostedZone on resource: arn:aws:route53:::hostedzone/ZXXX because no resource-based policy allows the route53:AssociateVPCWithHostedZone action

Please double check on you side :)

Copy link
Collaborator

@sergenyalcin sergenyalcin left a comment

Choose a reason for hiding this comment

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

Thanks @ytsarev LGTM!

@haarchri
Copy link
Member

we tested in our environment with multiple accounts - its working
@ytsarev thanks for driving this topic =)

@ytsarev
Copy link
Collaborator Author

ytsarev commented Jun 22, 2023

Thanks a ton for the feedback! Merging :)

@ytsarev ytsarev merged commit bcfbe95 into crossplane-contrib:main Jun 22, 2023
8 checks passed
@ytsarev ytsarev deleted the r53-zone-assoc-392 branch June 22, 2023 11:02
@mmuthukrishnan
Copy link

@haarchri @sergenyalcin @ytsarev Thanks for getting this through. It will be much helpful for us.

@mmuthukrishnan
Copy link

How long does it take to get reflected in the upbound marketplace docs ?
https://marketplace.upbound.io/providers/upbound/provider-aws/v0.36.0/managed-resources

@jeanduplessis
Copy link
Collaborator

The next round of provider releases is due on 29 June.

@shukla2009
Copy link

shukla2009 commented Aug 23, 2023

Hi , I am trying to use cross accounts zoneassociation and it goes in loop creating and deleting association
This is how TF fixed it hashicorp/terraform-provider-aws#14872

Error Reported something like this
Provider produced inconsistent result after apply: When applying changes to aws_route53_vpc_association_authorization.XXXYYZZ, provider "provider["registry.terraform.io/hashicorp/aws"]" produced an unexpected new value: Root resource was present, but now absent. This is a bug in the provider, which should be reported in the provider's own issue tracker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants