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

Implement VPC Endpoint for AWS Provider #817

Merged
merged 3 commits into from
Dec 28, 2021

Conversation

darryl-sw
Copy link
Contributor

@darryl-sw darryl-sw commented Aug 31, 2021

Description of your changes

Fixes #715 #467

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

  • Verified that the VPC Endpoint can be created upstream in AWS
  • Ensured that resource is marked as READY only when VPC Endpoint status is available
  • Verified that the VPC Endpoint can be deleted upstream in AWS

Copy link
Collaborator

@chlunde chlunde 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 your contribution, could you also look at bit at supporting updates?

examples/ec2/vpcendpoint.yaml Outdated Show resolved Hide resolved
pkg/controller/ec2/vpcendpoint/setup.go Outdated Show resolved Hide resolved
pkg/controller/ec2/vpcendpoint/setup.go Outdated Show resolved Hide resolved
pkg/controller/ec2/vpcendpoint/setup.go Show resolved Hide resolved
@darryl-sw darryl-sw force-pushed the feature/vpc-endpoint branch 2 times, most recently from ed0db62 to c7116fa Compare September 8, 2021 09:27
@darryl-sw
Copy link
Contributor Author

@chlunde I've completed the requested changes. Could you help me take a look at the changes please? Thanks!

@turkenh turkenh requested a review from chlunde September 17, 2021 09:46
apis/ec2/v1alpha1/custom_types.go Show resolved Hide resolved
apis/ec2/v1alpha1/referencers.go Outdated Show resolved Hide resolved
pkg/controller/ec2/vpcendpoint/setup.go Outdated Show resolved Hide resolved
@darryl-sw
Copy link
Contributor Author

@turkenh, I failed the check-diff job in the previous run. Did a make generate and git diff locally to get this output.

