diff --git a/pkg/ccm/loadbalancer.go b/pkg/ccm/loadbalancer.go index 35df7e90..8605c88b 100644 --- a/pkg/ccm/loadbalancer.go +++ b/pkg/ccm/loadbalancer.go @@ -146,16 +146,17 @@ func (l *LoadBalancer) EnsureLoadBalancer( //nolint:gocyclo // not really comple spec.Version = lb.Version spec.Name = &name updatePayload := &loadbalancer.UpdateLoadBalancerPayload{ - ExternalAddress: spec.ExternalAddress, - Listeners: spec.Listeners, - Name: spec.Name, - Networks: spec.Networks, - Options: spec.Options, - PlanId: spec.PlanId, - Region: spec.Region, - Labels: spec.Labels, - TargetPools: spec.TargetPools, - Version: spec.Version, + ExternalAddress: spec.ExternalAddress, + Listeners: spec.Listeners, + Name: spec.Name, + Networks: spec.Networks, + Options: spec.Options, + PlanId: spec.PlanId, + Region: spec.Region, + Labels: spec.Labels, + TargetPools: spec.TargetPools, + DisableTargetSecurityGroupAssignment: lb.DisableTargetSecurityGroupAssignment, + Version: spec.Version, } lb, err = l.client.UpdateLoadBalancer(ctx, name, updatePayload) if err != nil { @@ -298,10 +299,11 @@ func (l *LoadBalancer) EnsureLoadBalancerDeleted( Observability: nil, PrivateNetworkOnly: lb.Options.PrivateNetworkOnly, }, - TargetPools: lb.TargetPools, - Version: lb.Version, - PlanId: lb.PlanId, - Labels: lb.Labels, + TargetPools: lb.TargetPools, + DisableTargetSecurityGroupAssignment: lb.DisableTargetSecurityGroupAssignment, + Version: lb.Version, + PlanId: lb.PlanId, + Labels: lb.Labels, } _, err = l.client.UpdateLoadBalancer(ctx, name, payload) if err != nil { diff --git a/pkg/ccm/loadbalancer_spec.go b/pkg/ccm/loadbalancer_spec.go index e88ff129..d18ca712 100644 --- a/pkg/ccm/loadbalancer_spec.go +++ b/pkg/ccm/loadbalancer_spec.go @@ -294,6 +294,11 @@ func lbSpecFromService( lb.Labels = new(opts.ExtraLabels) } + // For new lb's always set DisableTargetSecurityGroupAssignment to true + // This function is also used by the LB UPDATE call, but for UPDATE we just + // use the value from the already created lb instead of this one. + lb.DisableTargetSecurityGroupAssignment = new(true) + // Add metric metricsRemoteWrite settings lb.Options.Observability = observability diff --git a/pkg/ccm/loadbalancer_test.go b/pkg/ccm/loadbalancer_test.go index e9cb66ad..b741ddf5 100644 --- a/pkg/ccm/loadbalancer_test.go +++ b/pkg/ccm/loadbalancer_test.go @@ -212,7 +212,8 @@ var _ = Describe("LoadBalancer", func() { }, }, nil }) - mockClient.EXPECT().CreateLoadBalancer(gomock.Any(), gomock.Any()).MinTimes(1).Return(&loadbalancer.LoadBalancer{}, nil) + // isCreatedWithEnabledDisableTargetSecurityGroupAssignment ensures that DisableTargetSecurityGroupAssignment is set to true + mockClient.EXPECT().CreateLoadBalancer(gomock.Any(), isCreatedWithEnabledDisableTargetSecurityGroupAssignment()).MinTimes(1).Return(&loadbalancer.LoadBalancer{}, nil) _, err := lbInModeIgnoreAndObs.EnsureLoadBalancer(context.Background(), clusterName, minimalLoadBalancerService(), []*corev1.Node{}) Expect(err).To(MatchError(notYetReadyError)) @@ -220,6 +221,57 @@ var _ = Describe("LoadBalancer", func() { // Expected CreateLoadBalancer to have been called. }) + DescribeTable("LoadBalancer UPDATE behavior for DisableTargetSecurityGroupAssignment", + func(disableTargetSG bool, matcher gomock.Matcher) { + svc := minimalLoadBalancerService() + spec, _, err := lbSpecFromService(svc, []*corev1.Node{}, lbOpts, nil) + Expect(err).NotTo(HaveOccurred()) + + myLb := &loadbalancer.LoadBalancer{ + Errors: []loadbalancer.LoadBalancerError{}, + ExternalAddress: spec.ExternalAddress, + Listeners: spec.Listeners, + Name: spec.Name, + Networks: spec.Networks, + Options: spec.Options, + PrivateAddress: spec.PrivateAddress, + Status: new(loadbalancer.LOADBALANCERSTATUS_STATUS_READY), + TargetPools: spec.TargetPools, + DisableTargetSecurityGroupAssignment: &disableTargetSG, + Version: new("current-version"), + PlanId: new(p10), + } + + mockClient.EXPECT().GetLoadBalancer(gomock.Any(), gomock.Any()).Return(myLb, nil) + + mockClient.EXPECT().UpdateLoadBalancer( + gomock.Any(), + loadBalancer.GetLoadBalancerName(context.Background(), clusterName, svc), + matcher, + ).MinTimes(1).Return(myLb, nil) + + // Trigger an update by changing the service + svc = svc.DeepCopy() + svc.Spec.Ports = append(svc.Spec.Ports, corev1.ServicePort{ + Name: "a-port", + Protocol: corev1.ProtocolTCP, + Port: 80, + NodePort: 1234, + }) + + _, err = loadBalancer.EnsureLoadBalancer(context.Background(), clusterName, svc, []*corev1.Node{}) + Expect(err).NotTo(HaveOccurred()) + }, + Entry("should not set DisableTargetSecurityGroupAssignment to true when it is initially false", + false, + hasNotEnabledDisableTargetSecurityGroupAssignment(), + ), + Entry("should keep DisableTargetSecurityGroupAssignment as true when it is initially true", + true, + hasEnabledDisableTargetSecurityGroupAssignment(), + ), + ) + It("should update observability credential if credentials are specified in load balancer", func() { svc := minimalLoadBalancerService() spec, _, err := lbSpecFromService(svc, []*corev1.Node{}, lbOpts, &loadbalancer.LoadbalancerOptionObservability{ @@ -715,6 +767,33 @@ func hasNoObservabilityConfigured() gomock.Matcher { }) } +// hasNotEnabledDisableTargetSecurityGroupAssignment ensures that the given UpdateLoadBalancerPayload +// has not set DisableTargetSecurityGroupAssignment to true. +func hasNotEnabledDisableTargetSecurityGroupAssignment() gomock.Matcher { + return gomock.Cond(func(x any) bool { + lb := x.(*loadbalancer.UpdateLoadBalancerPayload) + return lb.DisableTargetSecurityGroupAssignment != nil && *lb.DisableTargetSecurityGroupAssignment != true + }) +} + +// hasEnabledDisableTargetSecurityGroupAssignment ensures that the given UpdateLoadBalancerPayload +// has set DisableTargetSecurityGroupAssignment to true. +func hasEnabledDisableTargetSecurityGroupAssignment() gomock.Matcher { + return gomock.Cond(func(x any) bool { + lb := x.(*loadbalancer.UpdateLoadBalancerPayload) + return lb.DisableTargetSecurityGroupAssignment != nil && *lb.DisableTargetSecurityGroupAssignment == true + }) +} + +// isCreatedWithEnabledDisableTargetSecurityGroupAssignment ensures that new lbs are always created +// with DisableTargetSecurityGroupAssignment set to true. +func isCreatedWithEnabledDisableTargetSecurityGroupAssignment() gomock.Matcher { + return gomock.Cond(func(x any) bool { + lb := x.(*loadbalancer.CreateLoadBalancerPayload) + return lb.DisableTargetSecurityGroupAssignment != nil && *lb.DisableTargetSecurityGroupAssignment == true + }) +} + // externalAddressSet ensures that the given UpdateLoadBalancerPayload has external address set to address. func externalAddressSet(address string) gomock.Matcher { return gomock.Cond(func(x any) bool {