✨ New API: OpenStackCluster.Spec.ExternalNetwork #1790

EmilienM · 2023-12-15T16:49:40Z

What this PR does / why we need it:

This PR converts OpenStackCluster.Spec.ExternalNetworkID to OpenStackCluster.Spec.ExternalNetwork.

The field externalNetworkID has been renamed to externalNetwork and isn't a string anymore but a reference to a NetworkFilter object.

externalNetworkID: "e60f19e7-cb37-49f9-a2ee-0a1281f6e03e"

becomes

externalNetwork:
  id: "e60f19e7-cb37-49f9-a2ee-0a1281f6e03e"

It is now possible to specify a NetworkFilter object to select the external network to use for the cluster. The NetworkFilter object allows to select the network by name, by ID or by tags.

externalNetwork:
  name: "public"

If a network is provided, it'll be added to OpenStackCluster.Status.ExternalNetwork. If the network can't be found, an error will be returned.
If no network is provided, CAPO will try to find network marked "External" and add it to OpenStackCluster.Status.ExternalNetwork. If it can't find a network marked "External",
OpenStackCluster.Status.ExternalNetwork will be set to nil.
If more than one network is found, an error will be returned.

It is now possible for a user to specify that no external network should be used by setting DisableExternalNetwork to true:

disableExternalNetwork: true

squashed commits
API conversion
includes documentation
adds unit tests

netlify · 2023-12-15T16:49:45Z

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name	Link
🔨 Latest commit	`87cb356`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/659d94639afe8c0008a99fc6
😎 Deploy Preview	https://deploy-preview-1790--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

EmilienM · 2023-12-15T16:50:40Z

/hold cancel

EmilienM · 2023-12-15T16:54:59Z

/cc dulek stephenfin

EmilienM · 2023-12-15T16:55:54Z

/cc MaysaMacedo

k8s-ci-robot · 2023-12-15T16:55:57Z

@EmilienM: GitHub didn't allow me to request PR reviews from the following users: MaysaMacedo.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc MaysaMacedo

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

EmilienM · 2023-12-15T17:54:23Z

/test pull-cluster-api-provider-openstack-e2e-full-test

EmilienM · 2023-12-15T20:05:17Z

/test pull-cluster-api-provider-openstack-e2e-full-test

EmilienM · 2023-12-15T20:05:32Z

Ready for review, CI all green.

docs/book/src/topics/crd-changes/v1alpha7-to-v1alpha8.md

EmilienM · 2023-12-18T14:06:58Z

/uncc seanschneeweiss

docs/book/src/topics/crd-changes/v1alpha7-to-v1alpha8.md

api/v1alpha7/conversion_test.go

api/v1alpha6/conversion_test.go

EmilienM · 2023-12-18T20:48:16Z

/test pull-cluster-api-provider-openstack-e2e-full-test

EmilienM · 2024-01-08T13:41:33Z

/hold cancel

EmilienM · 2024-01-08T20:11:57Z

/test pull-cluster-api-provider-openstack-e2e-full-test

lentzi90

/lgtm
Leaving the approval to someone else. More coverage for those conversions could be nice as @mdbooth mentioned but I don't see it as critical right now

dulek

/lgtm from my side too.

mdbooth · 2024-01-09T15:04:35Z

api/v1alpha7/conversion.go

+	"externalNetwork": conversion.UnconditionalFieldRestorer(
+		func(c *infrav1.OpenStackCluster) *infrav1.NetworkFilter {
+			return &c.Spec.ExternalNetwork
+		},
+	),
+	"disableExternalNetwork": conversion.UnconditionalFieldRestorer(
+		func(c *infrav1.OpenStackCluster) *bool {
+			return &c.Spec.DisableExternalNetwork
+		},
+	),


I don't think we need these as they're already covered by spec above. In any case we would not want separate field restorers for individual fields like this. We want a single field restorer to cover a complete 'scope', e.g. the spec.

We only treat bastion separate because it's expected to be modified frequently, which makes it different to the rest of the spec. We don't want to exclude a spec from restore just because the bastion was modified, as it's reasonable that these fields might be managed differently.

I wonder if we should be excluding the bastion from the spec restorer, though 🤔 Yes, I'm pretty sure we should. I'll have to think about that.

mdbooth · 2024-01-09T16:41:49Z

I just pushed an update which fixes conversion of the new fields. The changes are:

For OpenStackCluster we use the existing spec restorer, and add restoration of the lost fields in restorev1alpha8ClusterSpec
We add a filter to the cluster spec restorer to fix the overlap with the bastion restorer.
We change OpenStackCluster template to just have a single big restorer for the spec, as we don't need to worry about frequent updates to a template

mdbooth · 2024-01-09T16:46:26Z

/approve

I'm happy for this to merge once somebody else has looked it over.

k8s-ci-robot · 2024-01-09T16:46:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dulek, EmilienM, mdbooth

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mdbooth]

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

EmilienM · 2024-01-09T16:47:19Z

/test pull-cluster-api-provider-openstack-e2e-full-test

MaysaMacedo · 2024-01-09T18:28:22Z

pkg/cloud/services/networking/network.go

