Skip to content

⚠️ move loadbalancer opts to struct #1187

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 1 commit into from
Apr 1, 2022
Merged

⚠️ move loadbalancer opts to struct #1187

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 10 additions & 0 deletions api/v1alpha3/conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ limitations under the License.
package v1alpha3

import (
unsafe "unsafe"

corev1 "k8s.io/api/core/v1"
conversion "k8s.io/apimachinery/pkg/conversion"
ctrlconversion "sigs.k8s.io/controller-runtime/pkg/conversion"
Expand Down Expand Up @@ -117,6 +119,10 @@ func Convert_v1alpha3_OpenStackClusterSpec_To_v1beta1_OpenStackClusterSpec(in *O
Name: in.CloudsSecret.Name,
}
}
out.APIServerLoadBalancer = infrav1.APIServerLoadBalancer{
Enabled: in.ManagedAPIServerLoadBalancer,
AdditionalPorts: *(*[]int)(unsafe.Pointer(&in.APIServerLoadBalancerAdditionalPorts)),
Copy link
Member

@chrischdi chrischdi Apr 1, 2022

Choose a reason for hiding this comment

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

What's the reason for using unsafe here? couldn't it be as easy as the following?:

Suggested change
AdditionalPorts: *(*[]int)(unsafe.Pointer(&in.APIServerLoadBalancerAdditionalPorts)),
AdditionalPorts: in.APIServerLoadBalancerAdditionalPorts,

}
return autoConvert_v1alpha3_OpenStackClusterSpec_To_v1beta1_OpenStackClusterSpec(in, out, s)
}

Expand All @@ -139,6 +145,10 @@ func Convert_v1beta1_OpenStackClusterSpec_To_v1alpha3_OpenStackClusterSpec(in *i
Name: in.Bastion.Instance.IdentityRef.Name,
}
}

out.ManagedAPIServerLoadBalancer = in.APIServerLoadBalancer.Enabled
out.APIServerLoadBalancerAdditionalPorts = *(*[]int)(unsafe.Pointer(&in.APIServerLoadBalancer.AdditionalPorts))
Copy link
Member

@chrischdi chrischdi Apr 1, 2022

Choose a reason for hiding this comment

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

What's the reason for using unsafe here? couldn't it be as easy as the following?:

Suggested change
out.APIServerLoadBalancerAdditionalPorts = *(*[]int)(unsafe.Pointer(&in.APIServerLoadBalancer.AdditionalPorts))
out.APIServerLoadBalancerAdditionalPorts = in.APIServerLoadBalancer.AdditionalPorts

Copy link
Member Author

@apricote apricote Apr 1, 2022

Choose a reason for hiding this comment

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

I was too slow with answering, not its merged :(

I copied this from the previous autogenerated conversion.

The code is coming from conversion-gen: https://github.com/kubernetes/code-generator/blob/7e38d5771c5636f13555f0058e5936c521d2fe13/cmd/conversion-gen/generators/conversion.go#L929

Copy link
Member

@chrischdi chrischdi Apr 1, 2022

Choose a reason for hiding this comment

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

So this whole part is auto generated? If so we have to keep it, otherwise I would prefer to clean it up in a follow-up PR :-)

Copy link
Member Author

@apricote apricote Apr 5, 2022

Choose a reason for hiding this comment

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

Nope, my changes are not auto-generated, but I did not understand the semantics of the conversion code enough to remove the unsafe.Pointer logic provided by conversion-gen.


return autoConvert_v1beta1_OpenStackClusterSpec_To_v1alpha3_OpenStackClusterSpec(in, out, s)
}

Expand Down
83 changes: 83 additions & 0 deletions api/v1alpha3/conversion_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,93 @@ import (
runtime "k8s.io/apimachinery/pkg/runtime"
runtimeserializer "k8s.io/apimachinery/pkg/runtime/serializer"
utilconversion "sigs.k8s.io/cluster-api/util/conversion"
ctrlconversion "sigs.k8s.io/controller-runtime/pkg/conversion"

infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1"
)

