Implement SslPolicies for TargetHttpsProxy #1019

spencerhance · 2020-02-04T18:28:27Z

This is the main feature PR for SslPolicies

This relies on a few other PRs, but can be reviewed in parallel

k8s-ci-robot · 2020-02-04T18:28:35Z

Hi @spencerhance. 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.

spencerhance · 2020-02-04T19:14:16Z

/assign @rramkumar1

rramkumar1 · 2020-02-06T19:05:35Z

/ok-to-test

pkg/loadbalancers/target_proxies.go

spencerhance · 2020-02-12T01:35:05Z

@rramkumar1 Added a bunch of unit tests, can you take another look?

I also refactored the syncing logic so that we sync on target proxy creation and later syncs

rramkumar1 · 2020-02-13T14:25:28Z

pkg/loadbalancers/loadbalancers_test.go

@@ -97,6 +99,18 @@ func newTestJig(t *testing.T) *testJig {
 		tp.SslCertificates = request.SslCertificates
 		return nil
 	}
+	mockGCE.MockTargetHttpsProxies.SetSslPolicyHook = func(ctx context.Context, key *meta.Key, ref *compute.SslPolicyReference, proxies *cloud.MockTargetHttpsProxies) error {


Is this hook special? Can it be added to k8s_cloud_provider mock.go instead?

If its too much work to re-vendor then I suppose we can have this here but would be good to upstream

Can I do this in a follow up and add a TODO here? Re-vendoring always takes like a couple days to get all the approvals.

rramkumar1 · 2020-02-13T14:26:01Z

pkg/utils/utils.go

@@ -439,3 +439,8 @@ func IsLegacyL4ILBService(svc *api_v1.Service) bool {
 	}
 	return false
 }
+
+// NewString returns a pointer to the provider string literal


rramkumar1 · 2020-02-13T14:26:17Z

pkg/utils/utils.go

@@ -439,3 +439,8 @@ func IsLegacyL4ILBService(svc *api_v1.Service) bool {
 	}
 	return false
 }
+
+// NewString returns a pointer to the provider string literal
+func NewString(s string) *string {


NewStringPointer

rramkumar1 · 2020-02-13T14:28:45Z

pkg/loadbalancers/loadbalancers_test.go

+		desc string
+		fc   *frontendconfigv1beta1.FrontendConfig
+		want *string
+	}{


Do you need a case for frontend config with empty string ssl policy?

rramkumar1 · 2020-02-13T14:31:04Z

pkg/loadbalancers/loadbalancers_test.go

+			want:  "global/sslPolicies/test-policy",
+		},
+		{
+			desc:  "proxy with different ssl policy",


Is this a duplicate of the test case below?

rramkumar1 · 2020-02-13T14:31:50Z

pkg/loadbalancers/loadbalancers_test.go

+			want:  "",
+		},
+	}
+


You need a case where a proxy has a policy and then the frontend config policy string is nil.

rramkumar1 · 2020-02-17T17:19:53Z

Be sure to send a PR for the k8s_cloud_provider change

/lgtm

k8s-ci-robot · 2020-02-17T17:20:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rramkumar1, spencerhance

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~~ [rramkumar1]

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 added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 4, 2020

k8s-ci-robot requested review from bowei and rramkumar1 February 4, 2020 18:29

k8s-ci-robot assigned rramkumar1 Feb 4, 2020

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 6, 2020

rramkumar1 reviewed Feb 6, 2020

View reviewed changes

pkg/loadbalancers/target_proxies.go Show resolved Hide resolved

pkg/loadbalancers/target_proxies.go Outdated Show resolved Hide resolved

pkg/loadbalancers/target_proxies.go Outdated Show resolved Hide resolved

spencerhance force-pushed the implement-feature-ssl-policy branch 2 times, most recently from b6d2046 to 561d69b Compare February 6, 2020 19:17

rramkumar1 reviewed Feb 6, 2020

View reviewed changes

pkg/loadbalancers/target_proxies.go Show resolved Hide resolved

pkg/loadbalancers/target_proxies.go Show resolved Hide resolved

pkg/loadbalancers/target_proxies.go Show resolved Hide resolved

spencerhance force-pushed the implement-feature-ssl-policy branch 4 times, most recently from 351064e to c5703b8 Compare February 12, 2020 01:33

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 12, 2020

spencerhance force-pushed the implement-feature-ssl-policy branch from c5703b8 to 15cf41a Compare February 12, 2020 01:33

rramkumar1 reviewed Feb 13, 2020

View reviewed changes

Implement SslPolicies for TargetHttpsProxy

a0f6af4

spencerhance force-pushed the implement-feature-ssl-policy branch from 15cf41a to a0f6af4 Compare February 13, 2020 18:32

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 17, 2020

k8s-ci-robot merged commit 05d9c8d into kubernetes:master Feb 17, 2020

rnaveiras mentioned this pull request Feb 24, 2020

Support SSL policies #246

Closed

spencerhance deleted the implement-feature-ssl-policy branch June 29, 2020 22:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement SslPolicies for TargetHttpsProxy #1019

Implement SslPolicies for TargetHttpsProxy #1019

spencerhance commented Feb 4, 2020

k8s-ci-robot commented Feb 4, 2020

spencerhance commented Feb 4, 2020

rramkumar1 commented Feb 6, 2020

spencerhance commented Feb 12, 2020 •

edited

Loading

rramkumar1 Feb 13, 2020

spencerhance Feb 13, 2020

rramkumar1 Feb 13, 2020

spencerhance Feb 13, 2020

rramkumar1 Feb 13, 2020

spencerhance Feb 13, 2020

rramkumar1 Feb 13, 2020

spencerhance Feb 13, 2020

rramkumar1 Feb 13, 2020

spencerhance Feb 13, 2020

rramkumar1 Feb 13, 2020

spencerhance Feb 13, 2020

rramkumar1 commented Feb 17, 2020

k8s-ci-robot commented Feb 17, 2020

Implement SslPolicies for TargetHttpsProxy #1019

Implement SslPolicies for TargetHttpsProxy #1019

Conversation

spencerhance commented Feb 4, 2020

k8s-ci-robot commented Feb 4, 2020

spencerhance commented Feb 4, 2020

rramkumar1 commented Feb 6, 2020

spencerhance commented Feb 12, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rramkumar1 commented Feb 17, 2020

k8s-ci-robot commented Feb 17, 2020

spencerhance commented Feb 12, 2020 •

edited

Loading