Skip to content
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

Allow cluster operator to configure default resolver #5907

Closed
lbernick opened this issue Dec 21, 2022 · 9 comments
Closed

Allow cluster operator to configure default resolver #5907

lbernick opened this issue Dec 21, 2022 · 9 comments
Assignees
Labels
area/resolution Issues related to remote resolution kind/feature Categorizes issue or PR as related to a new feature.

Comments

@lbernick
Copy link
Member

Feature request

Allow cluster operators to configure a default resolver via a configmap. I would imagine default params could be configured in the resolver configmap itself rather than the tekton remote resolution configmap (e.g. how the git resolver allows specifying a default URL).

Use case

All of a company's Tasks and Pipelines are stored in the same git repo and they don't want to have to specify this for every TaskRun and PipelineRun.

@lbernick lbernick added the kind/feature Categorizes issue or PR as related to a new feature. label Dec 21, 2022
@lbernick lbernick added the area/resolution Issues related to remote resolution label Feb 15, 2023
@QuanZhang-William
Copy link
Member

/assign

@QuanZhang-William
Copy link
Member

In the API WG we discussed the option to add the configuration as a new feature flag default-resolver-type in the resolver/config-feature-flag.yaml file.

The main issue I noticed in the approach is that the ConfigMap is in the tekton-pipelines-resolvers ns instead of tekton-pipelines. Here is a simplified code (I have made sure that RBAC is working properly, aka, I don't think it is failing due to authorization issue) and it gives the following error when starting Webhook and Controllers:

{"severity":"fatal","timestamp":"2023-03-02T22:25:17.834Z","logger":"tekton-pipelines-webhook","caller":"sharedmain/main.go:285","message":"Failed to start configuration manager","commit":"b3d12c6-dirty","error":"configmap \"resolvers-feature-flags\" not found","stacktrace":"knative.dev/pkg/injection/sharedmain.MainWithConfig\n\tknative.dev/[email protected]/injection/sharedmain/main.go:285\nknative.dev/pkg/injection/sharedmain.MainWithContext\n\tknative.dev/[email protected]/injection/sharedmain/main.go:210\nmain.main\n\tgithub.com/tektoncd/pipeline/cmd/webhook/main.go:262\nruntime.main\n\truntime/proc.go:250"}

With some investigation, The error is caused by that Knative allows to register the ConfigMap Watcher to watch a single namespace when starting the SharedMain, and it is defaulted to tekton-pipelines namespace defined by the env. It's not clear to me how to make it to watch ConfigMap from multiple ns when starting the Webhook (Happy to learn if you know!).


Alternatives are:

  1. to configure the value in the config-defaults.yaml
  2. or use a new dedicated Config Map for this FR like config-trusted-resources.yaml

They are in the tekton-pipelines ns, which can help make the implementation a lot neater, and we don't need add an extra config dependency to tekton-pipelines-resolvers ns for controllers & webhook. I think this option makes sense because the FR is more like a "default value" instead of "feature flag"...

WDYT @lbernick @dibyom @abayer @vdemeester

@abayer
Copy link
Contributor

abayer commented Mar 3, 2023

Ah, yeah, I hit that problem with ConfigMap watching. In retrospect, thanks to this and some other issues, I really, really wish I hadn't put the resolvers in a separate namespace. Ah well.

I do think we should put the configuration of a default resolver in tekton-pipelines, not tekton-pipelines-resolvers - but I can't entirely justify that feeling. I guess I just feel like a default resolver for Pipelines usage is something that should be configured on Pipelines, not on the resolvers...

@afrittoli
Copy link
Member

Ah, yeah, I hit that problem with ConfigMap watching. In retrospect, thanks to this and some other issues, I really, really wish I hadn't put the resolvers in a separate namespace. Ah well.

I do think we should put the configuration of a default resolver in tekton-pipelines, not tekton-pipelines-resolvers - but I can't entirely justify that feeling. I guess I just feel like a default resolver for Pipelines usage is something that should be configured on Pipelines, not on the resolvers...

I tend to agree, that feels like a pipeline configuration as opposed to resolver configuration.

@lbernick
Copy link
Member Author

lbernick commented Mar 3, 2023

Ah, yeah, I hit that problem with ConfigMap watching. In retrospect, thanks to this and some other issues, I really, really wish I hadn't put the resolvers in a separate namespace. Ah well.

Some users are requesting we put them back into the same namespace (#5607 and #5931 )-- is that an option?

@QuanZhang-William
Copy link
Member

QuanZhang-William commented Mar 3, 2023

Ah, yeah, I hit that problem with ConfigMap watching. In retrospect, thanks to this and some other issues, I really, really wish I hadn't put the resolvers in a separate namespace. Ah well.

Some users are requesting we put them back into the same namespace (#5607 and #5931 )-- is that an option?

This is something worth investigation (and not sure if we have investigated it before @abayer?), while I don't think this specific FR should not necessarily be "blocked" by it?

I agree with the point that this is more like a pipeline default configuration, and putting to the config-defaults.yaml makes more sense to me.

QuanZhang-William added a commit to QuanZhang-William/community that referenced this issue Mar 3, 2023
This commit adds a new TEP: Configure Default Resolver.

This commit adds proposal and implementation details to introduce a new field `default-resolver-type` in the ConfigMap to configure the default resolver type in Tekton. The design fulfills the requirement of [feature request #5907][#5907]

/kind tep

[#5907]: tektoncd/pipeline#5907
QuanZhang-William added a commit to QuanZhang-William/community that referenced this issue Mar 3, 2023
This commit adds a new TEP: Configure Default Resolver.

This commit adds proposal and implementation details to introduce a new field `default-resolver-type` in the ConfigMap to configure the default resolver type in Tekton. The design fulfills the requirement of [feature request #5907][#5907]

/kind tep

[#5907]: tektoncd/pipeline#5907
QuanZhang-William added a commit to QuanZhang-William/community that referenced this issue Mar 6, 2023
This commit adds a new TEP: Configure Default Resolver.

This commit adds proposal and implementation details to introduce a new field `default-resolver-type` in the ConfigMap to configure the default resolver type in Tekton. The design fulfills the requirement of [feature request #5907][#5907]

/kind tep

[#5907]: tektoncd/pipeline#5907
QuanZhang-William added a commit to QuanZhang-William/community that referenced this issue Mar 6, 2023
This commit adds a new TEP: Configure Default Resolver.

This commit adds proposal and implementation details to introduce a new field `default-resolver-type` in the ConfigMap to configure the default resolver type in Tekton. The design fulfills the requirement of [feature request #5907][#5907]

/kind tep

[#5907]: tektoncd/pipeline#5907
QuanZhang-William added a commit to QuanZhang-William/community that referenced this issue Mar 6, 2023
This commit adds a new TEP: Configure Default Resolver.

This commit adds proposal and implementation details to introduce a new field `default-resolver-type` in the ConfigMap to configure the default resolver type in Tekton. The design fulfills the requirement of [feature request #5907][#5907]

/kind tep

[#5907]: tektoncd/pipeline#5907
QuanZhang-William added a commit to QuanZhang-William/community that referenced this issue Mar 6, 2023
This commit adds a new TEP: Configure Default Resolver.

This commit adds proposal and implementation details to introduce a new field `default-resolver-type` in the ConfigMap to configure the default resolver type in Tekton. The design fulfills the requirement of [feature request #5907][#5907]

/kind tep

[#5907]: tektoncd/pipeline#5907
QuanZhang-William added a commit to QuanZhang-William/community that referenced this issue Mar 7, 2023
This commit adds a new TEP: Configure Default Resolver.

This commit adds proposal and implementation details to introduce a new field `default-resolver-type` in the ConfigMap to configure the default resolver type in Tekton. The design fulfills the requirement of [feature request #5907][#5907]

/kind tep

[#5907]: tektoncd/pipeline#5907
QuanZhang-William added a commit to QuanZhang-William/community that referenced this issue Mar 7, 2023
This commit adds a new TEP: Configure Default Resolver.

This commit adds proposal and implementation details to introduce a new field `default-resolver-type` in the ConfigMap to configure the default resolver type in Tekton. The design fulfills the requirement of [feature request #5907][#5907]

/kind tep

[#5907]: tektoncd/pipeline#5907
tekton-robot pushed a commit to tektoncd/community that referenced this issue Mar 7, 2023
This commit adds a new TEP: Configure Default Resolver.

This commit adds proposal and implementation details to introduce a new field `default-resolver-type` in the ConfigMap to configure the default resolver type in Tekton. The design fulfills the requirement of [feature request #5907][#5907]

/kind tep

[#5907]: tektoncd/pipeline#5907
@QuanZhang-William
Copy link
Member

@QuanZhang-William
Copy link
Member

/cc @lbernick @xchapter7x

@jwitrick
Copy link

Please revisit this ... It should not be too difficult to split them. This is still BREAKING when using Kustomize.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/resolution Issues related to remote resolution kind/feature Categorizes issue or PR as related to a new feature.
Projects
Status: Done
Development

No branches or pull requests

5 participants