From 518b5d4d9ed83bc9155b1110b1db20ddcd38cf4a Mon Sep 17 00:00:00 2001 From: Avi Sharma Date: Tue, 20 Jun 2023 17:01:28 +0530 Subject: [PATCH] Address review comments Signed-off-by: Avi Sharma --- docs/runtime-core/readiness-framework.md | 4 +- .../guide-with-examples.md | 55 ++++++++- readiness/controller/main.go | 16 +-- .../pkg/conditions/resourceexistence.go | 17 +-- .../pkg/conditions/resourceexistence_test.go | 116 ++++++++++++++---- .../pkg/conditions/testdata/rbac.yaml | 33 +++++ .../readinessprovider_controller.go | 3 + .../pkg/readinessprovider/suite_test.go | 56 +-------- 8 files changed, 195 insertions(+), 105 deletions(-) create mode 100644 readiness/controller/pkg/conditions/testdata/rbac.yaml diff --git a/docs/runtime-core/readiness-framework.md b/docs/runtime-core/readiness-framework.md index e12419a9aa..3e758b604e 100644 --- a/docs/runtime-core/readiness-framework.md +++ b/docs/runtime-core/readiness-framework.md @@ -20,7 +20,7 @@ Readiness checks can be of 2 types: ### Example -The following manifest defines a Readiness resource with 2 checks. +The following manifest defines a Readiness resource with two checks. These checks are required to be satisfied by atleast one **active** ReadinessProvider (See [ReadinessProvider Example](#example-1)), so that `my-org-baseline` can be evaluated to ready. ```yaml @@ -45,7 +45,7 @@ The ReadinessProvider API allows users to define a set of conditions. These cond ### Example -The above manifest creates 2 ReadinessProvider resources which will evaluate if `cert-manager` and `kapp-controller` are available in the cluster. The providers also specify `checkRefs` which will aid in making the `my-org-baseline` Readiness resource **_ready_**. +The below manifest creates two ReadinessProvider resources which will evaluate if `cert-manager` and `kapp-controller` are available in the cluster. The providers also specify `checkRefs` which will aid in making the `my-org-baseline` Readiness resource **_ready_**. ```yaml --- diff --git a/docs/runtime-core/readiness-framework/guide-with-examples.md b/docs/runtime-core/readiness-framework/guide-with-examples.md index e5809dd1aa..2c19e3ca4a 100644 --- a/docs/runtime-core/readiness-framework/guide-with-examples.md +++ b/docs/runtime-core/readiness-framework/guide-with-examples.md @@ -35,6 +35,48 @@ Let's assume we have the following checks approved by the organization org1 2. com.org1.k8s.secret-management 3. com.org1.k8s.certificate-management +### Service Account + +For the readiness providers to be able to query various reources, a service account which has required role bindings can be provided in the spec. +A sample yaml is defined below, which grants premissions to read CRDs. For creating these resources, run `kubectl apply -f `. We'll be referring to the details of the created service account in the following sections. + +```yaml +--- +apiVersion: v1 +kind: ServiceAccount +metadata: + name: crd-read-sa + namespace: default +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: crd-read-role + namespace: default +rules: + - apiGroups: + - "apiextensions.k8s.io" + resources: + - customresourcedefinitions + verbs: + - get +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + name: crd-read-rolebinding + namespace: default +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: crd-read-role +subjects: + - kind: ServiceAccount + name: crd-read-sa + namespace: default + +``` + ### Readiness Providers Now, let's defined three readiness providers, one for each of the above checks. @@ -73,7 +115,10 @@ spec: resourceExistenceCondition: apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition - name: packagerepositories.packaging.carvel.dev + name: packagerepositories.packaging.carvel.dev + serviceAccount: + name: crd-read-sa + namespace: default ``` Save the above manifest in a file and run `kubectl apply -f ` to deploy it on the Kubernetes cluster where the readiness framework is already installed. @@ -126,6 +171,9 @@ spec: apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition name: secrettemplates.secretgen.carvel.dev + serviceAccount: + name: crd-read-sa + namespace: default ``` #### Certificate Management Provider @@ -134,7 +182,7 @@ The manifest for the certificate management provider is given as follows. Instal ```yaml apiVersion: core.tanzu.vmware.com/v1alpha2 -kind: ReadinessProvider # CapabilityProvider +kind: ReadinessProvider metadata: name: cert-manager spec: @@ -171,6 +219,9 @@ spec: apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition name: orders.acme.cert-manager.io + serviceAccount: + name: crd-read-sa + namespace: default ``` ### Readiness Definition diff --git a/readiness/controller/main.go b/readiness/controller/main.go index e724682151..91b420d331 100644 --- a/readiness/controller/main.go +++ b/readiness/controller/main.go @@ -12,8 +12,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" - "k8s.io/client-go/discovery" - "k8s.io/client-go/dynamic" "k8s.io/client-go/kubernetes" clientgoscheme "k8s.io/client-go/kubernetes/scheme" cliflag "k8s.io/component-base/cli/flag" @@ -23,6 +21,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log/zap" corev1alpha2 "github.com/vmware-tanzu/tanzu-framework/apis/core/v1alpha2" + capabilitiesDiscovery "github.com/vmware-tanzu/tanzu-framework/capabilities/client/pkg/discovery" "github.com/vmware-tanzu/tanzu-framework/readiness/controller/pkg/conditions" readinesscontroller "github.com/vmware-tanzu/tanzu-framework/readiness/controller/pkg/readiness" readinessprovidercontroller "github.com/vmware-tanzu/tanzu-framework/readiness/controller/pkg/readinessprovider" @@ -111,15 +110,9 @@ func main() { k8sClientset := kubernetes.NewForConfigOrDie(restConfig) - dynamicClient, err := dynamic.NewForConfig(restConfig) + clusterQueryClient, err := capabilitiesDiscovery.NewClusterQueryClientForConfig(restConfig) if err != nil { - setupLog.Error(err, "unable to create dynamic client") - os.Exit(1) - } - - discoveryClient, err := discovery.NewDiscoveryClientForConfig(restConfig) - if err != nil { - setupLog.Error(err, "unable to create discovery client") + setupLog.Error(err, "unable to create cluster query client") os.Exit(1) } @@ -153,8 +146,9 @@ func main() { Clientset: k8sClientset, Log: ctrl.Log.WithName("controllers").WithName("ReadinessProvider").WithValues("apigroup", "core"), Scheme: mgr.GetScheme(), - ResourceExistenceCondition: conditions.NewResourceExistenceConditionFunc(dynamicClient, discoveryClient), + ResourceExistenceCondition: conditions.NewResourceExistenceConditionFunc(), RestConfig: restConfig, + DefaultQueryClient: clusterQueryClient, }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "ReadinessProvider") os.Exit(1) diff --git a/readiness/controller/pkg/conditions/resourceexistence.go b/readiness/controller/pkg/conditions/resourceexistence.go index 9ee4a661ba..5786b52249 100644 --- a/readiness/controller/pkg/conditions/resourceexistence.go +++ b/readiness/controller/pkg/conditions/resourceexistence.go @@ -7,32 +7,19 @@ import ( "context" corev1 "k8s.io/api/core/v1" - "k8s.io/client-go/discovery" - "k8s.io/client-go/dynamic" corev1alpha2 "github.com/vmware-tanzu/tanzu-framework/apis/core/v1alpha2" capabilitiesDiscovery "github.com/vmware-tanzu/tanzu-framework/capabilities/client/pkg/discovery" ) // NewResourceExistenceConditionFunc returns a function for evaluating evaluate a ResourceExistenceCondition -func NewResourceExistenceConditionFunc(dynamicClient *dynamic.DynamicClient, discoveryClient *discovery.DiscoveryClient) func(context.Context, *capabilitiesDiscovery.ClusterQueryClient, *corev1alpha2.ResourceExistenceCondition, string) (corev1alpha2.ReadinessConditionState, string) { - return func(ctx context.Context, client *capabilitiesDiscovery.ClusterQueryClient, c *corev1alpha2.ResourceExistenceCondition, conditionName string) (corev1alpha2.ReadinessConditionState, string) { +func NewResourceExistenceConditionFunc() func(context.Context, *capabilitiesDiscovery.ClusterQueryClient, *corev1alpha2.ResourceExistenceCondition, string) (corev1alpha2.ReadinessConditionState, string) { + return func(ctx context.Context, queryClient *capabilitiesDiscovery.ClusterQueryClient, c *corev1alpha2.ResourceExistenceCondition, conditionName string) (corev1alpha2.ReadinessConditionState, string) { if c == nil { return corev1alpha2.ConditionFailureState, "resourceExistenceCondition is not defined" } var err error - var queryClient *capabilitiesDiscovery.ClusterQueryClient - - // Create client using default config if no ClusterQueryClient provided - if client != nil { - queryClient = client - } else { - if queryClient, err = capabilitiesDiscovery.NewClusterQueryClient(dynamicClient, discoveryClient); err != nil { - return corev1alpha2.ConditionFailureState, err.Error() - } - } - var resourceToFind corev1.ObjectReference if c.Namespace == nil { diff --git a/readiness/controller/pkg/conditions/resourceexistence_test.go b/readiness/controller/pkg/conditions/resourceexistence_test.go index 91b622d299..8d2464853d 100644 --- a/readiness/controller/pkg/conditions/resourceexistence_test.go +++ b/readiness/controller/pkg/conditions/resourceexistence_test.go @@ -5,6 +5,7 @@ package conditions import ( "context" + "os" "path/filepath" "testing" @@ -13,8 +14,8 @@ import ( v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/discovery" "k8s.io/client-go/dynamic" + "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" ctrl "sigs.k8s.io/controller-runtime" @@ -25,16 +26,21 @@ import ( corev1alpha2 "github.com/vmware-tanzu/tanzu-framework/apis/core/v1alpha2" capabilitiesDiscovery "github.com/vmware-tanzu/tanzu-framework/capabilities/client/pkg/discovery" + "github.com/vmware-tanzu/tanzu-framework/util/config" + testutil "github.com/vmware-tanzu/tanzu-framework/util/test" ) -var cfg *rest.Config -var k8sClient client.Client -var testEnv *envtest.Environment -var ctx context.Context -var cancel context.CancelFunc -var dynamicClient *dynamic.DynamicClient -var discoveryClient *discovery.DiscoveryClient -var queryClient *capabilitiesDiscovery.ClusterQueryClient +var ( + cfg *rest.Config + k8sClient client.Client + testEnv *envtest.Environment + ctx context.Context + cancel context.CancelFunc + queryClient *capabilitiesDiscovery.ClusterQueryClient + k8sClientset *kubernetes.Clientset +) + +const defaultNamespace = "default" func TestResourceExistenceCondition(t *testing.T) { RegisterFailHandler(Fail) @@ -72,14 +78,24 @@ var _ = BeforeSuite(func() { Expect(err).NotTo(HaveOccurred()) Expect(k8sClient).NotTo(BeNil()) - dynamicClient, err = dynamic.NewForConfig(cfg) + queryClient, err = capabilitiesDiscovery.NewClusterQueryClientForConfig(cfg) Expect(err).NotTo(HaveOccurred()) + Expect(queryClient).NotTo(BeNil()) - discoveryClient, err = discovery.NewDiscoveryClientForConfig(cfg) + // k8sClientset is package-scoped + k8sClientset, err = kubernetes.NewForConfig(cfg) Expect(err).NotTo(HaveOccurred()) + Expect(k8sClientset).NotTo(BeNil()) + + manifestBytes, err := os.ReadFile("testdata/rbac.yaml") + Expect(err).ToNot(HaveOccurred()) - queryClient, err = capabilitiesDiscovery.NewClusterQueryClient(dynamicClient, discoveryClient) + dynamicClient, err := dynamic.NewForConfig(cfg) Expect(err).NotTo(HaveOccurred()) + Expect(dynamicClient).NotTo(BeNil()) + + err = testutil.CreateResourcesFromManifest(manifestBytes, cfg, dynamicClient) + Expect(err).ToNot(HaveOccurred()) go func() { defer GinkgoRecover() @@ -101,7 +117,7 @@ var _ = Describe("Readiness controller", func() { newPod := v1.Pod{ ObjectMeta: metav1.ObjectMeta{ Name: "testpod", - Namespace: "default", + Namespace: defaultNamespace, }, Spec: v1.PodSpec{ Containers: []v1.Container{ @@ -116,7 +132,7 @@ var _ = Describe("Readiness controller", func() { err := k8sClient.Create(context.TODO(), &newPod) Expect(err).To(BeNil()) - state, msg := NewResourceExistenceConditionFunc(dynamicClient, discoveryClient)(context.TODO(), nil, &corev1alpha2.ResourceExistenceCondition{ + state, msg := NewResourceExistenceConditionFunc()(context.TODO(), queryClient, &corev1alpha2.ResourceExistenceCondition{ APIVersion: "v1", Kind: "Pod", Namespace: &newPod.Namespace, @@ -129,11 +145,11 @@ var _ = Describe("Readiness controller", func() { }) It("should fail when querying a non-existing namespaced resource", func() { - state, msg := NewResourceExistenceConditionFunc(dynamicClient, discoveryClient)(context.TODO(), nil, &corev1alpha2.ResourceExistenceCondition{ + state, msg := NewResourceExistenceConditionFunc()(context.TODO(), queryClient, &corev1alpha2.ResourceExistenceCondition{ APIVersion: "v1", Kind: "Pod", Namespace: func() *string { - n := "default" + n := defaultNamespace return &n }(), Name: "somename", @@ -145,7 +161,7 @@ var _ = Describe("Readiness controller", func() { }) It("should succeed when querying an existing cluster scoped resource", func() { - state, msg := NewResourceExistenceConditionFunc(dynamicClient, discoveryClient)(context.TODO(), nil, &corev1alpha2.ResourceExistenceCondition{ + state, msg := NewResourceExistenceConditionFunc()(context.TODO(), queryClient, &corev1alpha2.ResourceExistenceCondition{ APIVersion: "apiextensions.k8s.io/v1", Kind: "CustomResourceDefinition", Name: "readinesses.core.tanzu.vmware.com", @@ -157,7 +173,7 @@ var _ = Describe("Readiness controller", func() { }) It("should fail when querying a non-existing cluster scoped resource", func() { - state, msg := NewResourceExistenceConditionFunc(dynamicClient, discoveryClient)(context.TODO(), nil, &corev1alpha2.ResourceExistenceCondition{ + state, msg := NewResourceExistenceConditionFunc()(context.TODO(), queryClient, &corev1alpha2.ResourceExistenceCondition{ APIVersion: "apiextensions.k8s.io/v1", Kind: "CustomResourceDefinition", Name: "readinesses.config.tanzu.vmware.com", @@ -169,21 +185,71 @@ var _ = Describe("Readiness controller", func() { }) It("should fail when resourceExistenceCondition is undefined", func() { - state, msg := NewResourceExistenceConditionFunc(dynamicClient, discoveryClient)(context.TODO(), nil, nil, "undefinedCondition") + state, msg := NewResourceExistenceConditionFunc()(context.TODO(), queryClient, nil, "undefinedCondition") Expect(state).To(Equal(corev1alpha2.ConditionFailureState)) Expect(msg).To(Equal("resourceExistenceCondition is not defined")) }) - It("should succeed when custom query client is provided", func() { - state, msg := NewResourceExistenceConditionFunc(dynamicClient, discoveryClient)(context.TODO(), queryClient, &corev1alpha2.ResourceExistenceCondition{ - APIVersion: "apiextensions.k8s.io/v1", - Kind: "CustomResourceDefinition", - Name: "readinesses.core.tanzu.vmware.com", + It("should succeed when query client has required permissions", func() { + customQueryClient, err := getCustomQueryClient() + Expect(err).To(BeNil()) + Expect(customQueryClient).ToNot(BeNil()) + + newPod := v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: defaultNamespace, + }, + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Name: "test-container", + Image: "test:tag", + }, + }, + }, + } + + err = k8sClient.Create(context.TODO(), &newPod) + Expect(err).To(BeNil()) + + state, msg := NewResourceExistenceConditionFunc()(context.TODO(), customQueryClient, &corev1alpha2.ResourceExistenceCondition{ + APIVersion: "v1", + Kind: "Pod", + Namespace: &newPod.Namespace, + Name: newPod.Name, }, - "crdCondition") + "podCondition") Expect(state).To(Equal(corev1alpha2.ConditionSuccessState)) Expect(msg).To(Equal("resource found")) }) + + It("should fail when query client is missing required permissions", func() { + customQueryClient, err := getCustomQueryClient() + Expect(err).To(BeNil()) + Expect(customQueryClient).ToNot(BeNil()) + + state, _ := NewResourceExistenceConditionFunc()(context.TODO(), customQueryClient, &corev1alpha2.ResourceExistenceCondition{ + APIVersion: "apps/v1", + Kind: "Deployment", + Name: "test-deploy", + Namespace: func() *string { + n := defaultNamespace + return &n + }(), + }, + "deploymentCondition") + + Expect(state).To(Equal(corev1alpha2.ConditionFailureState)) + }) }) + +func getCustomQueryClient() (*capabilitiesDiscovery.ClusterQueryClient, error) { + customCfg, err := config.GetConfigForServiceAccount(ctx, k8sClientset, cfg, defaultNamespace, "pod-sa") + if err != nil { + return nil, err + } + return capabilitiesDiscovery.NewClusterQueryClientForConfig(customCfg) +} diff --git a/readiness/controller/pkg/conditions/testdata/rbac.yaml b/readiness/controller/pkg/conditions/testdata/rbac.yaml new file mode 100644 index 0000000000..6485fb9217 --- /dev/null +++ b/readiness/controller/pkg/conditions/testdata/rbac.yaml @@ -0,0 +1,33 @@ +--- +apiVersion: v1 +kind: ServiceAccount +metadata: + name: pod-sa + namespace: default +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: pod-read-role + namespace: default +rules: + - apiGroups: + - "" + resources: + - pods + verbs: + - get +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + name: pod-read-rolebinding + namespace: default +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: pod-read-role +subjects: + - kind: ServiceAccount + name: pod-sa + namespace: default diff --git a/readiness/controller/pkg/readinessprovider/readinessprovider_controller.go b/readiness/controller/pkg/readinessprovider/readinessprovider_controller.go index 9d35971ff5..24addef406 100644 --- a/readiness/controller/pkg/readinessprovider/readinessprovider_controller.go +++ b/readiness/controller/pkg/readinessprovider/readinessprovider_controller.go @@ -33,6 +33,7 @@ type ReadinessProviderReconciler struct { Scheme *runtime.Scheme ResourceExistenceCondition func(context.Context, *capabilitiesDiscovery.ClusterQueryClient, *corev1alpha2.ResourceExistenceCondition, string) (corev1alpha2.ReadinessConditionState, string) RestConfig *rest.Config + DefaultQueryClient *capabilitiesDiscovery.ClusterQueryClient } //+kubebuilder:rbac:groups=core.tanzu.vmware.com,resources=readinessproviders,verbs=get;list;watch;create;update;patch;delete @@ -72,6 +73,8 @@ func (r *ReadinessProviderReconciler) Reconcile(ctx context.Context, req ctrl.Re if err != nil { return ctrl.Result{}, errors.Errorf("unable to create ClusterQueryClient: %s", err.Error()) } + } else { + clusterQueryClient = r.DefaultQueryClient } // Evaluate provider conditions diff --git a/readiness/controller/pkg/readinessprovider/suite_test.go b/readiness/controller/pkg/readinessprovider/suite_test.go index ad03fb678f..05973d0461 100644 --- a/readiness/controller/pkg/readinessprovider/suite_test.go +++ b/readiness/controller/pkg/readinessprovider/suite_test.go @@ -144,6 +144,10 @@ var _ = BeforeSuite(func() { Expect(err).ToNot(HaveOccurred()) Expect(dynamicClient).ToNot(BeNil()) + queryClient, err := capabilitiesDiscovery.NewClusterQueryClientForConfig(k8sManager.GetConfig()) + Expect(err).ToNot(HaveOccurred()) + Expect(queryClient).ToNot(BeNil()) + err = (&ReadinessProviderReconciler{ Client: k8sManager.GetClient(), Clientset: kubernetes.NewForConfigOrDie(k8sManager.GetConfig()), @@ -162,7 +166,8 @@ var _ = BeforeSuite(func() { return corev1alpha2.ConditionSuccessState, "TestSuccess" }, - RestConfig: k8sManager.GetConfig(), + RestConfig: k8sManager.GetConfig(), + DefaultQueryClient: queryClient, }).SetupWithManager(k8sManager) Expect(err).ToNot(HaveOccurred()) @@ -411,54 +416,6 @@ var _ = Describe("Readiness Provider controller", func() { Expect(err).NotTo(BeNil()) }) - It("should succeed when service account has required roles", func() { - newPod := corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "testpod", - Namespace: "default", - }, - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Name: "test-container", - Image: "test:tag", - }, - }, - }, - } - - err := k8sClient.Create(context.TODO(), &newPod) - Expect(err).To(BeNil()) - - readinessProvider := getTestReadinessProvider() - readinessProvider.Spec.ServiceAccount = &corev1alpha2.ServiceAccountSource{ - Name: "pod-sa", - Namespace: "default", - } - readinessProvider.Spec.Conditions = append(readinessProvider.Spec.Conditions, corev1alpha2.ReadinessProviderCondition{ - Name: "pod-exists", - ResourceExistenceCondition: &corev1alpha2.ResourceExistenceCondition{ - APIVersion: "v1", - Kind: "Pod", - Name: newPod.Name, - Namespace: &newPod.Namespace, - }, - }) - err = k8sClient.Create(ctx, readinessProvider) - Expect(err).To(BeNil()) - - var status corev1alpha2.ReadinessProviderStatus - Eventually(func() bool { - err := k8sClient.Get(ctx, types.NamespacedName{Name: readinessProvider.Name}, readinessProvider) - status = readinessProvider.Status - log.Print(status) - return err == nil && - readinessProvider.Status.State == corev1alpha2.ProviderSuccessState - }, timeout, interval).Should(BeTrue()) - Expect(len(status.Conditions)).To(Equal(1)) - Expect(status.Conditions[0].State).To(Equal(corev1alpha2.ConditionSuccessState)) - }) - It("should fail when service account token cannot be retrieved", func() { sa := corev1.ServiceAccount{ ObjectMeta: metav1.ObjectMeta{ @@ -488,7 +445,6 @@ var _ = Describe("Readiness Provider controller", func() { Eventually(func() bool { err := k8sClient.Get(ctx, types.NamespacedName{Name: readinessProvider.Name}, readinessProvider) status = readinessProvider.Status - log.Print(status) return err == nil && readinessProvider.Status.State == corev1alpha2.ProviderFailureState }, timeout, interval).Should(BeTrue())