Address review comments

Signed-off-by: Avi Sharma <avi.08.sh@gmail.com>

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
 ---

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 <filename>`. 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 <filename>` 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

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)

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 {

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)
+}