func TestConvertTo(t *testing.T) {
g := gomega.NewWithT(t)
scheme := runtime.NewScheme()
g.Expect(AddToScheme(scheme)).To(gomega.Succeed())
g.Expect(infrav1.AddToScheme(scheme)).To(gomega.Succeed())

tests := []struct {
name string
spoke ctrlconversion.Convertible
hub ctrlconversion.Hub
want ctrlconversion.Hub
}{
{
name: "APIServer LoadBalancer Configuration",
spoke: &OpenStackCluster{
Spec: OpenStackClusterSpec{
ManagedAPIServerLoadBalancer: true,
APIServerLoadBalancerAdditionalPorts: []int{80, 443},
},
},
hub: &infrav1.OpenStackCluster{},
want: &infrav1.OpenStackCluster{
Spec: infrav1.OpenStackClusterSpec{
APIServerLoadBalancer: infrav1.APIServerLoadBalancer{
Enabled: true,
AdditionalPorts: []int{80, 443},
},
},
},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := tt.spoke.ConvertTo(tt.hub)
g.Expect(err).NotTo(gomega.HaveOccurred())
g.Expect(tt.hub).To(gomega.Equal(tt.want))
})
}
}

func TestConvertFrom(t *testing.T) {
g := gomega.NewWithT(t)
scheme := runtime.NewScheme()
g.Expect(AddToScheme(scheme)).To(gomega.Succeed())
g.Expect(infrav1.AddToScheme(scheme)).To(gomega.Succeed())

tests := []struct {
name string
spoke ctrlconversion.Convertible
hub ctrlconversion.Hub
want ctrlconversion.Convertible
}{
{
name: "APIServer LoadBalancer Configuration",
spoke: &OpenStackCluster{},
hub: &infrav1.OpenStackCluster{
Spec: infrav1.OpenStackClusterSpec{
APIServerLoadBalancer: infrav1.APIServerLoadBalancer{
Enabled: true,
AdditionalPorts: []int{80, 443},
},
},
},
want: &OpenStackCluster{
Spec: OpenStackClusterSpec{
ManagedAPIServerLoadBalancer: true,
APIServerLoadBalancerAdditionalPorts: []int{80, 443},
},
},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := tt.spoke.ConvertFrom(tt.hub)
g.Expect(err).NotTo(gomega.HaveOccurred())
g.Expect(tt.spoke).To(gomega.Equal(tt.want))
})
}
}

func TestFuzzyConversion(t *testing.T) {
g := gomega.NewWithT(t)
scheme := runtime.NewScheme()
Expand Down
7 changes: 3 additions & 4 deletions api/v1alpha3/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.

36 changes: 36 additions & 0 deletions api/v1alpha4/conversion_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,24 @@ func TestConvertTo(t *testing.T) {
},
},
},
{
name: "APIServer LoadBalancer Configuration",
spoke: &OpenStackCluster{
Spec: OpenStackClusterSpec{
ManagedAPIServerLoadBalancer: true,
APIServerLoadBalancerAdditionalPorts: []int{80, 443},
},
},
hub: &infrav1.OpenStackCluster{},
want: &infrav1.OpenStackCluster{
Spec: infrav1.OpenStackClusterSpec{
APIServerLoadBalancer: infrav1.APIServerLoadBalancer{
Enabled: true,
AdditionalPorts: []int{80, 443},
},
},
},
},
}

for _, tt := range tests {
Expand Down Expand Up @@ -121,6 +139,24 @@ func TestConvertFrom(t *testing.T) {
},
},
},
{
name: "APIServer LoadBalancer Configuration",
spoke: &OpenStackCluster{},
hub: &infrav1.OpenStackCluster{
Spec: infrav1.OpenStackClusterSpec{
APIServerLoadBalancer: infrav1.APIServerLoadBalancer{
Enabled: true,
AdditionalPorts: []int{80, 443},
},
},
},
want: &OpenStackCluster{
Spec: OpenStackClusterSpec{
ManagedAPIServerLoadBalancer: true,
APIServerLoadBalancerAdditionalPorts: []int{80, 443},
},
},
},
}

