Prevent leaking seclist ingress and egress rules when updating a service #238

templecloud · 2018-08-21T09:33:00Z

This commit fixes a particular type of seclist rule leak in the CCM. When a services port or health check port is mutated then the old/original 'single-range' port rules created in the 'CCM ingress' and 'K8sWorker egress' seclists (or equivalent) are not deleted. This can be replicated consistently by running the 'Canary' e2e tests that performs a mutation test on a service. This fix also includes small optimisation to only create seclist rules for a loadbalancer after it has been successfully created.

This commit includes the following:

Two small conservative fixes to clean up the leaking ingress/egress rules when a service port is mutated. This required a few small alterations to pass up existing port information to the 'getEgressRules()' functions.
Some additional unit tests to check the above fixes.
Added an OCI client to the e2e test framework and added a simple sanity check to count the number seclist rules for a port before and after mutation. To keep things simple this test only runs if configured to do so. Configuring the tests consists of specifying the OCID of the target seclists to check as environment variables.

This commit does not include any refactoring or tidying up of these existing seclist rule management functionality. I think this could be harmonised a little and made more clear; then surrounded with further tests. In particular the getLoadBalancerEgressRules() and getNodeIngressRules() are related but work differently. I rolled back such changes for this commit as there was other ongoing work on seclists.

Anyway, please check carefully; this is my first foray into this stuff and needed a fun rebase to get the new changes to logging etc.

templecloud · 2018-08-21T09:36:13Z

Makefile

@@ -29,7 +29,7 @@ SRC_DIRS := cmd pkg # directories which hold app source (not vendored)

 # Allows overriding where the CCM should look for the cloud provider config
 # when running via make run-dev.
-CLOUD_PROVIDER_CFG ?= $$(pwd)/cloud-provider.yaml
+CLOUDCONFIG ?= $$(pwd)/cloud-provider.yaml


Need to remember why I wanted to change this. Can change back.

templecloud · 2018-08-21T09:38:10Z

hack/test-e2e.sh

-        --kubeconfig="${KUBECONFIG}" \
-        --cloud-config="${CLOUDCONFIG}" \
-        --delete-namespace=false
+        -- --kubeconfig=${KUBECONFIG} --cloud-config=${CLOUDCONFIG} --delete-namespace=true


Agh, the change in the Makefile above might have been to match this usage in the tests. I can still roll it back.

It seems irrelevant to this PR so would prefer it to be reversed to simplify review.

I rolled this back, but reckon we should have kept it. No-one will fix it in a single PR and in any other PR the same argument can be made! Anyway, should be reverted now!

templecloud · 2018-08-21T09:39:40Z

pkg/oci/ccm.go

@@ -235,9 +235,9 @@ func buildConfigurationProvider(logger *zap.Logger, config *Config) (common.Conf
 	return cp, nil
 }

-// newRateLimiter builds and returns a struct containing read and write
+// NewRateLimiter builds and returns a struct containing read and write


I needed this when creating the new OCI client for the e2e tests. It seemed wrong to cut and paste the code. This function seems to be a 'constructor', so I made it public.

templecloud · 2018-08-21T09:41:15Z

pkg/oci/load_balancer.go

@@ -293,7 +287,18 @@ func (cp *CloudProvider) createLoadBalancer(ctx context.Context, spec *LBSpec) (
 	}

 	logger.With("loadBalancerID", *lb.Id).Info("Load balancer created")
-	return loadBalancerToStatus(lb)
+	status, err := loadBalancerToStatus(lb)


I moved adding seclists to after we have created the loadbalancer. If creation fails for some reason the rules will not be added. Could there be unforseen issues with this?

templecloud · 2018-08-21T09:44:31Z

pkg/oci/load_balancer_security_lists.go

