Skip to content
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

🐛 Use namespace of the reference on external.Get #11361

Merged
merged 8 commits into from
Nov 18, 2024
Merged

🐛 Use namespace of the reference on external.Get #11361

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4ba1527
Use namespace of the reference on Get
Danil-Grigorev Oct 31, 2024
25a4bd3
Updating tests
Danil-Grigorev Nov 1, 2024
f5ecffe
Update references usage across the code
Danil-Grigorev Nov 1, 2024
f3693a8
Ensure refrence namespace is populated in MD
Danil-Grigorev Nov 5, 2024
d9753b0
Use kref for logging
Danil-Grigorev Nov 5, 2024
a3e3e9a
Ensure ref NS in MS is set, and ignored in hash
Danil-Grigorev Nov 5, 2024
de51a24
Double-check and populate ns for MS and MD template
Danil-Grigorev Nov 18, 2024
ca36516
Review: log messages
Danil-Grigorev Nov 18, 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
2 changes: 1 addition & 1 deletion bootstrap/util/configowner.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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"
Copy link
Member

@sbueringer sbueringer Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@Danil-Grigorev Did some analysis (sharing here for future-us :))

Ref fields with namespace defaulting before this PR:

  • Cluster.Spec.InfrastructureRef
  • Cluster.Spec.ControlPlaneRef
  • Machine.Spec.InfrastructureRef
  • Machine.Spec.Bootstrap.ConfigRef
  • MachinePool.Spec.Template.Spec.Bootstrap.ConfigRef
  • MachinePool.Spec.Template.Spec.InfrastructureRef
  • MHC.Spec.RemediationTemplate
  • KCP.spec.machineTemplate.infrastructureRef
  • ClusterClass (all refs)

Namespace fields for which we have to add defaulting:

  • MachineDeployment (not used anymore for rollout decisions, namespace is set in most cases by Cluster topology controller)
    • Defaulting already added with this PR
    • DeepCopy ref and set namespace if empty directly before external.Get (in reconcileExternalTemplateReference)
  • MachineSet (not used anymore for rollout decisions, namespace is set in most cases by the MD controller)
    • Defaulting already added with this PR
    • DeepCopy ref and set namespace if empty directly before external.Get (in reconcileExternalTemplateReference)

So I think we're almost good. I think we just can't assume that the defaulting/mutating webhook always run before our controllers (e.g. if a MD/MS is never modified). I think for this case it would be good to add safeguards in the reconcileExternalTemplateReference funcs to set the namespace if it is empty directly before we use it (but after UpdateReferenceAPIContract))

Copy link
Member

@sbueringer sbueringer Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

(DeepCopy has been added now, updated the tasks accordingly)

"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)
sbueringer marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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