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

Fix VPC update, only send vpc attributes if set #595

Merged
merged 1 commit into from
Mar 12, 2021

Conversation

enderv
Copy link
Contributor

@enderv enderv commented Mar 12, 2021

Signed-off-by: chris cvodak@ea.com

Description of your changes

Fixes #558 Only send vpc attributes if set. Also added lateInitialization as EnableDnsSupport has a default of true if not set.

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Ran and added to unit tests. Created a test vpc setting neither enableDnsHostNames or enableDnsSupport of the values in the issue. Modified the vpc changing values to ensure they were updated in aws correctly.

Signed-off-by: chris <cvodak@ea.com>
Copy link
Member

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

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

Thanks so much for fixing this @enderv!

@@ -191,17 +192,22 @@ func (e *external) Update(ctx context.Context, mgd resource.Managed) (managed.Ex
return managed.ExternalUpdate{}, errors.New(errUnexpectedObject)
}

for _, input := range []*awsec2.ModifyVpcAttributeInput{
{
if cr.Spec.ForProvider.EnableDNSSupport != nil {
Copy link
Member

Choose a reason for hiding this comment

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

I don't love that we will make all of these calls if the fields are set even if they don't need to be changed (i.e. the diff was elsewhere in the spec). That being said, this is the current existing pattern (as opposed to getting again, calculating diff, then only making the specific call), so I am fine with moving forward with this fix 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

@hasheddan I think I just ran into a bug here. So in my case I'm importing and existing vpc. I'm using the crossplane.io/external-name annotation in order to pull in an existing vpc. In doing so I'm getting an error of:

 Message:               update failed: failed to modify the VPC resource attributes: api error UnauthorizedOperation: You are not authorized to perform this operation. Encoded authorization failure message: 17IZjLqa5Zh3OjVaZ3t9FH0FftiP1SQ3_yRUhVUg42S9N-S5HC6P5UNgad1lXNzm9uEfvQyWMV-JzvjbCbdBoeCDdaAeadxZWWO5F94c-Qm0ba2sMINryty1b1F_wuzxG9Pi_1Mb9QEwuMSjR2zK7Bttkm20YGfP-NKxSIenBSHVCmy5yfW915cTLqakozjE8Dmt2Ml95_XT8nQ5QZj-bYHkCIu5WjTY5VQQNe7Ur7hPXNoj2Zqp3mUnwpJKmPp0XAHgRrQtnClrc_aFldtspgv2Ty97AhgIQtOWHIvX4iadQ1uafOakWfKoONODQaUw8zqHHsBNt6a1-Cy7nQ9Q1weM0tyeVykTioT1iRqq2arbvdvSjWvPq9P8qqpkovXFBqxRExx8wibFiZ6G5bV3lmAv4wMZOa11p0nuisNmnc2xJXDb8gyX2dwSQSXL336pRHgKt4S-VITej52_-o-qD7wqEzmN6rZibbfIA2xwGddtEBX95zwgqJcgWLIr-Rvz4q4ktgLuIJH5G5rOkmH1ZiCMTVswY98WjouXAl9bXUaNnp55LzvhSdNajsW77Uuu5kDlc0A4WAUrdaGD6He8foMacrySqBwyrlJLULa-NxkHcKC5p2tt5i3k-BYurVJswQnqXTIbmH4lwV5GIimXuPkcaGX9lKXhvGUl4zAiYCSmG_QweVQT0cglv8dg9R4oOwws8WNtDM_aHQbDjxO13SO1I17geD1ERFiHqdN7igqbPLs2aRUA7f5vVS7Z-DsF-p5Mf5uke7qv-4eyd7on2bV-e-rDrH5W6KfwoyBLvbq7xZGY7rIG3is9aQgDoBFwk2NVcIAKPlP0OQ2y0OKxx83q_mUcDcpsYdz5ZmZjghiaJr9UMxzEfnuEwTvBwVPqRJiw1CdWhoqmMoIPGg

In checking CloudTrail I see it's attempting to do a ModifyVpcResource on enableDnsSupport even though it's already set.

Obviously in our case we are using vpc as a ReadOnly resource for reference but it shouldn't try to Modify an attribute if it matches. I've tried setting the forProvider to not set these 2 attributes and it still tries to Modify... I've tried to set these forProvider parameters to match what they currently are on vpc and it still tries to modify.

@hasheddan hasheddan merged commit 8767b03 into crossplane-contrib:master Mar 12, 2021
@muvaf muvaf changed the title Fix for 558, only send vpc attributes if set Fix VPC update, only send vpc attributes if set May 3, 2021
tektondeploy pushed a commit to gtn3010/provider-aws that referenced this pull request Mar 12, 2024
…ed-acls

S3: BucketACL: Enable canned ACL support
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.

Basic VPC creation failed to sync "CannotUpdateExternalResource" / ReconcileError
3 participants