Add sslPolicy for NLB to change listener's security policy

[FrankYang0529]

closes #9633

[k8s-ci-robot]

Hi @FrankYang0529. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[rifelpet]

/ok-to-test

Thanks for adding this! I haven't looked at the ELB API behavior closely but we should make sure that when someone removes listenerPolicies from their ClusterSpec that the API ELB is updated to use AWS' default listener policy. We'll also want to add terraform and cloudformation support but that can happen in followup PRs.

[FrankYang0529]

@rifelpet Thanks for reviewing! I have two questions about generating terraform and cloudformation.

• For terraform, it uses another resource name for listener's policy. Is it correct to only add RenderTerraform and TerraformLink functions for LoadBalancerListener, so we can have corresponding terraform resource?
• For cloudformation, I am not sure why we only generate TCP listeners? Cloudformation has PolicyNames field in listener. So if we distinguish whether load balancer port is 443 in RenderCloudformation, we can add corresponding policy names.

[rifelpet]

• We should be able to render two terraform resources from one RenderTerraform function, heres an example. You'll just need an additional terraform type with all the fields necessary for the additional resource type.

• Regarding cloudformation, my guess is that Kops' ACM Certificate support wasnt extended to include cloudformation, so we may be hardcoding the TCP listener there. If that's the case then it would be good to support both ACM certs and this new security policy in cloudformation but again, this can be a followup PR.

Also we'll want to add this to an integration test which will test both the terraform and cloudformation outputs. Add the new field to the ClusterSpec in the two in-*.yaml files here, and run ./hack/update-expected.sh and you should see the terraform and CF files in the same directory be updated to match.

[olemarkus]

Small one from me: Add a line about this in the cluster spec docs (docs/cluster_spec.md)

[FrankYang0529]

Updated it. Thank you!

[olemarkus]

/lgtm

[rifelpet]

So I tested this out and ran into some trouble. I tried to add this policy to my API ELB: ELBSecurityPolicy-TLS-1-2-2017-01 but received this error during update cluster --yes:

W0826 15:49:52.508545   63215 executor.go:128] error running task "LoadBalancer/api.foo.k8s.local" (8m0s remaining to succeed): error updating load balancer listener's security policy: PolicyNotFound: There is no policy with name ELBSecurityPolicy-TLS-1-2-2017-01 for load balancer api-ho-dev-a-us-west-2-k8-a656uf
        status code: 400, request id: 0814b82c-2e84-4f08-a67e-b8f0cd5fd979

even though that is a valid policy name according to aws elb describe-load-balancer-policies.

Reading these docs it seems I would need to "create" the policy for the load balancer from the predefined policies, and then I can assign it to the listener.

aws elb create-load-balancer-policy --load-balancer-name my-loadbalancer
--policy-name my-SSLNegotiation-policy  --policy-type-name SSLNegotiationPolicyType
--policy-attributes AttributeName=Reference-Security-Policy,AttributeValue=ELBSecurityPolicy-2016-08

I'm wondering if Kops should try to reconcile this as well, creating the load-balancer-policy if it does not already exist, based on the API Field being used as the Rerence-Security-Policy. Once it is ensured to exist for the API ELB then Kops can SetLoadBalancerPoliciesOfListener. Thoughts?

I also think we will want to add API validation that makes sure this field is only set if the sslCertificate field is also set. We could add the validation here:

kops/pkg/apis/kops/validation/aws.go

Lines 33 to 37 in ffaf75f

	if c.Spec.API != nil {
	if c.Spec.API.LoadBalancer != nil {
	allErrs = append(allErrs, awsValidateAdditionalSecurityGroups(field.NewPath("spec", "api", "loadBalancer", "additionalSecurityGroups"), c.Spec.API.LoadBalancer.AdditionalSecurityGroups)...)
	}
	}

and I think field.Forbidden() might be the most appropriate validation error for this.

[rifelpet]

Rather than looking for the listener on port 443 perhaps it would be more flexible to perform this logic on any listener that has a Protocol of SSL?

[FrankYang0529]

We only apply security policies when specifying an ACM certificate. However, we set the listener on port 443 when the ACM certificate is existent. So I only set security policies for the listener on port 443 here.

kops/pkg/model/awsmodel/api_loadbalancer.go

Lines 112 to 114 in d90c90c

	if lbSpec.SSLCertificate != "" {
	listeners["443"] = &awstasks.LoadBalancerListener{InstancePort: 443, SSLCertificateID: lbSpec.SSLCertificate}
	}

[FrankYang0529]

