Include drone-convert-pathschanged in drone server

[cognifloyd]

This proposal is based on this request from @jimsheldon: https://twitter.com/bitberk/status/1420765101582397441

Perhaps you could consider integrating our config plugin https://github.com/meltwater/drone-convert-pathschanged into the drone server? Interest has been steadily growing, it would be great if the functionality was built directly into drone (similar to starlark)!

Extension: https://github.com/meltwater/drone-convert-pathschanged Description: A Drone conversion extension to include/exclude pipelines and steps based on paths changed. Pipeline trigger/when examples: https://github.com/meltwater/drone-convert-pathschanged#examples

Why move this into drone server:

• This plugin has to query the scm provider to get the list of paths changed. That means duplicating some of the scm auth/access infrastructure that is already in the drone server. Plus, an extra access token has to be provisioned to acceess the scm provider since extensions cannot (and should not be able to) reuse the auth credentials provided to drone-server.

• Today, it only supports Github and Bitbucket. Putting it in drone server itself would make it much easier to add the feature for the other providers.

• Also, putting this in drone server itself should allow for using drone's signature validation. As described here:

  This occurs due to Drone's order of operations, in that the Drone file's signature is checked after the this plugin has rewritten sections based on the paths-changed triggers, resulting in a different signature for the file.

@jimsheldon, do you have anything to add here?

[bradrydzewski]

👋 hey there, thanks for submitting a proposal.

This comment does not address whether or not we should adopt the paths changed functionality in Drone core (that requires more thought and reflection on my end, and I presume other Drone community members will chime in as well). I just wanted to add some more color to the discussion.

There are multiple extensions that solve this problem, some of which solve this in slightly different ways. Here is another popular extension: https://github.com/bitsbeats/drone-tree-config

That means duplicating some of the scm auth/access infrastructure that is already in the drone server. [...] Plus, an extra access token has to be provisioned to acceess the scm provider

This is a problem that impacts multiple extensions so I think it is something we will need to solve regardless. I am thinking we could add a configuration parameter to optionally include the tokens in the encrypted payload sent to the extension.

Today, it only supports Github and Bitbucket. Putting it in drone server itself would make it much easier to add the feature for the other providers.

This also impacts multiple extensions. I think extension authors should be leveraging the go-scm library that we use in drone to abstract calls across multiple providers. This library already includes the necessary functions that are required to support the paths changed functionality [1][2].

Also, putting this in drone server itself should allow for using drone's signature validation

This also impacts multiple extensions and is something we should probably solve across the board. The actual solution should be relatively simple. We should snapshot the original contents [3] and then use that snapshot when validating the signature [4].

[1] https://github.com/drone/go-scm/blob/master/scm/git.go#L79 [2] https://github.com/drone/go-scm/blob/master/scm/pr.go#L79 [3] https://github.com/drone/drone/blob/master/trigger/trigger.go#L199 [4] https://github.com/drone/drone/blob/master/trigger/trigger.go#L268:L269

[cognifloyd]

There are multiple extensions that solve this problem, some of which solve this in slightly different ways. Here is another popular extension: https://github.com/bitsbeats/drone-tree-config

Hmm, that has some overlapping implementation details, but it doesn't provide the same feature. drone-tree-config inspects changed files to support multiple .drone.yml files. drone-convert-pathschanged inspects changed files to allow skipping pipelines or pipeline steps based on which files have changed.

I think extension authors should be leveraging the go-scm library that we use in drone to abstract calls across multiple providers.

It does use go-scm to create a GitHub client object using client.Git.ListChanges client.Git.CompareChanges: https://github.com/meltwater/drone-convert-pathschanged/blob/master/providers/github.go

But it uses something else for bitbucket: https://github.com/meltwater/drone-convert-pathschanged/blob/master/providers/bitbucket_server.go

I don't know why there's a difference between the implementations. I'm just reading through the code. Maybe @jimsheldon knows more?

[cognifloyd]

That means duplicating some of the scm auth/access infrastructure that is already in the drone server. [...] Plus, an extra access token has to be provisioned to acceess the scm provider

This is a problem that impacts multiple extensions so I think it is something we will need to solve regardless. I am thinking we could add a configuration parameter to optionally include the tokens in the encrypted payload sent to the extension.

On the same topic, in https://github.com/meltwater/drone-convert-pathschanged/issues/13#issuecomment-602985193 you said:

We could consider sending the user token from Drone to this extension in the payload. It would have to be opt-in since everyone might not be comfortable with the token leaving the Drone server. It doesn't solve the scope problem (Drone has the same scope) but it would be one less token to manage.

[colinhoglund]

I am thinking we could add a configuration parameter to optionally include the tokens in the encrypted payload sent to the extension.

+1 to something like this approach to avoid the need to provision separate machine user tokens with broad access.

As an alternative, would it make sense for the Drone server itself to send the list of "files changed" as part of the Request object to extensions? It seems like this would remove the need for some extensions to bother with a github token entirely.

Hey @bradrydzewski 👋 , any thoughts on this approach?

[lviana]

Having a single user token configured to retrieve change sets is going to limit the number of builds than can use this feature/extension due to the rate limits in place. It would be ideal if we could make the api calls using the same user token that is used for the build execution, or receiving the list of files changed in the payload like @colinhoglund mentioned above.

One more important point that may affect some people is the fact that some companies have more than one (Github) organization defined, and having a single token that can access all repos on all orgs is too permissive.

[bradrydzewski]

We have made the necessary changes to drone-go to include token in the payload: https://github.com/drone/drone-go/commit/f9e4fe31c2af2461f73672d33c6fb0316b0b17a1

We need to make corresponding changes to Drone core to ensure we are sending the token. I will talk with the team and hopefully we can get these changes released soon.

[bradrydzewski]

@colinhoglund the server was augmented to pass user token so that it can be re-used by extensions. This change is available in the latest drone image. The relevant changeset can be found at https://github.com/harness/drone/commit/7b42cd9bbd66e4ae23241fef4a915a2864c459b3. Hopefully this help simplify some things while we evaluate the broader proposal internally.

[colinhoglund]

@colinhoglund the server was augmented to pass user token so that it can be re-used by extensions. This change is available in the latest drone image. The relevant changeset can be found at harness/drone@7b42cd9. Hopefully this help simplify some things while we evaluate the broader proposal internally.

Thanks @bradrydzewski! I tested this briefly in minikube and the token appears to be passed as expected.

Minor comment: At one point it was mentioned that passing tokens to extensions might be opt-in only (for security reasons). I'm not particularly opinionated about this since anyone running a Drone installation can take care that any extensions that receive the token handle it with care. Mostly just curious if there is still a plan to make this optional.

[bradrydzewski]

@colinhoglund yes, we will handle this in a follow-up patch

[jimsheldon]

I can confirm this is working. I built the current drone server off master, updated drone-go to v1.7.2-0.20220308165842-f9e4fe31c2af in the paths changed extension, then req.Token.Access worked to authenticate with GitHub.

I will get this into the paths changed extension once a new version of the drone server is released.

Thanks!

[colinhoglund]

FYI @jimsheldon, it looks like the server change was unofficially released as part of v2.12.0. However, the related changes to https://github.com/drone/drone-go are not yet part of a Github release (here and here).

Hey @bradrydzewski, Would it be possible to get the drone-go changes in a new github release?

[colinhoglund]

Hey @bradrydzewski, just resurfacing this to see if there are any plans to push out a new drone-go release.