Prevent leaking seclist ingress and egress rules when updating a service #238
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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, actualPorts *portSpec, desiredPorts portSpec) error { | ||
|
There was a problem hiding this comment. We have actualPorts and desiredPorts here but one is a pointer, the other isn't. Is there any reason? There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. |
||
| for _, lbSubnet := range lbSubnets { | ||
| secList, etag, err := s.getSecurityList(ctx, lbSubnet) | ||
| if err != nil { | ||
|
|
@@ -152,12 +152,20 @@ func (s *baseSecurityListManager) updateLoadBalancerRules(ctx context.Context, l | |
|
|
||
| logger := s.logger.With("securityListID", *secList.Id) | ||
|
|
||
| // 0 denotes nil ports. | ||
| var currentBackEndPort = 0 | ||
| var currentHealthCheck = 0 | ||
| if actualPorts != nil { | ||
| currentBackEndPort = actualPorts.BackendPort | ||
| currentHealthCheck = actualPorts.HealthCheckerPort | ||
| } | ||
|
|
||
| lbEgressRules := getLoadBalancerEgressRules(logger, secList.EgressSecurityRules, nodeSubnets, currentBackEndPort, desiredPorts.BackendPort, s.serviceLister) | ||
| lbEgressRules = getLoadBalancerEgressRules(logger, lbEgressRules, nodeSubnets, currentHealthCheck, desiredPorts.HealthCheckerPort, s.serviceLister) | ||
|
|
||
| lbIngressRules := secList.IngressSecurityRules | ||
| if desiredPorts.ListenerPort != 0 { | ||
| lbIngressRules = getLoadBalancerIngressRules(logger, lbIngressRules, sourceCIDRs, desiredPorts.ListenerPort, s.serviceLister) | ||
| } | ||
|
|
||
| if !securityListRulesChanged(secList, lbIngressRules, lbEgressRules) { | ||
|
|
@@ -222,7 +230,7 @@ type defaultSecurityListManager struct { | |
| // Egress rules added: | ||
| // from LB subnets to backend subnets on the backend port | ||
| func (s *defaultSecurityListManager) Update(ctx context.Context, lbSubnets []*core.Subnet, backendSubnets []*core.Subnet, sourceCIDRs []string, actualPorts *portSpec, desiredPorts portSpec) error { | ||
| if err := s.updateLoadBalancerRules(ctx, lbSubnets, backendSubnets, sourceCIDRs, actualPorts, desiredPorts); err != nil { | ||
| return err | ||
| } | ||
|
|
||
|
|
@@ -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, ports) | ||
|
There was a problem hiding this comment. Chose sending in port twice over nil. I think this makes sense. |
||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
@@ -258,14 +266,14 @@ type frontendSecurityListManager struct { | |
| // from source cidrs to lb subnets on the listener port | ||
| func (s *frontendSecurityListManager) Update(ctx context.Context, lbSubnets []*core.Subnet, _ []*core.Subnet, sourceCIDRs []string, actualPorts *portSpec, desiredPorts portSpec) error { | ||
| noSubnets := []*core.Subnet{} | ||
| return s.updateLoadBalancerRules(ctx, lbSubnets, noSubnets, sourceCIDRs, actualPorts, desiredPorts) | ||
| } | ||
|
|
||
| // Delete the ingress security list rules associated with the listener. | ||
| func (s *frontendSecurityListManager) Delete(ctx context.Context, lbSubnets []*core.Subnet, backendSubnets []*core.Subnet, ports portSpec) error { | ||
| noSubnets := []*core.Subnet{} | ||
| noSourceCIDRs := []string{} | ||
| return s.updateLoadBalancerRules(ctx, lbSubnets, noSubnets, noSourceCIDRs, &ports, ports) | ||
| } | ||
|
|
||
| // securityListManagerNOOP implements the securityListManager interface but does | ||
|
|
@@ -352,6 +360,14 @@ func getNodeIngressRules( | |
| desiredPorts portSpec, | ||
| serviceLister listersv1.ServiceLister, | ||
| ) []core.IngressSecurityRule { | ||
| // 0 denotes nil ports. | ||
| var currentBackEndPort = 0 | ||
| var currentHealthCheckPort = 0 | ||
| if actualPorts != nil { | ||
| currentBackEndPort = actualPorts.BackendPort | ||
| currentHealthCheckPort = actualPorts.HealthCheckerPort | ||
| } | ||
|
|
||
| desiredBackend := sets.NewString() | ||
| desiredHealthChecker := sets.NewString() | ||
| for _, lbSubnet := range lbSubnets { | ||
|
|
@@ -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 && | ||
|
There was a problem hiding this comment. 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. |
||
| *rule.TcpOptions.DestinationPortRange.Min == *rule.TcpOptions.DestinationPortRange.Max && | ||
| *rule.TcpOptions.DestinationPortRange.Min != desiredPorts.BackendPort && *rule.TcpOptions.DestinationPortRange.Max != desiredPorts.BackendPort && | ||
|
There was a problem hiding this comment. 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. There was a problem hiding this comment. 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? |
||
| *rule.TcpOptions.DestinationPortRange.Min != desiredPorts.HealthCheckerPort && *rule.TcpOptions.DestinationPortRange.Max != desiredPorts.HealthCheckerPort { | ||
| var rulePort = *rule.TcpOptions.DestinationPortRange.Min | ||
| if rulePort == currentBackEndPort || rulePort == currentHealthCheckPort { | ||
| logger.With( | ||
| "source", *rule.Source, | ||
| "destinationPortRangeMin", *rule.TcpOptions.DestinationPortRange.Min, | ||
| "destinationPortRangeMax", *rule.TcpOptions.DestinationPortRange.Max, | ||
| ).Debug("Deleting load balancer ingres security rule") | ||
| continue | ||
| } | ||
| } | ||
|
|
||
| if rule.TcpOptions == nil || rule.TcpOptions.SourcePortRange != nil || rule.TcpOptions.DestinationPortRange == nil { | ||
| // this rule doesn't apply to this service so nothing to do but keep it | ||
| ingressRules = append(ingressRules, rule) | ||
|
|
@@ -521,7 +554,7 @@ func getLoadBalancerEgressRules( | |
| logger *zap.SugaredLogger, | ||
| rules []core.EgressSecurityRule, | ||
| nodeSubnets []*core.Subnet, | ||
| actualPort, desiredPort int, | ||
| serviceLister listersv1.ServiceLister, | ||
| ) []core.EgressSecurityRule { | ||
| nodeCIDRs := sets.NewString() | ||
|
|
@@ -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. | ||
|
There was a problem hiding this comment. 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. |
||
| if rule.TcpOptions != nil && rule.TcpOptions.DestinationPortRange != nil && | ||
| *rule.TcpOptions.DestinationPortRange.Min == *rule.TcpOptions.DestinationPortRange.Max && | ||
| *rule.TcpOptions.DestinationPortRange.Min != desiredPort && *rule.TcpOptions.DestinationPortRange.Max != desiredPort && | ||
| *rule.TcpOptions.DestinationPortRange.Min == actualPort && *rule.TcpOptions.DestinationPortRange.Max == actualPort { | ||
| logger.With( | ||
| "destination", *rule.Destination, | ||
| "destinationPortRangeMin", *rule.TcpOptions.DestinationPortRange.Min, | ||
| "destinationPortRangeMax", *rule.TcpOptions.DestinationPortRange.Max, | ||
| ).Debug("Deleting load balancer ingres security rule") | ||
| continue | ||
| } | ||
|
|
||
| if rule.TcpOptions == nil || rule.TcpOptions.SourcePortRange != nil || rule.TcpOptions.DestinationPortRange == nil || | ||
| *rule.TcpOptions.DestinationPortRange.Min != desiredPort || *rule.TcpOptions.DestinationPortRange.Max != desiredPort { | ||
| // this rule doesn't apply to this service so nothing to do but keep it | ||
| egressRules = append(egressRules, rule) | ||
| continue | ||
|
|
@@ -545,19 +591,19 @@ func getLoadBalancerEgressRules( | |
| continue | ||
| } | ||
|
|
||
| inUse, err := healthCheckPortInUse(serviceLister, int32(desiredPort)) | ||
| if err != nil { | ||
| // Unable to determine if this port is in use by another service, so I guess | ||
| // we better err on the safe side and keep the rule. | ||
| logger.With(zap.Error(err), "port", desiredPort).Error("Failed to determine if port is still in use") | ||
| egressRules = append(egressRules, rule) | ||
| continue | ||
| } | ||
|
|
||
| if inUse { | ||
| // This rule is no longer needed for this service, but is still used | ||
| // by another service, so we must still keep it. | ||
| logger.With("port", desiredPort).Debug("Port still in use by another service.") | ||
| egressRules = append(egressRules, rule) | ||
| continue | ||
| } | ||
|
|
@@ -579,7 +625,7 @@ func getLoadBalancerEgressRules( | |
| // All the remaining node cidr's are new and don't have a corresponding rule | ||
| // so we need to create one for each. | ||
| for _, desired := range nodeCIDRs.List() { | ||
| rule := makeEgressSecurityRule(desired, desiredPort) | ||
| logger.With( | ||
| "destination", *rule.Destination, | ||
| "destinationPortRangeMin", *rule.TcpOptions.DestinationPortRange.Min, | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agh, the change in the Makefile above might have been to match this usage in the tests. I can still roll it back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems irrelevant to this PR so would prefer it to be reversed to simplify review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!