Skip to content

Commit

Permalink
🐛 Use namespace of the reference on external.Get (kubernetes-sigs#11361)
Browse files Browse the repository at this point in the history 
* Use namespace of the reference on Get

Signed-off-by: Danil-Grigorev <[email protected]>

* Updating tests

Signed-off-by: Danil-Grigorev <[email protected]>

* Update references usage across the code

Signed-off-by: Danil-Grigorev <[email protected]>

* Ensure refrence namespace is populated in MD

Signed-off-by: Danil-Grigorev <[email protected]>

* Use kref for logging

Signed-off-by: Danil-Grigorev <[email protected]>

* Ensure ref NS in MS is set, and ignored in hash

Signed-off-by: Danil-Grigorev <[email protected]>

* Double-check and populate ns for MS and MD template

Signed-off-by: Danil-Grigorev <[email protected]>

* Review: log messages

Signed-off-by: Danil-Grigorev <[email protected]>

---------

Signed-off-by: Danil-Grigorev <[email protected]>
  • Loading branch information
@Danil-Grigorev
Danil-Grigorev authored Nov 18, 2024
1 parent c739d0a commit a21faa0
Show file tree
Hide file tree
Showing 37 changed files with 245 additions and 80 deletions.
2 changes: 1 addition & 1 deletion bootstrap/util/configowner.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ func getConfigOwner(ctx context.Context, c client.Client, obj metav1.Object, get

// GetOwnerByRef finds and returns the owner by looking at the object reference.
func GetOwnerByRef(ctx context.Context, c client.Client, ref *corev1.ObjectReference) (*ConfigOwner, error) {
obj, err := external.Get(ctx, c, ref, ref.Namespace)
obj, err := external.Get(ctx, c, ref)
if err != nil {
return nil, err
}
Expand Down
16 changes: 8 additions & 8 deletions cmd/clusterctl/client/tree/discovery.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ func Discovery(ctx context.Context, c client.Client, namespace, name string, opt
tree := NewObjectTree(cluster, options.toObjectTreeOptions())

// Adds cluster infra
clusterInfra, err := external.Get(ctx, c, cluster.Spec.InfrastructureRef, cluster.Namespace)
clusterInfra, err := external.Get(ctx, c, cluster.Spec.InfrastructureRef)
if err != nil {
return nil, errors.Wrap(err, "get InfraCluster reference from Cluster")
}
Expand All @@ -95,7 +95,7 @@ func Discovery(ctx context.Context, c client.Client, namespace, name string, opt
}

// Adds control plane
controlPlane, err := external.Get(ctx, c, cluster.Spec.ControlPlaneRef, cluster.Namespace)
controlPlane, err := external.Get(ctx, c, cluster.Spec.ControlPlaneRef)
if err == nil {
addControlPlane(cluster, controlPlane, tree, options)
}
Expand All @@ -112,13 +112,13 @@ func Discovery(ctx context.Context, c client.Client, namespace, name string, opt

if visible {
if (m.Spec.InfrastructureRef != corev1.ObjectReference{}) {
if machineInfra, err := external.Get(ctx, c, &m.Spec.InfrastructureRef, cluster.Namespace); err == nil {
if machineInfra, err := external.Get(ctx, c, &m.Spec.InfrastructureRef); err == nil {
tree.Add(m, machineInfra, ObjectMetaName("MachineInfrastructure"), NoEcho(true))
}
}

if m.Spec.Bootstrap.ConfigRef != nil {
if machineBootstrap, err := external.Get(ctx, c, m.Spec.Bootstrap.ConfigRef, cluster.Namespace); err == nil {
if machineBootstrap, err := external.Get(ctx, c, m.Spec.Bootstrap.ConfigRef); err == nil {
tree.Add(m, machineBootstrap, ObjectMetaName("BootstrapConfig"), NoEcho(true))
}
}
Expand Down Expand Up @@ -154,7 +154,7 @@ func Discovery(ctx context.Context, c client.Client, namespace, name string, opt

if len(machinePoolList.Items) > 0 { // Add MachinePool objects
tree.Add(cluster, workers)
addMachinePoolsToObjectTree(ctx, c, cluster.Namespace, workers, machinePoolList, machinesList, tree, addMachineFunc)
addMachinePoolsToObjectTree(ctx, c, workers, machinePoolList, machinesList, tree, addMachineFunc)
}

// Handles orphan machines.
Expand Down Expand Up @@ -275,17 +275,17 @@ func addMachineDeploymentToObjectTree(ctx context.Context, c client.Client, clus
return nil
}

func addMachinePoolsToObjectTree(ctx context.Context, c client.Client, namespace string, workers *unstructured.Unstructured, machinePoolList *expv1.MachinePoolList, machinesList *clusterv1.MachineList, tree *ObjectTree, addMachineFunc func(parent client.Object, m *clusterv1.Machine)) {
func addMachinePoolsToObjectTree(ctx context.Context, c client.Client, workers *unstructured.Unstructured, machinePoolList *expv1.MachinePoolList, machinesList *clusterv1.MachineList, tree *ObjectTree, addMachineFunc func(parent client.Object, m *clusterv1.Machine)) {
for i := range machinePoolList.Items {
mp := &machinePoolList.Items[i]
_, visible := tree.Add(workers, mp, GroupingObject(true))

if visible {
if machinePoolBootstrap, err := external.Get(ctx, c, mp.Spec.Template.Spec.Bootstrap.ConfigRef, namespace); err == nil {
if machinePoolBootstrap, err := external.Get(ctx, c, mp.Spec.Template.Spec.Bootstrap.ConfigRef); err == nil {
tree.Add(mp, machinePoolBootstrap, ObjectMetaName("BootstrapConfig"), NoEcho(true))
}

if machinePoolInfra, err := external.Get(ctx, c, &mp.Spec.Template.Spec.InfrastructureRef, namespace); err == nil {
if machinePoolInfra, err := external.Get(ctx, c, &mp.Spec.Template.Spec.InfrastructureRef); err == nil {
tree.Add(mp, machinePoolInfra, ObjectMetaName("MachinePoolInfrastructure"), NoEcho(true))
}
}
Expand Down
13 changes: 7 additions & 6 deletions controllers/external/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,23 +25,24 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apiserver/pkg/storage/names"
"k8s.io/klog/v2"
"sigs.k8s.io/controller-runtime/pkg/client"

clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
)

// Get uses the client and reference to get an external, unstructured object.
func Get(ctx context.Context, c client.Reader, ref *corev1.ObjectReference, namespace string) (*unstructured.Unstructured, error) {
func Get(ctx context.Context, c client.Reader, ref *corev1.ObjectReference) (*unstructured.Unstructured, error) {
if ref == nil {
return nil, errors.Errorf("cannot get object - object reference not set")
}
obj := new(unstructured.Unstructured)
obj.SetAPIVersion(ref.APIVersion)
obj.SetKind(ref.Kind)
obj.SetName(ref.Name)
key := client.ObjectKey{Name: obj.GetName(), Namespace: namespace}
if err := c.Get(ctx, key, obj); err != nil {
return nil, errors.Wrapf(err, "failed to retrieve %s external object %q/%q", obj.GetKind(), key.Namespace, key.Name)
obj.SetNamespace(ref.Namespace)
if err := c.Get(ctx, client.ObjectKeyFromObject(obj), obj); err != nil {
return nil, errors.Wrapf(err, "failed to retrieve %s %s", obj.GetKind(), klog.KRef(ref.Namespace, ref.Name))
}
return obj, nil
}
Expand All @@ -54,7 +55,7 @@ func Delete(ctx context.Context, c client.Writer, ref *corev1.ObjectReference) e
obj.SetName(ref.Name)
obj.SetNamespace(ref.Namespace)
if err := c.Delete(ctx, obj); err != nil {
return errors.Wrapf(err, "failed to delete %s external object %q/%q", obj.GetKind(), obj.GetNamespace(), obj.GetName())
return errors.Wrapf(err, "failed to delete %s %s", obj.GetKind(), klog.KRef(ref.Namespace, ref.Name))
}
return nil
}
Expand Down Expand Up @@ -92,7 +93,7 @@ type CreateFromTemplateInput struct {

// CreateFromTemplate uses the client and the reference to create a new object from the template.
func CreateFromTemplate(ctx context.Context, in *CreateFromTemplateInput) (*corev1.ObjectReference, error) {
from, err := Get(ctx, in.Client, in.TemplateRef, in.Namespace)
from, err := Get(ctx, in.Client, in.TemplateRef)
if err != nil {
return nil, err
}
Expand Down
4 changes: 2 additions & 2 deletions controllers/external/util_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ func TestGetResourceFound(t *testing.T) {
}

fakeClient := fake.NewClientBuilder().WithObjects(testResource.DeepCopy()).Build()
got, err := Get(ctx, fakeClient, testResourceReference, metav1.NamespaceDefault)
got, err := Get(ctx, fakeClient, testResourceReference)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(got).To(BeComparableTo(testResource))
}
Expand All @@ -79,7 +79,7 @@ func TestGetResourceNotFound(t *testing.T) {
}

fakeClient := fake.NewClientBuilder().Build()
_, err := Get(ctx, fakeClient, testResourceReference, metav1.NamespaceDefault)
_, err := Get(ctx, fakeClient, testResourceReference)
g.Expect(err).To(HaveOccurred())
g.Expect(apierrors.IsNotFound(errors.Cause(err))).To(BeTrue())
}
Expand Down
2 changes: 1 addition & 1 deletion controlplane/kubeadm/internal/control_plane.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ func (c *ControlPlane) UpToDateMachines() collections.Machines {
func getInfraResources(ctx context.Context, cl client.Client, machines collections.Machines) (map[string]*unstructured.Unstructured, error) {
result := map[string]*unstructured.Unstructured{}
for _, m := range machines {
infraObj, err := external.Get(ctx, cl, &m.Spec.InfrastructureRef, m.Namespace)
infraObj, err := external.Get(ctx, cl, &m.Spec.InfrastructureRef)
if err != nil {
if apierrors.IsNotFound(errors.Cause(err)) {
continue
Expand Down
2 changes: 1 addition & 1 deletion controlplane/kubeadm/internal/controllers/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1399,7 +1399,7 @@ kubernetesVersion: metav1.16.1
machine := machineList.Items[0]
g.Expect(machine.Name).To(HavePrefix(kcp.Name))
// Newly cloned infra objects should have the infraref annotation.
infraObj, err := external.Get(ctx, r.Client, &machine.Spec.InfrastructureRef, machine.Spec.InfrastructureRef.Namespace)
infraObj, err := external.Get(ctx, r.Client, &machine.Spec.InfrastructureRef)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(infraObj.GetAnnotations()).To(HaveKeyWithValue(clusterv1.TemplateClonedFromNameAnnotation, genericInfrastructureMachineTemplate.GetName()))
g.Expect(infraObj.GetAnnotations()).To(HaveKeyWithValue(clusterv1.TemplateClonedFromGroupKindAnnotation, genericInfrastructureMachineTemplate.GroupVersionKind().GroupKind().String()))
Expand Down
2 changes: 1 addition & 1 deletion controlplane/kubeadm/internal/controllers/helpers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ func (r *KubeadmControlPlaneReconciler) reconcileExternalReference(ctx context.C
return err
}

obj, err := external.Get(ctx, r.Client, ref, controlPlane.Cluster.Namespace)
obj, err := external.Get(ctx, r.Client, ref)
if err != nil {
if apierrors.IsNotFound(err) {
controlPlane.InfraMachineTemplateIsNotFound = true
Expand Down
4 changes: 2 additions & 2 deletions controlplane/kubeadm/internal/controllers/helpers_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,7 @@ func TestCloneConfigsAndGenerateMachine(t *testing.T) {
g.Expect(m.Name).NotTo(BeEmpty())
g.Expect(m.Name).To(HavePrefix(kcp.Name + namingTemplateKey))

infraObj, err := external.Get(ctx, r.Client, &m.Spec.InfrastructureRef, m.Spec.InfrastructureRef.Namespace)
infraObj, err := external.Get(ctx, r.Client, &m.Spec.InfrastructureRef)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(infraObj.GetAnnotations()).To(HaveKeyWithValue(clusterv1.TemplateClonedFromNameAnnotation, genericInfrastructureMachineTemplate.GetName()))
g.Expect(infraObj.GetAnnotations()).To(HaveKeyWithValue(clusterv1.TemplateClonedFromGroupKindAnnotation, genericInfrastructureMachineTemplate.GroupVersionKind().GroupKind().String()))
Expand Down Expand Up @@ -469,7 +469,7 @@ func TestCloneConfigsAndGenerateMachineFail(t *testing.T) {
Status: corev1.ConditionFalse,
Severity: clusterv1.ConditionSeverityError,
Reason: controlplanev1.InfrastructureTemplateCloningFailedReason,
Message: "failed to retrieve GenericMachineTemplate external object \"default\"/\"something_invalid\": genericmachinetemplates.generic.io \"something_invalid\" not found",
Message: "failed to retrieve GenericMachineTemplate default/something_invalid: genericmachinetemplates.generic.io \"something_invalid\" not found",
}))
}

Expand Down
4 changes: 2 additions & 2 deletions exp/internal/controllers/machinepool_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -341,10 +341,10 @@ func (r *MachinePoolReconciler) reconcileDeleteExternal(ctx context.Context, mac
continue
}

obj, err := external.Get(ctx, r.Client, ref, machinePool.Namespace)
obj, err := external.Get(ctx, r.Client, ref)
if err != nil && !apierrors.IsNotFound(errors.Cause(err)) {
return false, errors.Wrapf(err, "failed to get %s %q for MachinePool %q in namespace %q",
ref.GroupVersionKind(), ref.Name, machinePool.Name, machinePool.Namespace)
ref.GroupVersionKind(), ref.Name, machinePool.Name, ref.Namespace)
}
if obj != nil {
objects = append(objects, obj)
Expand Down
4 changes: 2 additions & 2 deletions exp/internal/controllers/machinepool_controller_phases.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,11 +113,11 @@ func (r *MachinePoolReconciler) reconcileExternal(ctx context.Context, m *expv1.
return external.ReconcileOutput{}, err
}

obj, err := external.Get(ctx, r.Client, ref, m.Namespace)
obj, err := external.Get(ctx, r.Client, ref)
if err != nil {
if apierrors.IsNotFound(errors.Cause(err)) {
return external.ReconcileOutput{}, errors.Wrapf(err, "could not find %v %q for MachinePool %q in namespace %q, requeuing",
ref.GroupVersionKind(), ref.Name, m.Name, m.Namespace)
ref.GroupVersionKind(), ref.Name, m.Name, ref.Namespace)
}
return external.ReconcileOutput{}, err
}
Expand Down
19 changes: 17 additions & 2 deletions exp/internal/controllers/machinepool_controller_phases_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,12 +78,14 @@ func TestReconcileMachinePoolPhases(t *testing.T) {
APIVersion: builder.BootstrapGroupVersion.String(),
Kind: builder.TestBootstrapConfigKind,
Name: "bootstrap-config1",
Namespace: metav1.NamespaceDefault,
},
},
InfrastructureRef: corev1.ObjectReference{
APIVersion: builder.InfrastructureGroupVersion.String(),
Kind: builder.TestInfrastructureMachineTemplateKind,
Name: "infra-config1",
Namespace: metav1.NamespaceDefault,
},
},
},
Expand Down Expand Up @@ -808,6 +810,7 @@ func TestReconcileMachinePoolBootstrap(t *testing.T) {
APIVersion: builder.BootstrapGroupVersion.String(),
Kind: builder.TestBootstrapConfigKind,
Name: "bootstrap-config1",
Namespace: metav1.NamespaceDefault,
},
},
},
Expand Down Expand Up @@ -949,6 +952,7 @@ func TestReconcileMachinePoolBootstrap(t *testing.T) {
APIVersion: builder.BootstrapGroupVersion.String(),
Kind: builder.TestBootstrapConfigKind,
Name: "bootstrap-config1",
Namespace: metav1.NamespaceDefault,
},
DataSecretName: ptr.To("data"),
},
Expand Down Expand Up @@ -1032,6 +1036,7 @@ func TestReconcileMachinePoolBootstrap(t *testing.T) {
APIVersion: builder.BootstrapGroupVersion.String(),
Kind: builder.TestBootstrapConfigKind,
Name: "bootstrap-config1",
Namespace: metav1.NamespaceDefault,
},
DataSecretName: ptr.To("data"),
},
Expand Down Expand Up @@ -1107,12 +1112,14 @@ func TestReconcileMachinePoolInfrastructure(t *testing.T) {
APIVersion: builder.BootstrapGroupVersion.String(),
Kind: builder.TestBootstrapConfigKind,
Name: "bootstrap-config1",
Namespace: metav1.NamespaceDefault,
},
},
InfrastructureRef: corev1.ObjectReference{
APIVersion: builder.InfrastructureGroupVersion.String(),
Kind: builder.TestInfrastructureMachineTemplateKind,
Name: "infra-config1",
Namespace: metav1.NamespaceDefault,
},
},
},
Expand Down Expand Up @@ -1186,12 +1193,14 @@ func TestReconcileMachinePoolInfrastructure(t *testing.T) {
APIVersion: builder.BootstrapGroupVersion.String(),
Kind: builder.TestBootstrapConfigKind,
Name: "bootstrap-config1",
Namespace: metav1.NamespaceDefault,
},
},
InfrastructureRef: corev1.ObjectReference{
APIVersion: builder.InfrastructureGroupVersion.String(),
Kind: builder.TestInfrastructureMachineTemplateKind,
Name: "infra-config1",
Namespace: metav1.NamespaceDefault,
},
},
},
Expand Down Expand Up @@ -1245,12 +1254,14 @@ func TestReconcileMachinePoolInfrastructure(t *testing.T) {
APIVersion: builder.BootstrapGroupVersion.String(),
Kind: builder.TestBootstrapConfigKind,
Name: "bootstrap-config1",
Namespace: metav1.NamespaceDefault,
},
},
InfrastructureRef: corev1.ObjectReference{
APIVersion: builder.InfrastructureGroupVersion.String(),
Kind: builder.TestInfrastructureMachineTemplateKind,
Name: "infra-config1",
Namespace: metav1.NamespaceDefault,
},
},
},
Expand Down Expand Up @@ -1437,7 +1448,7 @@ func TestReconcileMachinePoolMachines(t *testing.T) {
g.Expect(machineList.Items).To(HaveLen(2))
for i := range machineList.Items {
machine := &machineList.Items[i]
_, err := external.Get(ctx, r.Client, &machine.Spec.InfrastructureRef, machine.Namespace)
_, err := external.Get(ctx, r.Client, &machine.Spec.InfrastructureRef)
g.Expect(err).ToNot(HaveOccurred())
}
})
Expand Down Expand Up @@ -1508,7 +1519,7 @@ func TestReconcileMachinePoolMachines(t *testing.T) {
g.Expect(machineList.Items).To(HaveLen(2))
for i := range machineList.Items {
machine := &machineList.Items[i]
_, err := external.Get(ctx, r.Client, &machine.Spec.InfrastructureRef, machine.Namespace)
_, err := external.Get(ctx, r.Client, &machine.Spec.InfrastructureRef)
g.Expect(err).ToNot(HaveOccurred())
}
})
Expand Down Expand Up @@ -1755,12 +1766,14 @@ func TestReconcileMachinePoolScaleToFromZero(t *testing.T) {
APIVersion: builder.BootstrapGroupVersion.String(),
Kind: builder.TestBootstrapConfigKind,
Name: "bootstrap-config1",
Namespace: ns.Name,
},
},
InfrastructureRef: corev1.ObjectReference{
APIVersion: builder.InfrastructureGroupVersion.String(),
Kind: builder.TestInfrastructureMachineTemplateKind,
Name: "infra-config1",
Namespace: ns.Name,
},
},
},
Expand Down Expand Up @@ -2181,12 +2194,14 @@ func getMachinePool(replicas int, mpName, clusterName, nsName string) expv1.Mach
APIVersion: builder.BootstrapGroupVersion.String(),
Kind: builder.GenericBootstrapConfigKind,
Name: "bootstrap-config1",
Namespace: nsName,
},
},
InfrastructureRef: corev1.ObjectReference{
APIVersion: builder.InfrastructureGroupVersion.String(),
Kind: builder.GenericInfrastructureMachineKind,
Name: "infra-config1",
Namespace: nsName,
},
},
},
Expand Down
Loading

0 comments on commit a21faa0

Please sign in to comment.