Skip to content

Commit d94b7a7

Browse files
templecloudprydie
templecloud
authored and
prydie
committed
Prevent leaking seclist rules when updating a service (#238)
1 parent 45ff9c8 commit d94b7a7

File tree

14 files changed

+460
-59
lines changed

14 files changed

+460
-59
lines changed

‎Makefile‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -43,15 +43,15 @@ all: check test build
4343

4444
.PHONY: gofmt
4545
gofmt:
46-
@./hack/check-gofmt.sh ${SRC_DIRS}
46+
@./hack/check-gofmt.sh $(SRC_DIRS)
4747

4848
.PHONY: golint
4949
golint:
50-
@./hack/check-golint.sh ${SRC_DIRS}
50+
@./hack/check-golint.sh $(SRC_DIRS)
5151

5252
.PHONY: govet
5353
govet:
54-
@./hack/check-govet.sh ${SRC_DIRS}
54+
@./hack/check-govet.sh $(SRC_DIRS)
5555

5656
.PHONY: check
5757
check: gofmt govet golint
@@ -62,17 +62,17 @@ build-dirs:
6262

6363
.PHONY: build
6464
build: build-dirs manifests
65-
@GOOS=${GOOS} GOARCH=${ARCH} go build \
65+
@GOOS=$(GOOS) GOARCH=$(ARCH) go build \
6666
-i \
6767
-o dist/oci-cloud-controller-manager \
6868
-installsuffix "static" \
69-
-ldflags "-X main.version=${VERSION} -X main.build=${BUILD}" \
69+
-ldflags "-X main.version=$(VERSION) -X main.build=$(BUILD)" \
7070
./cmd/oci-cloud-controller-manager
7171

7272
.PHONY: manifests
7373
manifests: build-dirs
7474
@cp -a manifests/* dist
75-
@sed ${SED_INPLACE} \
75+
@sed $(SED_INPLACE) \
7676
's#${IMAGE}:[0-9]\+.[0-9]\+.[0-9]\+#${IMAGE}:${VERSION}#g' \
7777
dist/oci-cloud-controller-manager.yaml
7878

@@ -114,18 +114,18 @@ clean:
114114

115115
.PHONY: deploy
116116
deploy:
117-
kubectl -n kube-system set image ds/${BIN} ${BIN}=${IMAGE}:${VERSION}
117+
kubectl -n kube-system set image ds/$(BIN) $(BIN)=$(IMAGE):$(VERSION)
118118

119119
.PHONY: run-dev
120120
run-dev: build
121121
@dist/oci-cloud-controller-manager \
122-
--kubeconfig=${KUBECONFIG} \
123-
--cloud-config=${CLOUD_PROVIDER_CFG} \
122+
--kubeconfig=$(KUBECONFIG) \
123+
--cloud-config=$(CLOUD_PROVIDER_CFG) \
124124
--cluster-cidr=10.244.0.0/16 \
125125
--leader-elect-resource-lock=configmaps \
126126
--cloud-provider=oci \
127127
-v=4
128128

129129
.PHONY: version
130130
version:
131-
@echo ${VERSION}
131+
@echo $(VERSION)

‎hack/boilerplate/boilerplate.py‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ def file_extension(filename):
136136
".wercker",
137137
"vendor",
138138

139-
# Imported from Kubernetes maintaining origional copyright header
139+
# Imported from Kubernetes maintaining original copyright header
140140
"hack/boilerplate/boilerplate.py",
141141
"hack/boilerplate/boilerplate_test.py",
142142
"hack/boilerplate/test",
@@ -146,9 +146,10 @@ def file_extension(filename):
146146
"test/e2e/load_balancer.go",
147147
"test/e2e/framework/cleanup.go",
148148
"test/e2e/framework/framework.go",
149+
"test/e2e/framework/seclist_util.go",
149150
"test/e2e/framework/service_util.go",
150151
"test/e2e/framework/util.go",
151-
"test/e2e/framework/networking_utils.go",
152+
"test/e2e/framework/networking_util.go",
152153
"test/e2e/framework/ginkgowrapper/wrapper.go",
153154
]
154155

‎hack/test-e2e.sh‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,7 @@ function run_e2e_tests() {
2626
ginkgo -v -progress \
2727
-focus "\[Canary\]" \
2828
test/e2e \
29-
-- \
30-
--kubeconfig="${KUBECONFIG}" \
31-
--cloud-config="${CLOUDCONFIG}" \
32-
--delete-namespace=false
29+
-- --kubeconfig=${KUBECONFIG} --cloud-config=${CLOUDCONFIG} --delete-namespace=true
3330
}
3431

3532
# Main ************************************************************************

‎pkg/oci/ccm.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ func NewCloudProvider(config *Config) (cloudprovider.Interface, error) {
8585
return nil, err
8686
}
8787

88-
rateLimiter := newRateLimiter(logger.Sugar(), config.RateLimiter)
88+
rateLimiter := NewRateLimiter(logger.Sugar(), config.RateLimiter)
8989

9090
c, err := client.New(logger.Sugar(), cp, &rateLimiter)
9191
if err != nil {
@@ -235,9 +235,9 @@ func buildConfigurationProvider(logger *zap.Logger, config *Config) (common.Conf
235235
return cp, nil
236236
}
237237

238-
// newRateLimiter builds and returns a struct containing read and write
238+
// NewRateLimiter builds and returns a struct containing read and write
239239
// rate limiters. Defaults are used where no (0) value is provided.
240-
func newRateLimiter(logger *zap.SugaredLogger, config *RateLimiterConfig) client.RateLimiter {
240+
func NewRateLimiter(logger *zap.SugaredLogger, config *RateLimiterConfig) client.RateLimiter {
241241
if config == nil {
242242
config = &RateLimiterConfig{}
243243
}

‎pkg/oci/ccm_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ func TestBuildRateLimiterWithConfig(t *testing.T) {
3333
RateLimitBucketWrite: bucketWrite,
3434
}
3535

36-
rateLimiter := newRateLimiter(zap.S(), rateLimiterConfig)
36+
rateLimiter := NewRateLimiter(zap.S(), rateLimiterConfig)
3737

3838
if rateLimiter.Reader.QPS() != qpsRead {
3939
t.Errorf("unexpected QPS (read) value: expected %f but found %f", qpsRead, rateLimiter.Reader.QPS())
@@ -47,7 +47,7 @@ func TestBuildRateLimiterWithConfig(t *testing.T) {
4747
func TestBuildRateLimiterWithDefaults(t *testing.T) {
4848
rateLimiterConfig := &RateLimiterConfig{}
4949

50-
rateLimiter := newRateLimiter(zap.S(), rateLimiterConfig)
50+
rateLimiter := NewRateLimiter(zap.S(), rateLimiterConfig)
5151

5252
if rateLimiter.Reader.QPS() != rateLimitQPSDefault {
5353
t.Errorf("unexpected QPS (read) value: expected %f but found %f", rateLimitQPSDefault, rateLimiter.Reader.QPS())

‎pkg/oci/load_balancer.go‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -265,12 +265,6 @@ func (cp *CloudProvider) createLoadBalancer(ctx context.Context, spec *LBSpec) (
265265
return nil, errors.Wrap(err, "getting subnets for nodes")
266266
}
267267

268-
for _, ports := range spec.Ports {
269-
if err = spec.securityListManager.Update(ctx, lbSubnets, nodeSubnets, spec.SourceCIDRs, nil, ports); err != nil {
270-
return nil, err
271-
}
272-
}
273-
274268
// Then we create the load balancer and wait for it to be online.
275269
certs, err := spec.Certificates()
276270
if err != nil {
@@ -302,7 +296,18 @@ func (cp *CloudProvider) createLoadBalancer(ctx context.Context, spec *LBSpec) (
302296
}
303297

304298
logger.With("loadBalancerID", *lb.Id).Info("Load balancer created")
305-
return loadBalancerToStatus(lb)
299+
status, err := loadBalancerToStatus(lb)
300+
if status != nil && len(status.Ingress) > 0 {
301+
// If the LB is successfully provisioned then open lb/node subnet seclists egress/ingress.
302+
for _, ports := range spec.Ports {
303+
if err = spec.securityListManager.Update(ctx, lbSubnets, nodeSubnets, spec.SourceCIDRs, nil, ports); err != nil {
304+
return nil, err
305+
}
306+
}
307+
}
308+
309+
return status, err
310+
306311
}
307312

308313
// EnsureLoadBalancer creates a new load balancer or updates the existing one.

‎pkg/oci/load_balancer_security_lists.go‎

Lines changed: 61 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,7 @@ func (s *baseSecurityListManager) updateBackendRules(ctx context.Context, lbSubn
143143

144144
// updateLoadBalancerRules handles updating the ingress and egress rules for the load balance subnets.
145145
// If the listener is nil, then only egress rules from the load balancer to the backend subnets will be checked.
146-
func (s *baseSecurityListManager) updateLoadBalancerRules(ctx context.Context, lbSubnets []*core.Subnet, nodeSubnets []*core.Subnet, sourceCIDRs []string, ports portSpec) error {
146+
func (s *baseSecurityListManager) updateLoadBalancerRules(ctx context.Context, lbSubnets []*core.Subnet, nodeSubnets []*core.Subnet, sourceCIDRs []string, actualPorts *portSpec, desiredPorts portSpec) error {
147147
for _, lbSubnet := range lbSubnets {
148148
secList, etag, err := s.getSecurityList(ctx, lbSubnet)
149149
if err != nil {
@@ -152,12 +152,20 @@ func (s *baseSecurityListManager) updateLoadBalancerRules(ctx context.Context, l
152152

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

155-
lbEgressRules := getLoadBalancerEgressRules(logger, secList.EgressSecurityRules, nodeSubnets, ports.BackendPort, s.serviceLister)
156-
lbEgressRules = getLoadBalancerEgressRules(logger, lbEgressRules, nodeSubnets, ports.HealthCheckerPort, s.serviceLister)
155+
// 0 denotes nil ports.
156+
var currentBackEndPort = 0
157+
var currentHealthCheck = 0
158+
if actualPorts != nil {
159+
currentBackEndPort = actualPorts.BackendPort
160+
currentHealthCheck = actualPorts.HealthCheckerPort
161+
}
162+
163+
lbEgressRules := getLoadBalancerEgressRules(logger, secList.EgressSecurityRules, nodeSubnets, currentBackEndPort, desiredPorts.BackendPort, s.serviceLister)
164+
lbEgressRules = getLoadBalancerEgressRules(logger, lbEgressRules, nodeSubnets, currentHealthCheck, desiredPorts.HealthCheckerPort, s.serviceLister)
157165

158166
lbIngressRules := secList.IngressSecurityRules
159-
if ports.ListenerPort != 0 {
160-
lbIngressRules = getLoadBalancerIngressRules(logger, lbIngressRules, sourceCIDRs, ports.ListenerPort, s.serviceLister)
167+
if desiredPorts.ListenerPort != 0 {
168+
lbIngressRules = getLoadBalancerIngressRules(logger, lbIngressRules, sourceCIDRs, desiredPorts.ListenerPort, s.serviceLister)
161169
}
162170

163171
if !securityListRulesChanged(secList, lbIngressRules, lbEgressRules) {
@@ -222,7 +230,7 @@ type defaultSecurityListManager struct {
222230
// Egress rules added:
223231
// from LB subnets to backend subnets on the backend port
224232
func (s *defaultSecurityListManager) Update(ctx context.Context, lbSubnets []*core.Subnet, backendSubnets []*core.Subnet, sourceCIDRs []string, actualPorts *portSpec, desiredPorts portSpec) error {
225-
if err := s.updateLoadBalancerRules(ctx, lbSubnets, backendSubnets, sourceCIDRs, desiredPorts); err != nil {
233+
if err := s.updateLoadBalancerRules(ctx, lbSubnets, backendSubnets, sourceCIDRs, actualPorts, desiredPorts); err != nil {
226234
return err
227235
}
228236

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

241-
err := s.updateLoadBalancerRules(ctx, lbSubnets, noSubnets, noSourceCIDRs, ports)
249+
err := s.updateLoadBalancerRules(ctx, lbSubnets, noSubnets, noSourceCIDRs, &ports, ports)
242250
if err != nil {
243251
return err
244252
}
@@ -258,14 +266,14 @@ type frontendSecurityListManager struct {
258266
// from source cidrs to lb subnets on the listener port
259267
func (s *frontendSecurityListManager) Update(ctx context.Context, lbSubnets []*core.Subnet, _ []*core.Subnet, sourceCIDRs []string, actualPorts *portSpec, desiredPorts portSpec) error {
260268
noSubnets := []*core.Subnet{}
261-
return s.updateLoadBalancerRules(ctx, lbSubnets, noSubnets, sourceCIDRs, desiredPorts)
269+
return s.updateLoadBalancerRules(ctx, lbSubnets, noSubnets, sourceCIDRs, actualPorts, desiredPorts)
262270
}
263271

264272
// Delete the ingress security list rules associated with the listener.
265273
func (s *frontendSecurityListManager) Delete(ctx context.Context, lbSubnets []*core.Subnet, backendSubnets []*core.Subnet, ports portSpec) error {
266274
noSubnets := []*core.Subnet{}
267275
noSourceCIDRs := []string{}
268-
return s.updateLoadBalancerRules(ctx, lbSubnets, noSubnets, noSourceCIDRs, ports)
276+
return s.updateLoadBalancerRules(ctx, lbSubnets, noSubnets, noSourceCIDRs, &ports, ports)
269277
}
270278

271279
// securityListManagerNOOP implements the securityListManager interface but does
@@ -352,6 +360,14 @@ func getNodeIngressRules(
352360
desiredPorts portSpec,
353361
serviceLister listersv1.ServiceLister,
354362
) []core.IngressSecurityRule {
363+
// 0 denotes nil ports.
364+
var currentBackEndPort = 0
365+
var currentHealthCheckPort = 0
366+
if actualPorts != nil {
367+
currentBackEndPort = actualPorts.BackendPort
368+
currentHealthCheckPort = actualPorts.HealthCheckerPort
369+
}
370+
355371
desiredBackend := sets.NewString()
356372
desiredHealthChecker := sets.NewString()
357373
for _, lbSubnet := range lbSubnets {
@@ -362,6 +378,23 @@ func getNodeIngressRules(
362378
ingressRules := []core.IngressSecurityRule{}
363379

364380
for _, rule := range rules {
381+
// Remove (do not re-add) any rule that represents the old case when
382+
// mutating a single ranged backend port or health check port.
383+
if rule.TcpOptions != nil && rule.TcpOptions.DestinationPortRange != nil &&
384+
*rule.TcpOptions.DestinationPortRange.Min == *rule.TcpOptions.DestinationPortRange.Max &&
385+
*rule.TcpOptions.DestinationPortRange.Min != desiredPorts.BackendPort && *rule.TcpOptions.DestinationPortRange.Max != desiredPorts.BackendPort &&
386+
*rule.TcpOptions.DestinationPortRange.Min != desiredPorts.HealthCheckerPort && *rule.TcpOptions.DestinationPortRange.Max != desiredPorts.HealthCheckerPort {
387+
var rulePort = *rule.TcpOptions.DestinationPortRange.Min
388+
if rulePort == currentBackEndPort || rulePort == currentHealthCheckPort {
389+
logger.With(
390+
"source", *rule.Source,
391+
"destinationPortRangeMin", *rule.TcpOptions.DestinationPortRange.Min,
392+
"destinationPortRangeMax", *rule.TcpOptions.DestinationPortRange.Max,
393+
).Debug("Deleting load balancer ingres security rule")
394+
continue
395+
}
396+
}
397+
365398
if rule.TcpOptions == nil || rule.TcpOptions.SourcePortRange != nil || rule.TcpOptions.DestinationPortRange == nil {
366399
// this rule doesn't apply to this service so nothing to do but keep it
367400
ingressRules = append(ingressRules, rule)
@@ -521,7 +554,7 @@ func getLoadBalancerEgressRules(
521554
logger *zap.SugaredLogger,
522555
rules []core.EgressSecurityRule,
523556
nodeSubnets []*core.Subnet,
524-
port int,
557+
actualPort, desiredPort int,
525558
serviceLister listersv1.ServiceLister,
526559
) []core.EgressSecurityRule {
527560
nodeCIDRs := sets.NewString()
@@ -531,8 +564,21 @@ func getLoadBalancerEgressRules(
531564

532565
egressRules := []core.EgressSecurityRule{}
533566
for _, rule := range rules {
567+
// Remove (do not re-add) any rule that represents the old case when mutating a single ranged port.
568+
if rule.TcpOptions != nil && rule.TcpOptions.DestinationPortRange != nil &&
569+
*rule.TcpOptions.DestinationPortRange.Min == *rule.TcpOptions.DestinationPortRange.Max &&
570+
*rule.TcpOptions.DestinationPortRange.Min != desiredPort && *rule.TcpOptions.DestinationPortRange.Max != desiredPort &&
571+
*rule.TcpOptions.DestinationPortRange.Min == actualPort && *rule.TcpOptions.DestinationPortRange.Max == actualPort {
572+
logger.With(
573+
"destination", *rule.Destination,
574+
"destinationPortRangeMin", *rule.TcpOptions.DestinationPortRange.Min,
575+
"destinationPortRangeMax", *rule.TcpOptions.DestinationPortRange.Max,
576+
).Debug("Deleting load balancer ingres security rule")
577+
continue
578+
}
579+
534580
if rule.TcpOptions == nil || rule.TcpOptions.SourcePortRange != nil || rule.TcpOptions.DestinationPortRange == nil ||
535-
*rule.TcpOptions.DestinationPortRange.Min != port || *rule.TcpOptions.DestinationPortRange.Max != port {
581+
*rule.TcpOptions.DestinationPortRange.Min != desiredPort || *rule.TcpOptions.DestinationPortRange.Max != desiredPort {
536582
// this rule doesn't apply to this service so nothing to do but keep it
537583
egressRules = append(egressRules, rule)
538584
continue
@@ -545,19 +591,19 @@ func getLoadBalancerEgressRules(
545591
continue
546592
}
547593

548-
inUse, err := healthCheckPortInUse(serviceLister, int32(port))
594+
inUse, err := healthCheckPortInUse(serviceLister, int32(desiredPort))
549595
if err != nil {
550596
// Unable to determine if this port is in use by another service, so I guess
551597
// we better err on the safe side and keep the rule.
552-
logger.With(zap.Error(err), "port", port).Error("Failed to determine if port is still in use")
598+
logger.With(zap.Error(err), "port", desiredPort).Error("Failed to determine if port is still in use")
553599
egressRules = append(egressRules, rule)
554600
continue
555601
}
556602

557603
if inUse {
558604
// This rule is no longer needed for this service, but is still used
559605
// by another service, so we must still keep it.
560-
logger.With("port", port).Debug("Port still in use by another service.")
606+
logger.With("port", desiredPort).Debug("Port still in use by another service.")
561607
egressRules = append(egressRules, rule)
562608
continue
563609
}
@@ -579,7 +625,7 @@ func getLoadBalancerEgressRules(
579625
// All the remaining node cidr's are new and don't have a corresponding rule
580626
// so we need to create one for each.
581627
for _, desired := range nodeCIDRs.List() {
582-
rule := makeEgressSecurityRule(desired, port)
628+
rule := makeEgressSecurityRule(desired, desiredPort)
583629
logger.With(
584630
"destination", *rule.Destination,
585631
"destinationPortRangeMin", *rule.TcpOptions.DestinationPortRange.Min,

0 commit comments

Comments
 (0)