diff --git a/controllers/base_request_controller_test.go b/controllers/base_request_controller_test.go index 0e440c69..c2caa9f1 100644 --- a/controllers/base_request_controller_test.go +++ b/controllers/base_request_controller_test.go @@ -11,7 +11,6 @@ import ( . "github.com/onsi/gomega" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" @@ -37,7 +36,7 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() { r = &BaseRequestReconciler{ BaseReconciler: BaseReconciler{ Client: fakeClient, - Scheme: &runtime.Scheme{}, + Scheme: fakeClient.Scheme(), APIReader: fakeClient, ReconcililationInterval: 0, }, @@ -66,7 +65,6 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() { BaseBuilder: builders.BaseBuilder{ Client: fakeClient, Ctx: ctx, - Scheme: &runtime.Scheme{}, APIReader: fakeClient, Request: request, }, @@ -175,7 +173,7 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() { r = &BaseRequestReconciler{ BaseReconciler: BaseReconciler{ Client: fakeClient, - Scheme: &runtime.Scheme{}, + Scheme: fakeClient.Scheme(), APIReader: fakeClient, ReconcililationInterval: 0, }, @@ -216,7 +214,6 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() { builder = &builders.BaseBuilder{ Client: fakeClient, Ctx: ctx, - Scheme: &runtime.Scheme{}, APIReader: fakeClient, Request: request, } @@ -261,7 +258,6 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() { builder = &builders.BaseBuilder{ Client: fakeClient, Ctx: ctx, - Scheme: &runtime.Scheme{}, APIReader: fakeClient, Request: request, } @@ -309,7 +305,6 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() { builder = &builders.BaseBuilder{ Client: fakeClient, Ctx: ctx, - Scheme: &runtime.Scheme{}, APIReader: fakeClient, Request: request, } @@ -365,7 +360,7 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() { r = &BaseRequestReconciler{ BaseReconciler: BaseReconciler{ Client: fakeClient, - Scheme: &runtime.Scheme{}, + Scheme: fakeClient.Scheme(), APIReader: fakeClient, logger: logger, ReconcililationInterval: 0, @@ -396,7 +391,6 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() { builder = &builders.BaseBuilder{ Client: fakeClient, Ctx: ctx, - Scheme: &runtime.Scheme{}, APIReader: fakeClient, Template: template, Request: request, @@ -448,7 +442,6 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() { builder = &builders.BaseBuilder{ Client: fakeClient, Ctx: ctx, - Scheme: &runtime.Scheme{}, APIReader: fakeClient, Template: template, Request: request, @@ -502,7 +495,6 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() { builder = &builders.BaseBuilder{ Client: fakeClient, Ctx: ctx, - Scheme: &runtime.Scheme{}, APIReader: fakeClient, Template: template, Request: request, @@ -556,7 +548,6 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() { builder = &builders.BaseBuilder{ Client: fakeClient, Ctx: ctx, - Scheme: &runtime.Scheme{}, APIReader: fakeClient, Template: template, Request: request, @@ -622,7 +613,6 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() { builder = &builders.BaseBuilder{ Client: fakeClient, Ctx: ctx, - Scheme: &runtime.Scheme{}, APIReader: fakeClient, Template: template, Request: request, @@ -687,7 +677,6 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() { builder = &builders.BaseBuilder{ Client: fakeClient, Ctx: ctx, - Scheme: &runtime.Scheme{}, APIReader: fakeClient, Template: template, Request: request, @@ -740,7 +729,6 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() { builder = &builders.BaseBuilder{ Client: fakeClient, Ctx: ctx, - Scheme: &runtime.Scheme{}, APIReader: fakeClient, Template: template, Request: request, diff --git a/controllers/base_template_controller_test.go b/controllers/base_template_controller_test.go index bcd79183..d54795ce 100644 --- a/controllers/base_template_controller_test.go +++ b/controllers/base_template_controller_test.go @@ -11,7 +11,6 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -72,7 +71,6 @@ var _ = Describe("BaseTemplateReconciler", Ordered, func() { r = &BaseTemplateReconciler{ BaseReconciler: BaseReconciler{ Client: fakeClient, - Scheme: &runtime.Scheme{}, APIReader: fakeClient, logger: logger, ReconcililationInterval: 0, @@ -109,7 +107,6 @@ var _ = Describe("BaseTemplateReconciler", Ordered, func() { builder = &builders.BaseBuilder{ Client: fakeClient, Ctx: ctx, - Scheme: &runtime.Scheme{}, APIReader: fakeClient, Template: template, } @@ -153,7 +150,6 @@ var _ = Describe("BaseTemplateReconciler", Ordered, func() { r = &BaseTemplateReconciler{ BaseReconciler: BaseReconciler{ Client: fakeClient, - Scheme: &runtime.Scheme{}, APIReader: fakeClient, logger: logger, ReconcililationInterval: 0, @@ -164,7 +160,6 @@ var _ = Describe("BaseTemplateReconciler", Ordered, func() { builder = &builders.BaseBuilder{ Client: fakeClient, Ctx: ctx, - Scheme: &runtime.Scheme{}, APIReader: fakeClient, Template: template, } @@ -201,7 +196,7 @@ var _ = Describe("BaseTemplateReconciler", Ordered, func() { r = &BaseTemplateReconciler{ BaseReconciler: BaseReconciler{ Client: fakeClient, - Scheme: &runtime.Scheme{}, + Scheme: fakeClient.Scheme(), APIReader: fakeClient, logger: logger, ReconcililationInterval: 0, @@ -237,7 +232,6 @@ var _ = Describe("BaseTemplateReconciler", Ordered, func() { builder = &builders.BaseBuilder{ Client: fakeClient, Ctx: ctx, - Scheme: &runtime.Scheme{}, APIReader: fakeClient, Template: template, } @@ -287,7 +281,6 @@ var _ = Describe("BaseTemplateReconciler", Ordered, func() { builder = &builders.BaseBuilder{ Client: fakeClient, Ctx: ctx, - Scheme: &runtime.Scheme{}, APIReader: fakeClient, Template: template, } @@ -339,7 +332,6 @@ var _ = Describe("BaseTemplateReconciler", Ordered, func() { builder = &builders.BaseBuilder{ Client: fakeClient, Ctx: ctx, - Scheme: &runtime.Scheme{}, APIReader: fakeClient, Template: template, } @@ -391,7 +383,6 @@ var _ = Describe("BaseTemplateReconciler", Ordered, func() { builder = &builders.BaseBuilder{ Client: fakeClient, Ctx: ctx, - Scheme: &runtime.Scheme{}, APIReader: fakeClient, Template: template, } diff --git a/controllers/builders/base_builder.go b/controllers/builders/base_builder.go index 81751373..6e6876ad 100644 --- a/controllers/builders/base_builder.go +++ b/controllers/builders/base_builder.go @@ -76,7 +76,6 @@ type BaseBuilder struct { Client client.Client Ctx context.Context - Scheme *runtime.Scheme // APIReader should be generated with mgr.GetAPIReader() to create a non-cached client object. This is used // for certain Get() calls where we need to ensure we are getting the latest version from the API, and not a cached @@ -120,7 +119,7 @@ func (b *BaseBuilder) GetCtx() context.Context { // // *runtime.Scheme: A pointer back to the runtime.Scheme from the controller-runtime struct. func (b *BaseBuilder) GetScheme() *runtime.Scheme { - return b.Scheme + return b.Client.Scheme() } // GetTemplate provides an access method to the generic api.OzTemplateResource interface @@ -339,7 +338,7 @@ func (b *BaseBuilder) createAccessRole(podName string) (*rbacv1.Role, error) { // Set the ownerRef for the Deployment // More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/owners-dependents/ - if err := ctrlutil.SetControllerReference(b.Request, role, b.Scheme); err != nil { + if err := ctrlutil.SetControllerReference(b.Request, role, b.GetScheme()); err != nil { return nil, err } @@ -385,7 +384,7 @@ func (b *BaseBuilder) createAccessRoleBinding() (*rbacv1.RoleBinding, error) { // Set the ownerRef for the Deployment // More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/owners-dependents/ - if err := ctrlutil.SetControllerReference(b.Request, rb, b.Scheme); err != nil { + if err := ctrlutil.SetControllerReference(b.Request, rb, b.GetScheme()); err != nil { return nil, err } @@ -413,10 +412,29 @@ func (b *BaseBuilder) createAccessRoleBinding() (*rbacv1.RoleBinding, error) { func (b *BaseBuilder) createPod(podTemplateSpec corev1.PodTemplateSpec) (*corev1.Pod, error) { logger := log.FromContext(b.Ctx) - // Desired pod + // We'll populate this pod object pod := &corev1.Pod{} pod.Name = generateResourceName(b.Request) pod.Namespace = b.Template.GetNamespace() + + // Verify first whether or not a pod already exists with this name. If it + // does, we just return it back. The issue here is that updating a Pod is + // an unusual thing to do once it's alive, and can cause race condition + // issues if you do not do the updates properly. + // + // https://github.com/diranged/oz/issues/27 + err := b.Client.Get(b.Ctx, types.NamespacedName{ + Name: pod.Name, + Namespace: pod.Namespace, + }, pod) + + // If there was no error on this get, then the object already exists in K8S + // and we need to just return that. + if err == nil { + return pod, err + } + + // Finish filling out the desired PodSpec at this point. pod.Spec = *podTemplateSpec.Spec.DeepCopy() pod.ObjectMeta.Annotations = podTemplateSpec.ObjectMeta.Annotations @@ -424,13 +442,9 @@ func (b *BaseBuilder) createPod(podTemplateSpec corev1.PodTemplateSpec) (*corev1 // // pod.ObjectMeta.Labels = podTemplateSpec.Labels - // Generate an emptyPod resource. We use this to define the type, name and namespace of the resource, which - // will be passed into the CreateOrUpdate function. - emptyPod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: pod.Name, Namespace: pod.Namespace}} - // Set the ownerRef for the Deployment // More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/owners-dependents/ - if err := ctrlutil.SetControllerReference(b.Request, pod, b.Scheme); err != nil { + if err := ctrlutil.SetControllerReference(b.Request, pod, b.GetScheme()); err != nil { return nil, err } @@ -439,19 +453,7 @@ func (b *BaseBuilder) createPod(podTemplateSpec corev1.PodTemplateSpec) (*corev1 // In an update event, wec an only update the Annotations and the OwnerReference. Nothing else. logger.V(1).Info(fmt.Sprintf("Creating or Updating Pod %s (ns: %s)", pod.Name, pod.Namespace)) logger.V(1).Info("Pod Json", "json", ObjectToJSON(pod)) - if _, err := ctrlutil.CreateOrUpdate(b.Ctx, b.Client, emptyPod, func() error { - // Mutable fields that we can update on each iteration. - emptyPod.ObjectMeta.Annotations = pod.GetObjectMeta().GetAnnotations() - emptyPod.OwnerReferences = pod.OwnerReferences - - // We can only set the PodSpec once. If the PodSpec is empty, then we pass in the desired - // PodSpec. After that, we ignore all future updates to it. - if len(emptyPod.Spec.Containers) < 1 { - emptyPod.Spec = pod.Spec - } - - return nil - }); err != nil { + if err := b.Client.Create(b.Ctx, pod); err != nil { return nil, err } @@ -466,6 +468,7 @@ func (b *BaseBuilder) createPod(podTemplateSpec corev1.PodTemplateSpec) (*corev1 // // shortUID: A 10-digit long shortened UID func getShortUID(obj client.Object) string { + // TODO: If the UID isn't there, we should generate something random OR throw an error. return string(obj.GetUID())[0:shortUIDLength] } diff --git a/controllers/builders/base_builder_test.go b/controllers/builders/base_builder_test.go new file mode 100644 index 00000000..ca5c33cf --- /dev/null +++ b/controllers/builders/base_builder_test.go @@ -0,0 +1,205 @@ +package builders + +import ( + "context" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + api "github.com/diranged/oz/api/v1alpha1" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" +) + +var _ = Describe("BaseBuilder", Ordered, func() { + Context("Functions()", func() { + var ( + namespace *corev1.Namespace + deployment *appsv1.Deployment + ctx = context.Background() + request *api.PodAccessRequest + template *api.PodAccessTemplate + builder *BaseBuilder + ) + + // NOTE: We use a real k8sClient for these tests beacuse we need to + // verify things like UID generation happening in the backend, as well + // as generation spec updates. + BeforeAll(func() { + By("Creating the Namespace to perform the tests") + namespace = &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: randomString(8), + }, + } + err := k8sClient.Create(ctx, namespace) + Expect(err).To(Not(HaveOccurred())) + }) + + BeforeEach(func() { + // Create a fake deployment target + deployment = &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-dep", + Namespace: namespace.GetName(), + }, + Spec: appsv1.DeploymentSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "testLabel": "testValue", + }, + }, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + api.DefaultContainerAnnotationKey: "contb", + "Foo": "bar", + }, + Labels: map[string]string{ + "testLabel": "testValue", + }, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "conta", + Image: "nginx:latest", + }, + { + Name: "contb", + Image: "nginx:latest", + }, + }, + }, + }, + }, + } + err := k8sClient.Create(ctx, deployment) + Expect(err).To(Not(HaveOccurred())) + + // Create a default PodAccessTemplate. We'll mutate it for specific tests. + template = &api.PodAccessTemplate{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-template", + Namespace: deployment.Namespace, + }, + Spec: api.PodAccessTemplateSpec{ + AccessConfig: api.AccessConfig{ + AllowedGroups: []string{"testGroupA"}, + DefaultDuration: "1h", + MaxDuration: "2h", + }, + ControllerTargetRef: &api.CrossVersionObjectReference{ + APIVersion: "apps/v1", + Kind: "Deployment", + Name: deployment.Name, + }, + ControllerTargetMutationConfig: &api.PodTemplateSpecMutationConfig{}, + }, + } + err = k8sClient.Create(ctx, template) + Expect(err).To(Not(HaveOccurred())) + + // Create a simple PodAccessRequest resource to test the template with + request = &api.PodAccessRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-req", + Namespace: template.Namespace, + }, + Spec: api.PodAccessRequestSpec{ + TemplateName: template.Name, + Duration: "5m", + }, + } + err = k8sClient.Create(ctx, request) + Expect(err).To(Not(HaveOccurred())) + + // Create the PodAccessBuilder finally - fully populated with the + // Request, Template and fake clients. + builder = &BaseBuilder{ + Client: k8sClient, + Ctx: ctx, + APIReader: k8sClient, + Request: request, + Template: template, + } + }) + + AfterEach(func() { + err := k8sClient.Delete(ctx, deployment) + Expect(err).To(Not(HaveOccurred())) + err = k8sClient.Delete(ctx, request) + Expect(err).To(Not(HaveOccurred())) + err = k8sClient.Delete(ctx, template) + Expect(err).To(Not(HaveOccurred())) + }) + + It("Get* Funcs should work", func() { + Expect(builder.GetClient()).To(Equal(k8sClient)) + Expect(builder.GetCtx()).To(Equal(ctx)) + Expect(builder.GetScheme()).To(Equal(k8sClient.Scheme())) + Expect(builder.GetTemplate()).To(Equal(template)) + Expect(builder.GetRequest()).To(Equal(request)) + }) + + It("getShortUID should work", func() { + ret := getShortUID(request) + Expect(len(ret)).To(Equal(8)) + }) + + It("generateResourceName should work", func() { + ret := generateResourceName(request) + Expect(len(ret)).To(Equal(17)) + }) + + It("GetTargetRefResource() should return a valid Client.Object", func() { + ret, err := builder.GetTargetRefResource() + Expect(err).To(Not(HaveOccurred())) + Expect(ret.GetName()).To(Equal("test-dep")) + }) + + It("createPod() should work sanely", func() { + // Get the PodTemplateSpec + pts, err := builder.getPodTemplateFromController() + Expect(err).To(Not(HaveOccurred())) + + // First, we should create the pod and return it. + pod, err := builder.createPod(pts) + Expect(err).To(Not(HaveOccurred())) + + // Store the original resourceVersino + origResourceVersion := pod.ResourceVersion + + // Mutate the pod ourslves. This simulates a third party resource, + // eg, "istio", mutating the pod. + pod.ObjectMeta.SetAnnotations(map[string]string{ + "MyAnnotation": "bar", + "MyOtherAnnotation": "baz", + }) + err = k8sClient.Update(ctx, pod) + Expect(err).To(Not(HaveOccurred())) + + // VERIFY: The resourceVersion should have changed + postAnnotationUpdateVersion := pod.ResourceVersion + Expect(origResourceVersion).To(Not(Equal(postAnnotationUpdateVersion))) + + // Next, re-run the createPod function. We want this function to + // never re-create the Pod object once it's been created, or update + // it. + _, err = builder.createPod(pts) + Expect(err).To(Not(HaveOccurred())) + + // Re-get the pod from the API + err = k8sClient.Get(ctx, types.NamespacedName{ + Name: pod.Name, + Namespace: pod.Namespace, + }, pod) + Expect(err).To(Not(HaveOccurred())) + + // VERIFY: The Pod resourceVersion has not changed + Expect(pod.ObjectMeta.ResourceVersion).To(Equal(postAnnotationUpdateVersion)) + }) + }) +}) diff --git a/controllers/builders/builders_suite_test.go b/controllers/builders/builders_suite_test.go index 2de34e46..a2085775 100644 --- a/controllers/builders/builders_suite_test.go +++ b/controllers/builders/builders_suite_test.go @@ -1,8 +1,11 @@ -package builders_test +package builders import ( + "fmt" + "math/rand" "path/filepath" "testing" + "time" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -63,3 +66,11 @@ var _ = AfterSuite(func() { err := testEnv.Stop() Expect(err).NotTo(HaveOccurred()) }) + +// Utility function for generating a random string for certain tests +func randomString(length int) string { + rand.Seed(time.Now().UnixNano()) + b := make([]byte, length) + rand.Read(b) + return fmt.Sprintf("%x", b)[:length] +} diff --git a/controllers/builders/pod_access_builder_test.go b/controllers/builders/pod_access_builder_test.go index 1bb28cfb..fb7a0770 100644 --- a/controllers/builders/pod_access_builder_test.go +++ b/controllers/builders/pod_access_builder_test.go @@ -11,7 +11,6 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" ) @@ -114,7 +113,6 @@ var _ = Describe("PodAccessBuilder", Ordered, func() { BaseBuilder: BaseBuilder{ Client: fakeClient, Ctx: ctx, - Scheme: &runtime.Scheme{}, APIReader: fakeClient, Request: request, Template: template, diff --git a/controllers/exec_access_request_controller.go b/controllers/exec_access_request_controller.go index a38f0e73..84489a10 100644 --- a/controllers/exec_access_request_controller.go +++ b/controllers/exec_access_request_controller.go @@ -106,7 +106,6 @@ func (r *ExecAccessRequestReconciler) Reconcile( BaseBuilder: builders.BaseBuilder{ Client: r.Client, Ctx: ctx, - Scheme: r.Scheme, APIReader: r.APIReader, Request: resource, Template: tmpl, diff --git a/controllers/exec_access_template_controller.go b/controllers/exec_access_template_controller.go index 8e9f8be6..e55722d9 100644 --- a/controllers/exec_access_template_controller.go +++ b/controllers/exec_access_template_controller.go @@ -74,7 +74,6 @@ func (r *ExecAccessTemplateReconciler) Reconcile( BaseBuilder: builders.BaseBuilder{ Client: r.Client, Ctx: ctx, - Scheme: r.Scheme, Template: resource, }, } diff --git a/controllers/pod_access_request_controller.go b/controllers/pod_access_request_controller.go index e933de1a..6381e3f7 100644 --- a/controllers/pod_access_request_controller.go +++ b/controllers/pod_access_request_controller.go @@ -102,7 +102,6 @@ func (r *PodAccessRequestReconciler) Reconcile( BaseBuilder: builders.BaseBuilder{ Client: r.Client, Ctx: ctx, - Scheme: r.Scheme, APIReader: r.APIReader, Request: resource, Template: tmpl, diff --git a/controllers/pod_access_template_controller.go b/controllers/pod_access_template_controller.go index 5190239e..89f36842 100644 --- a/controllers/pod_access_template_controller.go +++ b/controllers/pod_access_template_controller.go @@ -74,7 +74,6 @@ func (r *PodAccessTemplateReconciler) Reconcile( BaseBuilder: builders.BaseBuilder{ Client: r.Client, Ctx: ctx, - Scheme: r.Scheme, Template: resource, }, }