Skip to content

Conversation

Skarlso
Copy link
Contributor

@Skarlso Skarlso commented Oct 31, 2022

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #3088

Special notes for your reviewer:

TODOs:

  • IPv6 - res.Attributes["ipv6.deny_all_igw_traffic"] and the ipType for the NLB
  • Hosts re-write
  • SSL
  • Bunch of tests
  • Update status with an NLB if present
  • Documentation
  • Samples configuration
  • Deregister nodes when they are being deleted and they are part of NLB

Checklist:

  • squashed commits
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 31, 2022
@Skarlso Skarlso force-pushed the create_nlb branch 8 times, most recently from cff8230 to 1f1a982 Compare November 1, 2022 11:15
@Skarlso
Copy link
Contributor Author

Skarlso commented Nov 1, 2022

I just learned that you couldn't attach security groups to network load balancers.

@Skarlso
Copy link
Contributor Author

Skarlso commented Nov 1, 2022

Almost there. :D It can't create the target group which I don't understand at all why because the error is super unhelpful.

name="test-aws-cluster" reconcileID=35f899c1-3e15-484e-b030-6870452713c5 cluster="default/test-aws-cluster" group=<
	{
	  HealthCheckEnabled: true,
	  HealthCheckIntervalSeconds: 10000000000,
	  HealthCheckProtocol: "TCP",
	  HealthCheckTimeoutSeconds: 5000000000,
	  HealthyThresholdCount: 5,
	  Name: "apiserver-target",
	  Port: 6443,
	  Protocol: "TCP",
	  VpcId: "vpc-064d130f24068f471"
	}
 > listener={Protocol:TCP Port:6443 TargetGroup:{Name:0xc0011018d0 Port:0xc000b59520 Protocol:TCP VpcID:0xc0011018e0 HealthCheck:0xc0004c0150}}
E1101 20:46:18.337089     986 logger.go:84] "failed to create LB" err=<
	failed to create target group for load balancer: MalformedInput: 
		status code: 400, request id: ced119e5-11d9-4109-a1bf-b98f1554f126
 > controller="awscluster" controllerGroup="infrastructure.cluster.x-k8s.io" controllerKind="AWSCluster" aWSCluster="default/test-aws-cluster" namespace="default" name="test-aws-cluster" reconcileID=35f899c1-3e15-484e-b030-6870452713c5 cluster="default/test-aws-cluster"
E1101 20:46:18.337178     986 logger.go:84] "failed to reconcile load balancer" err=<
	failed to create target group for load balancer: MalformedInput: 
		status code: 400, request id: ced119e5-11d9-4109-a1bf-b98f1554f126

It looks okay to me. :D

@Skarlso
Copy link
Contributor Author

Skarlso commented Nov 1, 2022

IT WORKED! I have a working NLB! :D

It was the health check, apparently; I'm not sure why yet.

@Skarlso
Copy link
Contributor Author

Skarlso commented Nov 2, 2022

Screenshot 2022-11-02 at 11 05 38

BOOM!

@Skarlso Skarlso force-pushed the create_nlb branch 2 times, most recently from c157768 to 6dfd0f9 Compare November 2, 2022 21:07
@Skarlso
Copy link
Contributor Author

Skarlso commented Nov 3, 2022

Screenshot 2022-11-03 at 13 25 49

IT'S ALIVE

@Skarlso Skarlso force-pushed the create_nlb branch 3 times, most recently from b5d43af to a7b9282 Compare November 4, 2022 19:15
Copy link
Member

@Ankitasw Ankitasw left a comment

Choose a reason for hiding this comment

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

Minor comments.
Otherwise LGTM, great work 🎉

@Ankitasw
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 29, 2022
@Ankitasw
Copy link
Member

cc @richardcase for final approval

Copy link
Member

Choose a reason for hiding this comment

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

Do we want to expose the V2 part in our API to the user?

Copy link
Member

Choose a reason for hiding this comment

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

Will there be confusion with listener and v2Listeners? Guessing this could be coverted in the docs (which i haven't got to yet ;) )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question! I only exposed it because the other one was exposed as well. Do you think it's usually not needed at all? I'm happy not to include this at all. It doesn't seem like users would define extra listeners.

That said, there might be an option to add an extra health check or something... 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In any case, I renamed it. :)

@richardcase
Copy link
Member

And i should've said, looks great @Skarlso....excellent work. Most of my comments are nits.

@johannesfrey
Copy link

johannesfrey commented Dec 1, 2022

Also, totally forgot so say awesome work @Skarlso ❤️ 🙂. Only some nits.

@Skarlso
Copy link
Contributor Author

Skarlso commented Dec 1, 2022

Thank you, all! :)

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 1, 2022
@Skarlso Skarlso force-pushed the create_nlb branch 3 times, most recently from 140b19f to 8c36924 Compare December 1, 2022 11:50
@Skarlso Skarlso requested a review from richardcase December 1, 2022 11:51
@Skarlso
Copy link
Contributor Author

Skarlso commented Dec 1, 2022

/test pull-cluster-api-provider-aws-e2e-eks

@Ankitasw
Copy link
Member

Ankitasw commented Dec 2, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 2, 2022
@richardcase
Copy link
Member

/lgtm

@richardcase
Copy link
Member

I meant:

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: richardcase

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 2, 2022
@k8s-ci-robot k8s-ci-robot merged commit 7549bed into kubernetes-sigs:main Dec 2, 2022
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NLB support

5 participants