refactor: cleanup tekton operands #641

codingben · 2023-08-01T09:15:36Z

What this PR does / why we need it:

Cleanup Tekton operands to improve code quality
and readability. Also set specific RBAC groups
to core instead of the wildcard.

Which issue(s) this PR fixes:

Fixes #548

Release note:

None

openshift-ci · 2023-08-01T09:15:40Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

0xFelix · 2023-08-01T09:51:29Z

internal/operands/tekton-tasks/reconcile.go

@@ -307,3 +276,21 @@ func getTektonTasksNamespace(request *common.Request) string {
 	}
 	return request.Instance.Namespace
 }
+
+func filterResources[PT interface {


Appreciate use of generics! Can this be simplified to just use the metav1.Object interface? It is working right now but looks very complicated.

Yes, I agree that's very complicated, also there is no Go docs for it, but it works. I tried to do it with just metav1.Object, I couldn't make it work. @akrejcir suggested to do that way yesterday #532 (comment).

@akrejcir's yesterday's explanation for it:

If you would define simply [T metav1.Object] it means that parameter T is a type that implements interface metav1.Object . Which means it is a pointer to a struct, for example T = *v1.Pod. When you have : []T it is in fact []*v1.Pod. So when a method accepts []T parameter, it does not accept []v1.Pod to it. The PT interface {*T ; meta1.Object } is a trick. It means that type PT satisfies an anonymous typeset defined directly in the function signature. This typeset interface { *T; metav1.Object } means that it is a pointer to T and implements interface metav1.Object. Which means that for example PT = *v1.Pod . It would be nice if golang compiler was smarter and we didn't need PT. :)

Better explanation: https://stackoverflow.com/a/71444968

IMO we should not merge this just because it works.

I see the pain in changing the slices to hold pointers instead. Is there another maybe less 'generic' but still prettier solution?

What about casting to keep this simple?

func filterResources[T any](items []T, trimSuffix bool) []T { var results []T for _, obj := range items { metaObj, _ := any(obj).(metav1.ObjectMeta) name := metaObj.GetName() if trimSuffix { name = strings.TrimSuffix(name, "-task") } if _, ok := AllowedTasks[name]; ok { results = append(results, obj) } } return results }

Ah sweet, thanks @lyarwood. Let's use it then.

Hi @lyarwood, this function doesn't work, unit tests are failing because of it. Currently only @akrejcir's function works now, I'll keep it to proceed with other PR comments because it takes too much time.

internal/operands/tekton-tasks/reconcile.go

0xFelix · 2023-08-01T11:43:29Z

internal/operands/tekton-tasks/reconcile.go

@@ -307,3 +276,21 @@ func getTektonTasksNamespace(request *common.Request) string {
 	}
 	return request.Instance.Namespace
 }
+
+func filterResources[PT interface {


IMO we should not merge this just because it works.

I see the pain in changing the slices to hold pointers instead. Is there another maybe less 'generic' but still prettier solution?

0xFelix · 2023-08-01T16:24:38Z

internal/operands/tekton-tasks/reconcile.go

+func filterResources[T any](items []T, trimSuffix bool) []T {
+	var results []T
+	for _, item := range items {
+		object, _ := any(item).(metav1.ObjectMeta)


This is still an unchecked cast. Please return early with empty list at least if that happens.

I've reverted back to @akrejcir's function at the moment, because this new suggested function is failing unit tests.

codingben · 2023-08-02T10:05:57Z

IMO we should not merge this just because it works.
I see the pain in changing the slices to hold pointers instead. Is there another maybe less 'generic' but still prettier solution?

@0xFelix I didn't see another way, I spent too much time on it. It seems like this generics feature is kind of new, and there is not enough documentation IMO, all examples based on simple things like "int/float/string" whatever.

Simpler solution would be not to use generics at all:

func New(bundle *tektonbundle.Bundle) operands.Operand {
	return &tektonTasks{
		tasks:           filterTasks(bundle.Tasks),
		serviceAccounts: filterServiceAccounts(bundle.ServiceAccounts),
		roleBindings:    filterRoleBindings(bundle.RoleBindings),
		clusterRoles:    filterClusterRoles(bundle.ClusterRoles),
	}
}

And just use separated functions for it.

0xFelix · 2023-08-02T13:54:12Z

I still appreciate the use of generics, but I'm afraid this is making it more complicated than it needs to be. Golang can be repetitive, but at least it is easy to grok what happens?

jcanocan · 2023-08-03T08:59:35Z

I'm not particularly against the use of generics, thanks for experimenting generics and pushing forward the use of them. However, I agree with @0xFelix that it may make the code a bit more hard to read. Nevertheless, I guess that's something that a good function documentation can solve.
Apologizes in advance for my ignorance, but we need to use workarounds to make it work (are we? Right?), I'm not sure if it's a good idea. I didn't have the chance to study in deep generics in go, so if we consider this is the intended way of go to use generics, please go ahead.
Also, can we do the same with the DeepCopy functions ?

jcanocan · 2023-08-07T15:28:19Z

internal/operands/tekton-pipelines/reconcile.go

@@ -17,7 +17,7 @@ import (
 )

 // +kubebuilder:rbac:groups=tekton.dev,resources=pipelines,verbs=list;watch;create;update;delete
-// +kubebuilder:rbac:groups=*,resources=configmaps,verbs=list;watch;create;delete
+// +kubebuilder:rbac:groups=core,resources=configmaps,verbs=list;watch;create;delete


Just out of curiosity, is there any difference between using * or core? AFAIK, it will refer to the same apiGroup, isn't it?

Yes, * would grant permission to any API group in your cluster. Imagine that some custom API group, e.g. kubevirt.io would contain a custom ConfigMap resource that may be accessible (e.g. some ConfigMap that contains secrets).

I see that it's also possible to just specify an empty API group, as "" (groups="") and it would also refer to the core objects. Anyways, by using core we embrace least privilege principle and readability.

From the beginning I think that It was specified as * to speed up the development process (also some verbs) - because to have a specific value requires some effort.

I love the explanation of that topic here [1]. Very good explanation and examples.

[1] https://learnk8s.io/rbac-kubernetes#modelling-access-to-resources

codingben · 2023-08-08T14:45:34Z

@akrejcir All comments in issue #548 were resolved. Please review if you have some free time.

akrejcir · 2023-08-08T14:55:05Z

internal/tekton-bundle/tekton-bundle.go

-	bundle := &Bundle{}
-	for _, file := range files {
-		decoder := yaml.NewYAMLOrJSONDecoder(bytes.NewReader(file), 1024)
+		defer file.Close()


An unintuitive behavior of golang is that defer statements are executed when returning from a function, and not when a scope ends with }, like for example a destructor in C++ would be executed. When you call defer in a loop, all the defers will be remembered and only called on return. In this case, all the files will be closed only on return.

Ack, thanks for the explanation. Good to know.

Done. I'll leave that discussion open so others can learn as well.

ksimon1 · 2023-09-25T12:48:46Z

@codingben can you please rebase this PR?

codingben · 2023-10-01T12:42:30Z

@codingben can you please rebase this PR?

@ksimon1 Ack, doing it.

github-actions · 2023-10-02T10:04:59Z

/cc sradco

codingben · 2023-10-02T10:12:03Z

/cc sradco

I think it's wrong comment, I don't think we need more reviews in this PR.

codingben · 2023-10-02T12:39:15Z

Hi @lyarwood @0xFelix, I see a few tests failed because of:

�[0mCommon Instance Types �[38;5;243moperand �[38;5;9m�[1m[It] should reconcile from URL when provided�[0m
�[38;5;243m/go/src/github.com/kubevirt/ssp-operator/tests/common_instancetypes_test.go:39�[0m

  �[38;5;9m[FAILED] Expected success, but got an error:
      <*errors.StatusError | 0xc00012e640>: 
      virtualmachineclusterpreferences.instancetype.kubevirt.io "centos.9.stream.desktop" not found
      {
          ErrStatus: {
              TypeMeta: {Kind: "", APIVersion: ""},
              ListMeta: {
                  SelfLink: "",
                  ResourceVersion: "",
                  Continue: "",
                  RemainingItemCount: nil,
              },
              Status: "Failure",
              Message: "virtualmachineclusterpreferences.instancetype.kubevirt.io \"centos.9.stream.desktop\" not found",
              Reason: "NotFound",
              Details: {
                  Name: "centos.9.stream.desktop",
                  Group: "instancetype.kubevirt.io",
                  Kind: "virtualmachineclusterpreferences",
                  UID: "",
                  Causes: nil,
                  RetryAfterSeconds: 0,
              },
              Code: 404,
          },
      }�[0m
  �[38;5;9mIn �[1m[It]�[0m�[38;5;9m at: �[1m/go/src/github.com/kubevirt/ssp-operator/tests/common_instancetypes_test.go:58�[0m �[38;5;243m@ 10/02/23 11:01:11.283�[0m

Can you please check? it seems like it wasn't found, I'm not familiar enough with InstanceTypes resources.

codingben · 2023-10-02T12:41:37Z

It seems like there is no CentOS 9 folder in v0.1.0: https://github.com/kubevirt/common-instancetypes/tree/v0.1.0/preferences/centos/9_stream

0xFelix · 2023-10-02T12:46:53Z

centos.9.stream was renamed to centos.stream9 recently, but that should not be the issue. SSP should reconcile the preferences from the URL but it looks like the cluster was not in a clean state before running this test?

0xFelix · 2023-10-02T12:48:14Z

The URL looks to be incorrect in general, because it is only referring to Preferences but not Instancetypes?

lyarwood · 2023-10-02T12:53:07Z

centos.9.stream was renamed to centos.stream9 recently, but that should not be the issue. SSP should reconcile the preferences from the URL but it looks like the cluster was not in a clean state before running this test?

Yeah this looks more like a race between the CR being updated and the test trying to assert that things have been reconciled?

The URL looks to be incorrect in general, because it is only referring to Preferences but not Instancetypes?

This was intentional, we don't have a unified cluster wide resources bundle so I just went with one, IMHO the test doesn't need it either.

codingben · 2023-10-02T12:55:30Z

Thanks for looking on it, the fact that this happened only in this PR, let's re-run the tests then?

codingben · 2023-10-02T13:00:15Z

/retest

codingben · 2023-10-02T13:12:55Z

/retest-required

lyarwood · 2023-10-02T15:30:02Z

#693 for the common-instancetypes failure, smells like a race to me.

Cleanup Tekton operands to improve code quality and readability. Also set specific RBAC groups to core instead of the wildcard. Signed-off-by: Ben Oukhanov <[email protected]>

github-actions · 2023-10-04T09:04:12Z

/cc sradco

sonarcloud · 2023-10-04T09:04:18Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

jcanocan · 2023-10-04T09:23:08Z

internal/tekton-bundle/tekton-bundle.go

+			switch kind {
+			case tasksString:
+				task := pipeline.Task{}
+				err = getObject(obj, &task)
+				if err != nil {
+					return err
+				}
+				bundle.Tasks = append(bundle.Tasks, task)
+			case pipelineKindString:
+				p := pipeline.Pipeline{}
+				err = getObject(obj, &p)
+				if err != nil {
+					return err
+				}
+				bundle.Pipelines = append(bundle.Pipelines, p)
+			case serviceAccountKind:
+				sa := v1.ServiceAccount{}
+				err = getObject(obj, &sa)
+				if err != nil {
+					return err
 				}
+				bundle.ServiceAccounts = append(bundle.ServiceAccounts, sa)
+			case roleBindingKind:
+				rb := rbac.RoleBinding{}
+				err = getObject(obj, &rb)
+				if err != nil {
+					return err
+				}
+				bundle.RoleBindings = append(bundle.RoleBindings, rb)
+			case clusterRoleKind:
+				cr := rbac.ClusterRole{}
+				err = getObject(obj, &cr)
+				if err != nil {
+					return err
+				}
+				bundle.ClusterRoles = append(bundle.ClusterRoles, cr)
+			case configMapKind:
+				cm := v1.ConfigMap{}
+				err = getObject(obj, &cm)
+				if err != nil {
+					return err
+				}
+				bundle.ConfigMaps = append(bundle.ConfigMaps, cm)
+			default:
+				continue
 			}


Can we move the error checking to the end of the switch statement? Something like:

switch kind { case tasksString: task := pipeline.Task{} err = getObject(obj, &task) bundle.Tasks = append(bundle.Tasks, task) [...] } if err != nil { return err }

Thanks, I'll look on it. I'd love to merge this PR firstly because it's open too much time already and too many CI issues there.

codingben · 2023-10-04T12:53:18Z

/retest

codingben · 2023-10-04T14:36:27Z

@ksimon1 Seems like there is an issue with example Tekton pipelines again?

ksimon1 · 2023-10-06T14:07:08Z

/retest

codingben · 2023-10-10T07:56:29Z

@akrejcir @ksimon1 @0xFelix Can you please approve and merge it?

jcanocan · 2023-10-10T07:57:19Z

/lgtm

kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Aug 1, 2023

kubevirt-bot added the size/L label Aug 1, 2023

kubevirt-bot requested review from 0xFelix and akrejcir August 1, 2023 09:15

0xFelix reviewed Aug 1, 2023

View reviewed changes

jcanocan reviewed Aug 7, 2023

View reviewed changes

codingben marked this pull request as ready for review August 8, 2023 14:06

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 8, 2023

kubevirt-bot requested review from 0xFelix and ksimon1 August 8, 2023 14:07

codingben marked this pull request as draft August 8, 2023 14:22

kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 8, 2023

codingben marked this pull request as ready for review August 8, 2023 14:39

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 8, 2023

kubevirt-bot requested a review from opokornyy August 8, 2023 14:39

codingben mentioned this pull request Aug 8, 2023

fix: Issues left for next PR from #532 #548

Closed

akrejcir reviewed Aug 8, 2023

View reviewed changes

codingben marked this pull request as draft August 8, 2023 14:55

kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL and removed size/L labels Aug 8, 2023

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 30, 2023

akrejcir added this to the v0.19.0 milestone Sep 18, 2023

codingben mentioned this pull request Oct 2, 2023

rbac: Audit * verbs of kubevirt-tekton-tasks #684

Merged

kubevirt-bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 2, 2023

kubevirt-bot requested a review from sradco October 2, 2023 10:05

refactor: cleanup tekton operands

459407a

Cleanup Tekton operands to improve code quality and readability. Also set specific RBAC groups to core instead of the wildcard. Signed-off-by: Ben Oukhanov <[email protected]>

jcanocan reviewed Oct 4, 2023

View reviewed changes

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 10, 2023

kubevirt-bot merged commit ef4e613 into kubevirt:main Oct 10, 2023
5 checks passed

refactor: cleanup tekton operands #641

refactor: cleanup tekton operands #641

Conversation

codingben commented Aug 1, 2023 • edited Loading

openshift-ci bot commented Aug 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyarwood Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codingben Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codingben commented Aug 2, 2023 • edited Loading

0xFelix commented Aug 2, 2023

jcanocan commented Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codingben Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

codingben commented Aug 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksimon1 commented Sep 25, 2023

codingben commented Oct 1, 2023

github-actions bot commented Oct 2, 2023

codingben commented Oct 2, 2023

codingben commented Oct 2, 2023

codingben commented Oct 2, 2023

0xFelix commented Oct 2, 2023

0xFelix commented Oct 2, 2023

lyarwood commented Oct 2, 2023

codingben commented Oct 2, 2023

codingben commented Oct 2, 2023

codingben commented Oct 2, 2023

lyarwood commented Oct 2, 2023

github-actions bot commented Oct 4, 2023

sonarcloud bot commented Oct 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codingben commented Oct 4, 2023

codingben commented Oct 4, 2023

ksimon1 commented Oct 6, 2023

codingben commented Oct 10, 2023

jcanocan commented Oct 10, 2023

codingben commented Aug 1, 2023 •

edited

Loading

lyarwood Aug 1, 2023 •

edited

Loading

codingben Aug 2, 2023 •

edited

Loading

codingben commented Aug 2, 2023 •

edited

Loading

jcanocan commented Aug 3, 2023 •

edited

Loading

codingben Aug 7, 2023 •

edited

Loading