Incorrect doc for PortResourceSpec or incorrect implementation of the data structure?

[evrardjp-cagip]

Problem description

Hello,

According to portResourceSpec, viewable from https://github.com/k-orc/openstack-resource-controller/blob/main/website/docs/crd-reference.md#portresourcespec , generated from https://github.com/k-orc/openstack-resource-controller/blob/f5d3d53c5908bf6462edff8ae99f3d6348b5801a/api/v1alpha1/port_types.go#L133

Here the PortSecurityGroupRefs is a list of OpenStackName(s).

My understanding is (with the docs) that OpenStackName is a string containing the name of the resource in OpenStack. But the code is indeed far different: Port SecurityGroup Ref seem to expect a reference to a kubernetes CR (a securityGroup one), similar to KubernetesNameRef (Check the line above).

Question: Could you clarify what is intended? Shouldn't we have a pointer to KubernetesNameRef instead? What's the reason it's not the case? (I didn't dig, sorry).

ORC version

v2.1.0

Additional information

I am on an old v1.29.7 kube. OpenStack is running on an ancient version.

If I don't import the kubernetes CR for SecurityGroup (only giving the name of the security group instead), you would see the following in the log:

Relevant log output

root@management-cluster-control-plane-lcg9m:~# kubectl get ports -n dev007  --kubeconfig /etc/kubernetes/admin.conf 
NAME           ID    AVAILABLE   ADDRESSES   MESSAGE
dev007-vip-2         False                   Waiting for SecurityGroup/k8s-api to be created

[mandre]

Good catch, this is a mistake. It should be:

SecurityGroupRefs []KubernetesNameRef `json:"securityGroupRefs,omitempty"`

In practice, both OpenStackName and KubernetesNameRef's underlying type is string, however they differ by their validations.

https://github.com/k-orc/openstack-resource-controller/blob/f5d3d53c5908bf6462edff8ae99f3d6348b5801a/api/v1alpha1/openstack_types.go#L23-L26

vs

https://github.com/k-orc/openstack-resource-controller/blob/f5d3d53c5908bf6462edff8ae99f3d6348b5801a/api/v1alpha1/common_types.go#L97-L99

[mandre]

Unfortunately, merging this fix would require cutting a new major release as it touches the published API. We may want to group it with other non-backward compatible changes, FYI.

[evrardjp-cagip]

Yeah I assumed so.

Thanks a lot for the fix! FYI: I didn't assume you would have to fix it, I could have done it. I just needed a heads up first into the project.

And .... I am fine waiting. :)

[mandre]

Thanks a lot for the fix! FYI: I didn't assume you would have to fix it, I could have done it. I just needed a heads up first into the project.

For next time 😉