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

Add support for GitHub private releases as plugin source #8944

Merged

Conversation

patriziobruno
Copy link
Contributor

@patriziobruno patriziobruno commented Feb 8, 2022

Description

This change adds support for downloading plugins from private GitHub releases. I added the change under the feature flag PULUMI_EXPERIMENTAL. It works as follows:
If the plugin can't be downloaded from Pulumi's public GitHub releases, an attempt is made to download from private GitHub releases using GitHub info and credentials passed as environment variables. To be compatible with GitHub Actions, env variable names come from official documentation.

  • GITHUB_REPO_OWNER
  • GITHUB_TOKEN
  • GITHUB_ACTOR
  • GITHUB_PERSONAL_ACCESS_TOKEN (this is not an official pipeline context var)
    The implementation uses either GITHUB_TOKEN for service2service authentication or GITHUB_ACTOR + GITHUB_PERSONAL_ACCESS_TOKEN for HTTP basic authentication. It gets the requested releaseId from GitHub's Releases API and, if successful, downloads the release from the URL of the asset matching the expected file-name format pulumi-<type>-<name>-v<version>-<operatingSystem>-<arch>.tar.gz.

If this method fails, pulumi plugin download falls back on get.pulumi.com.

Before adding testing, I wanted to gather feedback from the maintainers on the implementation.

Fixes #8943

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

@github-actions
Copy link

github-actions bot commented Feb 8, 2022

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation
  • /run-containers - used to test the Pull Request against the containers tests

1 similar comment
@github-actions
Copy link

github-actions bot commented Feb 8, 2022

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation
  • /run-containers - used to test the Pull Request against the containers tests

CHANGELOG_PENDING.md Outdated Show resolved Hide resolved
sdk/go/common/workspace/plugins.go Show resolved Hide resolved
sdk/go/common/workspace/plugins.go Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Feb 9, 2022

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation
  • /run-containers - used to test the Pull Request against the containers tests

@patriziobruno
Copy link
Contributor Author

@Frassle I was wondering if it made sense to put this code under a different feature flag from PULUMI_EXPERIMENTAL. Maybe, PULUMI_EXPERIMENTAL_GH_PRIVATE_REL? To let users decide which experiments they want to use and which not?

@github-actions
Copy link

github-actions bot commented Feb 9, 2022

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation
  • /run-containers - used to test the Pull Request against the containers tests

1 similar comment
@github-actions
Copy link

github-actions bot commented Feb 9, 2022

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation
  • /run-containers - used to test the Pull Request against the containers tests

@patriziobruno patriziobruno force-pushed the patriziobruno/8943/private-github-rel branch from 179ccca to 7e42255 Compare February 10, 2022 11:01
@github-actions
Copy link

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation
  • /run-containers - used to test the Pull Request against the containers tests

@Frassle
Copy link
Member

Frassle commented Feb 10, 2022

@Frassle I was wondering if it made sense to put this code under a different feature flag from PULUMI_EXPERIMENTAL. Maybe, PULUMI_EXPERIMENTAL_GH_PRIVATE_REL? To let users decide which experiments they want to use and which not?

We've discussed this internally before and decided to just stick to the simplicity of everything under one experimental flag for now. We don't expect to have things live as experimental for long.

@github-actions
Copy link

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation
  • /run-containers - used to test the Pull Request against the containers tests

@github-actions
Copy link

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation
  • /run-containers - used to test the Pull Request against the containers tests

@github-actions
Copy link

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation
  • /run-containers - used to test the Pull Request against the containers tests

Copy link
Member

@Frassle Frassle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks reasonable to me.

@Frassle
Copy link
Member

Frassle commented Feb 11, 2022

/run-acceptance-tests

@github-actions
Copy link

Please view the results of the PR Build + Acceptance Tests Run Here

Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. LGTM

@patriziobruno
Copy link
Contributor Author

Are there any additional steps before merging this?

@Frassle
Copy link
Member

Frassle commented Feb 11, 2022

Spoken to @stack72 about this. We do want to make sure we can support your workflow @patriziobruno, but further thinking about this we're not sure the proposed download flow is one we should support.

Would it be acceptable to use the pluginDownloadURL to point to your private github release instead of the download flow trying to work that out from env[GITHUB_REPOSITORY_OWNER]. We'd then change the pluginDownloadURL using code to add github auth tokens (if and only if the url was for something at github.com) so it could auth to that custom url.

