Skip to content

⚠️ Change ClusterResourceSetBinding Bindings field from []*ResourceSetBinding to []ResourceSetBinding #12476

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
Jul 14, 2025
+243 −32
Merged

⚠️ Change ClusterResourceSetBinding Bindings field from []*ResourceSetBinding to []ResourceSetBinding #12476

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
17 changes: 17 additions & 0 deletions api/addons/v1beta1/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 v1beta1

import (
"reflect"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
apimachineryconversion "k8s.io/apimachinery/pkg/conversion"
"sigs.k8s.io/controller-runtime/pkg/conversion"
Expand Down Expand Up @@ -105,6 +107,21 @@ func Convert_v1beta1_ClusterResourceSetStatus_To_v1beta2_ClusterResourceSetStatu
return nil
}

func Convert_Pointer_v1beta1_ResourceSetBinding_To_v1beta2_ResourceSetBinding(in **ResourceSetBinding, out *addonsv1.ResourceSetBinding, s apimachineryconversion.Scope) error {
if in == nil || *in == nil {
return nil
}
return autoConvert_v1beta1_ResourceSetBinding_To_v1beta2_ResourceSetBinding(*in, out, s)
}

func Convert_v1beta2_ResourceSetBinding_To_Pointer_v1beta1_ResourceSetBinding(in *addonsv1.ResourceSetBinding, out **ResourceSetBinding, s apimachineryconversion.Scope) error {
if in == nil || reflect.DeepEqual(*in, addonsv1.ResourceSetBinding{}) {
return nil
}
*out = &ResourceSetBinding{}
return autoConvert_v1beta2_ResourceSetBinding_To_v1beta1_ResourceSetBinding(in, *out, s)
}

// Implement local conversion func because conversion-gen is not aware of conversion func in other packages (see https://github.com/kubernetes/code-generator/issues/94)

func Convert_v1_Condition_To_v1beta1_Condition(in *metav1.Condition, out *clusterv1beta1.Condition, s apimachineryconversion.Scope) error {
Expand Down
22 changes: 20 additions & 2 deletions api/addons/v1beta1/conversion_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,9 @@ func TestFuzzyConversion(t *testing.T) {
FuzzerFuncs: []fuzzer.FuzzerFuncs{ClusterResourceSetFuzzFuncs},
}))
t.Run("for ClusterResourceSetBinding", utilconversion.FuzzTestFunc(utilconversion.FuzzTestFuncInput{
Hub: &addonsv1.ClusterResourceSetBinding{},
Spoke: &ClusterResourceSetBinding{},
Hub: &addonsv1.ClusterResourceSetBinding{},
Spoke: &ClusterResourceSetBinding{},
FuzzerFuncs: []fuzzer.FuzzerFuncs{ClusterResourceSetBindingFuzzFuncs},
}))
}

Expand Down Expand Up @@ -70,3 +71,20 @@ func spokeClusterResourceSetStatus(in *ClusterResourceSetStatus, c randfill.Cont
}
}
}

func ClusterResourceSetBindingFuzzFuncs(_ runtimeserializer.CodecFactory) []interface{} {
return []interface{}{
hubClusterResourceSetStatus,
spokeClusterResourceSetBindingSpec,
}
}

func spokeClusterResourceSetBindingSpec(in *ClusterResourceSetBindingSpec, c randfill.Continue) {
c.FillNoCustom(in)

for i, b := range in.Bindings {
if b != nil && reflect.DeepEqual(*b, ResourceSetBinding{}) {
in.Bindings[i] = nil
}
}
}
58 changes: 54 additions & 4 deletions api/addons/v1beta1/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.

