Feat/floatingip - #392

Failed to assess the semver bump. See logs for details.

Hi @mandre, thanks for the quick feedback. I was going to address the lint issues before I ask for review but I didn't have much time to look at it. I'll go over your comments and lint validations these days.
Regarding 'Label PR / semver' job, I'm not sure how to fix it, if you could give me some hints I would appreciate it.

Hi @mandre, thanks for the quick feedback. I was going to address the lint issues before I ask for review but I didn't have much time to look at it. I'll go over your comments and lint validations these days. Regarding 'Label PR / semver' job, I'm not sure how to fix it, if you could give me some hints I would appreciate it.

It could not rebase your PR due to a conflict: https://github.com/k-orc/openstack-resource-controller/actions/runs/14848877163/job/41688586737?pr=392#step:3:11

You should rebase manually on top of main and fix the conflict.

Failed to assess the semver bump. See logs for details.

You should do a rebase (and not a merge) because go-apidiff rebases your changes and is not able to apply one of them, you need to update it so it applies cleanly on top of main.

You should do a rebase (and not a merge) because go-apidiff rebases your changes and is not able to apply one of them, you need to update it so it applies cleanly on top of main.

Thanks, looks better now.
FYI I've just pushed a change into adapter.template to support non-named Openstack resources. Before I added Name into FloatingIP ResourceSpec but as there is no name in Openstack side, it was not really a good solution. Let me know if this does not make sense for you

I hope I addressed all the comments, although I haven't got time to test it on a real cluster/openstack site yet. I will try to find the time during these days and let you know so you don't review untested code. I will mark the PR as draft for now

Hi @mandre, after some struggle with e2e I believe it's ready for review again. Sorry for the commit spam, but I had to use the workflows you already have in place for deploying devstack as I don't have a local instance of it, and in the one I used for testing I don't have admin credentials.
There is one thing I wanted to hear your opinion about: SubnetRef and FloatingIP fields within spec.resource are both optional and you could use either one of them, but I was wondering if it makes sense to validate (only during resource creation) that if a FloatingIP is specified, the subnet should also be defined (even if it's not mandatory in gophercloud). My question relates to the fact that the IP must belong to a subnet in the network so it would be weird that you know which IP is valid without knowing if a proper subnet is part of the referenced network.

What happened to the floatingip-import-error test you had at some point? From what I recall, they were good.

I believe at some moment during the e2e nightmare I decided it was not useful, but now I can't recall why. I'll create it again

I'm not sure if we should call it networkRef, as currently implemented, or floatingNetworkRef to be closer to neutron API.

About that, I don't have a clear answer, I'm fine with naming it as you prefer.

I'm not a big fan of new field's name, but I can't come up with a better name. Perhaps we could just make getResourceName() gracefully handle the case where the spec does not have a name instead?

I can make getResourceName use reflection to detect if the ResourceSpec contains a name or not, I didn't do it this way before as it would be more disruptive, and I believe getResourceName should not be even defined for nameless resources, to make sure we don't use it during reconciliation (otherwise we would need to consider getResourceName to return empty string in multiple places, something that currently can't happen as all named resources will either have Resource.Name or Metadata.Name)

func getResourceName(orcObject orcObjectPT) string {
	if orcObject.Spec.Resource.Name != nil {
		return string(*orcObject.Spec.Resource.Name)
	}
	return orcObject.Name
}

Before we merge your patch, could you also squash the commits?

done 😄

If you could also improve the commit message, trying to stick to the kubernetes guidelines as much as possible.

sorry, I didn't notice the squashed commits remained. Will do

@mandre I believe all comments were addressed (I hope I didn't miss anyone).
There is only one thing without a resolution:

I'm not a big fan of new field's name, but I can't come up with a better name. Perhaps we could just make getResourceName() gracefully handle the case where the spec does not have a name instead?

I can make getResourceName use reflection to detect if the ResourceSpec contains a name or not, I didn't do it this way before as it would be more disruptive, and I believe getResourceName should not be even defined for nameless resources, to make sure we don't use it during reconciliation (otherwise we would need to consider getResourceName to return empty string in multiple places, something that currently can't happen as all named resources will either have Resource.Name or Metadata.Name)

func getResourceName(orcObject orcObjectPT) string {
	if orcObject.Spec.Resource.Name != nil {
		return string(*orcObject.Spec.Resource.Name)
	}
	return orcObject.Name
}

Please let me know if you believe I should change something related to this template.

Related to

Also, @mdbooth raised a good point yesterday. We should make FloatingNetworkRef and FloatingSubnetRef mutually exclusive, with exactly one being set, because a subnet always has a network, and letting users set a network at the same time as the subnet is only a potential source of error.

I think the following validation enforces it:

// +kubebuilder:validation:XValidation:rule="has(self.floatingNetworkRef) != has(self.floatingSubnetRef)",message="Exactly one of 'floatingNetworkRef' or 'floatingSubnetRef' must be set"

I added one more test called floatingip-create-subnet-ref as now the create-full did not cover subnetRef (being mutually exclusive with networkRef)

@mandre I believe all comments were addressed (I hope I didn't miss anyone). There is only one thing without a resolution:

I'm not a big fan of new field's name, but I can't come up with a better name. Perhaps we could just make getResourceName() gracefully handle the case where the spec does not have a name instead?

I can make getResourceName use reflection to detect if the ResourceSpec contains a name or not, I didn't do it this way before as it would be more disruptive, and I believe getResourceName should not be even defined for nameless resources, to make sure we don't use it during reconciliation (otherwise we would need to consider getResourceName to return empty string in multiple places, something that currently can't happen as all named resources will either have Resource.Name or Metadata.Name)

func getResourceName(orcObject orcObjectPT) string {
	if orcObject.Spec.Resource.Name != nil {
		return string(*orcObject.Spec.Resource.Name)
	}
	return orcObject.Name
}

Please let me know if you believe I should change something related to this template.

Nah, it's alright. We don't expose it as an API so we can easily change it later if we don't like it.

Related to

Also, @mdbooth raised a good point yesterday. We should make FloatingNetworkRef and FloatingSubnetRef mutually exclusive, with exactly one being set, because a subnet always has a network, and letting users set a network at the same time as the subnet is only a potential source of error.
I think the following validation enforces it:
// +kubebuilder:validation:XValidation:rule="has(self.floatingNetworkRef) != has(self.floatingSubnetRef)",message="Exactly one of 'floatingNetworkRef' or 'floatingSubnetRef' must be set"

I added one more test called floatingip-create-subnet-ref as now the create-full did not cover subnetRef (being mutually exclusive with networkRef)

Good call. Let's get this merged :)