Setup a second NLB listener when an AWS ACM certificate is used #10157

rifelpet · 2020-11-02T18:39:24Z

This uses commits from #9761 combined with the new NLB support to provision a second NLB listener. It requires converting the Listeners and TargetGroups NLB fields to slices in which each listener should forward traffic to the target group of the same list index. I couldn't use a map of port number to TargetGroup because fi/loader.go uses reflection to set values of fields that are Tasks, and map values aren't addressable which resulted in a panic.

I also temporarily pulled in the commit from https://github.com/kubernetes/kops/pull/10076/files to run the e2e presubmit job with an ACM cert. Before we merge this I'll remove the commit.

ref: #9756

k8s-ci-robot · 2020-11-02T18:41:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rifelpet

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:

~~OWNERS~~ [rifelpet]

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

rifelpet · 2020-11-06T18:03:50Z

/hold for #10156

hakman · 2020-11-08T05:17:51Z

/test pull-kops-e2e-k8s-containerd

pkg/model/awsmodel/api_loadbalancer.go

upup/pkg/fi/cloudup/awstasks/network_load_balancer.go

hakman · 2020-11-08T07:22:42Z

upup/pkg/fi/cloudup/awstasks/network_load_balancer.go

+					actual.TargetGroups = append(actual.TargetGroups, &TargetGroup{ARN: targetGroupARN})
+
+					cloud := c.Cloud.(awsup.AWSCloud)
+					descResp, err := cloud.ELBV2().DescribeTargetGroups(&elbv2.DescribeTargetGroupsInput{
+						TargetGroupArns: []*string{targetGroupARN},
+					})
+					if err != nil {
+						return nil, fmt.Errorf("error querying for NLB listener target groups: %v", err)
+					}
+					if len(descResp.TargetGroups) != 1 {
+						return nil, fmt.Errorf("unexpected DescribeTargetGroups response: %v", descResp)
+					}
+					actualListener.TargetGroupName = aws.StringValue(descResp.TargetGroups[0].TargetGroupName)


Can we compare by ID and avoid listing the TGs? Should be similar to what we do with ASGs and LTs.
I guess we can do this in a future PR as this is working ok for now.

Not sure I understand, which field would we use with "compare by ID" ? We need a field that can be both provided by the model and something we can look up in Find(). It seems like our only options for that are TargetGroupName or tags.

I meant that at the time this runs there is also full info on target groups populated in e.TargetGroups, so you can just get the name from there without an extra call to DescribeTargetGroups.
Though this is an optimisation that can be done later.

If we're removing a listener (and therefore target group), e.TargetGroups won't have the target group being removed so we wont be able to lookup its name from e.TargetGroups. We'd have to DescribeTargetGroups to find it.

Though maybe the value of actualListener.TargetGroupName doesn't actually matter for a listener being deleted? When we add target group deletion support we probably won't be using the listener.TargetGroupName to determine what to delete, instead just use NLBTargetGroupName() to determine all the possible TG names and look for any that exist in AWS that shouldn't and delete them. just thinking out loud here...

If delete is done by ARN, probably no describing will be needed. I agree that the compare part should check which ones are removed and just delete them.

upup/pkg/fi/cloudup/awstasks/network_load_balancer.go

hakman · 2020-11-09T03:35:01Z

upup/pkg/fi/cloudup/awstasks/targetgroup.go

+func (a OrderTargetGroupsByPort) Less(i, j int) bool {
+	if a[i].ARN != nil || a[j].ARN != nil {
+		return fi.StringValue(a[i].ARN) < fi.StringValue(a[j].ARN)
+	}
+	return fi.StringValue(a[i].Name) < fi.StringValue(a[j].Name)
+}
+


Not quite by port. Name should be enough?

Still curious, why compare both by Name and ARN? Names should be unique.

hakman · 2020-11-10T15:03:20Z

upup/pkg/fi/cloudup/awstasks/autoscalinggroup.go

@@ -163,12 +163,11 @@ func (e *AutoscalingGroup) Find(c *fi.Context) (*AutoscalingGroup, error) {
 	}



Suggested change

actual.TargetGroups = []*TargetGroup{}

hakman · 2020-11-10T15:08:05Z

pkg/model/awsmodel/autoscalinggroup.go