@@ -152,12 +152,20 @@ func (s *baseSecurityListManager) updateLoadBalancerRules(ctx context.Context, l

 		logger := s.logger.With("securityListID", *secList.Id)

-		lbEgressRules := getLoadBalancerEgressRules(logger, secList.EgressSecurityRules, nodeSubnets, ports.BackendPort, s.serviceLister)
-		lbEgressRules = getLoadBalancerEgressRules(logger, lbEgressRules, nodeSubnets, ports.HealthCheckerPort, s.serviceLister)
+		// -1 denotes nil ports.


I need to thread though the 'current' backEnd and health ports in-order to carefully delete the leaking rules. As I mentioned at some point I would like to tidy up this double invocation to getLoadBalancerEgressRules() and make it work more like getNodeIngressRules().

Isn't it just safe to use the zero value (i.e. 0) here as port 0 is reserved? Generally in-band errors / special values are a bit of a no-no (intertwined with the "make the zero value useful" philosophy).

templecloud · 2018-08-21T09:46:11Z

pkg/oci/load_balancer_security_lists.go

@@ -362,6 +378,23 @@ func getNodeIngressRules(
 	ingressRules := []core.IngressSecurityRule{}

 	for _, rule := range rules {
+		// Remove (do not re-add) any rule that represents the old case when
+		// mutating a single ranged backend port or health check port.
+		if rule.TcpOptions != nil && rule.TcpOptions.DestinationPortRange != nil &&


This is the first part of the fix. I wanted to be on the safe side so made it very restrictive. It should only target the 'single port rule mutation' described.

templecloud · 2018-08-21T09:46:52Z

pkg/oci/load_balancer_security_lists.go

@@ -531,8 +564,21 @@ func getLoadBalancerEgressRules(

 	egressRules := []core.EgressSecurityRule{}
 	for _, rule := range rules {
+		// Remove (do not re-add) any rule that represents the old case when mutating a single ranged port.


This is the second part of the fix. I wanted to be on the safe side so made it very restrictive. It should only target the 'single port rule mutation' described.

templecloud · 2018-08-21T09:49:44Z

pkg/oci/load_balancer_security_lists_test.go

@@ -195,6 +195,37 @@ func TestGetNodeIngressRules(t *testing.T) {
 				makeIngressSecurityRule("1", 80),
 				makeIngressSecurityRule("2", 80),
 			},
+		}, {


Unit test for health check port mutation. I chose to put more realistic strings in. IMHO it makes these tests easier to read/understand, especially if we added further input rules in. I can make it like the other though.

templecloud · 2018-08-21T09:50:18Z

pkg/oci/load_balancer_security_lists_test.go

 			expected: []core.EgressSecurityRule{
 				makeEgressSecurityRule("existing", 9000),
 				makeEgressSecurityRule("1", 80),
 				makeEgressSecurityRule("2", 80),
 			},
+		}, {


Unit test for service port mutation. I chose to put more realistic strings in. IMHO it makes these tests easier to read/understand, especially if we added further input rules in. I can make it like the other though.

templecloud · 2018-08-21T10:02:12Z

test/e2e/framework/framework.go

@@ -64,13 +69,20 @@ type Framework struct {
 	ClientSet         clientset.Interface
 	InternalClientset internalclientset.Interface

+	CloudProviderConfig *oci.Config      // If specified, the CloudProviderConfig. This provides information on the configuration of the test cluster.


Configurable OCI client to run some seclist sanity checks.

templecloud · 2018-08-21T10:02:58Z

test/e2e/framework/framework.go

+}
+
+// createOCIClient creates an OCI client derived from the CCM's cloud provider config file.
+func createOCIClient(cloudProviderConfig *oci.Config) (client.Interface, error) {


I did not see the point in adding configuration files. We grab the auth info from the cloud config.

templecloud · 2018-08-21T10:08:06Z

test/e2e/framework/seclist_utils.go

@@ -0,0 +1,143 @@
+/*


A set of utility functions that count the number of ingress/egress port rules in the specified seclists. Used by the test check the right number of rules are mutated and all the old rules are deleted.

templecloud · 2018-08-21T10:17:32Z

pkg/oci/load_balancer_security_lists.go

@@ -238,7 +246,7 @@ func (s *defaultSecurityListManager) Delete(ctx context.Context, lbSubnets []*co
 	noSubnets := []*core.Subnet{}
 	noSourceCIDRs := []string{}

-	err := s.updateLoadBalancerRules(ctx, lbSubnets, noSubnets, noSourceCIDRs, ports)
+	err := s.updateLoadBalancerRules(ctx, lbSubnets, noSubnets, noSourceCIDRs, &ports, ports)


Chose sending in port twice over nil. I think this makes sense.

owainlewis · 2018-08-22T15:32:50Z

hack/boilerplate/boilerplate.py

@@ -146,6 +146,7 @@ def file_extension(filename):
    "test/e2e/load_balancer.go",
    "test/e2e/framework/cleanup.go",
    "test/e2e/framework/framework.go",
+    "test/e2e/framework/seclist_utils.go",


seclist_utils => seclist_util to be consistent with test/e2e/framework/service_util.go

It was matching actually file called 'network_utils'. I made all util files singular.

owainlewis · 2018-08-23T12:41:04Z

pkg/oci/load_balancer_security_lists.go

@@ -143,7 +143,7 @@ func (s *baseSecurityListManager) updateBackendRules(ctx context.Context, lbSubn

 // updateLoadBalancerRules handles updating the ingress and egress rules for the load balance subnets.
 // If the listener is nil, then only egress rules from the load balancer to the backend subnets will be checked.
-func (s *baseSecurityListManager) updateLoadBalancerRules(ctx context.Context, lbSubnets []*core.Subnet, nodeSubnets []*core.Subnet, sourceCIDRs []string, ports portSpec) error {
+func (s *baseSecurityListManager) updateLoadBalancerRules(ctx context.Context, lbSubnets []*core.Subnet, nodeSubnets []*core.Subnet, sourceCIDRs []string, actualPorts *portSpec, desiredPorts portSpec) error {


We have actualPorts and desiredPorts here but one is a pointer, the other isn't. Is there any reason?

Yeagh, I agree - darn fuggly init?

To cut a long story short I think it derives from another bit of fuggly + typo on master (here)[https://github.com/oracle/oci-cloud-controller-manager/blob/e11429bc410ec62e52ba0aa504b777677bd3f908/pkg/oci/load_balancer_security_lists.go#L75].

I didn't fix it up on this run because I was trying to keep the changes small and close to the problem I am attempting to fix - as requested. It is really just cosmetic. I am happy to fix it up in this commit or as a subsequent one. Up to you. Please advise.

This logically makes sense. Update is used both for creation and modification and therefore there are call sites that don't have any "actual" (existing) ports associated.

I was just curious in case it was a typo. If there are call sites that require nil values then we can leave it as is.

owainlewis · 2018-08-23T12:46:34Z

test/e2e/framework/framework.go

@@ -95,7 +107,10 @@ func NewFramework(baseName string, client clientset.Interface) *Framework {
 		BaseName:  baseName,
 		ClientSet: client,
 	}
-
+	// Dev/CI only configuration. The seclist for CCM load-balancer routes.
+	f.CCMSecListID = os.Getenv("CCM_SECLIST_ID")


Both these values will default to an empty string if the env var is not set. Would that cause logical problems when passed to framework.CountSecListTunnelSinglePortRules ?

A good thing to query, but, hopefully it is ok. I hope it is the case that if these seclists values are not set then the test calculations will collapse into 0 - so they will reported a s valid. I tested it manually and it seemed to work ok. We could make it explicit, but, it would make the tests more verbose to read.

Rather than environment variables I'd prefer we used flags as we're already using them relatively extensively for configuration.

owainlewis · 2018-08-23T13:05:33Z

pkg/oci/load_balancer_security_lists.go

+		// mutating a single ranged backend port or health check port.
+		if rule.TcpOptions != nil && rule.TcpOptions.DestinationPortRange != nil &&
+			*rule.TcpOptions.DestinationPortRange.Min == *rule.TcpOptions.DestinationPortRange.Max &&
+			*rule.TcpOptions.DestinationPortRange.Min != desiredPorts.BackendPort && *rule.TcpOptions.DestinationPortRange.Max != desiredPorts.BackendPort &&


This large logical conjunction looks quite prone to subtle mistakes and I think is duplicated again below. Would be worth factoring this into a smaller helper function and unit testing.

Yeagh, I mentioned this in the JIRA and I started something like this on the 'other branch'. However, I thought we were meant to be making this fix with a few changes possible and then iterating to improve other bits? I am happy to have a go at improving this now, but, was trying to avoid a 'large commit'. Do you want me to refactor this code in this branch along with the fix attempt?

templecloud · 2018-08-23T15:57:05Z

Thanks for the review OL! You have suggested several good changes that I was originally going to raise a JIRA for and do as a separate bit work. Can you please confirm you want me to do it here and now? Then I will get cracking!

prydie · 2018-08-23T12:44:15Z

pkg/oci/load_balancer_security_lists.go

@@ -152,12 +152,20 @@ func (s *baseSecurityListManager) updateLoadBalancerRules(ctx context.Context, l

 		logger := s.logger.With("securityListID", *secList.Id)

-		lbEgressRules := getLoadBalancerEgressRules(logger, secList.EgressSecurityRules, nodeSubnets, ports.BackendPort, s.serviceLister)
-		lbEgressRules = getLoadBalancerEgressRules(logger, lbEgressRules, nodeSubnets, ports.HealthCheckerPort, s.serviceLister)
+		// -1 denotes nil ports.


Isn't it just safe to use the zero value (i.e. 0) here as port 0 is reserved? Generally in-band errors / special values are a bit of a no-no (intertwined with the "make the zero value useful" philosophy).

prydie · 2018-08-29T08:19:07Z

pkg/oci/load_balancer.go

@@ -319,6 +324,7 @@ func (cp *CloudProvider) EnsureLoadBalancer(ctx context.Context, clusterName str
 	}
 	subnets := []string{cp.config.LoadBalancer.Subnet1, cp.config.LoadBalancer.Subnet2}
 	spec, err := NewLBSpec(service, nodes, subnets, ssl, cp.securityListManagerFactory)
+


prydie · 2018-08-29T08:19:45Z

pkg/oci/load_balancer.go

@@ -319,6 +324,7 @@ func (cp *CloudProvider) EnsureLoadBalancer(ctx context.Context, clusterName str
 	}


Newline from below under here IMHO 😎

Classic Prydie! :)

prydie · 2018-08-29T08:21:59Z

pkg/oci/load_balancer_security_lists.go

@@ -143,7 +143,7 @@ func (s *baseSecurityListManager) updateBackendRules(ctx context.Context, lbSubn

 // updateLoadBalancerRules handles updating the ingress and egress rules for the load balance subnets.
 // If the listener is nil, then only egress rules from the load balancer to the backend subnets will be checked.
-func (s *baseSecurityListManager) updateLoadBalancerRules(ctx context.Context, lbSubnets []*core.Subnet, nodeSubnets []*core.Subnet, sourceCIDRs []string, ports portSpec) error {
+func (s *baseSecurityListManager) updateLoadBalancerRules(ctx context.Context, lbSubnets []*core.Subnet, nodeSubnets []*core.Subnet, sourceCIDRs []string, actualPorts *portSpec, desiredPorts portSpec) error {


This logically makes sense. Update is used both for creation and modification and therefore there are call sites that don't have any "actual" (existing) ports associated.

prydie · 2018-08-29T08:30:09Z

test/e2e/framework/framework.go

+func createCloudProviderConfig(cloudConfigFile string) (*oci.Config, error) {
+	file, err := os.Open(cloudConfigFile)
+	if err != nil {
+		return nil, errors.Wrap(err, fmt.Sprintf("Couldn't open cloud provider configuration: %s.", cloudConfigFile))


errors.Wrapf

prydie · 2018-08-29T08:30:14Z

test/e2e/framework/framework.go

+	defer file.Close()
+	cloudProviderConfig, err := oci.ReadConfig(file)
+	if err != nil {
+		return nil, errors.Wrap(err, fmt.Sprintf("Couldn't create cloud provider configuration: %s.", cloudConfigFile))


errors.Wrapf

prydie · 2018-08-29T08:30:21Z

test/e2e/framework/framework.go

+	rateLimiter := oci.NewRateLimiter(logger.Sugar(), cloudProviderConfig.RateLimiter)
+	ociClient, err := client.New(logger.Sugar(), ociClientConfig, &rateLimiter)
+	if err != nil {
+		return nil, errors.Wrap(err, fmt.Sprintf("Couldn't create oci client from configuration: %s.", cloudConfigFile))


errors.Wrapf

prydie · 2018-08-29T08:47:13Z

test/e2e/framework/framework.go

@@ -226,3 +251,30 @@ func (f *Framework) AfterEach() {
 		Failf(strings.Join(messages, ","))
 	}
 }
+
+// createCloudProviderConfig unmarshalls the CCM's cloud provider config from the specified location so it can be used for testing.


Generally doc strings are wrapped at 80 chars.

prydie · 2018-08-29T08:54:18Z

test/e2e/framework/framework.go

@@ -95,7 +107,10 @@ func NewFramework(baseName string, client clientset.Interface) *Framework {
 		BaseName:  baseName,
 		ClientSet: client,
 	}
-
+	// Dev/CI only configuration. The seclist for CCM load-balancer routes.
+	f.CCMSecListID = os.Getenv("CCM_SECLIST_ID")


Rather than environment variables I'd prefer we used flags as we're already using them relatively extensively for configuration.

prydie · 2018-08-29T08:55:47Z

test/e2e/framework/seclist_util.go

+
+// CountSecListTunnelSinglePortRules counts the number of 'single port'
+// (non-ranged) egress/ingress rules for the specified list and port.
+func CountSecListTunnelSinglePortRules(oci client.Interface, egressSecListID, ingressSecListID string, port int) (int, int) {


s/CountSecListTunnelSinglePortRules/CountSinglePortSecListRules/

Thought this might come up! Just wanted to mention I preferred environment variables here as I only intended these tests to be executed by our CCM ci pipeline (where we manage the seclist ids). Not by the canary tests etc where we don't. I felt threading them through was a bit noisy. Still if you want flags for consistency ill do it.

port.

templecloud · 2018-08-30T10:27:31Z

Thanks for the review guys. Made all changes. Also added some additional e2e configuration to the wercker yaml after chatting with @prydie. Squashed all commits.

kristenjacobs

In essence, this boils down to a couple of bits of logic that say if a rule matches our old port, and not the new one, then we assume that this is no longer required thus it can be deleted. Sounds good to me.

prydie · 2018-08-30T13:35:54Z

🎉

templecloud requested review from prydie, kristenjacobs and owainlewis August 21, 2018 09:33

templecloud commented Aug 21, 2018

View reviewed changes

templecloud assigned owainlewis and prydie Aug 21, 2018

owainlewis reviewed Aug 22, 2018

View reviewed changes

owainlewis reviewed Aug 23, 2018

View reviewed changes

prydie suggested changes Aug 29, 2018

View reviewed changes

owainlewis added bug enhancement labels Aug 29, 2018

Prevent leaking seclist ingress and egress rules when updating a service

aab7c20

port.

templecloud force-pushed the trjl/port-change-seclist-leak-fix branch from 391e9c2 to aab7c20 Compare August 30, 2018 10:24

kristenjacobs approved these changes Aug 30, 2018

View reviewed changes

prydie merged commit d94b7a7 into master Aug 30, 2018

templecloud deleted the trjl/port-change-seclist-leak-fix branch September 14, 2018 13:18

ayushverma14 pushed a commit to ayushverma14/oci-cloud-controller-manager that referenced this pull request Jul 18, 2022

Prevent leaking seclist rules when updating a service (oracle#238)

df1fbde

		@@ -319,6 +324,7 @@ func (cp *CloudProvider) EnsureLoadBalancer(ctx context.Context, clusterName str
		}

Prevent leaking seclist ingress and egress rules when updating a service #238

Prevent leaking seclist ingress and egress rules when updating a service #238

Conversation

templecloud commented Aug 21, 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

templecloud Aug 21, 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

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

owainlewis Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

templecloud Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

templecloud commented Aug 23, 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

templecloud commented Aug 30, 2018

kristenjacobs left a comment • edited Loading

Choose a reason for hiding this comment

prydie commented Aug 30, 2018

templecloud Aug 21, 2018 •

edited

Loading

owainlewis Aug 23, 2018 •

edited

Loading

templecloud Aug 23, 2018 •

edited

Loading

kristenjacobs left a comment •

edited

Loading