feat(manager): ✨ add region to openStackMachineSpec.ProviderID field from crd identityRef #2193

MatthieuFin · 2024-10-14T16:05:49Z

What this PR does / why we need it:

To made CAPO compatible with OCCM multi-cloud deployment we need to manage providerID format with region code as introduced here (openstack://region/instance_uuid). Use annotation from OpenstackMachine to define region in providerID field.

An empty or undefined annotation permit to be fully backward compatible. Moreover use annotation permit to manage some cluster with new OCCM provider-id string and other cluster with old format by same CAPO instance.

Fixes #2183

Special notes for your reviewer:

As discussed in issue Multi openstack cloud support with OCCM #2183 I first try to implement k8s secret call to get region name from config here, it works well but it's not backward compatible. And if I setup a feature flag wiath a cli arg I loose the possibility to manage some k8s cluster with new providerID format beside of some k8s cluster with old providerID format managed by same CAPO instance.
This feature need 🐛 Re-allow OpenStackMachine to use their own IdentityRef #2191 merged to work correctly !

k8s-ci-robot · 2024-10-14T16:05:59Z

Hi @MatthieuFin. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

netlify · 2024-10-14T16:06:04Z

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

Name	Link
🔨 Latest commit	`05c5cad`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/6710db29762ee20008bb3e2c
😎 Deploy Preview	https://deploy-preview-2193--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.

mdbooth · 2024-10-15T09:09:36Z

controllers/openstackmachine_controller.go

@@ -422,7 +422,7 @@ func (r *OpenStackMachineReconciler) reconcileMachineState(scope *scope.WithLogg
 		conditions.MarkTrue(openStackMachine, infrav1.InstanceReadyCondition)

 		// Set properties required by CAPI machine controller
-		openStackMachine.Spec.ProviderID = ptr.To(fmt.Sprintf("openstack:///%s", *openStackServer.Status.InstanceID))
+		openStackMachine.Spec.ProviderID = ptr.To(fmt.Sprintf("openstack://%s/%s", openStackMachine.ObjectMeta.Annotations["cluster.x-k8s.io/openstack.cloud"], *openStackServer.Status.InstanceID))


As discussed in slack, lets look into adding this to the API rather than as an annotation. This should be fine as long as any new field is optional, and the behaviour remains unchanged when it is not specified.

Something to think about: we permit the modification of identityRef on objects, but if we added region and it became part of the providerID, should it be immutable?

Something else to note here is that we need to preserve the immutability of providerID. This isn't a problem in the current code, as if we have an instance it's always going to have the same ID. With this change this is no longer true, as the behaviour can change across an upgrade. However we do this, I suggest that we add a check here and only set ProviderID if it is currently unset.

mdbooth · 2024-10-15T12:27:49Z

I've spent much of the morning thinking this through and discussing with @stephenfin and Sean Mooney. Based on that, I'd like to define the following contract:

The format of providerID is openstack://region/server uuid
If region is defined, it specifically means that the server with server uuid was created by and is known to the Nova API in region.
It explicitly does not make any direct assertions about any other OpenStack resources. It refers specifically to the Nova server.
region may be empty, in which case consumers of providerID which require a region must have some other way to obtain it. The lack of a region cannot be used to make assumptions about region.

It's also worth restating that providerID is immutable, so any change we make to it will not be applied to existing Machines.

In case anybody else was also fuzzy about the details, here's a restatement of my understanding of OpenStack regions: regions are effectively only a key used in the catalog of services returned by keystone during authentication. Region is not used during authentication. Keystone returns all service endpoints, and the client picks one from the desired region.

This means it makes no sense to use a region returned by keystone. Instead we should record the region we used to select and endpoint. i.e. We use the input from the API, not the output from keystone.

I propose we could add Region as an optional, immutable value in OpenStackIdentityReference, perhaps:

	// Region specifies an OpenStack region to use. If specified, it overrides
	// any value in clouds.yaml. If specified for an OpenStackMachine, its
	// value will be included in providerID.
	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="region is immutable"
	// +optional
	Region *string `json:"region,omitempty"`

I would expect us to use that value explicitly here:

cluster-api-provider-openstack/pkg/scope/provider.go

Lines 182 to 188 in 934d61d

    
           if cloud.AuthInfo != nil { 
        
           	clientOpts.AuthInfo = cloud.AuthInfo 
        
           	clientOpts.AuthType = cloud.AuthType 
        
           	clientOpts.RegionName = cloud.RegionName 
        
           	clientOpts.EndpointType = cloud.EndpointType 
        
           }

as well as when setting providerID.

MatthieuFin · 2024-10-15T13:00:44Z

Personally, I prefer don't use the new field Region from OpenStackIdentityReference in OS client creation

cluster-api-provider-openstack/pkg/scope/provider.go

Lines 182 to 188 in 934d61d

    
           if cloud.AuthInfo != nil { 
        
           	clientOpts.AuthInfo = cloud.AuthInfo 
        
           	clientOpts.AuthType = cloud.AuthType 
        
           	clientOpts.RegionName = cloud.RegionName 
        
           	clientOpts.EndpointType = cloud.EndpointType 
        
           }

If you have 2 providers which declare same region name, in this case you cannot differentiate these regions. If we keep them manage separately you could have openstack://provider1-region1/instance_uuid and openstack://provider2-region1/instance_uuid. and keep region field in client configuration to region1 in both cases.

Currently OCCM don't allow this but maybe a day it will be supported.

mdbooth · 2024-10-15T15:48:29Z

Personally, I prefer don't use the new field Region from OpenStackIdentityReference in OS client creation

cluster-api-provider-openstack/pkg/scope/provider.go

Lines 182 to 188 in 934d61d

if cloud.AuthInfo != nil {

clientOpts.AuthInfo = cloud.AuthInfo

clientOpts.AuthType = cloud.AuthType

clientOpts.RegionName = cloud.RegionName

clientOpts.EndpointType = cloud.EndpointType

}

If you have 2 providers which declare same region name, in this case you cannot differentiate these regions. If we keep them manage separately you could have openstack://provider1-region1/instance_uuid and openstack://provider2-region1/instance_uuid. and keep region field in client configuration to region1 in both cases.

Currently OCCM don't allow this but maybe a day it will be supported.

We should maintain compatibility with OCCM, as ultimately that's what's going to consume this value. However, if we're going to call the value we're putting here region it should be a region, and we should take reasonable steps to validate that it's the correct region. If we want it to be something else we should take the time to work out what that is and define it properly somewhere.

We could redefine it again in the future to be openstack://prefix-region/instance uuid, where prefix- is optional. That could be another API field. That sounds pretty esoteric, though, so I'm inclined to say YAGNI and leave it out for now.

MatthieuFin · 2024-10-15T21:01:11Z

Ok, that’s good for me, let’s see if we need to explore this topic further later! And you're right, we'll probably never need to replace the region name, at least it's too early to worry about that.

mdbooth · 2024-10-16T10:04:50Z

/ok-to-test

mdbooth

This looks great, and thanks for getting this done so quickly. I'm +1 on everything here.

As mentioned we should have apivalidations coverage of all API validations. They're very easy to write and there are lots of examples to copy from.

Ideally I'd also like some e2e test coverage of this. I think we can add this fairly easily by doing some like modifying the MachineDeployment template of the workers of one of the existing e2e scenarios and adding validation in the test suite that providerID is set on the CAPI Machine the way we expect.

Ping me on Slack if I can help with this.

mdbooth · 2024-10-16T10:40:53Z

api/v1beta1/identity_types.go

+	// Region specifies an OpenStack region to use. If specified, it overrides
+	// any value in clouds.yaml. If specified for an OpenStackMachine, its
+	// value will be included in providerID.
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="region is immutable"


Please can you add a test to https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/main/test/e2e/suites/apivalidations/openstackmachine_test.go to validate that the field is indeed immutable. I think we only need to test it for a single object, and OpenStackMachine makes sense. We should test:

The value can be set on creation

The value cannot be modified after creation

The value cannot be unset after creation

mdbooth · 2024-10-16T10:44:09Z

controllers/openstackmachine_controller.go

@@ -422,7 +422,7 @@ func (r *OpenStackMachineReconciler) reconcileMachineState(scope *scope.WithLogg
 		conditions.MarkTrue(openStackMachine, infrav1.InstanceReadyCondition)

 		// Set properties required by CAPI machine controller
-		openStackMachine.Spec.ProviderID = ptr.To(fmt.Sprintf("openstack:///%s", *openStackServer.Status.InstanceID))
+		openStackMachine.Spec.ProviderID = ptr.To(fmt.Sprintf("openstack://%s/%s", openStackMachine.Spec.IdentityRef.Region, *openStackServer.Status.InstanceID))


How can we get some test coverage of this?

Whoops! IdentityRef is a pointer and is usually nil.

Incidentally, wouldn't these errors be so much simpler to see if you'd had to write something like openStackMachine.Spec.IdentityRef->Region instead? Go automatic dereferencing is insidious.

controllers/openstackmachine_controller.go

mdbooth · 2024-10-16T15:42:47Z

test/e2e/suites/apivalidations/openstackmachine_test.go

@@ -132,6 +132,35 @@ var _ = Describe("OpenStackMachine API validations", func() {
 		Expect(k8sClient.Create(ctx, machine)).To(Succeed(), "Creating a machine with max metadata key and value should succeed")
 	})

+	It("should allow server identityRef Region field or not set on creation", func() {


These look great, thanks👌

mdbooth · 2024-10-16T15:44:13Z

api/v1beta1/identity_types.go

@@ -18,6 +18,7 @@ package v1beta1

 // OpenStackIdentityReference is a reference to an infrastructure
 // provider identity to be used to provision cluster resources.
+// +kubebuilder:validation:XValidation:rule="(!has(self.region) && !has(oldSelf.region)) || self.region == oldSelf.region",message="region is immutable"


I'm assuming this change means it didn't work correctly when applied to the field?

yes, "self == oldSelf" as you proposed, permit to set a value on an existing machine without region field defined. To do that I had to create rule on top level object because has(self) seems not valid syntax.

Thanks. Turns out I was missing some tests in the image API and this was broken: #2197

Glad to have been helpful !

mdbooth · 2024-10-17T08:41:52Z

This looks great. Can you squash the commits (into a single commit with a single, coherent description) and I'll approve. Thanks!

mdbooth · 2024-10-17T08:48:01Z

providerID looks normal on unaffected machines, e.g. https://storage.googleapis.com/kubernetes-ci-logs/pr-logs/pull/kubernetes-sigs_cluster-api-provider-openstack/2193/pull-cluster-api-provider-openstack-e2e-test/1846816338392125440/artifacts/clusters/bootstrap/resources/e2e-tk3rp9/Machine/cluster-e2e-tk3rp9-control-plane-2p6jt.yaml

providerID: openstack:///d8a589fa-98ca-4999-92da-bf8d3a2678d3

As mentioned we still need some test coverage of this actually working. I'll approve on the basis that:

it's a new feature, so it has no current users to regress
our existing test coverage shows it does not regress users who aren't using it
the API looks good
manual testing by @MatthieuFin

If we want to continue working in the future, though, we'll need a test.

MatthieuFin · 2024-10-17T08:55:36Z

I'm just currently testing on my side before squash that.

I did'nt write e2e tests but I agree we need it to be future proof.

I'm ok to try writing some tests about that, I'll probably ping you on slack for help on that purpose.

…from crd identityRef To made CAPO compatible with OCCM multi-cloud deployment we need to manage providerID format with region code as introduced [here](kubernetes/cloud-provider-openstack#1900) (`openstack://region/instance_uuid`). Add Region field to OpenStackIdentityReference type to define region name in providerID string. If specified, it overrides any value in clouds.yaml. An empty or undefined Region field permit to be fully backward compatible. Signed-off-by: MatthieuFin <[email protected]>

mdbooth · 2024-10-17T13:48:01Z

/approve

k8s-ci-robot · 2024-10-17T13:48:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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-10-17T14:10:21Z

/lgtm

k8s-ci-robot requested review from EmilienM and jichenjc October 14, 2024 16:05

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 14, 2024

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 14, 2024

MatthieuFin mentioned this pull request Oct 14, 2024

feat(manager): ✨ add region to openStackMachineSpec.ProviderID field from annotation #2187

Closed

mdbooth reviewed Oct 15, 2024

View reviewed changes

mdbooth added this to the v0.11 milestone Oct 15, 2024

MatthieuFin force-pushed the main branch from ddcc425 to 3e8f67b Compare October 15, 2024 13:43

MatthieuFin changed the title ~~feat(manager): ✨ add region to openStackMachineSpec.ProviderID field from annotation~~ feat(manager): ✨ add region to openStackMachineSpec.ProviderID field from crd identityRef Oct 15, 2024

MatthieuFin force-pushed the main branch from 3e8f67b to ae527bb Compare October 15, 2024 21:01

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 15, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 16, 2024

mdbooth reviewed Oct 16, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 16, 2024

mdbooth reviewed Oct 16, 2024

View reviewed changes

controllers/openstackmachine_controller.go Outdated Show resolved Hide resolved

mdbooth reviewed Oct 16, 2024

View reviewed changes

MatthieuFin force-pushed the main branch from b81ab3c to ec83410 Compare October 16, 2024 15:52

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 16, 2024

MatthieuFin force-pushed the main branch from 5699b42 to 05c5cad Compare October 17, 2024 09:38

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

k8s-ci-robot assigned EmilienM Oct 17, 2024

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

k8s-ci-robot merged commit 6543a62 into kubernetes-sigs:main Oct 17, 2024
9 checks passed

MatthieuFin mentioned this pull request Oct 28, 2024

Add e2e test to cover region in providerID clusterv1 machine object #2210

Open

feat(manager): ✨ add region to openStackMachineSpec.ProviderID field from crd identityRef #2193

feat(manager): ✨ add region to openStackMachineSpec.ProviderID field from crd identityRef #2193

Uh oh!

Conversation

MatthieuFin commented Oct 14, 2024

Uh oh!

k8s-ci-robot commented Oct 14, 2024

Uh oh!

netlify bot commented Oct 14, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

mdbooth commented Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MatthieuFin commented Oct 15, 2024

Uh oh!

mdbooth commented Oct 15, 2024

Uh oh!

MatthieuFin commented Oct 15, 2024

Uh oh!

mdbooth commented Oct 16, 2024

Uh oh!

mdbooth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdbooth commented Oct 17, 2024

Uh oh!

mdbooth commented Oct 17, 2024

Uh oh!

MatthieuFin commented Oct 17, 2024

Uh oh!

mdbooth commented Oct 17, 2024

Uh oh!

k8s-ci-robot commented Oct 17, 2024

Uh oh!

EmilienM commented Oct 17, 2024

Uh oh!

Uh oh!

Uh oh!

netlify bot commented Oct 14, 2024 •

edited

Loading

mdbooth commented Oct 15, 2024 •

edited

Loading