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

✨ clusterctl move support for a cross namespace ClusterClass reference #11649

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

✨ clusterctl move support for a cross namespace ClusterClass reference #11649

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0ddc544
Add e2e test to cover crossNamespace provisionging
Danil-Grigorev Nov 8, 2024
36b7d8a
Implement clusterctl move support for cross-ns CC
Danil-Grigorev Jan 7, 2025
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
12 changes: 12 additions & 0 deletions cmd/clusterctl/client/cluster/mover_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1846,6 +1846,8 @@ func Test_objectMoverService_ensureNamespaces(t *testing.T) {

cluster1 := test.NewFakeCluster("namespace-1", "cluster-1")
cluster2 := test.NewFakeCluster("namespace-2", "cluster-2")
cluster3 := test.NewFakeCluster("namespace-1", "cluster-3").WithTopologyClass("cluster-class-1").WithTopologyClassNamespace("namespace-2")
clusterClass1 := test.NewFakeClusterClass("namespace-2", "cluster-class-1")
globalObj := test.NewFakeClusterExternalObject("eo-1")

clustersObjs := append(cluster1.Objs(), cluster2.Objs()...)
Expand Down Expand Up @@ -1876,6 +1878,16 @@ func Test_objectMoverService_ensureNamespaces(t *testing.T) {
},
expectedNamespaces: []string{"namespace-1", "namespace-2"},
},
{
name: "ensureNamespaces moves namespace-1 and namespace-2 from cross-namespace CC reference",
fields: fields{
objs: append(cluster3.Objs(), clusterClass1.Objs()...),
},
args: args{
toProxy: test.NewFakeProxy(),
},
expectedNamespaces: []string{"namespace-1", "namespace-2"},
},
{
name: "ensureNamespaces moves namespace-2 to target which already has namespace-1",
fields: fields{
Expand Down
15 changes: 14 additions & 1 deletion cmd/clusterctl/client/cluster/objectgraph.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import (
)

const clusterTopologyNameKey = "cluster.spec.topology.class"
const clusterTopologyNamespaceKey = "cluster.spec.topology.classNamespace"
const clusterResourceSetBindingClusterNameKey = "clusterresourcesetbinding.spec.clustername"

type empty struct{}
Expand Down Expand Up @@ -149,6 +150,7 @@ func (n *node) captureAdditionalInformation(obj *unstructured.Unstructured) erro
n.additionalInfo = map[string]interface{}{}
}
n.additionalInfo[clusterTopologyNameKey] = cluster.GetClassKey().Name
n.additionalInfo[clusterTopologyNamespaceKey] = cluster.GetClassKey().Namespace
}
}