12 changes: 6 additions & 6 deletions api/addons/v1beta2/clusterresourcesetbinding_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,14 +92,14 @@ func (r *ResourceSetBinding) SetBinding(resourceBinding ResourceBinding) {
// GetOrCreateBinding returns the ResourceSetBinding for a given ClusterResourceSet if exists,
// otherwise creates one and updates ClusterResourceSet with it.
func (c *ClusterResourceSetBinding) GetOrCreateBinding(clusterResourceSet *ClusterResourceSet) *ResourceSetBinding {
for _, binding := range c.Spec.Bindings {
if binding.ClusterResourceSetName == clusterResourceSet.Name {
return binding
for i := range c.Spec.Bindings {
Copy link
Member

Choose a reason for hiding this comment

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

I've read the discussion and commented.
I can confirm that the copyloopvar linter is already in place and the original code is fine.
but it's certainly a good idea to keep it this way in terms of visualization.

Copy link
Member Author

@sbueringer sbueringer Jul 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The original code was not fine, that's why I changed it :) (tests broke)

The original code with the pointer change returned a copy of the binding and not the same binding that is part of the c.Spec.Bindings slice

The code is fine for the linter, but that doesn't mean it works as expected :)

Copy link
Member

Choose a reason for hiding this comment

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

Interesting. Surely the only this codes aren't problem, but considering about entire code base, the copy element might be invalid. This case is good example.
Thanks!

Copy link
Member Author

@sbueringer sbueringer Jul 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

To give some more context.

The previous problem (a few Go versions ago) with returning a pointer to a loop var was that the pointer was basically always referring to the last element that was stored in the loop var.

Then they changed it in go so a pointer to the loop var refers to the correct element.

But the loop var is a copy of the slice element not the actual element

if c.Spec.Bindings[i].ClusterResourceSetName == clusterResourceSet.Name {
return &c.Spec.Bindings[i]
}
}
binding := &ResourceSetBinding{ClusterResourceSetName: clusterResourceSet.Name, Resources: []ResourceBinding{}}
binding := ResourceSetBinding{ClusterResourceSetName: clusterResourceSet.Name, Resources: []ResourceBinding{}}
c.Spec.Bindings = append(c.Spec.Bindings, binding)
return binding
return &c.Spec.Bindings[len(c.Spec.Bindings)-1]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return &c.Spec.Bindings[len(c.Spec.Bindings)-1]
return &binding

I think it's simpler but is this to make the format consistent with the above?

Copy link
Member Author

Choose a reason for hiding this comment

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

Simpler but it's not necessarily correct. I'm sure about the case above, I'm not entirely sure if binding here is the same element as in the slice (probably both would work here in this case, but I want to play it safe)

}

// RemoveBinding removes the ClusterResourceSet from the ClusterResourceSetBinding Bindings list.
Expand Down Expand Up @@ -138,7 +138,7 @@ type ClusterResourceSetBindingSpec struct {
// bindings is a list of ClusterResourceSets and their resources.
// +optional
// +kubebuilder:validation:MaxItems=100
Bindings []*ResourceSetBinding `json:"bindings,omitempty"`
Bindings []ResourceSetBinding `json:"bindings,omitempty"`

// clusterName is the name of the Cluster this binding applies to.
// +required
Expand Down
8 changes: 2 additions & 6 deletions api/addons/v1beta2/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.

4 changes: 2 additions & 2 deletions cmd/clusterctl/internal/test/fake_objects.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1195,7 +1195,7 @@ func (f *FakeClusterResourceSet) Objs() []client.Object {
},
Spec: addonsv1.ClusterResourceSetBindingSpec{
ClusterName: cluster.Name,
Bindings: []*addonsv1.ResourceSetBinding{
Bindings: []addonsv1.ResourceSetBinding{
{
ClusterResourceSetName: crs.Name,
},
Expand All @@ -1219,7 +1219,7 @@ func (f *FakeClusterResourceSet) Objs() []client.Object {
ClusterResourceSetName: crs.Name,
Resources: []addonsv1.ResourceBinding{},
}
binding.Spec.Bindings = append(binding.Spec.Bindings, &resourceSetBinding)
binding.Spec.Bindings = append(binding.Spec.Bindings, resourceSetBinding)

// creates map entries for each cluster/resource of type Secret
for _, secret := range f.secrets {
Expand Down
17 changes: 17 additions & 0 deletions internal/api/addons/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 (
"reflect"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
apimachineryconversion "k8s.io/apimachinery/pkg/conversion"
"sigs.k8s.io/controller-runtime/pkg/conversion"
Expand Down Expand Up @@ -119,6 +121,21 @@ func Convert_v1beta2_ClusterResourceSetStatus_To_v1alpha3_ClusterResourceSetStat
return autoConvert_v1beta2_ClusterResourceSetStatus_To_v1alpha3_ClusterResourceSetStatus(in, out, s)
}

func Convert_Pointer_v1alpha3_ResourceSetBinding_To_v1beta2_ResourceSetBinding(in **ResourceSetBinding, out *addonsv1.ResourceSetBinding, s apimachineryconversion.Scope) error {
if in == nil || *in == nil {
return nil
}
return autoConvert_v1alpha3_ResourceSetBinding_To_v1beta2_ResourceSetBinding(*in, out, s)
}

func Convert_v1beta2_ResourceSetBinding_To_Pointer_v1alpha3_ResourceSetBinding(in *addonsv1.ResourceSetBinding, out **ResourceSetBinding, s apimachineryconversion.Scope) error {
if in == nil || reflect.DeepEqual(*in, addonsv1.ResourceSetBinding{}) {
return nil
}
*out = &ResourceSetBinding{}
return autoConvert_v1beta2_ResourceSetBinding_To_v1alpha3_ResourceSetBinding(in, *out, s)
}

// Implement local conversion func because conversion-gen is not aware of conversion func in other packages (see https://github.com/kubernetes/code-generator/issues/94)

func Convert_v1_Condition_To_v1alpha3_Condition(in *metav1.Condition, out *clusterv1alpha3.Condition, s apimachineryconversion.Scope) error {
Expand Down
18 changes: 18 additions & 0 deletions internal/api/addons/v1alpha3/conversion_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ func TestFuzzyConversion(t *testing.T) {
t.Run("for ClusterResourceSetBinding", utilconversion.FuzzTestFunc(utilconversion.FuzzTestFuncInput{
Hub: &addonsv1.ClusterResourceSetBinding{},
Spoke: &ClusterResourceSetBinding{},
FuzzerFuncs: []fuzzer.FuzzerFuncs{ClusterResourceSetBindingFuzzFuncs},
}))
}

Expand All @@ -59,3 +60,20 @@ func hubClusterResourceSetStatus(in *addonsv1.ClusterResourceSetStatus, c randfi
}
}
}

func ClusterResourceSetBindingFuzzFuncs(_ runtimeserializer.CodecFactory) []interface{} {
return []interface{}{
hubClusterResourceSetStatus,
spokeClusterResourceSetBindingSpec,
}
}

func spokeClusterResourceSetBindingSpec(in *ClusterResourceSetBindingSpec, c randfill.Continue) {
c.FillNoCustom(in)

for i, b := range in.Bindings {
if b != nil && reflect.DeepEqual(*b, ResourceSetBinding{}) {
in.Bindings[i] = nil
}
}
}
34 changes: 32 additions & 2 deletions internal/api/addons/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.

17 changes: 17 additions & 0 deletions internal/api/addons/v1alpha4/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 v1alpha4

import (
"reflect"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
apimachineryconversion "k8s.io/apimachinery/pkg/conversion"
"sigs.k8s.io/controller-runtime/pkg/conversion"
Expand Down Expand Up @@ -120,6 +122,21 @@ func Convert_v1beta2_ClusterResourceSetStatus_To_v1alpha4_ClusterResourceSetStat
return autoConvert_v1beta2_ClusterResourceSetStatus_To_v1alpha4_ClusterResourceSetStatus(in, out, s)
}

func Convert_Pointer_v1alpha4_ResourceSetBinding_To_v1beta2_ResourceSetBinding(in **ResourceSetBinding, out *addonsv1.ResourceSetBinding, s apimachineryconversion.Scope) error {
if in == nil || *in == nil {
return nil
}
return autoConvert_v1alpha4_ResourceSetBinding_To_v1beta2_ResourceSetBinding(*in, out, s)
}

func Convert_v1beta2_ResourceSetBinding_To_Pointer_v1alpha4_ResourceSetBinding(in *addonsv1.ResourceSetBinding, out **ResourceSetBinding, s apimachineryconversion.Scope) error {
if in == nil || reflect.DeepEqual(*in, addonsv1.ResourceSetBinding{}) {
return nil
}
*out = &ResourceSetBinding{}
return autoConvert_v1beta2_ResourceSetBinding_To_v1alpha4_ResourceSetBinding(in, *out, s)
}

// Implement local conversion func because conversion-gen is not aware of conversion func in other packages (see https://github.com/kubernetes/code-generator/issues/94)

func Convert_v1_Condition_To_v1alpha4_Condition(in *metav1.Condition, out *clusterv1alpha4.Condition, s apimachineryconversion.Scope) error {
Expand Down
Loading