diff --git a/apis/acm/v1alpha1/zz_generated.deepcopy.go b/apis/acm/v1alpha1/zz_generated.deepcopy.go
index 36535911..cf8dfdbe 100644
--- a/apis/acm/v1alpha1/zz_generated.deepcopy.go
+++ b/apis/acm/v1alpha1/zz_generated.deepcopy.go
@@ -1,3 +1,4 @@
+//go:build !ignore_autogenerated
 // +build !ignore_autogenerated
 
 /*
diff --git a/apis/acmpca/v1alpha1/zz_generated.deepcopy.go b/apis/acmpca/v1alpha1/zz_generated.deepcopy.go
index 05d1ef6f..943bc892 100644
--- a/apis/acmpca/v1alpha1/zz_generated.deepcopy.go
+++ b/apis/acmpca/v1alpha1/zz_generated.deepcopy.go
@@ -1,3 +1,4 @@
+//go:build !ignore_autogenerated
 // +build !ignore_autogenerated
 
 /*
diff --git a/apis/apigatewayv2/v1alpha1/zz_generated.deepcopy.go b/apis/apigatewayv2/v1alpha1/zz_generated.deepcopy.go
index 6de8677b..bcc03079 100644
--- a/apis/apigatewayv2/v1alpha1/zz_generated.deepcopy.go
+++ b/apis/apigatewayv2/v1alpha1/zz_generated.deepcopy.go
@@ -1,3 +1,4 @@
+//go:build !ignore_autogenerated
 // +build !ignore_autogenerated
 
 /*
diff --git a/apis/cache/v1alpha1/zz_generated.deepcopy.go b/apis/cache/v1alpha1/zz_generated.deepcopy.go
index ca1083d6..3fb44a5e 100644
--- a/apis/cache/v1alpha1/zz_generated.deepcopy.go
+++ b/apis/cache/v1alpha1/zz_generated.deepcopy.go
@@ -1,3 +1,4 @@
+//go:build !ignore_autogenerated
 // +build !ignore_autogenerated
 
 /*
diff --git a/apis/cache/v1beta1/zz_generated.deepcopy.go b/apis/cache/v1beta1/zz_generated.deepcopy.go
index d671c164..3543222b 100644
--- a/apis/cache/v1beta1/zz_generated.deepcopy.go
+++ b/apis/cache/v1beta1/zz_generated.deepcopy.go
@@ -1,3 +1,4 @@
+//go:build !ignore_autogenerated
 // +build !ignore_autogenerated

 ...
// 

34 files came appended with //go:build !ignore_autogenerated. By any chance, would you happen to know how I can resolve this?

@chlunde
Copy link
Collaborator

chlunde commented Sep 17, 2021

+++ b/apis/cache/v1beta1/zz_generated.deepcopy.go
@@ -1,3 +1,4 @@
+//go:build !ignore_autogenerated
 // +build !ignore_autogenerated

34 files came appended with //go:build !ignore_autogenerated. By any chance, would you happen to know how I can resolve this?

this happens when you have go 1.17 locally, and the build environment uses go 1.16 or older. A workaround is currently something like sed -i /go:build/ apis/*/*/*.go (and more) before commit/push.

@darryl-sw
Copy link
Contributor Author

@chlunde can you take a look at the changes please?

go.mod Outdated Show resolved Hide resolved
@AaronME AaronME self-assigned this Sep 24, 2021
@darryl-sw darryl-sw force-pushed the feature/vpc-endpoint branch 2 times, most recently from bfc7d5e to 010a825 Compare September 27, 2021 01:20
@chlunde
Copy link
Collaborator

chlunde commented Oct 3, 2021

@AaronME is there an issue in detect-noop?

@darryl-sw The tests are not running, when I run them locally I get an issue with a panic in IsPolicyUpToDate. I think we should harden it by using StringValue:

diff --git a/pkg/clients/aws.go b/pkg/clients/aws.go
index 7cd91a7f..83a8b35e 100644
--- a/pkg/clients/aws.go
+++ b/pkg/clients/aws.go
@@ -658,11 +658,11 @@ func IsPolicyUpToDate(local, remote *string) bool {
        var remoteUnmarshalled interface{}
 
        var err error
-       err = json.Unmarshal([]byte(*local), &localUnmarshalled)
+       err = json.Unmarshal([]byte(StringValue(local)), &localUnmarshalled)
        if err != nil {
                return false
        }
-       err = json.Unmarshal([]byte(*remote), &remoteUnmarshalled)
+       err = json.Unmarshal([]byte(StringValue(remote)), &remoteUnmarshalled)
        if err != nil {
                return false
        }

@darryl-sw I don't think you have run code gen, I get a diff here, could you run make generate?

diff --git a/pkg/controller/ec2/vpcendpoint/zz_conversions.go b/pkg/controller/ec2/vpcendpoint/zz_conversions.go
index 2be39997..e52368a3 100644
--- a/pkg/controller/ec2/vpcendpoint/zz_conversions.go
+++ b/pkg/controller/ec2/vpcendpoint/zz_conversions.go
@@ -88,11 +88,6 @@ func GenerateVPCEndpoint(resp *svcsdk.DescribeVpcEndpointsOutput) *svcapitypes.V
                } else {
                        cr.Spec.ForProvider.VPCEndpointType = nil
                }
-               if elem.VpcId != nil {
-                       cr.Spec.ForProvider.VPCID = elem.VpcId
-               } else {
-                       cr.Spec.ForProvider.VPCID = nil
-               }
                found = true
                break
        }
@@ -177,9 +172,6 @@ func GenerateCreateVpcEndpointInput(cr *svcapitypes.VPCEndpoint) *svcsdk.CreateV
        if cr.Spec.ForProvider.VPCEndpointType != nil {
                res.SetVpcEndpointType(*cr.Spec.ForProvider.VPCEndpointType)
        }
-       if cr.Spec.ForProvider.VPCID != nil {
-               res.SetVpcId(*cr.Spec.ForProvider.VPCID)
-       }
 
        return res
 }

@darryl-sw
Copy link
Contributor Author

@chlunde I've made the requested changes to isPolicyUpToDate and reran make generate.

@haarchri
Copy link
Member

haarchri commented Nov 7, 2021

@darryl-sw can you rebase and run make generate again ? And squash commits ? After that we can merge the feature - thanks

@darryl-sw
Copy link
Contributor Author

@haarchri running make generate with "ec2" added to GENERATED_SERVICES in the Makefile results in an error that I have not been able to resolve after rebasing.

13:15:38 [ OK ] go modules dependencies verified
13:15:39 [ .. ] go generate darwin_amd64
github.com/crossplane/provider-aws/apis/ec2/v1alpha1:-: CRD for Instance.ec2.aws.crossplane.io has no storage version
Error: not all generators ran successfully
run `controller-gen object:headerFile=../hack/boilerplate.go.txt paths=./... crd:trivialVersions=true,allowDangerousTypes=true,crdVersions=v1 output:artifacts:config=../package/crds -w` to see all available markers, or `controller-gen object:headerFile=../hack/boilerplate.go.txt paths=./... crd:trivialVersions=true,allowDangerousTypes=true,crdVersions=v1 output:artifacts:config=../package/crds -h` for usage
exit status 1
apis/generate.go:26: running "go": exit status 1

I took a look at the file package/crds/ec2.aws.crossplane.io_instances.yaml and identified that there were two versions, both named v1alpha1, both of them with storage: false.

To further isolate the problem, I verified that simply adding "ec2" to GENERATED_SERVICES on the master branch and running make generate produces the same error message.

Would you, or anybody, be able to advice on how I can fix this please?

@haarchri
Copy link
Member

haarchri commented Nov 8, 2021

@darryl-sw sorry i missed that we are blocked with ec2 services because of #876

@darryl-sw
Copy link
Contributor Author

@haarchri shall I wait for #876 first before proceeding with the rebase then?

This was referenced Nov 12, 2021
@hanlins
Copy link
Contributor

hanlins commented Nov 22, 2021

Any updates on the PR? Blockers? cc @haarchri

@haarchri
Copy link
Member

@darryl-sw you can go forward the blockers are solved in master branch - thanks

@xvzf
Copy link

xvzf commented Dec 13, 2021

@haarchri what's the ETA for this to be released? :)

@haarchri
Copy link
Member

@darryl-sw can rebase and i can have a look - in the next days we will cut next release - so i have time for review since rebase is done

@darryl-sw darryl-sw force-pushed the feature/vpc-endpoint branch from 235282d to b9fac42 Compare December 14, 2021 08:54
@darryl-sw
Copy link
Contributor Author

@haarchri rebased and completed another round of upstream testing. :)

Copy link

@AaronME AaronME left a comment

Choose a reason for hiding this comment

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

Thanks @darryl-sw for your contribution. I was able to create and delete a VPCEndpoint using this.

@haarchri this is good to rebase and merge.

@haarchri
Copy link
Member

i will rebase now and adjust generated resolvers to get this PR merged ;)

@haarchri haarchri force-pushed the feature/vpc-endpoint branch from b9fac42 to 9d11d80 Compare December 27, 2021 20:29
@haarchri
Copy link
Member

@AaronME done!
can you check build submodule ? looks like we see changes in this PR - after that we are ready to merge =)

tested also on my side:

NAME                                                   READY   SYNCED   EXTERNAL-NAME
vpcendpoint.ec2.aws.crossplane.io/sample-vpcendpoint   True    True     vpce-0037d4f0338c77881

Signed-off-by: Aaron Eaton <[email protected]>
@AaronME AaronME merged commit 3bcf02e into crossplane-contrib:master Dec 28, 2021
tektondeploy pushed a commit to gtn3010/provider-aws that referenced this pull request Mar 12, 2024
…e-3.x

Update alpine Docker tag to v3.18.3
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.

VPC endpoints support for aws provider
7 participants