I'm happy to do the required code changes here if that would still work for your usecase. If that wouldn't work do share why, we'll think some more about what we can do.

@patriziobruno
Copy link
Contributor Author

@Frassle @stack72 the problem about the pluginDownloadURL approach is that endpoints pointed by a browser_download_url don't accept GitHub Actions tokens, nor Basic authentication. Clients need to authenticate using a browser and the cookies that GitHub.com sends, which is not doable in a pipeline. Furthermore, once the user has authenticated, how would pulumi CLI access the authentication cookies? Unless GitHub.com adds OAuth tokens as a means to authenticate access to their browser_download_url, there's no way to download a private release without using the release API.

@Frassle
Copy link
Member

Frassle commented Feb 11, 2022

Ah right I've followed through the REST calls this makes and I see what you mean there. pluginDownloadURL makes assumptions about the format of the url, github releases don't match that format even if you pre-looked up the api style url to use.

@stack72 I can't see a simple way to fit this into our pluginDownloadURL flow given the above. I think we need to give that feature a major rethink and for now accept the PR as is, with the expectation that we'll have to break all custom plugin flows at some point to tidy this up (4.0 or even 5.0 change).

@mikhailshilkov
Copy link
Member

I think we need to give that feature a major rethink and for now accept the PR as is, with the expectation that we'll have to break all custom plugin flows at some point to tidy this up (4.0 or even 5.0 change).

This sounds somewhat scary to me... Will you leave this PR's flow behind the flag until 4.0/5.0? Otherwise that breaking change may be quite hard to pull in. It would be nice to understand what that future tidy-up may look like.

@patriziobruno
Copy link
Contributor Author

patriziobruno commented Feb 14, 2022

@mikhailshilkov This workflow doesn't make any use of pluginDownloadURL, so it would be minimally affected by any breaking change. Should the new workflow introduce configuration to replace the environment variables used by the current code, backward compatibility may be kept with little effort by keep using the all of the env vars that are set by GitHub Actions and progressively deprecating the one that is not, GITHUB_PERSONAL_ACCESS_TOKEN.
example schema.yaml:

pluginDownload:
  workflow: privateGitHubRelease
  with:
    repoOwner: owner # defaults to env[GITHUB_REPO_OWNER]
    actor: github_username # defaults to env[GITHUB_ACTOR]
    personalAccessToken: /path/to/file/containing/token or env var name containing token or any other means that is not putting a plain text secret into schema.yaml # initially defaults to env[GITHUB_PERSONAL_ACCESS_TOKEN] for backward compatibility

I don't mean to give suggestions on how to change the plugin download workflow, just making an example that wouldn't break this code.

@Frassle
Copy link
Member

Frassle commented Feb 14, 2022

Will you leave this PR's flow behind the flag until 4.0/5.0?

Yes I think we'd want to do that.

This workflow doesn't make any use of pluginDownloadURL, so it would be minimally affected by any breaking change

I should have been more clear here. I think pluginDownloadURL is not descriptive enough to support all the cases we want to support custom download from, it's not even descriptive enough to support github releases let alone anything more esoteric. So I think potential breaking change 1 is redesigning pluginDownloadURL so it can describe these cases, which might not be doable in a way that also maintains the current behaviour (it might be possible to do this change compatibly, needs more thinking).
Breaking change 2 is that once we can support github releases via some sort of pluginDownloadURL feature we would not want to have it run implicitly for every plugin download that this PR is adding, and so we would break this flow at somepoint.

It would be nice to understand what that future tidy-up may look like.

The example schema.yaml in @patriziobruno post looks something like what I expect we'd change to.

@github-actions
Copy link

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation
  • /run-containers - used to test the Pull Request against the containers tests

@github-actions
Copy link

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation
  • /run-containers - used to test the Pull Request against the containers tests

@Frassle
Copy link
Member

Frassle commented Feb 17, 2022

I'm going to merge this in with the understanding that it will stay under EXPERIMENTAL until we can spend the engineering time to rework custom plugin downloads to support this properly. Some time after adding proper support for this via the custom plugin work we will remove this code flow, and your workflow will be broken if you haven't updated to use the supported custom plugin settings.

I have raised #9007 to track the re-design work.

@Frassle Frassle merged commit fd25e56 into pulumi:master Feb 17, 2022
@patriziobruno patriziobruno deleted the patriziobruno/8943/private-github-rel branch February 17, 2022 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI: support for private GitHub releases in --server/pluginDownloadURL
4 participants