-
Notifications
You must be signed in to change notification settings - Fork 220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TEP-0041: Add TEP for Tekton Component Versioning #329
TEP-0041: Add TEP for Tekton Component Versioning #329
Conversation
4ad12b5
to
f5827f2
Compare
Hi @imjasonh, as per yesterday's API call I explored the way how $ kn version
Version: v20210201-5b7586b3
Build Date: 2021-02-01 10:43:49
Git Revision: 5b7586b3
Supported APIs:
* Serving
- serving.knative.dev/v1 (knative-serving v0.20.0)
* Eventing
- sources.knative.dev/v1alpha2 (knative-eventing v0.20.0)
- eventing.knative.dev/v1beta1 (knative-eventing v0.20.0) And for checking the version of knative components we do kubectl get namespace knative-serving -o 'go-template={{index .metadata.labels "serving.knative.dev/release"}}'
kubectl get namespace knative-eventing -o 'go-template={{index .metadata.labels "eventing.knative.dev/release"}}' where we again need to have access to get the namespace. They are not handling the case for whether all |
Thanks, that's really helpful context. Do you know if they have had any issues with this in the past, and any solutions they've considered? Could we grant One reason I'm hesitant about adding a CRD for this is that it requires us to update the CRD in lock-step with the installed components, which themselves have to already be labeled in lock-step with actual binary releases. With each new moving piece it adds another opportunity to get things out of sync and confuse future debuggers. Add to that the complexity of defining (/versioning?) the new CRD type, granting the right users update and read access to it, etc., and it seems like a large hammer for this particular nail. |
Unfortunately, "enteprise" corporation running tekton usually lock down heavily their installations and rather have a policy of not letting that access. I agree with you this bring much complexity and I am not so sure if the use case warrants it. I think we may want to explain in the TEP why is it something that it is so important to be able to know the tekton components version in a locked down environment where the controllers are not readable. |
That make sense. In that case, are we sure the same enterprise corporations want users to be able to determine the version (or existence) of a Tekton installation at all? 🤔 |
If we want to provide read access to read the namespace or in the TEP, we mentioned a solution to keep the version information in a configmap, and the particular configmap can be provided read access for users, then we need to know the namespace where the installation is done to get the details. Like we have the use case of tekton-pipelines and openshift-pipelines namespace as of now, and there can be many more if the installation needs to be in a user-provided namespace.
The benefit we get with this is we can define it cluster-wide. For now, this can be only providing version information but in the future, it may be extended for providing other metadata information if required like minimum k8s version, etc. |
assigning it to@ImJasonH @afrittoli |
1ca1f0b
to
1f3f0d1
Compare
1f3f0d1
to
cb39711
Compare
/assign @afrittoli @imjasonh |
/test .* |
/cc @afrittoli @imjasonh |
@vdemeester @bobcatfish can you also please review this as well? 🙂 |
/assign |
ee626dd
to
7fa14db
Compare
|
||
#### Disadvantages | ||
|
||
1. User need to know the namespace in which Tekton components are installed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like the smallest disadvantage of all the alternatives. Can we promote this to the proposed solution, and move the others to "alternatives considered" in a section below?
Unless there's disagreement, we can start working on this as the solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
from what we have seen, it is rare that the installation namespace get changed from the default one we provide for a platform.
There is no incentive for a user to do that, as long as we don't support/encourage multi-tenancy (say, more than one instances of pipeline controller in different namespace. 😇
7fa14db
to
653c139
Compare
@vinamra28 another possible alternative, I noticed that knative handles this by adding a version label to every CRD they install (knative/serving#2820) so their docs recommend checking the version installed by getting the label from the namespace:
I'm wondering if maybe it would be more reasonable to require the CLI to be able to read labels on the namespace? (vs currently we require reading the Deployment?) Sorry for the last minute alternative entry but im curious what you think! (probably worth adding as an alternative even if we don't pursue it - but if the CLI already needs read access to the namespace, this might be an ok alternative?) But if that's no good, I'm 👍 to the configmap proposal! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems reasonable to me - it seems simple enough to implement.
Only version from releases will be available, but that is the same with the existing labels we have.
I would be nice to call out that this will require work on the release pipeline to be able to produce the config maps, as well as any relationship with operator, as already called out by @bobcatfish.
Since the proposal and design are already in, we could submit this a "implementable" directly I think.
|
||
## Drawbacks | ||
|
||
## Alternatives |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we provide a role & rolebinding as part of installing tekton that allows for read access to the existing version labels? if not, can the problem statement go into more detail about why this is not reasonable? it seems like we need to provide role and rolebinding regardless
I think this would require giving read access to the entire deployment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vinamra28 another possible alternative, I noticed that knative handles this by adding a version label to every CRD they install (knative/serving#2820) so their docs recommend checking the version installed by getting the label from the namespace:
kubectl get namespace knative-serving -o 'go-template={{index .metadata.labels "serving.knative.dev/release"}}'
I'm wondering if maybe it would be more reasonable to require the CLI to be able to read labels on the namespace? (vs currently we require reading the Deployment?)
Sorry for the last minute alternative entry but im curious what you think! (probably worth adding as an alternative even if we don't pursue it - but if the CLI already needs read access to the namespace, this might be an ok alternative?)
But if that's no good, I'm to the configmap proposal!
Note that this is really close to what we are doing today too (at least for tektoncd/pipeline
and I think also for tektoncd/triggers
), but not on the namespace
. This probably reduce a little bit some rbac needs as it's probably more common to be able to list namespace than deployments in other namespace. But there is also a very high possibility that users cannot list namespace (or the list is filter to theirs).
On the proposed solution, there could be yet another alternative : make the mutating webhook append the version on object on creation (in an annotation). This would be easy to implement (we can pass the version info as a flag just like we do on the controller). The main disadvantages here is that to get information, the client (tkn
, …) would need to create a resource (a dummy Task
should suffice), which means that a user would need to have a list create
access to Task
to be able to get the version. Today, we are adding version as an annotation or a label on runs if I remember correctly.
Basic test scenarios of getting the version information should be added in all the components. | ||
Other components like CLI can add the test cases based on their usage. | ||
|
||
## Design Evaluation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bobcatfish of course, but I wonder what do you mean about this ? Operator folks are reviewer here as well, so they are/should be collaborating already 🙃
cc @tektoncd/experimental-maintainers
@vdemeester another disadvantage which I can think of is that we cannot create a dummy resource for |
23d701c
to
cd86e27
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i left a bunch of requests for tweaks to the TEP content but generally im in favor of this proposal!
/approve
|
||
## Requirements | ||
|
||
None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This proposal is to allow the system authenticated users of the cluster
to access the version of the Tekton component installed even if they don't
have proper rights to read the version from the controller deployments.
That text above ^^ i think would be great to move from the proposal section to the requirements, e.g.:
- users can access information about the versions of installed tekton components:
- they do not need to have read access to Deployments
- < maybe we can elaborate on what access we do expect them to have? >
1. Tekton CLI will not be able to access the version information from previously | ||
released tekton components without this functionality. | ||
|
||
2. Tekton CLI needs to handle backward compatibility, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does backward compatibility here mean the CLI needs to be able to read the versions of previously released components? Or does this mean it needs to gracefully error out when trying to read the version of previously released components? either way i think this point would be clearer with a bit more detail
## Design Details | ||
|
||
As discussed in the proposal the possible solutions with their | ||
`pros` and `cons` are:- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like this section is missing? i think the pros might be something like:
pros:
- dont need to create and manage any new types
cons:
- no guarantee that configmap and actual installed version will be in sync
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this can be merged with the advantages/disadvantages section below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of pros
and cons
shall I rename to advantages
and disadvantages
?
1. Client will have to create a dummy Tekton resource in order to find the version. | ||
2. In case of `Dashboard` no dummy resource can be created. | ||
3. There can be a scenario where no Tekton resource is present on the cluster. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add an alternative where we take the same approach as knative and make the version information available in labels on the namespace? (https://knative.dev/docs/install/check-install-version/)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approaches would work and wouldn't require to create a new resource (configMap). It is also really close to what we are doing now, a diff would only be the following for tektoncd/pipeline
.
diff --git a/config/100-namespace.yaml b/config/100-namespace.yaml
index 5254a8ce..043f30b1 100644
--- a/config/100-namespace.yaml
+++ b/config/100-namespace.yaml
@@ -19,3 +19,4 @@ metadata:
labels:
app.kubernetes.io/instance: default
app.kubernetes.io/part-of: tekton-pipelines
+ pipeline.tekton.dev/release: "devel"
The problem is that only tektoncd/pipeline
component defines this (applies the namespace) and if other component start to define this (in a 100-namespace.yaml
file), the last installed component wins… The operator could help here, but the problem wouldn't be solved for when you install it on your own.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bobcatfish The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
cd86e27
to
1010f87
Compare
Signed-off-by: vinamra28 <[email protected]>
1010f87
to
eb26f66
Compare
The PR is now ready for review |
1. User need to know the namespace in which Tekton components are installed | ||
since the scope is only the namespace. | ||
|
||
2. No guarantee that `ConfigMap` and actual installed version will be in sync. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The syncing seems to be the job of tektoncd/operator
.
The operator can sync the configmap(s) if it exist.
We can also think about letting the operator create the ConfigMaps.
Yes! this will make this solution dependent on our operator.
But, hey! the lifecycle management of TektonComponents are becoming more and more complex, and the operator could be the common agent which takes care of issues like the one addressed in this TEP.
In addition, operator can be the single point where we have to add this logic of creating/updating the ConfigMaps, instead of adding it to Pipeline, Triggers, Dashboard ...
### 3. Creating a CRD | ||
|
||
Tekton components should create a CRD which is to provide information like | ||
metadata about components. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of creating, new CRDs, we can consider using already existing CRDs of operator for this purpose.
TektonPipeline, TektonTrigger, TektonDasboard ... which handle the lifecycle of the respective components.
In additon, Operator have also TektonConfig CRD. This CRD is an umbrella CRD which can install 1 or more TektonComponents.
We might be able to achieve what we want by giving access to the status subresource of 'instance' of these CRDs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot totally rely on operator's CRDs as it depends on users how are they installing Pipelines or Triggers.
@bobcatfish @vinamra28 @vdemeester We have this PR on the operator, in which it can be configured to install the pipeline in the user-provided namespace. tektoncd/operator#305 Posting it here, as it effects this TEP |
hi, @piyush-garg, this pr just auto-creates the Before this PR our users had to install operator and create an instance of TektonConfig CRD (one of https://github.com/tektoncd/operator/tree/main/config/crs/kubernetes/config). This PR just elimitates 1 installation step. Note 1: even with this feature and this PR, it is safe to assume that a vast majority of our users will not change the default namespace (per platform supported by operator) where the components are installed. At present, for Kubernetes builds Note 2: This PR introduces a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the updates.
It feels like a good direction to take.
/lgtm
Proposal to add versioning in Tekton components so that any system authenticated users who are having access to cluster may be able to view the version of Tekton component installed.
/kind tep
Signed-off-by: vinamra28 [email protected]