+// The external network can be specified in the cluster spec or will be searched for if not specified.
+// OpenStackCluster.Status.ExternalNetwork will be set to nil if one of these conditions are met:
+// - no external network was given in the cluster spec and no external network was found
+// - the user has set OpenStackCluster.Spec.ExternalNetworkType to 'none'.


this doc looks outdated

EmilienM · 2024-01-09T18:45:59Z

/test pull-cluster-api-provider-openstack-e2e-full-test

MaysaMacedo · 2024-01-09T18:49:40Z

/lgtm

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 15, 2023

k8s-ci-robot requested review from lentzi90 and seanschneeweiss December 15, 2023 16:49

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 15, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 15, 2023

EmilienM force-pushed the external-network branch from 701c4ba to eb4e8cc Compare December 15, 2023 16:52

k8s-ci-robot requested review from dulek and stephenfin December 15, 2023 16:55

EmilienM force-pushed the external-network branch 2 times, most recently from 3ee79a5 to 50a169a Compare December 15, 2023 20:03

EmilienM commented Dec 18, 2023

View reviewed changes

docs/book/src/topics/crd-changes/v1alpha7-to-v1alpha8.md Outdated Show resolved Hide resolved

docs/book/src/topics/crd-changes/v1alpha7-to-v1alpha8.md Show resolved Hide resolved

EmilienM force-pushed the external-network branch from 50a169a to 5342f4f Compare December 18, 2023 14:06

k8s-ci-robot removed the request for review from seanschneeweiss December 18, 2023 14:07

EmilienM force-pushed the external-network branch from 5342f4f to 9a23991 Compare December 18, 2023 15:02

dulek suggested changes Dec 18, 2023

View reviewed changes

docs/book/src/topics/crd-changes/v1alpha7-to-v1alpha8.md Outdated Show resolved Hide resolved

EmilienM commented Dec 18, 2023

View reviewed changes

api/v1alpha7/conversion_test.go Outdated Show resolved Hide resolved

EmilienM force-pushed the external-network branch from 9a23991 to e9cf857 Compare December 18, 2023 18:36

MaysaMacedo reviewed Dec 18, 2023

View reviewed changes

api/v1alpha6/conversion_test.go Outdated Show resolved Hide resolved

EmilienM force-pushed the external-network branch from e9cf857 to 24667f4 Compare December 18, 2023 18:52

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 5, 2024

EmilienM force-pushed the external-network branch from 5a177ab to d4a19b5 Compare January 5, 2024 14:51

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 5, 2024

EmilienM force-pushed the external-network branch from d4a19b5 to 068c545 Compare January 5, 2024 14:54

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 8, 2024

EmilienM force-pushed the external-network branch from 068c545 to 17b73a6 Compare January 8, 2024 15:03

lentzi90 reviewed Jan 9, 2024

View reviewed changes

k8s-ci-robot assigned lentzi90 Jan 9, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 9, 2024

dulek approved these changes Jan 9, 2024

View reviewed changes

mdbooth force-pushed the external-network branch from 17b73a6 to cbcd198 Compare January 9, 2024 16:37

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 9, 2024

mdbooth reviewed Jan 9, 2024

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 9, 2024

MaysaMacedo reviewed Jan 9, 2024

View reviewed changes

New API for External Network

87cb356

EmilienM force-pushed the external-network branch from cbcd198 to 87cb356 Compare January 9, 2024 18:45

k8s-ci-robot assigned MaysaMacedo Jan 9, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 9, 2024

k8s-ci-robot merged commit 0a4899a into kubernetes-sigs:main Jan 9, 2024

EmilienM deleted the external-network branch January 9, 2024 20:57

✨ New API: OpenStackCluster.Spec.ExternalNetwork #1790

✨ New API: OpenStackCluster.Spec.ExternalNetwork #1790

Uh oh!

Conversation

EmilienM commented Dec 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Dec 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Uh oh!

EmilienM commented Dec 15, 2023

Uh oh!

EmilienM commented Dec 15, 2023

Uh oh!

EmilienM commented Dec 15, 2023

Uh oh!

k8s-ci-robot commented Dec 15, 2023

Uh oh!

EmilienM commented Dec 15, 2023

Uh oh!

EmilienM commented Dec 15, 2023

Uh oh!

EmilienM commented Dec 15, 2023

Uh oh!

Uh oh!

Uh oh!

EmilienM commented Dec 18, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EmilienM commented Dec 18, 2023

Uh oh!

EmilienM commented Jan 8, 2024

Uh oh!

EmilienM commented Jan 8, 2024

Uh oh!

lentzi90 left a comment

Choose a reason for hiding this comment

Uh oh!

dulek left a comment

Choose a reason for hiding this comment

Uh oh!

mdbooth Jan 9, 2024

Choose a reason for hiding this comment

Uh oh!

mdbooth commented Jan 9, 2024

Uh oh!

mdbooth commented Jan 9, 2024

Uh oh!

k8s-ci-robot commented Jan 9, 2024

Uh oh!

EmilienM commented Jan 9, 2024

Uh oh!

MaysaMacedo Jan 9, 2024

Choose a reason for hiding this comment

Uh oh!

EmilienM commented Jan 9, 2024

Uh oh!

MaysaMacedo commented Jan 9, 2024

Uh oh!

Uh oh!

EmilienM commented Dec 15, 2023 •

edited

Loading

netlify bot commented Dec 15, 2023 •

edited

Loading