Add field in config to disable hub devconsole integration #569

piyush-garg · 2021-12-22T11:54:12Z

This will add a struct in config by named hub, where user can add flags and field
to set properties related to hub

Right now, only one field is available to enable/disable devconsole pipeline
builder and hub resources integration on OpenShift, by default the integration is
enabled but it can be disabled by adding the field with value
false in tektonconfig CR. By default the field will not be there in CR and
the integration is enabled.

The field getting added is with possible values as true and false.

if user provides invalid key and invalid value it will be handled by webhook

Also, if the field is not there in CR, webhook will not add it.

This patch also add the config CRD view role for all system authenticated users.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Release Notes

Add field in config to disable hub devconsole integration

tekton-robot · 2021-12-22T11:57:43Z

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/apis/operator/v1alpha1/tektonconfig_defaults.go	72.7%	82.4%	9.6
pkg/apis/operator/v1alpha1/tektonconfig_types.go	33.3%	50.0%	16.7
pkg/apis/operator/v1alpha1/tektonconfig_validation.go	83.8%	65.3%	-18.5

tekton-robot · 2021-12-24T11:10:55Z

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/apis/operator/v1alpha1/tektonconfig_defaults.go	72.7%	82.4%	9.6
pkg/apis/operator/v1alpha1/tektonconfig_types.go	33.3%	50.0%	16.7
pkg/apis/operator/v1alpha1/tektonconfig_validation.go	83.8%	87.8%	4.0

tekton-robot · 2021-12-24T18:47:49Z

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/apis/operator/v1alpha1/tektonconfig_defaults.go	72.7%	82.4%	9.6
pkg/apis/operator/v1alpha1/tektonconfig_types.go	33.3%	50.0%	16.7
pkg/apis/operator/v1alpha1/tektonconfig_validation.go	83.8%	87.8%	4.0

pkg/apis/operator/v1alpha1/const.go

karthikjeeyar · 2022-01-04T10:48:43Z

We may need a new clusterRoleBinding addon for the developer (non-admin) users to read the tekton config CRs.
Right now, only kube:admin users are able to read the tektonconfig CR, but with below CRB on the cluster, even the developer users will get access to read the config.
@vdemeester @piyush-garg What do you think?

kind: ClusterRoleBinding
apiVersion: rbac.authorization.k8s.io/v1
metadata:
  name: tekton-config-view-auth
subjects:
  - kind: Group
    apiGroup: rbac.authorization.k8s.io
    name: 'system:authenticated'
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: tektonconfigs.operator.tekton.dev-v1alpha1-view

nikhil-thomas · 2022-01-04T23:43:01Z

Right now, only kube:admin users are able to read the tektonconfig CR,

@karthikjeeyar this is the intented, as CRDs like TektonConfig, TektonPipeline are internal. Non-admin users generally deal with CRDs like Task, TaskRun etc.
However, we can cosider adding such a rolebinding. Could you give us more details on how it will help a non-admin user.

nikhil-thomas · 2022-01-04T23:47:19Z

/approve

nikhil-thomas · 2022-01-05T00:00:07Z

Ideally, the operator should be reading the new param and set a value in a configMap for the Devconsole to read. In the current scope of this patch, the operator does not do anything in response to a change in the param added by this patch.

So we are hosting a spec field in TektonConfig for Devconsole code to read.

👉 this is alright functionaly. However, this is a tight coupling. ie, we will not be able to change the structure or name of this part in TektonConfig in the future without breaking Devconsole.

If there is a intermediary like a configMap where the operator can set configuration for Devconsole after reading spec in TektonConfig, then we will be able to change the api/behavior on operator side without breaking devconsole.

I understand that we agreed with @karthikjeeyar that devconsole side can proceed development based on the TektonConfig spec field name as a contract.

However, we all should be aware of the tight coupling here.

cc @sm43 @vdemeester

vdemeester · 2022-01-05T11:49:46Z

Ideally, the operator should be reading the new param and set a value in a configMap for the Devconsole to read. In the current scope of this patch, the operator does not do anything in response to a change in the param added by this patch.

So we are hosting a spec field in TektonConfig for Devconsole code to read.

point_right this is alright functionaly. However, this is a tight coupling. ie, we will not be able to change the structure or name of this part in TektonConfig in the future without breaking Devconsole.

If there is a intermediary like a configMap where the operator can set configuration for Devconsole after reading spec in TektonConfig, then we will be able to change the api/behavior on operator side without breaking devconsole.

I understand that we agreed with @karthikjeeyar that devconsole side can proceed development based on the TektonConfig spec field name as a contract.