@@ -370,7 +370,10 @@ func (b *AutoscalingGroupModelBuilder) buildAutoScalingGroupTask(c *fi.ModelBuil
 	if !featureflag.Spotinst.Enabled() {


Suggested change

if !featureflag.Spotinst.Enabled() {

t.TargetGroups = []*awstasks.TargetGroup{}

if !featureflag.Spotinst.Enabled() {

hakman · 2020-11-10T17:05:30Z

/lgtm

rifelpet · 2020-11-10T17:47:46Z

/hold cancel

…-upstream-release-1.19 Automated cherry pick of #10157: Add support for multiple NLB listeners and target

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 2, 2020

k8s-ci-robot requested review from johngmyers and rdrgmnzs November 2, 2020 18:41

k8s-ci-robot added area/api area/nodeup approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/aws Issues or PRs related to aws provider labels Nov 2, 2020

rifelpet force-pushed the acm-nlb branch 2 times, most recently from 242f0db to a0180ee Compare November 2, 2020 21:38

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. area/documentation area/provider/spotinst Issues or PRs related to spotinst provider labels Nov 2, 2020

rifelpet force-pushed the acm-nlb branch from a0180ee to b927fc5 Compare November 2, 2020 21:39

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2020

rifelpet mentioned this pull request Nov 2, 2020

Setup a second ELB listener when an AWS ACM certificate is used #9761

Closed

rifelpet marked this pull request as draft November 3, 2020 16:37

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 3, 2020

rifelpet force-pushed the acm-nlb branch 3 times, most recently from e46d44c to dec0763 Compare November 3, 2020 18:12

rifelpet mentioned this pull request Nov 3, 2020

Fix additionalSecurityGroups support for NLB #10162

Merged

rifelpet force-pushed the acm-nlb branch 4 times, most recently from 1d24b56 to 1f87260 Compare November 5, 2020 16:32

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 5, 2020

rifelpet force-pushed the acm-nlb branch from 1f87260 to 43f403e Compare November 5, 2020 16:44

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 6, 2020

kubernetes deleted a comment from rifelpet Nov 8, 2020

hakman reviewed Nov 9, 2020

View reviewed changes

Address feedback

4758ea9

rifelpet mentioned this pull request Nov 10, 2020

Add ACM cert permalink #10156

Merged

Update validation error permalink

0072abd

hakman requested changes Nov 10, 2020

View reviewed changes

Fix various NLB nits

0934374

k8s-ci-robot assigned hakman Nov 10, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 10, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 10, 2020

k8s-ci-robot merged commit e43efbe into kubernetes:master Nov 10, 2020

k8s-ci-robot added this to the v1.20 milestone Nov 10, 2020

hakman mentioned this pull request Nov 10, 2020

Automated cherry pick of #10157: Add support for multiple NLB listeners and target #10208

Merged

k8s-ci-robot added a commit that referenced this pull request Nov 10, 2020

Merge pull request #10208 from hakman/automated-cherry-pick-of-#10157…

dd415df

…-upstream-release-1.19 Automated cherry pick of #10157: Add support for multiple NLB listeners and target

rifelpet deleted the acm-nlb branch May 5, 2021 13:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setup a second NLB listener when an AWS ACM certificate is used #10157

Setup a second NLB listener when an AWS ACM certificate is used #10157

rifelpet commented Nov 2, 2020 •

edited

Loading

k8s-ci-robot commented Nov 2, 2020

rifelpet commented Nov 6, 2020

hakman commented Nov 8, 2020

hakman Nov 8, 2020 •

edited

Loading

rifelpet Nov 9, 2020

hakman Nov 10, 2020

rifelpet Nov 10, 2020

hakman Nov 10, 2020

hakman Nov 9, 2020

hakman Nov 10, 2020

hakman Nov 10, 2020

hakman Nov 10, 2020

hakman commented Nov 10, 2020

rifelpet commented Nov 10, 2020

		@@ -163,12 +163,11 @@ func (e AutoscalingGroup) Find(c fi.Context) (*AutoscalingGroup, error) {
		}

		@@ -370,7 +370,10 @@ func (b AutoscalingGroupModelBuilder) buildAutoScalingGroupTask(c fi.ModelBuil
		if !featureflag.Spotinst.Enabled() {

	if !featureflag.Spotinst.Enabled() {
	t.TargetGroups = []*awstasks.TargetGroup{}
	if !featureflag.Spotinst.Enabled() {

Setup a second NLB listener when an AWS ACM certificate is used #10157

Setup a second NLB listener when an AWS ACM certificate is used #10157

Conversation

rifelpet commented Nov 2, 2020 • edited Loading

k8s-ci-robot commented Nov 2, 2020

rifelpet commented Nov 6, 2020

hakman commented Nov 8, 2020

hakman Nov 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hakman commented Nov 10, 2020

rifelpet commented Nov 10, 2020

rifelpet commented Nov 2, 2020 •

edited

Loading

hakman Nov 8, 2020 •

edited

Loading