From edabda53159f41a70af775ce855d8c8d1002d260 Mon Sep 17 00:00:00 2001 From: sm43 Date: Tue, 13 Jul 2021 11:47:34 +0530 Subject: [PATCH 1/3] [OpenShift] Optimize rbac creation in TektonConfig extention This adds a label to namespace after creating rbac resources so that on further reconcilation, reconciler won't loop on it and ignore them. Only new namespaces would be looped for rbac resource creation. Also adds a filter to namespace events to queue in TektonConfig only if namespace doesn't match the regex. Signed-off-by: Shivam Mukhade --- pkg/reconciler/common/name_test.go | 3 +- .../openshift/tektonconfig/controller.go | 8 +- .../openshift/tektonconfig/extension.go | 10 ++- pkg/reconciler/openshift/tektonconfig/rbac.go | 83 +++++++++++++++---- 4 files changed, 82 insertions(+), 22 deletions(-) diff --git a/pkg/reconciler/common/name_test.go b/pkg/reconciler/common/name_test.go index 1f4505fd3c..cbcfde60b3 100644 --- a/pkg/reconciler/common/name_test.go +++ b/pkg/reconciler/common/name_test.go @@ -17,9 +17,10 @@ limitations under the License. package common import ( - utilrand "k8s.io/apimachinery/pkg/util/rand" "strings" "testing" + + utilrand "k8s.io/apimachinery/pkg/util/rand" ) func TestRestrictLengthWithRandomSuffix(t *testing.T) { diff --git a/pkg/reconciler/openshift/tektonconfig/controller.go b/pkg/reconciler/openshift/tektonconfig/controller.go index 04ca1032ba..3783929d09 100644 --- a/pkg/reconciler/openshift/tektonconfig/controller.go +++ b/pkg/reconciler/openshift/tektonconfig/controller.go @@ -25,6 +25,7 @@ import ( namespaceinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/namespace" "knative.dev/pkg/configmap" "knative.dev/pkg/controller" + "knative.dev/pkg/kmeta" ) // NewController initializes the controller and is called by the generated code @@ -41,8 +42,13 @@ func NewController(ctx context.Context, cmw configmap.Watcher) *controller.Impl // This is required because we want to get our TektonConfig reconciler triggered // for already existing and new namespaces, without manual intervention like adding // a label/annotation on namespace to make it manageable by Tekton controller. +// This will also filter the namespaces by regex `^(openshift|kube)-` +// and enqueue only when namespace doesn't match the regex func enqueueCustomName(impl *controller.Impl, name string) func(obj interface{}) { return func(obj interface{}) { - impl.EnqueueKey(types.NamespacedName{Namespace: "", Name: name}) + object, err := kmeta.DeletionHandlingAccessor(obj) + if err == nil && !nsRegex.MatchString(object.GetName()) { + impl.EnqueueKey(types.NamespacedName{Namespace: "", Name: name}) + } } } diff --git a/pkg/reconciler/openshift/tektonconfig/extension.go b/pkg/reconciler/openshift/tektonconfig/extension.go index f03f901126..2ab463e8a7 100644 --- a/pkg/reconciler/openshift/tektonconfig/extension.go +++ b/pkg/reconciler/openshift/tektonconfig/extension.go @@ -88,9 +88,15 @@ func (oe openshiftExtension) PostReconcile(ctx context.Context, comp v1alpha1.Te func (oe openshiftExtension) Finalize(ctx context.Context, comp v1alpha1.TektonComponent) error { configInstance := comp.(*v1alpha1.TektonConfig) if configInstance.Spec.Profile == common.ProfileAll { - return extension.TektonAddonCRDelete(oe.operatorClientSet.OperatorV1alpha1().TektonAddons(), common.AddonResourceName) + if err := extension.TektonAddonCRDelete(oe.operatorClientSet.OperatorV1alpha1().TektonAddons(), common.AddonResourceName); err != nil { + return err + } } - return nil + + r := rbac{ + kubeClientSet: oe.kubeClientSet, + } + return r.cleanUp(ctx) } // configOwnerRef returns owner reference pointing to passed instance diff --git a/pkg/reconciler/openshift/tektonconfig/rbac.go b/pkg/reconciler/openshift/tektonconfig/rbac.go index bcc490e588..1b0008db7d 100644 --- a/pkg/reconciler/openshift/tektonconfig/rbac.go +++ b/pkg/reconciler/openshift/tektonconfig/rbac.go @@ -18,20 +18,19 @@ package tektonconfig import ( "context" + "fmt" "regexp" + mf "github.com/manifestival/manifestival" + clientset "github.com/tektoncd/operator/pkg/client/clientset/versioned" "github.com/tektoncd/operator/pkg/reconciler/common" - corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" "k8s.io/apimachinery/pkg/api/errors" - v1 "k8s.io/client-go/kubernetes/typed/core/v1" - "knative.dev/pkg/logging" - - mf "github.com/manifestival/manifestival" - clientset "github.com/tektoncd/operator/pkg/client/clientset/versioned" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" + v1 "k8s.io/client-go/kubernetes/typed/core/v1" + "knative.dev/pkg/logging" ) const ( @@ -42,8 +41,12 @@ const ( serviceCABundleConfigMap = "config-service-cabundle" trustedCABundleConfigMap = "config-trusted-cabundle" clusterInterceptors = "openshift-pipelines-clusterinterceptors" + namespaceRbacLabel = "openshift-pipelines.tekton.dev/namespace-ready" ) +// Namespace Regex to ignore the namespace for creating rbac resources. +var nsRegex = regexp.MustCompile(common.NamespaceIgnorePattern) + type rbac struct { kubeClientSet kubernetes.Interface operatorClientSet clientset.Interface @@ -51,30 +54,65 @@ type rbac struct { ownerRef metav1.OwnerReference } -func (r *rbac) createResources(ctx context.Context) error { +func (r *rbac) cleanUp(ctx context.Context) error { - logger := logging.FromContext(ctx) - - // Maintaining a separate cluster role for the scc declaration. - // to assist us in managing this the scc association in a - // granular way. - if err := r.ensurePipelinesSCClusterRole(ctx); err != nil { + // fetch the list of all namespaces which have label + // `openshift-pipelines.tekton.dev/namespace-ready: true` + namespaces, err := r.kubeClientSet.CoreV1().Namespaces().List(ctx, metav1.ListOptions{ + LabelSelector: fmt.Sprintf("%s = true", namespaceRbacLabel), + }) + if err != nil { return err } + // loop on namespaces and remove label if exist + for _, n := range namespaces.Items { + labels := n.GetLabels() + delete(labels, namespaceRbacLabel) + n.SetLabels(labels) + if _, err := r.kubeClientSet.CoreV1().Namespaces().Update(ctx, &n, metav1.UpdateOptions{}); err != nil { + return err + } + } + return nil +} + +func (r *rbac) createResources(ctx context.Context) error { + + logger := logging.FromContext(ctx) - namespaces, err := r.kubeClientSet.CoreV1().Namespaces().List(ctx, metav1.ListOptions{}) + // fetch the list of all namespaces which doesn't have label + // `openshift-pipelines.tekton.dev/namespace-ready: true` + namespaces, err := r.kubeClientSet.CoreV1().Namespaces().List(ctx, metav1.ListOptions{ + LabelSelector: fmt.Sprintf("%s != true", namespaceRbacLabel), + }) if err != nil { return err } - re := regexp.MustCompile(common.NamespaceIgnorePattern) + // list of namespaces rbac resources need to be created + var rbacNamespaces []corev1.Namespace + // filter namespaces: + // ignore ns with name passing regex `^(openshift|kube)-` for _, n := range namespaces.Items { - - if ignore := re.MatchString(n.GetName()); ignore { - logger.Info("IGNORES Namespace: ", n.GetName()) + if ignore := nsRegex.MatchString(n.GetName()); ignore { continue } + rbacNamespaces = append(rbacNamespaces, n) + } + + if len(rbacNamespaces) == 0 { + return nil + } + + // Maintaining a separate cluster role for the scc declaration. + // to assist us in managing this the scc association in a + // granular way. + if err := r.ensurePipelinesSCClusterRole(ctx); err != nil { + return err + } + + for _, n := range rbacNamespaces { logger.Infow("Inject CA bundle configmap in ", "Namespace", n.GetName()) if err := r.ensureCABundles(ctx, &n); err != nil { @@ -98,6 +136,15 @@ func (r *rbac) createResources(ctx context.Context) error { if err := r.ensureClusterRoleBindings(ctx, sa); err != nil { return err } + + // Add `openshift-pipelines.tekton.dev/namespace-ready` label to namespace + // so that rbac won't loop on it again + nsLabels := n.GetLabels() + nsLabels[namespaceRbacLabel] = "true" + n.SetLabels(nsLabels) + if _, err := r.kubeClientSet.CoreV1().Namespaces().Update(ctx, &n, metav1.UpdateOptions{}); err != nil { + return err + } } return nil From 9447249f4871c52e2ea03b11ad3300a047d151fc Mon Sep 17 00:00:00 2001 From: sm43 Date: Tue, 13 Jul 2021 12:10:48 +0530 Subject: [PATCH 2/3] [OpenShift] Fixes Addon test Signed-off-by: Shivam Mukhade --- test/resources/tektonconfigs.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/resources/tektonconfigs.go b/test/resources/tektonconfigs.go index bfc1863bde..69d9bc0a4c 100644 --- a/test/resources/tektonconfigs.go +++ b/test/resources/tektonconfigs.go @@ -26,6 +26,7 @@ import ( mfc "github.com/manifestival/client-go-client" mf "github.com/manifestival/manifestival" + "github.com/tektoncd/operator/pkg/reconciler/common" "knative.dev/pkg/test/logging" @@ -63,9 +64,22 @@ func EnsureTektonConfigExists(kubeClientSet *kubernetes.Clientset, clients confi Name: names.TektonConfig, }, Spec: v1alpha1.TektonConfigSpec{ + Profile: common.ProfileAll, CommonSpec: v1alpha1.CommonSpec{ TargetNamespace: cm.Data["DEFAULT_TARGET_NAMESPACE"], }, + Addon: v1alpha1.Addon{ + Params: []v1alpha1.Param{ + { + Name: "pipelineTemplates", + Value: "true", + }, + { + Name: "clusterTasks", + Value: "true", + }, + }, + }, }, } return clients.Create(context.TODO(), tcCR, metav1.CreateOptions{}) From 7cb768016ed66f1bb30ba6ebe8e92bab9e99eb77 Mon Sep 17 00:00:00 2001 From: sm43 Date: Tue, 13 Jul 2021 12:34:53 +0530 Subject: [PATCH 3/3] Adding sleep after installing operator in e2e test After installation, pod comes in running state soon but TektonConfig creation takes sometime, which fails the test. Adding a sleep for a 30 seconds, this can be replaced with probes and we should check ready status of pod. Signed-off-by: Shivam Mukhade --- test/e2e-tests.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/e2e-tests.sh b/test/e2e-tests.sh index 70f8a0a8a0..7058249be8 100755 --- a/test/e2e-tests.sh +++ b/test/e2e-tests.sh @@ -32,6 +32,9 @@ failed=0 header "Setting up environment" install_operator_resources +echo "Wait for TektonConfig creation" +sleep 30 + header "Running Go e2e tests" go_test_e2e -timeout=20m ./test/e2e/common ${KUBECONFIG_PARAM} || failed=1 go_test_e2e -timeout=20m ./test/e2e/${TARGET} ${KUBECONFIG_PARAM} || failed=1