for _, tt := range tests {
Expand Down
18 changes: 18 additions & 0 deletions api/v1alpha4/openstackcluster_conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,13 @@ limitations under the License.
package v1alpha4

import (
unsafe "unsafe"

"k8s.io/apimachinery/pkg/conversion"
clusterv1alpha4 "sigs.k8s.io/cluster-api/api/v1alpha4"
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"

infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1"
)

// Convert_v1alpha4_APIEndpoint_To_v1beta1_APIEndpoint is an autogenerated conversion function.
Expand All @@ -31,3 +35,17 @@ func Convert_v1alpha4_APIEndpoint_To_v1beta1_APIEndpoint(in *clusterv1alpha4.API
func Convert_v1beta1_APIEndpoint_To_v1alpha4_APIEndpoint(in *clusterv1.APIEndpoint, out *clusterv1alpha4.APIEndpoint, s conversion.Scope) error {
return clusterv1alpha4.Convert_v1beta1_APIEndpoint_To_v1alpha4_APIEndpoint(in, out, s)
}

func Convert_v1alpha4_OpenStackClusterSpec_To_v1beta1_OpenStackClusterSpec(in *OpenStackClusterSpec, out *infrav1.OpenStackClusterSpec, s conversion.Scope) error {
out.APIServerLoadBalancer.Enabled = in.ManagedAPIServerLoadBalancer
out.APIServerLoadBalancer.AdditionalPorts = *(*[]int)(unsafe.Pointer(&in.APIServerLoadBalancerAdditionalPorts))

return autoConvert_v1alpha4_OpenStackClusterSpec_To_v1beta1_OpenStackClusterSpec(in, out, s)
}

func Convert_v1beta1_OpenStackClusterSpec_To_v1alpha4_OpenStackClusterSpec(in *infrav1.OpenStackClusterSpec, out *OpenStackClusterSpec, s conversion.Scope) error {
out.ManagedAPIServerLoadBalancer = in.APIServerLoadBalancer.Enabled
out.APIServerLoadBalancerAdditionalPorts = *(*[]int)(unsafe.Pointer(&in.APIServerLoadBalancer.AdditionalPorts))

return autoConvert_v1beta1_OpenStackClusterSpec_To_v1alpha4_OpenStackClusterSpec(in, out, s)
}
37 changes: 13 additions & 24 deletions api/v1alpha4/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.

9 changes: 3 additions & 6 deletions api/v1beta1/openstackcluster_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,10 @@ type OpenStackClusterSpec struct {
// +optional
ExternalNetworkID string `json:"externalNetworkId,omitempty"`

// ManagedAPIServerLoadBalancer defines whether a LoadBalancer for the
// APIServer should be created.
// APIServerLoadBalancer configures the optional LoadBalancer for the APIServer.
// It must be activated by setting `enabled: true`.
// +optional
ManagedAPIServerLoadBalancer bool `json:"managedAPIServerLoadBalancer"`
APIServerLoadBalancer APIServerLoadBalancer `json:"apiServerLoadBalancer,omitempty"`

// DisableAPIServerFloatingIP determines whether or not to attempt to attach a floating
// IP to the API server. This allows for the creation of clusters when attaching a floating
Expand Down Expand Up @@ -97,9 +97,6 @@ type OpenStackClusterSpec struct {
// will be created
APIServerPort int `json:"apiServerPort,omitempty"`

// APIServerLoadBalancerAdditionalPorts adds additional ports to the APIServerLoadBalancer
APIServerLoadBalancerAdditionalPorts []int `json:"apiServerLoadBalancerAdditionalPorts,omitempty"`

// ManagedSecurityGroups determines whether OpenStack security groups for the cluster
// will be managed by the OpenStack provider or whether pre-existing security groups will
// be specified as part of the configuration.
Expand Down
7 changes: 7 additions & 0 deletions api/v1beta1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,3 +296,10 @@ type Bastion struct {
//+optional
AvailabilityZone string `json:"availabilityZone,omitempty"`
}

type APIServerLoadBalancer struct {
// Enabled defines whether a LoadBalancer should be created.
Enabled bool `json:"enabled,omitempty"`
// AdditionalPorts adds additional tcp ports to the Loadbalacner
AdditionalPorts []int `json:"additionalPorts,omitempty"`
}
Loading