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.

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

[WIP] ✨ Add clusterctl changes for cross-ns CC ref #11395

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
1 change: 0 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -617,7 +617,6 @@ generate-e2e-templates-main: $(KUSTOMIZE)
$(KUSTOMIZE) build $(DOCKER_TEMPLATES)/main/cluster-template-topology --load-restrictor LoadRestrictionsNone > $(DOCKER_TEMPLATES)/main/cluster-template-topology.yaml
$(KUSTOMIZE) build $(DOCKER_TEMPLATES)/main/cluster-template-ignition --load-restrictor LoadRestrictionsNone > $(DOCKER_TEMPLATES)/main/cluster-template-ignition.yaml
$(KUSTOMIZE) build $(DOCKER_TEMPLATES)/main/clusterclass-quick-start-kcp-only --load-restrictor LoadRestrictionsNone > $(DOCKER_TEMPLATES)/main/clusterclass-quick-start-kcp-only.yaml

$(KUSTOMIZE) build $(INMEMORY_TEMPLATES)/main/cluster-template --load-restrictor LoadRestrictionsNone > $(INMEMORY_TEMPLATES)/main/cluster-template.yaml

.PHONY: generate-metrics-config
Expand Down
14 changes: 13 additions & 1 deletion api/v1beta1/cluster_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package v1beta1

import (
"cmp"
"fmt"
"net"
"strings"
Expand Down Expand Up @@ -509,6 +510,15 @@ type Topology struct {
// The name of the ClusterClass object to create the topology.
Class string `json:"class"`

// The namespace of the ClusterClass object to create the topology.
// Empty namespace assumes the namespace of the cluster object.
// Class namespace changes are not supported by the rebase procedure.
// +optional
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=253
// +kubebuilder:validation:Pattern="^[a-z0-9](?:[-a-z0-9]*[a-z0-9])?(?:\\.[a-z0-9](?:[-a-z0-9]*[a-z0-9])?)*$"
ClassNamespace string `json:"classNamespace,omitempty"`

// The Kubernetes version of the cluster.
Version string `json:"version"`

Expand Down Expand Up @@ -1037,7 +1047,9 @@ func (c *Cluster) GetClassKey() types.NamespacedName {
if c.Spec.Topology == nil {
return types.NamespacedName{}
}
return types.NamespacedName{Namespace: c.GetNamespace(), Name: c.Spec.Topology.Class}

namespace := cmp.Or(c.Spec.Topology.ClassNamespace, c.Namespace)
return types.NamespacedName{Namespace: namespace, Name: c.Spec.Topology.Class}
}

// GetConditions returns the set of conditions for this object.
Expand Down
23 changes: 23 additions & 0 deletions api/v1beta1/index/cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ import (
const (
// ClusterClassNameField is used by the Cluster controller to index Clusters by ClusterClass name.
ClusterClassNameField = "spec.topology.class"
// ClusterClassNamespaceField is used by the Cluster controller to index Clusters by ClusterClass namespace.
ClusterClassNamespaceField = "spec.topology.classNamespace"
)

// ByClusterClassName adds the cluster class name index to the
Expand All @@ -44,6 +46,18 @@ func ByClusterClassName(ctx context.Context, mgr ctrl.Manager) error {
return nil
}

// ByClusterClassNamespace adds the cluster class namespace index to the
// managers cache.
func ByClusterClassNamespace(ctx context.Context, mgr ctrl.Manager) error {
if err := mgr.GetCache().IndexField(ctx, &clusterv1.Cluster{},
ClusterClassNamespaceField,
ClusterByClusterClassClassNamespace,
); err != nil {
return errors.Wrap(err, "error setting index field")
}
return nil
}

// ClusterByClusterClassClassName contains the logic to index Clusters by ClusterClass name.
func ClusterByClusterClassClassName(o client.Object) []string {
cluster, ok := o.(*clusterv1.Cluster)
Expand All @@ -55,3 +69,12 @@ func ClusterByClusterClassClassName(o client.Object) []string {
}
return nil
}

// ClusterByClusterClassClassNamespace contains the logic to index Clusters by ClusterClass namespace.
func ClusterByClusterClassClassNamespace(o client.Object) []string {
cluster, ok := o.(*clusterv1.Cluster)
if !ok {
panic(fmt.Sprintf("Expected Cluster but got a %T", o))
}
return []string{cluster.GetClassKey().Namespace}
}
4 changes: 4 additions & 0 deletions api/v1beta1/index/index.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ func AddDefaultIndexes(ctx context.Context, mgr ctrl.Manager) error {
if err := ByClusterClassName(ctx, mgr); err != nil {
return err
}

if err := ByClusterClassNamespace(ctx, mgr); err != nil {
return err
}
}

if feature.Gates.Enabled(feature.MachinePool) {
Expand Down
7 changes: 7 additions & 0 deletions api/v1beta1/zz_generated.openapi.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 11 additions & 9 deletions cmd/clusterctl/client/clusterclass.go
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)
Copy link
Member

Choose a reason for hiding this comment

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

Just to check if I'm getting this right.
As of today, when we are deploying objects, clusterctl allows to override the namespace set into templates (use case: the user wants to deploy to namespace xyz instead of namespace original).

With this PR we are changing this behaviour, and always preserving the namespace from the template when we are adding the cluster class to the template being deployed (cc preserve: namespace original).

If this is correct, and if I'm not wrong, this change might break users that are deploying a Cluster with ClassNamespace empty, and are expecting that also the CC gets deployed in the target namespace xyz along side with the cluster.

Copy link
Member Author

Choose a reason for hiding this comment

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

It shouldn’t break these users, since regular process of overriding the namespace of the templated resources is still performed. This new field was never a part of this logic, and the SoT here is the template initially.

This change avoids problems in discovering the existence of the CC in the specified namespace, allowing to template only a Cluster resource.

There is no support in clusterclt to distinguish that some of the resources should go into A namespace, and some other into B, so this multi-namespace setup is intended to happen in a multiple passes.

  1. ClusterClass and referenced templates
  2. A cluster resource in a different namespace
  3. repeat 2 for any namespace without failures on clusterctl side, or any existing template modifications.

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
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",
Copy link
Member

Choose a reason for hiding this comment

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

Can we have a similar test when both namespaces are the same, and one where both are empty?

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
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: 0 additions & 6 deletions cmd/clusterctl/client/repository/template.go
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 @@ -172,10 +170,6 @@ func MergeTemplates(templates ...Template) (Template, error) {
}
}

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

merged.objs = append(merged.objs, tmpl.Objs()...)
}

Expand Down
7 changes: 7 additions & 0 deletions cmd/clusterctl/cmd/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,13 @@ func printVariablesOutput(template client.Template, options client.GetClusterTem
} else if val, ok := os.LookupEnv("KUBERNETES_VERSION"); ok {
variableMap[name] = ptr.To(val)
}
case "CLUSTER_CLASS_NAMESPACE":
// Namespace name from the cmd flags or from the kubeconfig is used instead of template default.
if val, ok := os.LookupEnv("CLUSTER_CLASS_NAMESPACE"); ok {
variableMap[name] = ptr.To(val)
} else {
variableMap[name] = ptr.To("")
}
}

if variableMap[name] != nil {
Expand Down
9 changes: 9 additions & 0 deletions config/crd/bases/cluster.x-k8s.io_clusters.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading