Repository: tektoncd/operator
Pull request #343 (2021-07-01 15:13:38)

Fix auto prune up
# Changes

<!-- 🎉🎉🎉 Thank you for the PR!!! 🎉🎉🎉 -->

<!-- Describe your changes here- ideally you can get that description straight from
your descriptive commit message(s)! -->

# Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

- [ ] Includes [tests](https://github.com/tektoncd/community/blob/master/standards.md#principles) (if functionality changed/added)
- [ ] Includes [docs](https://github.com/tektoncd/community/blob/master/standards.md#principles) (if user facing)
- [ ] Commit messages follow [commit message best practices](https://github.com/tektoncd/community/blob/master/standards.md#commit-messages)

_See [the contribution guide](https://github.com/tektoncd/operator/blob/master/CONTRIBUTING.md) for more details._

# Release Notes

<!--
Describe any user facing changes here, or delete this block.

Examples of user facing changes:
- API changes
- Bug fixes
- Any changes in behavior
- Changes requiring upgrade notices or deprecation warnings

For pull requests with a release note:

```release-note
Your release note here
```

For pull requests that require additional action from users switching to the new release, include the string "action required" (case insensitive) in the release note:

```release-note
action required: your release note here
```

For pull requests that don't need to be mentioned at release time, use the `/release-note-none` Prow command to add the `release-note-none` label to the PR. You can also write the string "NONE" as a release note in your PR description:

```release-note
NONE
```
-->


tekton-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **savitaashture**
You can assign the PR to them by writing `/assign @savitaashture` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=tektoncd%2Foperator).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/tektoncd/operator/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["savitaashture"]} -->
tekton-robot: The following is the coverage report on the affected files.
Say `/test pull-tekton-operator-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[pkg/reconciler/common/name.go](https://storage.cloud.google.com/tekton-prow/pr-logs/pull/tektoncd_operator/343/pull-tekton-operator-go-coverage/1410617710924009474/artifacts/line-cov.html#file1) | Do not exist | 100.0% | 
[pkg/reconciler/common/prune.go](https://storage.cloud.google.com/tekton-prow/pr-logs/pull/tektoncd_operator/343/pull-tekton-operator-go-coverage/1410617710924009474/artifacts/line-cov.html#file2) | 58.1% | 52.9% | -5.2

tekton-robot: @pradeepitm12: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-tekton-operator-build-tests | 14a0c0efd7eb7388eab99de5682f1f22d866293b | [link](https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_operator/343/pull-tekton-operator-build-tests/1410617710924009472/) | `/test pull-tekton-operator-build-tests`

[Full PR test history](https://tekton-releases.appspot.com/pr/tektoncd_operator/343). [Your PR dashboard](https://prow.tekton.dev/pr/pradeepitm12).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
tekton-robot: @pradeepitm12: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-tekton-operator-build-tests | 14a0c0efd7eb7388eab99de5682f1f22d866293b | [link](https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_operator/343/pull-tekton-operator-build-tests/1410617710924009472/) | `/test pull-tekton-operator-build-tests`

[Full PR test history](https://tekton-releases.appspot.com/pr/tektoncd_operator/343). [Your PR dashboard](https://prow.tekton.dev/pr/pradeepitm12).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
tekton-robot: The following is the coverage report on the affected files.
Say `/test pull-tekton-operator-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[pkg/reconciler/common/name.go](https://storage.cloud.google.com/tekton-prow/pr-logs/pull/tektoncd_operator/343/pull-tekton-operator-go-coverage/1410623121249013762/artifacts/line-cov.html#file1) | Do not exist | 100.0% | 
[pkg/reconciler/common/prune.go](https://storage.cloud.google.com/tekton-prow/pr-logs/pull/tektoncd_operator/343/pull-tekton-operator-go-coverage/1410623121249013762/artifacts/line-cov.html#file2) | 58.1% | 52.9% | -5.2

tekton-robot: @pradeepitm12: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-tekton-operator-unit-tests | b00c349217e3fb30b63cf3b795200137192fd9a6 | [link](https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_operator/343/pull-tekton-operator-unit-tests/1410623121249013760/) | `/test pull-tekton-operator-unit-tests`

[Full PR test history](https://tekton-releases.appspot.com/pr/tektoncd_operator/343). [Your PR dashboard](https://prow.tekton.dev/pr/pradeepitm12).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
tekton-robot: @pradeepitm12: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-tekton-operator-unit-tests | b00c349217e3fb30b63cf3b795200137192fd9a6 | [link](https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_operator/343/pull-tekton-operator-unit-tests/1410623121249013760/) | `/test pull-tekton-operator-unit-tests`

[Full PR test history](https://tekton-releases.appspot.com/pr/tektoncd_operator/343). [Your PR dashboard](https://prow.tekton.dev/pr/pradeepitm12).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
pradeepitm12: unable to access logs 
/test pull-tekton-operator-unit-tests
tekton-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/tektoncd/operator/pull/343#pullrequestreview-697930749" title="Approved">vdemeester</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=tektoncd%2Foperator).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/tektoncd/operator/blob/main/OWNERS)~~ [vdemeester]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
nikhil-thomas: /lgtm
nikhil-thomas: /hold
nikhil-thomas: /remove hold
/lgtm

Patch:
diff --git a/pkg/reconciler/common/common.go b/pkg/reconciler/common/common.go
index 9ba80cd4a..d371569a9 100644
--- a/pkg/reconciler/common/common.go
+++ b/pkg/reconciler/common/common.go
@@ -40,10 +40,11 @@ var (
 )
 
 const (
-	PipelineNotReady = "tekton-pipelines not ready"
-	PipelineNotFound = "tekton-pipelines not installed"
-	TriggerNotReady  = "tekton-triggers not ready"
-	TriggerNotFound  = "tekton-triggers not installed"
+	PipelineNotReady       = "tekton-pipelines not ready"
+	PipelineNotFound       = "tekton-pipelines not installed"
+	TriggerNotReady        = "tekton-triggers not ready"
+	TriggerNotFound        = "tekton-triggers not installed"
+	NamespaceIgnorePattern = "^(openshift|kube)-"
 )
 
 func PipelineReady(informer informer.TektonPipelineInformer) (*v1alpha1.TektonPipeline, error) {
diff --git a/pkg/reconciler/common/name.go b/pkg/reconciler/common/name.go
new file mode 100644
index 000000000..a1d7d8593
--- /dev/null
+++ b/pkg/reconciler/common/name.go
@@ -0,0 +1,72 @@
+/*
+Copyright 2020 The Tekton Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package common
+
+import (
+	"fmt"
+	"regexp"
+
+	utilrand "k8s.io/apimachinery/pkg/util/rand"
+)
+
+// NameGenerator generates names for objects. Some backends may have more information
+// available to guide selection of new names and this interface hides those details.
+type NameGenerator interface {
+	// RestrictLengthWithRandomSuffix generates a valid name from the base name, adding a random suffix to the
+	// the base. If base is valid, the returned name must also be valid. The generator is
+	// responsible for knowing the maximum valid name length.
+	RestrictLengthWithRandomSuffix(base string) string
+
+	// RestrictLength generates a valid name from the name of a step specified in a Task,
+	// shortening it to the maximum valid name length if needed.
+	RestrictLength(base string) string
+}
+
+// simpleNameGenerator generates random names.
+type simpleNameGenerator struct{}
+
+// SimpleNameGenerator is a generator that returns the name plus a random suffix of five alphanumerics
+// when a name is requested. The string is guaranteed to not exceed the length of a standard Kubernetes
+// name (63 characters)
+var SimpleNameGenerator NameGenerator = simpleNameGenerator{}
+
+const (
+	// TODO: make this flexible for non-core resources with alternate naming rules.
+	maxNameLength          = 63
+	randomLength           = 5
+	maxGeneratedNameLength = maxNameLength - randomLength - 1
+)
+
+func (simpleNameGenerator) RestrictLengthWithRandomSuffix(base string) string {
+	if len(base) > maxGeneratedNameLength {
+		base = base[:maxGeneratedNameLength]
+	}
+	return fmt.Sprintf("%s-%s", base, utilrand.String(randomLength))
+}
+
+var alphaNumericRE = regexp.MustCompile(`^[a-zA-Z0-9]+$`)
+
+func (simpleNameGenerator) RestrictLength(base string) string {
+	if len(base) > maxNameLength {
+		base = base[:maxNameLength]
+	}
+
+	for !alphaNumericRE.MatchString(base[len(base)-1:]) {
+		base = base[:len(base)-1]
+	}
+	return base
+}
diff --git a/pkg/reconciler/common/prune.go b/pkg/reconciler/common/prune.go
index acf944989..c4fafca6b 100644
--- a/pkg/reconciler/common/prune.go
+++ b/pkg/reconciler/common/prune.go
@@ -20,6 +20,7 @@ import (
 	"context"
 	"fmt"
 	"os"
+	"regexp"
 	"strings"
 
 	"github.com/tektoncd/operator/pkg/apis/operator/v1alpha1"
@@ -32,8 +33,6 @@ import (
 )
 
 const (
-	openshiftPrefix  = "openshift"
-	kubePrefix       = "kube"
 	tektonSA         = "tekton-pipelines-controller"
 	CronName         = "resource-pruner"
 	JobsTKNImageName = "IMAGE_JOB_PRUNER_TKN"
@@ -42,29 +41,33 @@ const (
 )
 
 func Prune(k kubernetes.Interface, ctx context.Context, tC *v1alpha1.TektonConfig) error {
-	if 0 < len(tC.Spec.Pruner.Resources) {
-		tknImage := os.Getenv(JobsTKNImageName)
-		if tknImage == "" {
-			return fmt.Errorf("%s environment variable not found", JobsTKNImageName)
-		}
-		pru := tC.Spec.Pruner
-		logger := logging.FromContext(ctx)
-		ownerRef := v1.OwnerReference{
-			APIVersion: ownerAPIVer,
-			Kind:       ownerKind,
-			Name:       tC.Name,
-			UID:        tC.ObjectMeta.UID,
-		}
 
-		pruningNamespaces, err := GetPrunableNamespaces(k, ctx)
-		if err != nil {
-			return err
-		}
-		if err := createCronJob(k, ctx, pru, tC.Spec.TargetNamespace, pruningNamespaces, ownerRef, tknImage); err != nil {
-			logger.Error("failed to create cronjob ", err)
+	if len(tC.Spec.Pruner.Resources) == 0 || tC.Spec.Pruner.Schedule == "" {
+		return checkAndDelete(k, ctx, tC.Spec.TargetNamespace)
+	}
+
+	tknImage := os.Getenv(JobsTKNImageName)
+	if tknImage == "" {
+		return fmt.Errorf("%s environment variable not found", JobsTKNImageName)
+	}
+	pru := tC.Spec.Pruner
+	logger := logging.FromContext(ctx)
+	ownerRef := v1.OwnerReference{
+		APIVersion: ownerAPIVer,
+		Kind:       ownerKind,
+		Name:       tC.Name,
+		UID:        tC.ObjectMeta.UID,
+	}
+
+	pruningNamespaces, err := GetPrunableNamespaces(k, ctx)
+	if err != nil {
+		return err
+	}
+	if err := createCronJob(k, ctx, pru, tC.Spec.TargetNamespace, pruningNamespaces, ownerRef, tknImage); err != nil {
+		logger.Error("failed to create cronjob ", err)
 
-		}
 	}
+
 	return nil
 }
 
@@ -75,10 +78,12 @@ func GetPrunableNamespaces(k kubernetes.Interface, ctx context.Context) ([]strin
 	}
 
 	var allNameSpaces []string
+	re := regexp.MustCompile(NamespaceIgnorePattern)
 	for _, ns := range nsList.Items {
-		if !strings.HasPrefix(ns.Name, openshiftPrefix) && !strings.HasPrefix(ns.Name, kubePrefix) {
-			allNameSpaces = append(allNameSpaces, ns.Name)
+		if ignore := re.MatchString(ns.GetName()); ignore {
+			continue
 		}
+		allNameSpaces = append(allNameSpaces, ns.Name)
 	}
 	return allNameSpaces, nil
 }
@@ -132,9 +137,9 @@ func getPruningContainers(resources, namespaces []string, keep int, tknImage str
 	containers := []corev1.Container{}
 	for _, ns := range namespaces {
 		cmdArgs := deleteCommand(resources, keep, ns)
-		cName := "pruner-tkn-" + ns
+		jobName := SimpleNameGenerator.RestrictLengthWithRandomSuffix("pruner-tkn-" + ns)
 		container := corev1.Container{
-			Name:                     cName,
+			Name:                     jobName,
 			Image:                    tknImage,
 			Command:                  []string{"/bin/sh", "-c"},
 			Args:                     cmdArgs,
@@ -154,3 +159,16 @@ func deleteCommand(resources []string, keep int, ns string) []string {
 	}
 	return cmdArgs
 }
+
+func checkAndDelete(k kubernetes.Interface, ctx context.Context, targetNamespace string) error {
+	if _, err := k.BatchV1beta1().CronJobs(targetNamespace).Get(ctx, CronName, v1.GetOptions{}); err != nil {
+		if strings.Contains(err.Error(), "not found") {
+			return nil
+		} else {
+			return err
+		}
+	}
+
+	//if there is no error it means cron is exists, but no prune in config it means delete it
+	return k.BatchV1beta1().CronJobs(targetNamespace).Delete(ctx, CronName, v1.DeleteOptions{})
+}
diff --git a/pkg/reconciler/openshift/tektonconfig/rbac.go b/pkg/reconciler/openshift/tektonconfig/rbac.go
index 0fdded9c4..bcc490e58 100644
--- a/pkg/reconciler/openshift/tektonconfig/rbac.go
+++ b/pkg/reconciler/openshift/tektonconfig/rbac.go
@@ -20,6 +20,8 @@ import (
 	"context"
 	"regexp"
 
+	"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"
@@ -39,7 +41,6 @@ const (
 	pipelineSA               = "pipeline"
 	serviceCABundleConfigMap = "config-service-cabundle"
 	trustedCABundleConfigMap = "config-trusted-cabundle"
-	namespaceIgnorePattern   = "^(openshift|kube)-"
 	clusterInterceptors      = "openshift-pipelines-clusterinterceptors"
 )
 
@@ -66,7 +67,7 @@ func (r *rbac) createResources(ctx context.Context) error {
 		return err
 	}
 
-	re := regexp.MustCompile(namespaceIgnorePattern)
+	re := regexp.MustCompile(common.NamespaceIgnorePattern)
 
 	for _, n := range namespaces.Items {
 


Test patch:
diff --git a/pkg/reconciler/common/name_test.go b/pkg/reconciler/common/name_test.go
new file mode 100644
index 000000000..1f4505fd3
--- /dev/null
+++ b/pkg/reconciler/common/name_test.go
@@ -0,0 +1,71 @@
+/*
+Copyright 2020 The Tekton Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package common
+
+import (
+	utilrand "k8s.io/apimachinery/pkg/util/rand"
+	"strings"
+	"testing"
+)
+
+func TestRestrictLengthWithRandomSuffix(t *testing.T) {
+	for _, c := range []struct {
+		in, want string
+	}{{
+		in:   "hello",
+		want: "hello-9l9zj",
+	}, {
+		in:   strings.Repeat("a", 100),
+		want: strings.Repeat("a", 57) + "-9l9zj",
+	}} {
+		t.Run(c.in, func(t *testing.T) {
+			TestingSeed()
+			got := SimpleNameGenerator.RestrictLengthWithRandomSuffix(c.in)
+			if got != c.want {
+				t.Errorf("RestrictLengthWithRandomSuffix:\n got %q\nwant %q", got, c.want)
+			}
+		})
+	}
+}
+
+func TestRestrictLength(t *testing.T) {
+	for _, c := range []struct {
+		in, want string
+	}{{
+		in:   "hello",
+		want: "hello",
+	}, {
+		in:   strings.Repeat("a", 100),
+		want: strings.Repeat("a", maxNameLength),
+	}, {
+		// Values that don't end with an alphanumeric value are
+		// trimmed until they do.
+		in:   "abcdefg   !@#!$",
+		want: "abcdefg",
+	}} {
+		t.Run(c.in, func(t *testing.T) {
+			got := SimpleNameGenerator.RestrictLength(c.in)
+			if got != c.want {
+				t.Errorf("RestrictLength:\n got %q\nwant %q", got, c.want)
+			}
+		})
+	}
+}
+
+func TestingSeed() {
+	utilrand.Seed(12345)
+}
diff --git a/pkg/reconciler/common/prune_test.go b/pkg/reconciler/common/prune_test.go
index 4c7a2d179..330be3934 100644
--- a/pkg/reconciler/common/prune_test.go
+++ b/pkg/reconciler/common/prune_test.go
@@ -49,7 +49,7 @@ func TestGetPrunableNamespaces(t *testing.T) {
 
 func TestCreateCronJob(t *testing.T) {
 	cronName := "resource-pruner"
-	resource := "pipelinerun"
+	resource := []string{"pipelinerun", "taskrun"}
 	cronJob := &v1beta1.CronJob{
 		TypeMeta: metav1.TypeMeta{
 			Kind:       "CronJob",
@@ -67,13 +67,14 @@ func TestCreateCronJob(t *testing.T) {
 		},
 	}
 
-	nsObj := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "ns-one"}}
+	nsObj1 := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "ns-one"}}
+
 	pru := v1alpha1.Prune{
-		Resources: []string{resource},
+		Resources: resource,
 		Keep:      2,
 		Schedule:  "*/5 * * * *",
 	}
-	client := fake.NewSimpleClientset(cronJob, nsObj)
+	client := fake.NewSimpleClientset(cronJob, nsObj1)
 	nsList := []string{"ns-one"}
 	if err := createCronJob(client, context.TODO(), pru, nsList[0], nsList, metav1.OwnerReference{}, "some-image"); err != nil {
 		t.Error("failed creating cronjob")
@@ -86,8 +87,8 @@ func TestCreateCronJob(t *testing.T) {
 		t.Error("cronjob not matched")
 	}
 	jobName := "pruner-tkn-" + nsList[0]
-	if cron.Spec.JobTemplate.Spec.Template.Spec.Containers[0].Name != jobName {
+	nameAfterRemovingRand := cron.Spec.JobTemplate.Spec.Template.Spec.Containers[0].Name[:len(jobName)]
+	if nameAfterRemovingRand != jobName {
 		t.Error("Job Name not matched")
 	}
-
 }