Skip to content

⚠️ OpenStackCluster api general cleanup #1930

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Mar 11, 2024
Merged

⚠️ OpenStackCluster api general cleanup #1930

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
0caecf4
Add optional.Int, optional.Bool, and optional.Restore*
mdbooth Mar 6, 2024
cc32a8a
PortOpts: Use RestoreString optional.String fields
mdbooth Mar 6, 2024
7a0e942
make nil resource filters ToListOpts() return empty ListOpts
mdbooth Mar 7, 2024
cdc16d2
Make NetworkFilter.IsEmpty() return true on nil receiver
mdbooth Mar 7, 2024
2d96db6
OpenStackCluster: Add explicit optional tags
mdbooth Mar 6, 2024
75079c7
OpenStackCluster: Set explicit listType=atomic
mdbooth Mar 6, 2024
ffbfd40
OpenStackClusterSpec: Network->pointer
mdbooth Feb 19, 2024
e39c730
OpenStackClusterSpec: ExternalNetwork->pointer
mdbooth Feb 19, 2024
e5d9630
OpenStackClusterSpec: ApiServerLoadBalancer->pointer
mdbooth Feb 19, 2024
2c7b44b
OpenStackClusterSpec: APIServerFloatingIP, APIServerFixedIP, APIServe…
mdbooth Feb 21, 2024
9223883
OpenStackClusterSpec: scalar fields to optional types
mdbooth Mar 6, 2024
70bfdaf
OpenStackClusterSpec: ControlPlaneEndpoint->pointer
mdbooth Mar 6, 2024
e6a194b
OpenStackCluster: improve doctext
mdbooth Mar 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions api/v1alpha5/conversion_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func TestConvertFrom(t *testing.T) {
},
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{
"cluster.x-k8s.io/conversion-data": "{\"spec\":{\"controlPlaneEndpoint\":{\"host\":\"\",\"port\":0},\"identityRef\":{\"cloudName\":\"\",\"name\":\"\"}},\"status\":{\"ready\":false}}",
"cluster.x-k8s.io/conversion-data": "{\"spec\":{\"identityRef\":{\"cloudName\":\"\",\"name\":\"\"}},\"status\":{\"ready\":false}}",
},
},
},
Expand All @@ -72,7 +72,7 @@ func TestConvertFrom(t *testing.T) {
},
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{
"cluster.x-k8s.io/conversion-data": "{\"spec\":{\"template\":{\"spec\":{\"controlPlaneEndpoint\":{\"host\":\"\",\"port\":0},\"identityRef\":{\"cloudName\":\"\",\"name\":\"\"}}}}}",
"cluster.x-k8s.io/conversion-data": "{\"spec\":{\"template\":{\"spec\":{\"identityRef\":{\"cloudName\":\"\",\"name\":\"\"}}}}}",
},
},
},
Expand Down
4 changes: 2 additions & 2 deletions api/v1alpha5/zz_generated.conversion.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 13 additions & 0 deletions api/v1alpha6/openstackcluster_conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"reflect"

apiconversion "k8s.io/apimachinery/pkg/conversion"
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
ctrlconversion "sigs.k8s.io/controller-runtime/pkg/conversion"

infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1"
Expand Down Expand Up @@ -201,6 +202,10 @@ func restorev1beta1ClusterSpec(previous *infrav1.OpenStackClusterSpec, dst *infr
dst.APIServerLoadBalancer = previous.APIServerLoadBalancer
}

if dst.ControlPlaneEndpoint == nil || *dst.ControlPlaneEndpoint == (clusterv1.APIEndpoint{}) {
dst.ControlPlaneEndpoint = previous.ControlPlaneEndpoint
}

optional.RestoreString(&previous.APIServerFloatingIP, &dst.APIServerFloatingIP)
optional.RestoreString(&previous.APIServerFixedIP, &dst.APIServerFixedIP)
optional.RestoreInt(&previous.APIServerPort, &dst.APIServerPort)
Expand Down Expand Up @@ -256,6 +261,10 @@ func Convert_v1alpha6_OpenStackClusterSpec_To_v1beta1_OpenStackClusterSpec(in *O
}
}

if in.ControlPlaneEndpoint != (clusterv1.APIEndpoint{}) {
out.ControlPlaneEndpoint = &in.ControlPlaneEndpoint
}

