pipeline icon indicating copy to clipboard operation
pipeline copied to clipboard

Add support for using multiple remote resolution Git providers in the same cluster

Open abayer opened this issue 3 years ago • 20 comments

With #5450 about to merge, we're adding support for remote resolution of Tasks and Pipelines using Git providers' APIs. Which is great! But it does come with a sizable limitation - you can only have one provider type/server/token per cluster. While this is fine to start with, we really should add the ability for admins to configure multiple supported providers, so that you can use both an internal GitHub Enterprise and github.com, or any other combination of the various providers supported through go-scm.

abayer avatar Sep 13 '22 14:09 abayer

I'm kicking around a couple ideas at the moment. Both revolve around having the provider configured in the git-resolver-config ConfigMap as the "default", with an optional parameter for the git resolver available to specify the "name" of a different provider configuration. The question, in my mind, is how to store those additional provider configurations. My current ideas:

  • ConfigMaps in the tekton-pipelines-resolvers namespace, containing the relevant fields from git-resolver-config and the unique "name" as an identifier for the provider config. For discovery of these ConfigMaps, I'm thinking requiring a label, like resolution.tekton.dev/git-provider-config: "true". Then the resolver will use watches to load/update those configurations into a map keyed on the "name".
  • A new CRD, something like ResolutionGitProvider, with instances of the CRD also living in the tekton-pipelines-resolvers namespace. This would also have the config fields from git-resolver-config, albeit as actual fields rather than map keys, with the name of the instance being used as the identifier. Again, they'd be loaded up via watches.

The ConfigMap approach is probably the right one, but I just really like the CRD approach - less things to document (key names, the required label, etc), and it just feels nice? I'm giving myself a week or two to come back to this and see how I feel about the options then.

abayer avatar Sep 13 '22 14:09 abayer

ConfigMaps in the tekton-pipelines-resolvers namespace, containing the relevant fields from git-resolver-config and the unique "name" as an identifier for the provider config.

Agreed that this is probably the good path to explore. Depending on what is best for the users, we might want to consider how to reuse api-token-secret-name that could store the tokens for different providers in a single secret under different api-token-secret-key. wdyt?

chuangw6 avatar Sep 16 '22 15:09 chuangw6

Ok, I ran into some walls with the config map approach - namely, how to know when a new provider's config map has been added so we can add it to the config map watch. I've prototyped a new approach:

Rather than put the provider-specific information in different config maps, instead the user would add new keys to the existing configmap, formatted like provider.(provider name).(key name). So for a provider we call banana, the git resolver config map would look like this:

metadata:
  name: git-resolver-config
  namespace: tekton-pipelines-resolvers

data:
  default-revision: master
  fetch-timeout: 1m

  scm-type: gitlab
  api-token-secret-key: default-token
  api-token-secret-name: default-token-secret
  api-token-secret-namespace: default-namespace

  provider.banana.api-token-secret-key: banana-token
  provider.banana.api-token-secret-name: banana-token-secret
  provider.banana.api-token-secret-namespace: banana-namespace
  provider.banana.scm-type: github
  provider.banana.server-url: https://private.github.internal

This is obviously a wee bit hacky - it's a workaround for configmaps just being map[string]string. When a git ResolutionRequest comes in with a providerName parameter, we'll look for provider.[providerName].[key] values, erroring if we can't find the required ones (token secret name and key, scm type). If no providerName is given, we'll fall back to the "global"/"default" configuration.

I have this passing unit tests correctly, so while I haven't tested it end to end, I'm very confident that it'll work just fine there. Buuuut I'm not nearly sure this is actually the right way to go - I just got frustrated with the configmap detection etc and decided to see how much work this approach would take. Turns out, it did not take much work at all.

cc @vdemeester - I'd appreciate your thoughts.

abayer avatar Sep 20 '22 20:09 abayer

I think it make sense 👍🏼

vdemeester avatar Sep 21 '22 09:09 vdemeester

I should also say that one of the reasons I've gone with this approach is that it doesn't close the door on moving to separate config maps or CRDs for provider-specific config in the future. This doesn't add any new resources/config maps, just fields in the existing config map, and we could easily deprecate this if/when we add an alternative approach without breaking compatibility in the near term or having to maintain support of complex code.

I'll open a PR for this once #5515 has merged.

abayer avatar Sep 21 '22 12:09 abayer

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale with a justification. Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with /close with a justification. If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

tekton-robot avatar Dec 20 '22 13:12 tekton-robot

Stale issues rot after 30d of inactivity. Mark the issue as fresh with /remove-lifecycle rotten with a justification. Rotten issues close after an additional 30d of inactivity. If this issue is safe to close now please do so with /close with a justification. If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

tekton-robot avatar Jan 19 '23 13:01 tekton-robot

@abayer is this an issue we should freeze?

afrittoli avatar Jan 19 '23 13:01 afrittoli

Yeah, I think so.

/lifecycle freeze

abayer avatar Jan 19 '23 13:01 abayer

Whoops

/lifecycle frozen

abayer avatar Jan 19 '23 13:01 abayer

I'm a new tekton user and am running into this: I was expecting to be able to provider default credentials for each provider url; possibly on a per-repository basis. e.g.

  • for github.com/org1 use secret A
  • for github.com/org2 use secret B
  • for mygithubenterprise.org/mycoolorg use secret C
  • for mygitlab.net/mycoolorg/myrepo use secret D

james-callahan avatar Aug 24 '23 01:08 james-callahan

the (newish) http resolver should let you do all ot this it you point it to the right raw URL and specify the token in the basic auth password (which are passed as params), see the example in the documentation:

https://tekton.dev/docs/pipelines/http-resolver/#task-resolution-with-basic-auth

still we could add a params to override the serverURL and scmType, as well think about a configmap per namespace

chmouel avatar Jan 02 '24 18:01 chmouel

/assign

I am working on it to make it to pipeline 0.59 release

piyush-garg avatar Apr 08 '24 08:04 piyush-garg

/assign

I am working on it to make it to pipeline 0.59 release

@piyush-garg How's work on this feature going? Is it ready to merge or shall we move it to v0.60?

afrittoli avatar Apr 23 '24 11:04 afrittoli

@piyush-garg How's work on this feature going? Is it ready to merge or shall we move it to v0.62?

chitrangpatel avatar Jun 17 '24 15:06 chitrangpatel

@chitrangpatel We should probably move this to 0.62 😓

vdemeester avatar Jun 17 '24 16:06 vdemeester