Expand Down Expand Up @@ -620,7 +622,18 @@ func (o *objectGraph) setSoftOwnership() {
for _, cluster := range clusters {
// if the cluster uses a managed topology and uses the clusterclass
// set the clusterclass as a soft owner of the cluster.
if className, ok := cluster.additionalInfo[clusterTopologyNameKey]; ok {
className, hasName := cluster.additionalInfo[clusterTopologyNameKey]
classNamespace, hasNamespace := cluster.additionalInfo[clusterTopologyNamespaceKey]
if hasNamespace && hasName {
if className == clusterClass.identity.Name && clusterClass.identity.Namespace == classNamespace {
cluster.addSoftOwner(clusterClass)

// Set isGlobalHierarchy on clusterClass if is referenced from a different namespace
if classNamespace != cluster.identity.Namespace {
clusterClass.isGlobalHierarchy = true
Copy link
Member

@fabriziopandini fabriziopandini Jan 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

While this probably achieve the goal, it seems a little bit hacky:

  • isGlobal and isGlobalHierarchy are meant to track if "object is a global resource (no namespace).", which this is not the case for cc
  • you are using isGlobalHierarchy without having an isGlobal object on top

Please take a look at the alternative solution I proposed above, happy to chat about it if you want

}
}
} else if hasName {
if className == clusterClass.identity.Name && clusterClass.identity.Namespace == cluster.identity.Namespace {
cluster.addSoftOwner(clusterClass)
}
Expand Down
56 changes: 56 additions & 0 deletions cmd/clusterctl/client/cluster/objectgraph_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,7 @@ func TestObjectGraph_getDiscoveryTypeMetaList(t *testing.T) {
type wantGraphItem struct {
virtual bool
isGlobal bool
isGlobalHierarchy bool
forceMove bool
forceMoveHierarchy bool
owners []string
Expand All @@ -262,6 +263,7 @@ func assertGraph(t *testing.T, got *objectGraph, want wantGraph) {
g.Expect(ok).To(BeTrue(), "node %q not found", uid)
g.Expect(gotNode.virtual).To(Equal(wantNode.virtual), "node %q.virtual does not have the expected value", uid)
g.Expect(gotNode.isGlobal).To(Equal(wantNode.isGlobal), "node %q.isGlobal does not have the expected value", uid)
g.Expect(gotNode.isGlobalHierarchy).To(Equal(wantNode.isGlobalHierarchy), "node %q.isGlobalHierarchy does not have the expected value", uid)
g.Expect(gotNode.forceMove).To(Equal(wantNode.forceMove), "node %q.forceMove does not have the expected value", uid)
g.Expect(gotNode.forceMoveHierarchy).To(Equal(wantNode.forceMoveHierarchy), "node %q.forceMoveHierarchy does not have the expected value", uid)
g.Expect(gotNode.owners).To(HaveLen(len(wantNode.owners)), "node %q.owner does not have the expected length", uid)
Expand Down Expand Up @@ -1399,8 +1401,10 @@ var objectGraphsTests = []struct {
isGlobal: true,
forceMove: true,
forceMoveHierarchy: true,
isGlobalHierarchy: true,
},
"/v1, Kind=Secret, infra1-system/infra1-identity-credentials": {
isGlobalHierarchy: true,
owners: []string{
"infrastructure.cluster.x-k8s.io/v1beta1, Kind=GenericClusterInfrastructureIdentity, infra1-identity",
},
Expand Down Expand Up @@ -2047,6 +2051,58 @@ func Test_objectGraph_setSoftOwnership(t *testing.T) {
},
},
},
{
name: "A different namespaced ClusterClass with a soft owned Cluster",
fields: fields{
objs: func() []client.Object {
objs := test.NewFakeClusterClass("ns1", "class1").Objs()
objs = append(objs, test.NewFakeCluster("ns2", "cluster1").WithTopologyClass("class1").WithTopologyClassNamespace("ns1").Objs()...)

return objs
}(),
},
want: wantGraph{
nodes: map[string]wantGraphItem{
"cluster.x-k8s.io/v1beta1, Kind=ClusterClass, ns1/class1": {
forceMove: true,
forceMoveHierarchy: true,
isGlobalHierarchy: true,
},
"infrastructure.cluster.x-k8s.io/v1beta1, Kind=GenericInfrastructureClusterTemplate, ns1/class1": {
owners: []string{
"cluster.x-k8s.io/v1beta1, Kind=ClusterClass, ns1/class1",
},
},
"controlplane.cluster.x-k8s.io/v1beta1, Kind=GenericControlPlaneTemplate, ns1/class1": {
owners: []string{
"cluster.x-k8s.io/v1beta1, Kind=ClusterClass, ns1/class1",
},
},
"cluster.x-k8s.io/v1beta1, Kind=Cluster, ns2/cluster1": {
forceMove: true,
forceMoveHierarchy: true,
softOwners: []string{
"cluster.x-k8s.io/v1beta1, Kind=ClusterClass, ns1/class1", // NB. this cluster is not linked to the clusterclass through owner ref, but it is detected as soft ownership
},
},
"infrastructure.cluster.x-k8s.io/v1beta1, Kind=GenericInfrastructureCluster, ns2/cluster1": {
owners: []string{
"cluster.x-k8s.io/v1beta1, Kind=Cluster, ns2/cluster1",
},
},
"/v1, Kind=Secret, ns2/cluster1-ca": {
softOwners: []string{
"cluster.x-k8s.io/v1beta1, Kind=Cluster, ns2/cluster1", // NB. this secret is not linked to the cluster through owner ref, but it is detected as soft ownership
},
},
"/v1, Kind=Secret, ns2/cluster1-kubeconfig": {
owners: []string{
"cluster.x-k8s.io/v1beta1, Kind=Cluster, ns2/cluster1",
},
},
},
},
},
{
name: "A Cluster with a soft owned ClusterResourceSetBinding",
fields: fields{
Expand Down
20 changes: 11 additions & 9 deletions cmd/clusterctl/client/clusterclass.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ import (
"github.com/pkg/errors"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/types"
"k8s.io/klog/v2"
"sigs.k8s.io/controller-runtime/pkg/client"

clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
Expand All @@ -34,7 +36,7 @@ import (
// addClusterClassIfMissing returns a Template that includes the base template and adds any cluster class definitions that
// are references in the template. If the cluster class referenced already exists in the cluster it is not added to the
// template.
func addClusterClassIfMissing(ctx context.Context, template Template, clusterClassClient repository.ClusterClassClient, clusterClient cluster.Client, targetNamespace string, listVariablesOnly bool) (Template, error) {
func addClusterClassIfMissing(ctx context.Context, template Template, clusterClassClient repository.ClusterClassClient, clusterClient cluster.Client, listVariablesOnly bool) (Template, error) {
classes, err := clusterClassNamesFromTemplate(template)
if err != nil {
return nil, err
Expand All @@ -44,7 +46,7 @@ func addClusterClassIfMissing(ctx context.Context, template Template, clusterCla
return template, nil
}

clusterClassesTemplate, err := fetchMissingClusterClassTemplates(ctx, clusterClassClient, clusterClient, classes, targetNamespace, listVariablesOnly)
clusterClassesTemplate, err := fetchMissingClusterClassTemplates(ctx, clusterClassClient, clusterClient, classes, listVariablesOnly)
if err != nil {
return nil, err
}
Expand All @@ -62,8 +64,8 @@ func addClusterClassIfMissing(ctx context.Context, template Template, clusterCla
// clusterClassNamesFromTemplate returns the list of ClusterClasses referenced
// by clusters defined in the template. If not clusters are defined in the template
// or if no cluster uses a cluster class it returns an empty list.
func clusterClassNamesFromTemplate(template Template) ([]string, error) {
classes := []string{}
func clusterClassNamesFromTemplate(template Template) ([]types.NamespacedName, error) {
classes := []types.NamespacedName{}

// loop through all the objects and if the object is a cluster
// check and see if cluster.spec.topology.class is defined.
Expand All @@ -80,14 +82,14 @@ func clusterClassNamesFromTemplate(template Template) ([]string, error) {
if cluster.Spec.Topology == nil {
continue
}
classes = append(classes, cluster.GetClassKey().Name)
classes = append(classes, cluster.GetClassKey())
}
return classes, nil
}

// fetchMissingClusterClassTemplates returns a list of templates for ClusterClasses that do not yet exist
// in the cluster. If the cluster is not initialized, all the ClusterClasses are added.
func fetchMissingClusterClassTemplates(ctx context.Context, clusterClassClient repository.ClusterClassClient, clusterClient cluster.Client, classes []string, targetNamespace string, listVariablesOnly bool) (Template, error) {
func fetchMissingClusterClassTemplates(ctx context.Context, clusterClassClient repository.ClusterClassClient, clusterClient cluster.Client, classes []types.NamespacedName, listVariablesOnly bool) (Template, error) {
// first check if the cluster is initialized.
// If it is initialized:
// For every ClusterClass check if it already exists in the cluster.
Expand Down Expand Up @@ -118,7 +120,7 @@ func fetchMissingClusterClassTemplates(ctx context.Context, clusterClassClient r
templates := []repository.Template{}
for _, class := range classes {
if clusterInitialized {
exists, err := clusterClassExists(ctx, c, class, targetNamespace)
exists, err := clusterClassExists(ctx, c, class.Name, class.Namespace)
if err != nil {
return nil, err
}
Expand All @@ -128,7 +130,7 @@ func fetchMissingClusterClassTemplates(ctx context.Context, clusterClassClient r
}
// The cluster is either not initialized or the ClusterClass does not yet exist in the cluster.
// Fetch the cluster class to install.
clusterClassTemplate, err := clusterClassClient.Get(ctx, class, targetNamespace, listVariablesOnly)
clusterClassTemplate, err := clusterClassClient.Get(ctx, class.Name, class.Namespace, listVariablesOnly)
if err != nil {
return nil, errors.Wrapf(err, "failed to get the cluster class template for %q", class)
}
Expand All @@ -142,7 +144,7 @@ func fetchMissingClusterClassTemplates(ctx context.Context, clusterClassClient r
if exists, err := objExists(ctx, c, obj); err != nil {
return nil, err
} else if exists {
return nil, fmt.Errorf("%s(%s) already exists in the cluster", obj.GetName(), obj.GetObjectKind().GroupVersionKind())
return nil, fmt.Errorf("%s(%s) already exists in the cluster", klog.KObj(&obj), obj.GetObjectKind().GroupVersionKind())
}
}
}
Expand Down
46 changes: 41 additions & 5 deletions cmd/clusterctl/client/clusterclass_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ func TestAddClusterClassIfMissing(t *testing.T) {
objs []client.Object
clusterClassTemplateContent []byte
targetNamespace string
clusterClassNamespace string
listVariablesOnly bool
wantClusterClassInTemplate bool
wantError bool
Expand All @@ -114,6 +115,28 @@ func TestAddClusterClassIfMissing(t *testing.T) {
wantClusterClassInTemplate: true,
wantError: false,
},
{
name: "should add the cluster class from a different namespace to the template if cluster is not initialized",
clusterInitialized: false,
objs: []client.Object{},
targetNamespace: "ns1",
clusterClassNamespace: "ns2",
clusterClassTemplateContent: clusterClassYAML("ns2", "dev"),
listVariablesOnly: false,
wantClusterClassInTemplate: true,
wantError: false,
},
{
name: "should add the cluster class form the same explicitly specified namespace to the template if cluster is not initialized",
clusterInitialized: false,
objs: []client.Object{},
targetNamespace: "ns1",
clusterClassNamespace: "ns1",
clusterClassTemplateContent: clusterClassYAML("ns1", "dev"),
listVariablesOnly: false,
wantClusterClassInTemplate: true,
wantError: false,
},
{
name: "should add the cluster class to the template if cluster is initialized and cluster class is not installed",
clusterInitialized: true,
Expand Down Expand Up @@ -189,17 +212,21 @@ func TestAddClusterClassIfMissing(t *testing.T) {

clusterClassClient := repository1.ClusterClasses("v1.0.0")

clusterWithTopology := []byte(fmt.Sprintf("apiVersion: %s\n", clusterv1.GroupVersion.String()) +
clusterWithTopology := fmt.Sprintf("apiVersion: %s\n", clusterv1.GroupVersion.String()) +
"kind: Cluster\n" +
"metadata:\n" +
" name: cluster-dev\n" +
fmt.Sprintf(" namespace: %s\n", tt.targetNamespace) +
"spec:\n" +
" topology:\n" +
" class: dev")
" class: dev"

if tt.clusterClassNamespace != "" {
clusterWithTopology = fmt.Sprintf("%s\n classNamespace: %s", clusterWithTopology, tt.clusterClassNamespace)
}

baseTemplate, err := repository.NewTemplate(repository.TemplateInput{
RawArtifact: clusterWithTopology,
RawArtifact: []byte(clusterWithTopology),
ConfigVariablesClient: test.NewFakeVariableClient(),
Processor: yaml.NewSimpleProcessor(),
TargetNamespace: tt.targetNamespace,
Expand All @@ -210,13 +237,22 @@ func TestAddClusterClassIfMissing(t *testing.T) {
}

g := NewWithT(t)
template, err := addClusterClassIfMissing(ctx, baseTemplate, clusterClassClient, cluster, tt.targetNamespace, tt.listVariablesOnly)
template, err := addClusterClassIfMissing(ctx, baseTemplate, clusterClassClient, cluster, tt.listVariablesOnly)
if tt.wantError {
g.Expect(err).To(HaveOccurred())
} else {
if tt.wantClusterClassInTemplate {
g.Expect(template.Objs()).To(ContainElement(MatchClusterClass("dev", tt.targetNamespace)))
if tt.clusterClassNamespace == tt.targetNamespace {
g.Expect(template.Objs()).To(ContainElement(MatchClusterClass("dev", tt.targetNamespace)))
} else if tt.clusterClassNamespace != "" {
g.Expect(template.Objs()).To(ContainElement(MatchClusterClass("dev", tt.clusterClassNamespace)))
g.Expect(template.Objs()).ToNot(ContainElement(MatchClusterClass("dev", tt.targetNamespace)))
} else {
g.Expect(template.Objs()).To(ContainElement(MatchClusterClass("dev", tt.targetNamespace)))
g.Expect(template.Objs()).ToNot(ContainElement(MatchClusterClass("dev", tt.clusterClassNamespace)))
}
} else {
g.Expect(template.Objs()).NotTo(ContainElement(MatchClusterClass("dev", tt.clusterClassNamespace)))
g.Expect(template.Objs()).NotTo(ContainElement(MatchClusterClass("dev", tt.targetNamespace)))
}
}
Expand Down
2 changes: 1 addition & 1 deletion cmd/clusterctl/client/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,7 @@ func (c *clusterctlClient) getTemplateFromRepository(ctx context.Context, cluste

clusterClassClient := repo.ClusterClasses(version)

template, err = addClusterClassIfMissing(ctx, template, clusterClassClient, cluster, targetNamespace, listVariablesOnly)
template, err = addClusterClassIfMissing(ctx, template, clusterClassClient, cluster, listVariablesOnly)
if err != nil {
return nil, err
}
Expand Down
6 changes: 2 additions & 4 deletions cmd/clusterctl/client/repository/template.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ limitations under the License.
package repository

import (
"fmt"

"github.com/pkg/errors"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/util/sets"
Expand Down Expand Up @@ -144,7 +142,7 @@ func NewTemplate(input TemplateInput) (Template, error) {

// MergeTemplates merges the provided Templates into one Template.
// Notes on the merge operation:
// - The merge operation returns an error if all the templates do not have the same TargetNamespace.
// - The merge operation sets targetNamespace empty if all the templates do not share the same TargetNamespace.
// - The Variables of the resulting template is a union of all Variables in the templates.
// - The default value is picked from the first template that defines it.
// The defaults of the same variable in the subsequent templates will be ignored.
Expand Down Expand Up @@ -173,7 +171,7 @@ func MergeTemplates(templates ...Template) (Template, error) {
}

if merged.targetNamespace != tmpl.TargetNamespace() {
return nil, fmt.Errorf("cannot merge templates with different targetNamespaces")
merged.targetNamespace = ""
}

merged.objs = append(merged.objs, tmpl.Objs()...)
Expand Down
Loading
Loading