out.IdentityRef.CloudName = in.CloudName
if in.IdentityRef != nil {
out.IdentityRef.Name = in.IdentityRef.Name
Expand Down Expand Up @@ -304,6 +313,10 @@ func Convert_v1beta1_OpenStackClusterSpec_To_v1alpha6_OpenStackClusterSpec(in *i
out.AllowAllInClusterTraffic = in.ManagedSecurityGroups.AllowAllInClusterTraffic
}

if in.ControlPlaneEndpoint != nil {
out.ControlPlaneEndpoint = *in.ControlPlaneEndpoint
}

out.CloudName = in.IdentityRef.CloudName
out.IdentityRef = &OpenStackIdentityReference{Name: in.IdentityRef.Name}

Expand Down
4 changes: 2 additions & 2 deletions api/v1alpha6/zz_generated.conversion.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 13 additions & 0 deletions api/v1alpha7/openstackcluster_conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package v1alpha7

import (
apiconversion "k8s.io/apimachinery/pkg/conversion"
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
ctrlconversion "sigs.k8s.io/controller-runtime/pkg/conversion"

infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1"
Expand Down Expand Up @@ -203,6 +204,10 @@ func restorev1beta1ClusterSpec(previous *infrav1.OpenStackClusterSpec, dst *infr
dst.APIServerLoadBalancer = previous.APIServerLoadBalancer
}

if dst.ControlPlaneEndpoint == nil || *dst.ControlPlaneEndpoint == (clusterv1.APIEndpoint{}) {
dst.ControlPlaneEndpoint = previous.ControlPlaneEndpoint
}

optional.RestoreString(&previous.APIServerFloatingIP, &dst.APIServerFloatingIP)
optional.RestoreString(&previous.APIServerFixedIP, &dst.APIServerFixedIP)
optional.RestoreInt(&previous.APIServerPort, &dst.APIServerPort)
Expand Down Expand Up @@ -258,6 +263,10 @@ func Convert_v1alpha7_OpenStackClusterSpec_To_v1beta1_OpenStackClusterSpec(in *O
}
}

if in.ControlPlaneEndpoint != (clusterv1.APIEndpoint{}) {
out.ControlPlaneEndpoint = &in.ControlPlaneEndpoint
}

out.IdentityRef.CloudName = in.CloudName
if in.IdentityRef != nil {
out.IdentityRef.Name = in.IdentityRef.Name
Expand Down Expand Up @@ -306,6 +315,10 @@ func Convert_v1beta1_OpenStackClusterSpec_To_v1alpha7_OpenStackClusterSpec(in *i
out.AllowAllInClusterTraffic = in.ManagedSecurityGroups.AllowAllInClusterTraffic
}

if in.ControlPlaneEndpoint != nil {
out.ControlPlaneEndpoint = *in.ControlPlaneEndpoint
}

out.CloudName = in.IdentityRef.CloudName
out.IdentityRef = &OpenStackIdentityReference{Name: in.IdentityRef.Name}

Expand Down
4 changes: 2 additions & 2 deletions api/v1alpha7/zz_generated.conversion.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 6 additions & 1 deletion api/v1beta1/openstackcluster_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,8 +158,13 @@ type OpenStackClusterSpec struct {
Tags []string `json:"tags,omitempty"`

// ControlPlaneEndpoint represents the endpoint used to communicate with the control plane.
// It is normally populated automatically by the OpenStackCluster
// controller during cluster provisioning. If it is set on creation the
// control plane endpoint will use the values set here in preference to
// values set elsewhere.
// ControlPlaneEndpoint cannot be modified after ControlPlaneEndpoint.Host has been set.
// +optional
ControlPlaneEndpoint clusterv1.APIEndpoint `json:"controlPlaneEndpoint"`
ControlPlaneEndpoint *clusterv1.APIEndpoint `json:"controlPlaneEndpoint,omitempty"`

// ControlPlaneAvailabilityZones is the az to deploy control plane to
// +listType=set
Expand Down
6 changes: 5 additions & 1 deletion api/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 7 additions & 2 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5536,8 +5536,13 @@ spec:
type: array
x-kubernetes-list-type: set
controlPlaneEndpoint:
description: ControlPlaneEndpoint represents the endpoint used to
communicate with the control plane.
description: |-
ControlPlaneEndpoint represents the endpoint used to communicate with the control plane.
It is normally populated automatically by the OpenStackCluster
controller during cluster provisioning. If it is set on creation the
control plane endpoint will use the values set here in preference to
values set elsewhere.
ControlPlaneEndpoint cannot be modified after ControlPlaneEndpoint.Host has been set.
properties:
host:
description: The hostname on which the API server is serving.
Expand Down
9 changes: 7 additions & 2 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclustertemplates.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2968,8 +2968,13 @@ spec:
type: array
x-kubernetes-list-type: set
controlPlaneEndpoint:
description: ControlPlaneEndpoint represents the endpoint
used to communicate with the control plane.
description: |-
ControlPlaneEndpoint represents the endpoint used to communicate with the control plane.
It is normally populated automatically by the OpenStackCluster
controller during cluster provisioning. If it is set on creation the
control plane endpoint will use the values set here in preference to
values set elsewhere.
ControlPlaneEndpoint cannot be modified after ControlPlaneEndpoint.Host has been set.
properties:
host:
description: The hostname on which the API server is serving.
Expand Down
6 changes: 3 additions & 3 deletions controllers/openstackcluster_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -747,7 +747,7 @@ func reconcileControlPlaneEndpoint(scope *scope.WithLogger, networkingService *n
// Control plane endpoint is already set
// Note that checking this here means that we don't re-execute any of
// the branches below if the control plane endpoint is already set.
case openStackCluster.Spec.ControlPlaneEndpoint.IsValid():
case openStackCluster.Spec.ControlPlaneEndpoint != nil && openStackCluster.Spec.ControlPlaneEndpoint.IsValid():
host = openStackCluster.Spec.ControlPlaneEndpoint.Host

// API server load balancer is disabled, but floating IP is not. Create
Expand All @@ -774,7 +774,7 @@ func reconcileControlPlaneEndpoint(scope *scope.WithLogger, networkingService *n
return err
}

openStackCluster.Spec.ControlPlaneEndpoint = clusterv1.APIEndpoint{
openStackCluster.Spec.ControlPlaneEndpoint = &clusterv1.APIEndpoint{
Host: host,
Port: int32(apiServerPort),
}
Expand All @@ -785,7 +785,7 @@ func reconcileControlPlaneEndpoint(scope *scope.WithLogger, networkingService *n
// getAPIServerPort returns the port to use for the API server based on the cluster spec.
func getAPIServerPort(openStackCluster *infrav1.OpenStackCluster) int {
switch {
case openStackCluster.Spec.ControlPlaneEndpoint.IsValid():
case openStackCluster.Spec.ControlPlaneEndpoint != nil && openStackCluster.Spec.ControlPlaneEndpoint.IsValid():
return int(openStackCluster.Spec.ControlPlaneEndpoint.Port)
case openStackCluster.Spec.APIServerPort != nil:
return *openStackCluster.Spec.APIServerPort
Expand Down
2 changes: 1 addition & 1 deletion controllers/openstackcluster_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -765,7 +765,7 @@ func Test_getAPIServerPort(t *testing.T) {
name: "with a control plane endpoint",
openStackCluster: &infrav1.OpenStackCluster{
Spec: infrav1.OpenStackClusterSpec{
ControlPlaneEndpoint: clusterv1.APIEndpoint{
ControlPlaneEndpoint: &clusterv1.APIEndpoint{
Host: "192.168.0.1",
Port: 6444,
},
Expand Down
2 changes: 1 addition & 1 deletion controllers/openstackmachine_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,7 +461,7 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope
} else if !pointer.BoolDeref(openStackCluster.Spec.DisableAPIServerFloatingIP, false) {
var floatingIPAddress *string
switch {
case openStackCluster.Spec.ControlPlaneEndpoint.IsValid():
case openStackCluster.Spec.ControlPlaneEndpoint != nil && openStackCluster.Spec.ControlPlaneEndpoint.IsValid():
floatingIPAddress = &openStackCluster.Spec.ControlPlaneEndpoint.Host
case openStackCluster.Spec.APIServerFloatingIP != nil:
floatingIPAddress = openStackCluster.Spec.APIServerFloatingIP
Expand Down
21 changes: 18 additions & 3 deletions docs/book/src/api/v1beta1/api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,12 @@ sigs.k8s.io/cluster-api/api/v1beta1.APIEndpoint
</td>
<td>
<em>(Optional)</em>
<p>ControlPlaneEndpoint represents the endpoint used to communicate with the control plane.</p>
<p>ControlPlaneEndpoint represents the endpoint used to communicate with the control plane.
It is normally populated automatically by the OpenStackCluster
controller during cluster provisioning. If it is set on creation the
control plane endpoint will use the values set here in preference to
values set elsewhere.
ControlPlaneEndpoint cannot be modified after ControlPlaneEndpoint.Host has been set.</p>
</td>
</tr>
<tr>
Expand Down Expand Up @@ -2232,7 +2237,12 @@ sigs.k8s.io/cluster-api/api/v1beta1.APIEndpoint
</td>
<td>
<em>(Optional)</em>
<p>ControlPlaneEndpoint represents the endpoint used to communicate with the control plane.</p>
<p>ControlPlaneEndpoint represents the endpoint used to communicate with the control plane.
It is normally populated automatically by the OpenStackCluster
controller during cluster provisioning. If it is set on creation the
control plane endpoint will use the values set here in preference to
values set elsewhere.
ControlPlaneEndpoint cannot be modified after ControlPlaneEndpoint.Host has been set.</p>
</td>
</tr>
<tr>
Expand Down Expand Up @@ -2805,7 +2815,12 @@ sigs.k8s.io/cluster-api/api/v1beta1.APIEndpoint
</td>
<td>
<em>(Optional)</em>
<p>ControlPlaneEndpoint represents the endpoint used to communicate with the control plane.</p>
<p>ControlPlaneEndpoint represents the endpoint used to communicate with the control plane.
It is normally populated automatically by the OpenStackCluster
controller during cluster provisioning. If it is set on creation the
control plane endpoint will use the values set here in preference to
values set elsewhere.
ControlPlaneEndpoint cannot be modified after ControlPlaneEndpoint.Host has been set.</p>
</td>
</tr>
<tr>
Expand Down
17 changes: 13 additions & 4 deletions pkg/cloud/services/loadbalancer/loadbalancer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ func getAPIServerVIPAddress(openStackCluster *infrav1.OpenStackCluster) (*string
return openStackCluster.Spec.APIServerFixedIP, nil

// If we are using the VIP as the control plane endpoint, use any value explicitly set on the control plane endpoint
case pointer.BoolDeref(openStackCluster.Spec.DisableAPIServerFloatingIP, false) && openStackCluster.Spec.ControlPlaneEndpoint.IsValid():
case pointer.BoolDeref(openStackCluster.Spec.DisableAPIServerFloatingIP, false) && openStackCluster.Spec.ControlPlaneEndpoint != nil && openStackCluster.Spec.ControlPlaneEndpoint.IsValid():
fixedIPAddress, err := lookupHost(openStackCluster.Spec.ControlPlaneEndpoint.Host)
if err != nil {
return nil, fmt.Errorf("lookup host: %w", err)
Expand All @@ -181,7 +181,7 @@ func getAPIServerFloatingIP(openStackCluster *infrav1.OpenStackCluster) (*string
return openStackCluster.Spec.APIServerFloatingIP, nil

// An IP address is specified explicitly in the control plane endpoint
case openStackCluster.Spec.ControlPlaneEndpoint.IsValid():
case openStackCluster.Spec.ControlPlaneEndpoint != nil && openStackCluster.Spec.ControlPlaneEndpoint.IsValid():
floatingIPAddress, err := lookupHost(openStackCluster.Spec.ControlPlaneEndpoint.Host)
if err != nil {
return nil, fmt.Errorf("lookup host: %w", err)
Expand Down Expand Up @@ -518,12 +518,18 @@ func (s *Service) ReconcileLoadBalancerMember(openStackCluster *infrav1.OpenStac
if openStackCluster.Status.APIServerLoadBalancer == nil {
return errors.New("network.APIServerLoadBalancer is not yet available in openStackCluster.Status")
}
if openStackCluster.Spec.ControlPlaneEndpoint == nil || !openStackCluster.Spec.ControlPlaneEndpoint.IsValid() {
return errors.New("ControlPlaneEndpoint is not yet set in openStackCluster.Spec")
}

loadBalancerName := getLoadBalancerName(clusterName)
s.scope.Logger().Info("Reconciling load balancer member", "loadBalancerName", loadBalancerName)

lbID := openStackCluster.Status.APIServerLoadBalancer.ID
portList := []int{int(openStackCluster.Spec.ControlPlaneEndpoint.Port)}
var portList []int
if openStackCluster.Spec.ControlPlaneEndpoint != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need the if since we already checked for it at L521?
I guess it's ok. Just in case it was a leftover.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but I've done similar in a few places. In my first pass at this I didn't double-check any pointers, but after hitting a few panics in various iterations I've decided to err on the side of pretty much always checking. Apart from anything else, this is safer against future cut and paste, where somebody might move either use such that one or the other is no longer guarded.

I'd prefer to fix this through the type system, but I'm not having much joy with that today: https://hachyderm.io/@mattb/112077588227331296

portList = append(portList, int(openStackCluster.Spec.ControlPlaneEndpoint.Port))
}
if openStackCluster.Spec.APIServerLoadBalancer != nil {
portList = append(portList, openStackCluster.Spec.APIServerLoadBalancer.AdditionalPorts...)
}
Expand Down Expand Up @@ -656,7 +662,10 @@ func (s *Service) DeleteLoadBalancerMember(openStackCluster *infrav1.OpenStackCl

lbID := lb.ID

portList := []int{int(openStackCluster.Spec.ControlPlaneEndpoint.Port)}
var portList []int
if openStackCluster.Spec.ControlPlaneEndpoint != nil {
portList = append(portList, int(openStackCluster.Spec.ControlPlaneEndpoint.Port))
}
if openStackCluster.Spec.APIServerLoadBalancer != nil {
portList = append(portList, openStackCluster.Spec.APIServerLoadBalancer.AdditionalPorts...)
}
Expand Down
Loading