Add support for security policy #291

MrHohn · 2018-05-31T21:01:52Z

~~The first commit is for updating vendor. I will soon split that into another PR.~~ Updated vendor in #293.
/assign @rramkumar1 @bowei

rramkumar1 · 2018-05-31T22:42:02Z

pkg/annotations/service.go

@@ -117,10 +117,10 @@ func (svc Service) GetBackendConfigs() (*BackendConfigs, error) {

 	configs := BackendConfigs{}
 	if err := json.Unmarshal([]byte(val), &configs); err != nil {
-		return nil, err
+		return nil, fmt.Errorf("invalid format in backend config annotation: %v, error: %v", val, err)


Can you use BackendConfigKey instead of "backend config annotation"

rramkumar1 · 2018-05-31T22:42:19Z

pkg/annotations/service.go

 	}
 	if configs.Default == "" && len(configs.Ports) == 0 {
-		return nil, fmt.Errorf("no backendConfigs found in annotation: %v", val)
+		return nil, fmt.Errorf("either one of `ports` or `default` should be specified in annotation: %v", val)


Use BackendConfigKey to specify exact annotation.

rramkumar1 · 2018-05-31T22:42:49Z

pkg/backendconfig/backendconfig.go

+	if err != nil {
+		return nil, fmt.Errorf("failed to get BackendConfig %s, referenced by service %s/%s: %v", configName, svc.Namespace, svc.Name, err)
+	} else if !exists {
+		return nil, fmt.Errorf("BackendConfig %s not exist, but is referenced by service %s/%s", configName, svc.Namespace, svc.Name)


"BackendConfig %s does not exist..."

rramkumar1 · 2018-05-31T22:44:13Z

pkg/backendconfig/backendconfig.go

+	}
+	backendConfig, ok := obj.(*backendconfigv1beta1.BackendConfig)
+	if !ok {
+		return nil, fmt.Errorf("invalid format detected in BackendConfig %s, referenced by service %s/%s", configName, svc.Namespace, svc.Name)


Can we just say "failed to parse BackendConfig %s" instead of "invalid format detected".

rramkumar1 · 2018-05-31T22:44:47Z

pkg/backendconfig/backendconfig_test.go

@@ -323,6 +323,18 @@ func TestGetBackendConfigForServicePort(t *testing.T) {
 			},
 			expectErr: true,
 		},
+		{
+			desc:    "invalid format in backend config",


"invalid field in backend config"

rramkumar1 · 2018-05-31T22:52:05Z

pkg/backends/backends.go

@@ -263,6 +283,38 @@ func (b *Backends) ensureHealthCheck(sp utils.ServicePort) (string, error) {
 	return b.healthChecker.Sync(hc)
 }

+// TODO(mrhohn): Emit event when attach/detach security policy to backend service.
+func (b *Backends) ensureSecurityPolicy(sp utils.ServicePort, be *BackendService, beName string) error {


Discussed offline, can we move this to pkg/backends/features/securitypolicy.go

Done. PTAL thanks!

MrHohn · 2018-06-04T17:17:59Z

pkg/backends/backends.go

+	// The idea is to avoid impacting the usual workflow when security policy
+	// is irrelevant, especially when the Beta endpoint goes down.
+	if unknown && desiredPolicyLink != "" ||
+		retrieveObjectName(existingPolicyLink) != retrieveObjectName(desiredPolicyLink) {


Humm..Seems like I put a wrong condition here. Will push a new commit.

MrHohn · 2018-06-07T18:32:47Z

PR needs to be revised after #305.
/hold

bowei · 2018-06-13T05:51:15Z

pkg/backendconfig/backendconfig.go

-		return nil, fmt.Errorf("failed to get BackendConfig %s for service %s/%s: %v", configName, svc.Namespace, svc.Name, err)
+	if err != nil {
+		return nil, fmt.Errorf("failed to get BackendConfig %s, referenced by service %s/%s: %v", configName, svc.Namespace, svc.Name, err)
+	} else if !exists {


do not need else

Will separate to another PR.

bowei · 2018-06-13T05:51:51Z

pkg/backendconfig/backendconfig.go

+	}
+	backendConfig, ok := obj.(*backendconfigv1beta1.BackendConfig)
+	if !ok {
+		return nil, fmt.Errorf("failed to parse BackendConfig %s, referenced by service %s/%s", configName, svc.Namespace, svc.Name)


is this actually a parse error?

Will separate to another PR.

bowei · 2018-06-13T05:54:00Z

pkg/backends/backends.go

+	// service.
+	beBeta, err := b.cloud.GetBetaGlobalBackendService(beGa.Name)
+	if err != nil {
+		glog.Errorf("Failed to get backend service %s through Beta API", beGa.Name)


log the error

Will separate to another PR.

bowei · 2018-06-13T06:58:53Z

pkg/backends/features/securitypolicy.go

+	ProjectID() string
+}
+
+func getSecurityPolicyLink(be *unity.BackendService) (securityPolicyLink string, unknown bool) {


Function removed.

bowei · 2018-06-13T06:59:12Z

pkg/backends/features/securitypolicy.go

+}
+
+// TODO(mrhohn): Emit event when attach/detach security policy to backend service.
+func EnsureSecurityPolicy(cloud SecurityPolicySetter, sp utils.ServicePort, be *unity.BackendService, beName string) error {


// EnsureSecurityPolicy ...

bowei · 2018-06-13T07:00:53Z

pkg/backends/features/securitypolicy_test.go

+)
+
+func TestEnsureSecurityPolicy(t *testing.T) {
+	testCases := []struct {


t.Parallel() (on all unit tests)

bowei · 2018-06-13T07:01:16Z

pkg/backends/features/securitypolicy_test.go

+	}
+
+	for _, tc := range testCases {
+		fakeCloud := fake.NewFakeBackendServices(tc.errFunc)


t.Run(tc.desc, func(t*testing.T) {

bowei · 2018-06-13T07:02:11Z

pkg/backends/features/securitypolicy_test.go

+	}
+
+	for _, tc := range testCases {
+		policyLink, unknown := getSecurityPolicyLink(tc.backendService)


t.Run(tc.desc, ...

bowei · 2018-06-13T07:45:00Z

pkg/backends/unity/backendservice.go

+	"k8s.io/ingress-gce/pkg/utils"
+)
+
+// BackendService embeds all of the GA, beta and alpha compute


can you describe what the semantics of the storing some combination of Alpha, Beta, GA is? which one is preferred? What if there is a conflict etc.

File removed.

MrHohn · 2018-06-15T01:05:02Z

Rebased on top of #305.

MrHohn · 2018-06-15T01:05:28Z

/hold cancel

MrHohn · 2018-06-15T07:12:05Z

TODO: Define securityPolicy as a feature name in features.go. Will update PR.

MrHohn · 2018-06-15T19:15:23Z

TODO: Define securityPolicy as a feature name in features.go. Will update PR.

Done.

MrHohn · 2018-06-18T23:55:30Z

@rramkumar1 @bowei Rebased PR, PTAL, thanks!

rramkumar1 · 2018-06-19T00:08:45Z

/lgtm

bowei

other than the test comments, lgtm

bowei · 2018-06-19T19:33:56Z

pkg/backends/features/securitypolicy_test.go

+)
+
+// TODO(mrhohn): Upstream this mock hook to gce provider.
+func setSecurityPolicyHook(_ context.Context, key *meta.Key, ref *computebeta.SecurityPolicyReference, _ *cloud.MockBetaBackendServices) error {


move this inside the test if possible, avoid global locking

Sure, done.

bowei · 2018-06-19T19:34:29Z

pkg/backends/features/securitypolicy_test.go

+			t.Parallel()
+
+			fakeGCE := gce.FakeGCECloud(gce.DefaultTestClusterValues())
+			fakeBeName := fmt.Sprintf("be-name-XXX-%s", tc.desc)


do we need tc.desc in the backend name?

The purpose is to avoid tests conflicting with each other given that they run in parallel. Changed to use index instead.

bowei · 2018-06-19T19:34:39Z

pkg/backends/features/securitypolicy_test.go

+			(fakeGCE.Compute().(*cloud.MockGCE)).MockBetaBackendServices.SetSecurityPolicyHook = setSecurityPolicyHook
+
+			if err := EnsureSecurityPolicy(fakeGCE, utils.ServicePort{BackendConfig: tc.desiredConfig}, tc.currentBackendService, fakeBeName); err != nil {
+				t.Errorf("%s: EnsureSecurityPolicy()=%v, want nil", tc.desc, err)


remove tc.desc, not needed

bowei · 2018-06-19T19:34:47Z

pkg/backends/features/securitypolicy_test.go

+				// Verify whether the desired policy is set.
+				policyRef, ok := mockSecurityPolcies[fakeBeName]
+				if !ok {
+					t.Errorf("%s: policy not set for backend service %s", tc.desc, fakeBeName)


bowei · 2018-06-19T19:35:02Z

pkg/backends/features/securitypolicy_test.go

+					desiredPolicyName = tc.desiredConfig.Spec.SecurityPolicy.Name
+				}
+				if utils.EqualResourceIDs(policyLink, desiredPolicyName) {
+					t.Errorf("%s: got policy %q, want %q", tc.desc, policyLink, desiredPolicyName)


bowei · 2018-06-19T19:35:08Z

pkg/backends/features/securitypolicy_test.go

+				// Verify not set call is made.
+				policyRef, ok := mockSecurityPolcies[fakeBeName]
+				if ok {
+					t.Errorf("%s: unexpected policy %q is set for backend service %s", tc.desc, policyRef, fakeBeName)


k8s-ci-robot · 2018-06-19T21:13:12Z

New changes are detected. LGTM label has been removed.

k8s-ci-robot assigned bowei and rramkumar1 May 31, 2018

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 31, 2018

rramkumar1 reviewed May 31, 2018

View reviewed changes

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 1, 2018

MrHohn force-pushed the cloud-armor-support branch 2 times, most recently from ca6fda8 to d3facb3 Compare June 1, 2018 17:31

MrHohn commented Jun 4, 2018

View reviewed changes

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

bowei reviewed Jun 13, 2018

View reviewed changes

MrHohn force-pushed the cloud-armor-support branch from e8ba106 to cf8c6a0 Compare June 15, 2018 01:02

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

MrHohn force-pushed the cloud-armor-support branch from 291823f to 6061aa5 Compare June 15, 2018 19:16

aaron-trout mentioned this pull request Jun 18, 2018

Support SSL policies #246

Closed

MrHohn force-pushed the cloud-armor-support branch from 6061aa5 to cbf463a Compare June 18, 2018 22:38

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 18, 2018

MrHohn force-pushed the cloud-armor-support branch from cbf463a to 7743c3e Compare June 18, 2018 22:44

Add support for security policy

a079c50

MrHohn force-pushed the cloud-armor-support branch from 7743c3e to 2f87d2c Compare June 18, 2018 23:54

MrHohn force-pushed the cloud-armor-support branch from 2f87d2c to 5e26719 Compare June 18, 2018 23:58

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 19, 2018

bowei reviewed Jun 19, 2018

View reviewed changes

MrHohn added 2 commits June 19, 2018 14:13

Unit test for security policy

4629c5c

Define FeatureSecurityPolicy in features.go

d40ea79

MrHohn force-pushed the cloud-armor-support branch from 5e26719 to d40ea79 Compare June 19, 2018 21:13

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 19, 2018

bowei merged commit 8821e6c into kubernetes:master Jun 19, 2018

Add support for security policy #291

Add support for security policy #291

Conversation

MrHohn commented May 31, 2018 • 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

rramkumar1 May 31, 2018 • 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

MrHohn commented Jun 7, 2018

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

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

MrHohn commented Jun 15, 2018

MrHohn commented Jun 15, 2018

MrHohn commented Jun 15, 2018

MrHohn commented Jun 15, 2018

MrHohn commented Jun 18, 2018

rramkumar1 commented Jun 19, 2018

bowei left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 19, 2018

MrHohn commented May 31, 2018 •

edited

Loading

rramkumar1 May 31, 2018 •

edited

Loading