Sounds good! Let's wait for #9011 to land and only add support for setting policies on NLBs.

[rifelpet]

@FrankYang0529 The NLB PR has been merged, are you still willing to update this to support NLB Listener SSLPolicies?

[FrankYang0529]

@rifelpet Yes, I will update it this week.

[FrankYang0529]

@rifelpet , I have updated it. We can support NLB Listener SSL Policy now.

[rifelpet]

This is much simpler now that it supports NLBs rather than classic load balancers :) nice work!

We'll want to add terraform and cloudformation support for this too. It should just be a matter of adding the new field here and setting its value below:

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

Lines 645 to 651 in ecea477

	type terraformNetworkLoadBalancerListener struct {
	LoadBalancer *terraform.Literal `json:"load_balancer_arn" cty:"load_balancer_arn"`
	Port int64 `json:"port" cty:"port"`
	Protocol string `json:"protocol" cty:"protocol"`
	CertificateARN *string `json:"certificate_arn,omitempty" cty:"certificate_arn"`
	DefaultAction []terraformNetworkLoadBalancerListenerAction `json:"default_action" cty:"default_action"`
	}

and cloudformation has its own struct later in the file too. (docs for terraform and cloudformation)

If you want I can do that in a followup PR.

[FrankYang0529]

@rifelpet Thanks for reviewing. I updated the code and added SSLPolicy in terraform and cloudformation.

[rifelpet]

looks good, one last suggestion:

Can you add an example sslPolicyto the two tests/integration/update_cluster/complex/in-*.yaml files? Then run ./hack/update-expected.sh and the kubernetes.tf and cloudformation.json in the same directory should get updated. commit all 4 changes here.

[FrankYang0529]

@rifelpet, I have added sslPolicy in two yaml files. However, I am not sure why I can't run tests successfully for k8s.io/kops/cmd/kops. Could you show me how to fix the test error? Thank you.

[rifelpet]

The verify-cloudformation failure is unrelated and was fixed by #10256. This PR's next invocation of that job should pass.

The TestLifecycleComplex failures are because the network load balancer task isn't recognizing the new field when describing the existing listeners in AWS. Reading the test failure output:

    lifecycle_integration_test.go:218: Target had changes after executing: Will modify resources:
          NetworkLoadBalancer/api-complex-example-com
          	Listeners           	 [{"Port":443,"TargetGroupName":"tls-complex-example-com-5nursn","SSLCertificateID":"arn:aws:acm:us-test-1:000000000000:certificate/123456789012-1234-1234-1234-12345678","SSLPolicy":""}, {"Port":8443,"TargetGroupName":"tcp-complex-example-com-vpjolq","SSLCertificateID":"","SSLPolicy":""}] -> [{"Port":443,"TargetGroupName":"tls-complex-example-com-5nursn","SSLCertificateID":"arn:aws:acm:us-test-1:000000000000:certificate/123456789012-1234-1234-1234-12345678","SSLPolicy":"ELBSecurityPolicy-2016-08"}, {"Port":8443,"TargetGroupName":"tcp-complex-example-com-vpjolq","SSLCertificateID":"","SSLPolicy":""}] 

This can be interpreted as "after applying the changes, Kops' interpretation of the NLB listener state doesn't match what it had just sent to AWS. Therefore there's either a bug in the Find() or RenderAWS() logic. In this case we can see the "actual" state read from AWS (the values before the ->) doesn't have the SSLPolicy set on the first listener, even though we included it in the CreateListener call. The SslPolicy does exist in the DescribeListener response, we just aren't doing anything with it.

Fixing it should just be a matter of setting the actualListener's value here, like we do with Port:

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

Lines 360 to 372 in a0c8925

	response, err := cloud.ELBV2().DescribeListeners(request)
	if err != nil {
	return nil, fmt.Errorf("error querying for NLB listeners :%v", err)
	}
	actual.Listeners = []*NetworkLoadBalancerListener{}
	actual.TargetGroups = []*TargetGroup{}
	for _, l := range response.Listeners {
	actualListener := &NetworkLoadBalancerListener{}
	actualListener.Port = int(aws.Int64Value(l.Port))
	if len(l.Certificates) != 0 {
	actualListener.SSLCertificateID = aws.StringValue(l.Certificates[0].CertificateArn) // What if there is more then one certificate, can we just grab the default certificate? we don't set it as default, we only set the one.
	}

[FrankYang0529]

@rifelpet Thank you for your elaboration. I know how it works now! Also, I have updated the code and I can run tests successfully.

[rifelpet]

looks great, thanks for sticking with this!

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FrankYang0529, 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