Skip to content

Set DisableTargetSecurityGroupAssignment to "true" for new loadbalancers #1239

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nschad wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Set DisableTargetSecurityGroupAssignment to "true" for new loadbalancers #1239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3555b28
Set DisableTargetSecurityGroupAssignment to "true" for new loadbalancers
nschad Jun 18, 2026
4aeca96
do not unnecessarily set DisableTargetSecurityGroupAssignment in Upda…
nschad Jun 18, 2026
2b43267
add unit test
nschad Jun 18, 2026
6934f8e
set DisableTargetSecurityGroupAssignment for UPDATE calls
nschad Jun 19, 2026
e57e4e0
add unit test for LB creation with DisableTargetSecurityGroupAssignme…
nschad Jun 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 16 additions & 14 deletions pkg/ccm/loadbalancer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,16 +148,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, l.projectID, name, updatePayload)
if err != nil {
Expand Down Expand Up @@ -300,10 +301,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, l.projectID, name, payload)
if err != nil {
Expand Down
3 changes: 3 additions & 0 deletions pkg/ccm/loadbalancer_spec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,9 @@ func lbSpecFromService(
lb.Labels = new(opts.ExtraLabels)
}

// For new lb's always set DisableTargetSecurityGroupAssignment to true
lb.DisableTargetSecurityGroupAssignment = new(true)

@hown3d hown3d Jun 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is not only called for new loadbalancers but also for updating existing ones. If you mention that this is immutable, we should not set it to true by default

@nschad nschad Jun 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm aware that is called by the Update() but the result in that case is not used. This is already tested

Setting it to true by default is literally the Task, lol.

@nschad nschad Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be eventually removed anyway again

@breuerfelix breuerfelix Jun 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it will not be removed in the future, it will just be made customizeable :)

@breuerfelix breuerfelix Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after having a chat with the LB guys, we should definitely also set this value in the update + delete call.
They said that their api (sometimes) needs those fields, even though they are immutable.
So in the update call, just use the value that got returned from the get.
Also in the update call when deleting -> use the value that we got from the returned LB.
It is also worth adding a test for that.
We should test that when creating -> always true.
When updating: true or false, depending on the value of the GET LB


// Add metric metricsRemoteWrite settings
lb.Options.Observability = observability

Expand Down
79 changes: 79 additions & 0 deletions pkg/ccm/loadbalancer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,67 @@ var _ = Describe("LoadBalancer", func() {
// Expected CreateLoadBalancer to have been called.
})

It("should create a load balancer with DisableTargetSecurityGroupAssignment set to true", func() {
mockClient.EXPECT().GetLoadBalancer(gomock.Any(), projectID, gomock.Any()).Return(nil, stackit.ErrorNotFound)
mockClient.EXPECT().ListCredentials(gomock.Any(), projectID).Return(&loadbalancer.ListCredentialsResponse{
Credentials: []loadbalancer.CredentialsResponse{},
}, nil)
mockClient.EXPECT().CreateCredentials(gomock.Any(), projectID, gomock.Any()).MinTimes(1).
DoAndReturn(func(_ context.Context, _ string, payload loadbalancer.CreateCredentialsPayload) (*loadbalancer.CreateCredentialsResponse, error) {
return &loadbalancer.CreateCredentialsResponse{
Credential: &loadbalancer.CredentialsResponse{
CredentialsRef: new("my-credential-ref"),
DisplayName: new(*payload.DisplayName),
Username: new(*payload.Username),
},
}, nil
})
mockClient.EXPECT().CreateLoadBalancer(gomock.Any(), projectID, hasEnabledDisableTargetSecurityGroupAssignment()).MinTimes(1).Return(&loadbalancer.LoadBalancer{}, nil)
_, err := lbInModeIgnoreAndObs.EnsureLoadBalancer(context.Background(), clusterName, minimalLoadBalancerService(), []*corev1.Node{})
Expect(err).To(MatchError(notYetReadyError))
})

It("should not set DisableTargetSecurityGroupAssignment to true when lb is updated", func() {
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: new(false),
Version: new("current-version"),
PlanId: new(p10),
}

mockClient.EXPECT().GetLoadBalancer(gomock.Any(), projectID, gomock.Any()).Return(myLb, nil)
// For simplicity, we return the original load balancer. In reality, the updated load balancer should be returned.
mockClient.EXPECT().UpdateLoadBalancer(
gomock.Any(),
projectID,
loadBalancer.GetLoadBalancerName(context.Background(), clusterName, svc),
hasNotEnabledDisableTargetSecurityGroupAssignment(),
).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())
})

It("should update observability credential if credentials are specified in load balancer", func() {
svc := minimalLoadBalancerService()
spec, _, err := lbSpecFromService(svc, []*corev1.Node{}, lbOpts, &loadbalancer.LoadbalancerOptionObservability{
Expand Down Expand Up @@ -705,6 +766,24 @@ 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 new lbs are always created
// with DisableTargetSecurityGroupAssignment set to true.
func hasEnabledDisableTargetSecurityGroupAssignment() 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 {
Expand Down