Skip to content

Comments

reconcile allowedaddresses in nic#83

Merged
ske-prow[bot] merged 7 commits intomainfrom
allowed-addresses
Jan 21, 2026
Merged

reconcile allowedaddresses in nic#83
ske-prow[bot] merged 7 commits intomainfrom
allowed-addresses

Conversation

@breuerfelix
Copy link
Member

@breuerfelix breuerfelix commented Jan 14, 2026

How to categorize this PR?

/kind enhancement

What this PR does / why we need it:

We need this MR in order to put the pod network CIDR into the allowedAddresses in the stackit MCM. Otherwise pod to pod communication does not work when using native kubernetes networking.

Special notes for your reviewer:

Breaking changes:

@ske-prow
Copy link

ske-prow bot commented Jan 14, 2026

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@ske-prow ske-prow bot added kind/enhancement Enhancement, improvement, extension do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 14, 2026
@breuerfelix
reconcile allowedaddresses in nic
8e55bdc 
Signed-off-by: Felix Breuer <f.breuer94@gmail.com>
@ske-prow ske-prow bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 14, 2026
@breuerfelix
wait fo server to become ready
67136bd 
Signed-off-by: Felix Breuer <f.breuer94@gmail.com>
@ske-prow ske-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 16, 2026
@breuerfelix
delete machines without providerID
2105a7e 
Signed-off-by: Felix Breuer <f.breuer94@gmail.com>
@breuerfelix
add tests to allowedAddresses
1d64de7 
Signed-off-by: Felix Breuer <f.breuer94@gmail.com>
@breuerfelix breuerfelix marked this pull request as ready for review January 19, 2026 11:17
@ske-prow ske-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 19, 2026
@breuerfelix
fix linter
186eef8 
Signed-off-by: Felix Breuer <f.breuer94@gmail.com>
@hown3d
Copy link
Member

hown3d commented Jan 19, 2026

In openstack this is done in the Route controller.

Why do you want to do this here?

@breuerfelix
Copy link
Member Author

breuerfelix commented Jan 20, 2026

In openstack this is done in the Route controller.

Why do you want to do this here?

https://github.com/gardener/machine-controller-manager-provider-openstack/blob/master/pkg/driver/executor/executor.go#L144

This is done in the Openstack MCM. It takes the podNetworkCIDR and puts it into the AllowedAddressPairs. Thats why I also wanna do it in the STACKIT MCM.

hown3d
hown3d requested changes Jan 20, 2026
View reviewed changes
Copy link
Member

@hown3d hown3d left a comment

Choose a reason for hiding this comment

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

overall good, some QOL changes.

pkg/provider/apis/provider_spec.go

// AllowedAddresses are the IP address ranges (CIDRs) allowed to originate traffic from the server's network interface.
// Optional field. If specified, these ranges are configured as AllowedAddresses on the network interface of the server to bypass anti-spoofing rules.
AllowedAddresses []string `json:"allowedAddresses,omitempty"`
Copy link
Member

@hown3d hown3d Jan 20, 2026

Choose a reason for hiding this comment

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

Should be part of NetworkingSpec

Copy link
Member Author

@breuerfelix breuerfelix Jan 20, 2026

Choose a reason for hiding this comment

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

I would not put that in NetworkingSpec since other MCM providers are also passing almost all config in the top level.
The NetworkingSpec has only 2 configuration options which are mutually exclusive. And even Networking is optional because if it is not set, the VM is put into the "default" network. So the Networking field has its own logic separate from the allowedAddresses.

pkg/provider/apis/validation/validation.go
}
}

// Validate AllowedAddresses
Copy link
Member

@hown3d hown3d Jan 20, 2026

Choose a reason for hiding this comment

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

move into validateNetworking function

Copy link
Member Author

@breuerfelix breuerfelix Jan 20, 2026

Choose a reason for hiding this comment

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

same as above

pkg/provider/core.go Outdated

func (p *Provider) getServerByName(ctx context.Context, projectID, region, serverName string) (*Server, error) {
// Check if the server got already created
labelSelector := fmt.Sprintf("mcm.gardener.cloud/machine=%s", serverName)
Copy link
Member

@hown3d hown3d Jan 20, 2026

Choose a reason for hiding this comment

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

Use constant StackitMachineLabel

Copy link
Member

@hown3d hown3d Jan 20, 2026

Choose a reason for hiding this comment

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

nit:
It was confusing that the labels are here put in as raw string with '=' concatination while in the request they were put in as map[string]string.
This is somewhat inconsistent. Labels should be used the same way across API requests.

Copy link
Member Author

@breuerfelix breuerfelix Jan 20, 2026

Choose a reason for hiding this comment

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

We have to refactor the labels anyways in another task so i would like to refactor them in another PR.

For the Labels... its the SDK ... but I changed our interface to consume a map and create the label string on the fly

pkg/provider/core.go
Copy link
Member

@hown3d hown3d Jan 20, 2026

Choose a reason for hiding this comment

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

nit: Might be worth to separate functions into other files (e.g. network.go for network related functions).
Having a single core.go that will grow is not really clear to overlook in the long run.

Copy link
Member Author

@breuerfelix breuerfelix Jan 20, 2026

Choose a reason for hiding this comment

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

solve in another story

@breuerfelix
resolve comments
00165b0 
Signed-off-by: Felix Breuer <f.breuer94@gmail.com>
@breuerfelix breuerfelix requested a review from hown3d January 20, 2026 13:12
@breuerfelix
resolve comments
70ee4c3 
Signed-off-by: Felix Breuer <f.breuer94@gmail.com>
@ske-prow ske-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jan 21, 2026
@ske-prow
Copy link

ske-prow bot commented Jan 21, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hown3d

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ske-prow
Copy link

ske-prow bot commented Jan 21, 2026

LGTM label has been added.

DetailsGit tree hash: eebd653688f9351ee136c13890f4bd9dcab6df21

@ske-prow ske-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 21, 2026
@ske-prow ske-prow bot merged commit 1d4a215 into main Jan 21, 2026
3 checks passed
@breuerfelix breuerfelix deleted the allowed-addresses branch February 18, 2026 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hown3d hown3d hown3d approved these changes

Assignees

@hown3d hown3d

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/enhancement Enhancement, improvement, extension lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@breuerfelix @hown3d