However, we all should be aware of the tight coupling here.

cc @sm43 @vdemeester

@nikhil-thomas I agree and disagree. For TektonPipeline, … I do agree those are internal, but TektonConfig is a different beast. It's the main entrypoint, it's the one that the user will interact in 99% of the time they might need to interact. So the "contract" there is both with user (actual person) as well as tools (devconsole, other tooling, …). Having a configmap instead of this would make sense but it would just move the "contract" of the devconsole in a specific object (here a configmap) — I don't think I really have strong opinion about that though.

nikhil-thomas · 2022-01-05T23:06:33Z

I think this patch will be fine. In case we need to change the TektonConfig field in the future, then we can do it without breaking DevConsole by using proper deprecation process like we handled serviceAccount to serviceAccountName in PipelineRuns. 🧑‍💻 👍

nikhil-thomas · 2022-01-05T23:09:11Z

we can merge this right after the @piyush-garg pushes the minor change in defaulting that we discussed.
(Removing explicit defaulting if nothing is specified in the CR, so that users on kubernetes don't see this field in their CRs.)

pkg/apis/operator/v1alpha1/tektonconfig_defaults.go

karthikjeeyar · 2022-01-06T14:34:23Z

However, we can cosider adding such a rolebinding. Could you give us more details on how it will help a non-admin user.

@nikhil-thomas Most of the time the developer console users will have limited permission (eg: create/edit permissions on one or two namespaces). So if the logged in user is a devleoper (non-admin) user, then the call to fetch TektonConfig CR will fail due to lack of permission as it a cluster wide CR. With the clusterRoleBinding I suggested previously, all the logged in users should be able to read the config and UI can behave based on the configuration.

nikhil-thomas · 2022-01-10T05:55:46Z

However, we can cosider adding such a rolebinding. Could you give us more details on how it will help a non-admin user.

@nikhil-thomas Most of the time the developer console users will have limited permission (eg: create/edit permissions on one or two namespaces). So if the logged in user is a devleoper (non-admin) user, then the call to fetch TektonConfig CR will fail due to lack of permission as it a cluster wide CR. With the clusterRoleBinding I suggested previously, all the logged in users should be able to read the config and UI can behave based on the configuration.

does this mean that devconsole uses the logged in users credentials to query the api.
if yes, then giving a non-admin user read permission on the entire TektonConfig instance specto disable-hub-integration feature is an overkill.

I highly recomend reconsidering how we are handling this.

@piyush-garg

vdemeester · 2022-01-10T08:58:34Z

does this mean that devconsole uses the logged in users credentials to query the api.

I think so yes. The Devconsole is querying k8s directly for that reason too, there is no "devconsole service" running anywhere.

karthikjeeyar · 2022-01-10T09:51:49Z

Yes, Devconsole uses Logged-in user credentials to query any resource.

tekton-robot · 2022-01-19T09:12:06Z

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/apis/operator/v1alpha1/tektonconfig_validation.go	83.8%	84.6%	0.8
pkg/apis/operator/v1alpha1/tektonhub_types.go	Do not exist	100.0%
pkg/apis/operator/v1alpha1/tektonhub_validation.go	Do not exist	100.0%

tekton-robot · 2022-01-19T09:14:29Z

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/apis/operator/v1alpha1/tektonconfig_validation.go	83.8%	84.6%	0.8
pkg/apis/operator/v1alpha1/tektonhub_types.go	Do not exist	100.0%
pkg/apis/operator/v1alpha1/tektonhub_validation.go	Do not exist	100.0%

nikhil-thomas · 2022-01-19T23:10:36Z

✅ lgtm

piyush-garg · 2022-01-20T06:49:17Z

@karthikjeeyar @nikhil-thomas @sm43

Have tested the PR. It is working as expected now.

Changes

Webhook will not add the param in config CR by default now. cc @karthikjeeyar
All system authenticated user will have access to read, list and watch

This will add a struct in config by named hub, where user can add flags and field to set properties related to hub Right now, only one field is available to enable/disable devconsole pipeline builder and hub resources integration on OpenShift, by default the integration is enabled but it can be disabled by adding the field with value false in tektonconfig CR. By default the field will not be there in CR and the integration is enabled. The field getting added is with possible values as true and false. if user provides invalid key and invalid value it will be handled by webhook Also, if the field is not there in CR, webhook will not add it. This patch also add the config CRD view role for all system authenticated users.

piyush-garg · 2022-01-20T07:04:12Z

I have a question - the tekton config CR read role we want to get added for both kubernetes and openshift platform or only openshift cc @vdemeester @nikhil-thomas

tekton-robot · 2022-01-20T07:06:32Z

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/apis/operator/v1alpha1/tektonconfig_validation.go	83.8%	84.6%	0.8
pkg/apis/operator/v1alpha1/tektonhub_types.go	Do not exist	100.0%
pkg/apis/operator/v1alpha1/tektonhub_validation.go	Do not exist	100.0%

vdemeester · 2022-01-20T09:42:58Z

I have a question - the tekton config CR read role we want to get added for both kubernetes and openshift platform or only openshift cc @vdemeester @nikhil-thomas

It's only useful for OpenShift today, but I don't see any reason for not putting it in both.

piyush-garg · 2022-01-20T11:09:20Z

I have a question - the tekton config CR read role we want to get added for both kubernetes and openshift platform or only openshift cc @vdemeester @nikhil-thomas

It's only useful for OpenShift today, but I don't see any reason for not putting it in both.

Yaa this just provides read permission, I think it will be fine to provide for both. On that note, this PR is good to review and merge.

tekton-robot · 2022-01-21T10:18:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nikhil-thomas, vdemeester

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [nikhil-thomas,vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

nikhil-thomas · 2022-01-21T11:58:01Z

/lgtm

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Dec 22, 2021

tekton-robot requested review from concaf and pradeepitm12 December 22, 2021 11:54

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 22, 2021

piyush-garg force-pushed the add_field_hub branch from d7f61f0 to f9033c5 Compare December 24, 2021 11:07

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 24, 2021

piyush-garg marked this pull request as ready for review December 24, 2021 11:09

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 24, 2021

piyush-garg force-pushed the add_field_hub branch from f9033c5 to 8e1e881 Compare December 24, 2021 18:44

sm43 reviewed Jan 4, 2022

View reviewed changes

pkg/apis/operator/v1alpha1/const.go Outdated Show resolved Hide resolved

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 4, 2022

sm43 reviewed Jan 6, 2022

View reviewed changes

pkg/apis/operator/v1alpha1/tektonconfig_defaults.go Outdated Show resolved Hide resolved

piyush-garg force-pushed the add_field_hub branch 2 times, most recently from d8a8c16 to 8a5d307 Compare January 19, 2022 09:10

piyush-garg force-pushed the add_field_hub branch from 8a5d307 to d69d5a1 Compare January 20, 2022 07:02

vdemeester approved these changes Jan 21, 2022

View reviewed changes

tekton-robot assigned nikhil-thomas Jan 21, 2022

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 21, 2022

tekton-robot merged commit 1463bf2 into tektoncd:main Jan 21, 2022

Add field in config to disable hub devconsole integration #569

Add field in config to disable hub devconsole integration #569

Uh oh!

Conversation

piyush-garg commented Dec 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Submitter Checklist

Release Notes

Uh oh!

tekton-robot commented Dec 22, 2021

Uh oh!

tekton-robot commented Dec 24, 2021

Uh oh!

tekton-robot commented Dec 24, 2021

Uh oh!

Uh oh!

karthikjeeyar commented Jan 4, 2022

Uh oh!

nikhil-thomas commented Jan 4, 2022

Uh oh!

nikhil-thomas commented Jan 4, 2022

Uh oh!

nikhil-thomas commented Jan 5, 2022

Uh oh!

vdemeester commented Jan 5, 2022

Uh oh!

nikhil-thomas commented Jan 5, 2022

Uh oh!

nikhil-thomas commented Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

karthikjeeyar commented Jan 6, 2022

Uh oh!

nikhil-thomas commented Jan 10, 2022

Uh oh!

vdemeester commented Jan 10, 2022

Uh oh!

karthikjeeyar commented Jan 10, 2022

Uh oh!

tekton-robot commented Jan 19, 2022

Uh oh!

tekton-robot commented Jan 19, 2022

Uh oh!

nikhil-thomas commented Jan 19, 2022

Uh oh!

piyush-garg commented Jan 20, 2022

Uh oh!

piyush-garg commented Jan 20, 2022

Uh oh!

tekton-robot commented Jan 20, 2022

Uh oh!

vdemeester commented Jan 20, 2022

Uh oh!

piyush-garg commented Jan 20, 2022

Uh oh!

tekton-robot commented Jan 21, 2022

Uh oh!

nikhil-thomas commented Jan 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

piyush-garg commented Dec 22, 2021 •

edited

Loading

nikhil-thomas commented Jan 5, 2022 •

edited

Loading