Skip to content

Commit

Permalink
Address PR review feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
shafeeqes committed Nov 15, 2022
1 parent 6611faf commit f449262
Show file tree
Hide file tree
Showing 6 changed files with 91 additions and 80 deletions.
3 changes: 1 addition & 2 deletions docs/concepts/gardenlet.md
Original file line number Diff line number Diff line change
Expand Up @@ -235,8 +235,7 @@ For example, if you set it to `[production]` then only the `BackupEntry`s for `S

The `Bastion` controller reconciles those `operations.gardener.cloud/v1alpha1.Bastion` resources whose `.spec.seedName` value is equal to the name of a `Seed` the respective gardenlet is responsible for.

The controller creates an `extensions.gardener.cloud/v1alpha1.Bastion` resource in the seed cluster with the same name as `operations.gardener.cloud/v1alpha1.Bastion` and waits until the responsible extension controller reconciled it (see [this](../extensions/bastion.md) for more details).
The status is populated in the `.status.conditions` and `.status.ingress` fields.
The controller creates an `extensions.gardener.cloud/v1alpha1.Bastion` resource in the seed cluster in the shoot namespace with the same name as `operations.gardener.cloud/v1alpha1.Bastion`. Then it waits until the responsible extension controller has reconciled it (see [this](../extensions/bastion.md) for more details).The status is populated in the `.status.conditions` and `.status.ingress` fields.

During the deletion of `operations.gardener.cloud/v1alpha1.Bastion` resources, the controller first sets the `Ready` condition to `False` and then deletes the `extensions.gardener.cloud/v1alpha1.Bastion` resource in the seed cluster.
Once this resource is gone, the finalizer of the `operations.gardener.cloud/v1alpha1.Bastion` resource is released, so it finally disappears from the system.
Expand Down
26 changes: 4 additions & 22 deletions pkg/gardenlet/controller/bastion/add.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,12 @@ package bastion

import (
"context"
"strings"

gardencorev1beta1 "github.com/gardener/gardener/pkg/apis/core/v1beta1"
extensionsv1alpha1 "github.com/gardener/gardener/pkg/apis/extensions/v1alpha1"
operationsv1alpha1 "github.com/gardener/gardener/pkg/apis/operations/v1alpha1"
"github.com/gardener/gardener/pkg/controllerutils/mapper"
predicateutils "github.com/gardener/gardener/pkg/controllerutils/predicate"
kutil "github.com/gardener/gardener/pkg/utils/kubernetes"
"github.com/gardener/gardener/pkg/extensions"

"github.com/go-logr/logr"
"k8s.io/apimachinery/pkg/types"
Expand All @@ -39,7 +37,7 @@ import (
)

// ControllerName is the name of this controller.
const ControllerName = "bastion-controller"
const ControllerName = "bastion"

// AddToManager adds Reconciler to the given manager.
func (r *Reconciler) AddToManager(mgr manager.Manager, gardenCluster, seedCluster cluster.Cluster) error {
Expand Down Expand Up @@ -83,26 +81,10 @@ func (r *Reconciler) AddToManager(mgr manager.Manager, gardenCluster, seedCluste

// MapExtensionsBastionToOperationsBastion is a mapper.MapFunc for mapping extensions Bastion in the seed cluster to operations Bastion in the project namespace.
func (r *Reconciler) MapExtensionsBastionToOperationsBastion(ctx context.Context, log logr.Logger, reader client.Reader, obj client.Object) []reconcile.Request {
extensionsBastion, ok := obj.(*extensionsv1alpha1.Bastion)
if !ok {
return nil
}

projectNamespaceName, err := GetProjectNameFromTechincalId(ctx, reader, extensionsBastion.Namespace)
shoot, err := extensions.GetShoot(ctx, r.SeedClient, obj.GetNamespace())
if err != nil {
return nil
}

return []reconcile.Request{{NamespacedName: types.NamespacedName{Namespace: projectNamespaceName, Name: extensionsBastion.Name}}}
}

// GetProjectNameFromTechincalId returns Shoot resource name from its UID.
func GetProjectNameFromTechincalId(ctx context.Context, reader client.Reader, shootTechnicalID string) (string, error) {
tokens := strings.Split(shootTechnicalID, "--")
projectName := tokens[len(tokens)-2]
project := &gardencorev1beta1.Project{}
if err := reader.Get(ctx, kutil.Key(projectName), project); err != nil {
return "", err
}
return *project.Spec.Namespace, nil
return []reconcile.Request{{NamespacedName: types.NamespacedName{Name: obj.GetName(), Namespace: shoot.Namespace}}}
}
67 changes: 28 additions & 39 deletions pkg/gardenlet/controller/bastion/add_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ import (
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
"k8s.io/utils/pointer"

"sigs.k8s.io/controller-runtime/pkg/client"
fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake"
Expand All @@ -31,7 +31,6 @@ import (
gardencorev1beta1 "github.com/gardener/gardener/pkg/apis/core/v1beta1"
extensionsv1alpha1 "github.com/gardener/gardener/pkg/apis/extensions/v1alpha1"
operationsv1alpha1 "github.com/gardener/gardener/pkg/apis/operations/v1alpha1"
"github.com/gardener/gardener/pkg/client/kubernetes"
)

const (
Expand All @@ -48,20 +47,35 @@ var _ = Describe("Add", func() {

operationsBastion *operationsv1alpha1.Bastion
extensionsBastion *extensionsv1alpha1.Bastion
project *gardencorev1beta1.Project
cluster *extensionsv1alpha1.Cluster

shootTechnicalID = "shoot--" + projectName + "--shootName"
projectNamespace = "garden" + projectName
)

BeforeEach(func() {
reconciler = &Reconciler{}
fakeClient = fakeclient.NewClientBuilder().WithScheme(kubernetes.GardenScheme).Build()
testScheme := runtime.NewScheme()
extensionsv1alpha1.AddToScheme(testScheme)
fakeClient = fakeclient.NewClientBuilder().WithScheme(testScheme).Build()

project = &gardencorev1beta1.Project{
ObjectMeta: metav1.ObjectMeta{Name: projectName},
Spec: gardencorev1beta1.ProjectSpec{
Namespace: pointer.String("test-" + projectName),
cluster = &extensionsv1alpha1.Cluster{
ObjectMeta: metav1.ObjectMeta{
Name: shootTechnicalID,
},
Spec: extensionsv1alpha1.ClusterSpec{
Shoot: runtime.RawExtension{
Object: &gardencorev1beta1.Shoot{
ObjectMeta: metav1.ObjectMeta{
Name: "shoot",
Namespace: projectNamespace,
},
},
},
},
}

reconciler = &Reconciler{
SeedClient: fakeClient,
}
})

Expand All @@ -72,7 +86,7 @@ var _ = Describe("Add", func() {
operationsBastion = &operationsv1alpha1.Bastion{
ObjectMeta: metav1.ObjectMeta{
Name: bastionName,
Namespace: *project.Spec.Namespace,
Namespace: projectNamespace,
},
}

Expand All @@ -84,40 +98,15 @@ var _ = Describe("Add", func() {
}
})

It("should do nothing if object is not extensions Bastion", func() {
Expect(reconciler.MapExtensionsBastionToOperationsBastion(ctx, log, fakeClient, &operationsv1alpha1.Bastion{})).To(BeEmpty())
})

It("should do nothing if operations Bastion does not exist", func() {
Expect(reconciler.MapExtensionsBastionToOperationsBastion(ctx, log, fakeClient, extensionsBastion)).To(BeEmpty())
})

It("should do nothing if project does not exist", func() {
Expect(fakeClient.Create(ctx, operationsBastion)).To(Succeed())
Expect(reconciler.MapExtensionsBastionToOperationsBastion(ctx, log, fakeClient, extensionsBastion)).To(BeEmpty())
})

It("should map the extensions Bastion to operations Bastion", func() {
Expect(fakeClient.Create(ctx, project)).To(Succeed())
Expect(fakeClient.Create(ctx, operationsBastion)).To(Succeed())
Expect(fakeClient.Create(ctx, cluster)).To(Succeed())
Expect(reconciler.MapExtensionsBastionToOperationsBastion(ctx, log, fakeClient, extensionsBastion)).To(ConsistOf(
reconcile.Request{NamespacedName: types.NamespacedName{Namespace: *project.Spec.Namespace, Name: operationsBastion.Name}},
reconcile.Request{NamespacedName: types.NamespacedName{Namespace: operationsBastion.Namespace, Name: operationsBastion.Name}},
))
})
})

Describe("#GetProjectNameFromTechincalId", func() {
It("should return empty string if project does not exist", func() {
projectName, err := GetProjectNameFromTechincalId(ctx, fakeClient, shootTechnicalID)
Expect(projectName).To(Equal(""))
Expect(err).NotTo(Succeed())
})

It("should return project name if project exist", func() {
Expect(fakeClient.Create(ctx, project)).To(Succeed())
projectName, err := GetProjectNameFromTechincalId(ctx, fakeClient, shootTechnicalID)
Expect(projectName).To(Equal(*project.Spec.Namespace))
Expect(err).To(BeNil())
It("should return nil if the cluster is not found", func() {
Expect(reconciler.MapExtensionsBastionToOperationsBastion(ctx, log, fakeClient, extensionsBastion)).To(BeNil())
})
})
})
9 changes: 2 additions & 7 deletions pkg/gardenlet/controller/bastion/reconciler.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import (
gardencorev1alpha1helper "github.com/gardener/gardener/pkg/apis/core/v1alpha1/helper"
gardencorev1beta1 "github.com/gardener/gardener/pkg/apis/core/v1beta1"
v1beta1constants "github.com/gardener/gardener/pkg/apis/core/v1beta1/constants"
gardencorev1beta1helper "github.com/gardener/gardener/pkg/apis/core/v1beta1/helper"
v1beta1helper "github.com/gardener/gardener/pkg/apis/core/v1beta1/helper"
extensionsv1alpha1 "github.com/gardener/gardener/pkg/apis/extensions/v1alpha1"
operationsv1alpha1 "github.com/gardener/gardener/pkg/apis/operations/v1alpha1"
Expand Down Expand Up @@ -166,8 +165,8 @@ func (r *Reconciler) reconcileBastion(
}

if lastObservedError != nil {
message := fmt.Sprintf("Error while waiting for %s to become ready", extensionKey(extensionsv1alpha1.BastionResource, extBastion.Namespace, extBastion.Name))
err := gardencorev1beta1helper.NewErrorWithCodes(fmt.Errorf("%s: %w", message, lastObservedError), gardencorev1beta1helper.DeprecatedDetermineErrorCodes(lastObservedError)...)
message := fmt.Sprintf("Error while waiting for %s %s/%s to become ready", extensionsv1alpha1.BastionResource, extBastion.Namespace, extBastion.Name)
err := v1beta1helper.NewErrorWithCodes(fmt.Errorf("%s: %w", message, lastObservedError), v1beta1helper.DeprecatedDetermineErrorCodes(lastObservedError)...)

if patchErr := patchReadyCondition(ctx, r.GardenClient, bastion, gardencorev1alpha1.ConditionFalse, "FailedReconciling", err.Error()); patchErr != nil {
log.Error(patchErr, "Failed patching ready condition")
Expand Down Expand Up @@ -281,7 +280,3 @@ echo "gardener ALL=(ALL) NOPASSWD:ALL" >/etc/sudoers.d/99-gardener-user

return []byte(userData)
}

func extensionKey(kind, namespace, name string) string {
return fmt.Sprintf("%s %s/%s", kind, namespace, name)
}
5 changes: 3 additions & 2 deletions test/integration/gardenlet/bastion/bastion_suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ var _ = BeforeSuite(func() {
testEnv = &gardenerenvtest.GardenerTestEnvironment{
Environment: &envtest.Environment{
CRDInstallOptions: envtest.CRDInstallOptions{
Paths: []string{filepath.Join("..", "..", "..", "..", "example", "seed-crds", "10-crd-extensions.gardener.cloud_bastions.yaml")},
Paths: []string{filepath.Join("..", "..", "..", "..", "example", "seed-crds", "10-crd-extensions.gardener.cloud_bastions.yaml"),
filepath.Join("..", "..", "..", "..", "example", "seed-crds", "10-crd-extensions.gardener.cloud_clusters.yaml")},
},
ErrorIfCRDPathMissing: true,
},
Expand All @@ -106,7 +107,7 @@ var _ = BeforeSuite(func() {
Expect(testEnv.Stop()).To(Succeed())
})

By("creating testClient")
By("creating test client")
testClient, err = client.New(restConfig, client.Options{Scheme: kubernetes.GardenScheme})
Expect(err).NotTo(HaveOccurred())

Expand Down
61 changes: 53 additions & 8 deletions test/integration/gardenlet/bastion/bastion_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
corev1 "k8s.io/api/core/v1"
networkingv1 "k8s.io/api/networking/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/utils/pointer"
"sigs.k8s.io/controller-runtime/pkg/client"
)
Expand All @@ -41,6 +42,7 @@ var _ = Describe("Bastion controller tests", func() {
shoot *gardencorev1beta1.Shoot
operationsBastion *operationsv1alpha1.Bastion
extensionsBastion *extensionsv1alpha1.Bastion
cluster *extensionsv1alpha1.Cluster
seedNamespace *corev1.Namespace

reconcileExtensionsBastion = func() {
Expand Down Expand Up @@ -169,27 +171,67 @@ var _ = Describe("Bastion controller tests", func() {
return mgrClient.Get(ctx, client.ObjectKeyFromObject(shoot), &gardencorev1beta1.Shoot{})
}).Should(Succeed())

By("Patch the Shoot status with TechincalID")
By("Patch the Shoot status with technical ID")
patch := client.MergeFrom(shoot.DeepCopy())
shootTechincalId := shootpkg.ComputeTechnicalID(project.Name, shoot)
shoot.Status.TechnicalID = shootTechincalId
technicalID := shootpkg.ComputeTechnicalID(project.Name, shoot)
shoot.Status.TechnicalID = technicalID
Expect(testClient.Status().Patch(ctx, shoot, patch)).To(Succeed())

By("creating seed namespace for test")
seedNamespace = &corev1.Namespace{
ObjectMeta: metav1.ObjectMeta{
Name: shootTechincalId,
Name: technicalID,
},
}

Expect(testClient.Create(ctx, seedNamespace)).To(Succeed())
log.Info("Created seed Namespace for test", "namespaceName", seedNamespace.Name)
log.Info("Created shoot namespace in seed for test", "namespaceName", seedNamespace.Name)

DeferCleanup(func() {
By("deleting seed namespace")
By("deleting shoot namespace in seed")
Expect(testClient.Delete(ctx, seedNamespace)).To(Or(Succeed(), BeNotFoundError()))
})

By("create Cluster extension resource")
cluster = &extensionsv1alpha1.Cluster{
ObjectMeta: metav1.ObjectMeta{
Name: technicalID,
},
Spec: extensionsv1alpha1.ClusterSpec{
Shoot: runtime.RawExtension{
Object: &gardencorev1beta1.Shoot{
TypeMeta: metav1.TypeMeta{
APIVersion: "core.gardener.cloud/v1beta1",
Kind: "Shoot",
},
ObjectMeta: metav1.ObjectMeta{
Name: shoot.Name,
Namespace: shoot.Namespace,
},
},
},
Seed: runtime.RawExtension{
Object: seed,
},
CloudProfile: runtime.RawExtension{
Object: &gardencorev1alpha1.CloudProfile{},
},
},
}

Expect(testClient.Create(ctx, cluster)).To(Succeed())
log.Info("Created Cluster for test", "cluster", client.ObjectKeyFromObject(cluster))

DeferCleanup(func() {
By("Delete Cluster")
Expect(testClient.Delete(ctx, cluster)).To(Or(Succeed(), BeNotFoundError()))
})

By("ensure mgrClient has observed cluster creation")
Eventually(func() error {
return mgrClient.Get(ctx, client.ObjectKeyFromObject(cluster), cluster)
}).Should(Succeed())

By("Create Bastion")
operationsBastion.Spec.ShootRef = corev1.LocalObjectReference{Name: shoot.Name}
operationsBastion.Spec.SeedName = &seed.Name
Expand All @@ -216,15 +258,18 @@ var _ = Describe("Bastion controller tests", func() {
By("ensure finalizer is added to Bastion")
Eventually(func(g Gomega) {
g.Expect(testClient.Get(ctx, client.ObjectKeyFromObject(operationsBastion), operationsBastion)).To(Succeed())
g.Expect(operationsBastion.Finalizers).To(ContainElement("gardener"))
g.Expect(operationsBastion.Finalizers).To(ConsistOf("gardener"))
}).Should(Succeed())
})

Context("reconciliation", func() {
It("should create or patch the Bastion in the Seed cluster", func() {
Eventually(func(g Gomega) {
g.Expect(testClient.Get(ctx, client.ObjectKeyFromObject(extensionsBastion), extensionsBastion)).To(Succeed())
g.Expect(extensionsBastion.GetAnnotations()).To(And(HaveKeyWithValue(v1beta1constants.GardenerOperation, v1beta1constants.GardenerOperationReconcile), HaveKey(v1beta1constants.GardenerTimestamp)))
g.Expect(extensionsBastion.GetAnnotations()).To(And(
HaveKeyWithValue(v1beta1constants.GardenerOperation, v1beta1constants.GardenerOperationReconcile),
HaveKey(v1beta1constants.GardenerTimestamp),
))
g.Expect(extensionsBastion.Spec.Type).To(Equal(*operationsBastion.Spec.ProviderType))
g.Expect(extensionsBastion.Spec.UserData).To(Equal(createUserData(operationsBastion)))
}).Should(Succeed())
Expand Down

0 comments on commit f449262

Please sign in to comment.