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 Loadbalancer, Listener, and TargetGroup from elasticloadbalancingv2 #865

Merged
merged 2 commits into from
Dec 29, 2021

Conversation

EdgeJ
Copy link
Contributor

@EdgeJ EdgeJ commented Oct 6, 2021

Description of your changes

Implements 3 resource types from the elasticloadbalancingv2 api group:

  • LoadBalancer
  • Listener
  • TargetGroup

Fixes #841
Fixes #699

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

There are 3 example yaml files to generate the new resources. The listeners.yaml file contains specifications to create 2 listeners and attach them to the same loadbalancer. Additionally, there are unit tests for the function used to generate DefaultActions types from the API that will work with the AWS SDK. This function was largely adapted from autogenerated code, but was needed in order to implement references within the API type.

@haarchri
Copy link
Member

haarchri commented Nov 5, 2021

@EdgeJ The code-generator now supports setting a different "model name" as part of the generator.yaml file : aws-controllers-k8s/community#985 - we need to bump to latest Code-generator

@EdgeJ
Copy link
Contributor Author

EdgeJ commented Nov 8, 2021

@haarchri following the advice in that thread doesn't seem to be working for me. I updated the tag used for the code generator to v0.15.2 (current latest) and added module_name: elasticloadbalancingv2 to generator-config.yaml. When attempting to generate the service, I get nil pointer errors:

➜ SERVICES=elasticloadbalancingv2 make services
HEAD is now at d2b0638 Update to ACK runtime `v0.15.2` (#234)
11:58:12 [ .. ] Generating elasticloadbalancingv2 controllers and CRDs
Error: template: /Users/jedge/git/crossplane/provider-aws/.work/code-generator/templates/crossplane/pkg/conversions.go.tpl:40:3: executing "/Users/jedge/git/crossplane/provider-aws/.work/code-generator/templates/crossplane/pkg/conversions.go.tpl" at <GoCodeSetUpdateInput .CRD "cr" "res" 1>: error calling GoCodeSetUpdateInput: runtime error: invalid memory address or nil pointer dereference
exit status 1
make: *** [services] Error 1

I'll keep debugging for now, but if you have any insight, it would help greatly.

@EdgeJ
Copy link
Contributor Author

EdgeJ commented Nov 9, 2021

I discovered the issue. For the Listener objects in particular, generation is broken by ACK metadata. Adding a configuration line include_ack_metadata: false seems to fix the nil pointer issue. A bigger problem is that the crossplane command for the code-generator does not honor the model_name configuration parameter. I added a PR to the repo to hopefully amend this: aws-controllers-k8s/code-generator#237

@janwillies
Copy link
Contributor

this is blocked by #920

@haarchri
Copy link
Member

@EdgeJ we resolved #920 - did you have time to rebase and check again ?

@EdgeJ
Copy link
Contributor Author

EdgeJ commented Dec 20, 2021

@EdgeJ we resolved #920 - did you have time to rebase and check again ?

Thanks @haarchri. Will take a look and build this with the latest code-generator.

@EdgeJ EdgeJ force-pushed the jedge/elbv2 branch 3 times, most recently from 88e0d33 to 6e98820 Compare December 21, 2021 15:59
Copy link
Member

@haarchri haarchri 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 contribution ;) i tested the resource examples - all are working creation/deletion- check my review - after that squash commits for clean history and we can merge the PR

apis/elbv2/v1alpha1/custom_types.go Outdated Show resolved Hide resolved
apis/elbv2/v1alpha1/custom_types.go Outdated Show resolved Hide resolved
apis/elbv2/v1alpha1/custom_types.go Outdated Show resolved Hide resolved
apis/elbv2/v1alpha1/custom_types.go Outdated Show resolved Hide resolved
apis/elbv2/v1alpha1/custom_types.go Outdated Show resolved Hide resolved
apis/elbv2/v1alpha1/custom_types.go Outdated Show resolved Hide resolved
apis/elbv2/v1alpha1/custom_types.go Outdated Show resolved Hide resolved
apis/elbv2/v1alpha1/custom_types.go Outdated Show resolved Hide resolved
apis/elbv2/v1alpha1/custom_types.go Outdated Show resolved Hide resolved
apis/elbv2/v1alpha1/custom_types.go Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Adds the Loadbalancer, Listener, and TargetGroup resources from the
elbv2 api.

Signed-off-by: EdgeJ <5093048+EdgeJ@users.noreply.github.com>
@EdgeJ
Copy link
Contributor Author

EdgeJ commented Dec 28, 2021

@haarchri I updated CRD field names, cleaned up the mistaken newline in Makefile, and squashed down my commits. One caveat is that make reviewable seems to be failing on code generation for the sfn group, but this seems to be a regression of some sort in master and not related to these changes. Removing sfn from the GENERATED_SERVICES variable allows make reviewable to pass. False alarm, just a little housekeeping needed. Must've gotten some cruft in the .work directory on my local. Running rm -rf .work/* and git clean -fdx seemed to fix it.

Copy link
Member

@haarchri haarchri left a comment

Choose a reason for hiding this comment

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

NAME                                                   READY   SYNCED   EXTERNAL-NAME
targetgroup.elbv2.aws.crossplane.io/test-targetgroup   True    True     arn:aws:elasticloadbalancing:us-east-1:255932642927:targetgroup/test-targetgroup/cbae8e0ba0ebb033

NAME                                                 READY   SYNCED   EXTERNAL-NAME
listener.elbv2.aws.crossplane.io/test-listener       True    True     arn:aws:elasticloadbalancing:us-east-1:255932642927:listener/app/test-loadbalancer/d1441d261a57f64d/bce43527425177dc
listener.elbv2.aws.crossplane.io/test-listener-two   True    True     arn:aws:elasticloadbalancing:us-east-1:255932642927:listener/app/test-loadbalancer/d1441d261a57f64d/9853d163a3de165f

NAME                                                     READY   SYNCED   EXTERNAL-NAME
loadbalancer.elbv2.aws.crossplane.io/test-loadbalancer   True    True     arn:aws:elasticloadbalancing:us-east-1:255932642927:loadbalancer/app/test-loadbalancer/d1441d261a57f64d

issue fixed for listener error 400 in observer if loadbalancer is removed before listener;)

@EdgeJ LGTM thanks for contribution & extend/fixes code-generator

Signed-off-by: haarchri <chhaar30@googlemail.com>
@haarchri haarchri merged commit 470d82a into crossplane-contrib:master Dec 29, 2021
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.

Add support for elasticloadbalancingv2 Please can NLB ,ALB and Gateway LB resource be added
4 participants