git-sync icon indicating copy to clipboard operation
git-sync copied to clipboard

Support GitHub App authentication

Open risset opened this issue 1 year ago • 8 comments

Older issues on this topic: https://github.com/kubernetes/git-sync/issues/450 https://github.com/kubernetes/git-sync/issues/769

Hey, I wanted to revive this discussion again since I have a use case that would benefit from using a GitHub app for auth, and using an operator like that github-app-operator isn't ideal for me right now.

Would the following design potentially work?

  • allow github app private key, app ID and installation ID as parameters to git-sync
  • if github app auth is enabled, in refreshCreds, request a new token (can check if it's expired first)
    • I think something similar to this should work: https://github.com/samirtahir91/github-app-operator/blob/0aca1da117933cad3075a55565eb422c0784f30c/internal/controller/githubapp_controller.go#L727
  • should then be able to authenticate over HTTPS with that token

CallAskPassURL is doing something a bit like this already, but doesnt give you much control over the request it makes.

I'm not too familar with git-sync internals yet but I have plenty of time to implement and try this out if maintainers think it might work

risset avatar May 14 '24 14:05 risset

In this area, git-sync is not very complex. Something like you describe sounds fine. Ideally we would start with an e2e case, but it's not clear to me if there's an easy way to e2e reproduce the experience locally. Is there a trivial github app that we can use to at least manually run tests? Hard to support something I can't test at all :)

thockin avatar May 14 '24 15:05 thockin

Yeah, testing this doesn't seem straightforward :( I'm not sure of being able to use an existing public app for testing. When creating a new app, GitHub gives you a private key which running applications can use along with the app ID and installation ID (tied to a repo or organization) to authenticate as that particular app installation. So whoever is running the test would probably have to create a dummy app that's installed to the repo being synced and provide those three values to the test, which isn't great.

risset avatar May 14 '24 16:05 risset

Can we automate or at least document clearly how I (an arbitrary person) could create my own "just for testing" app? Or that we could set up thru GH actions to run, but maybe an arbitrary person cant?

thockin avatar May 14 '24 16:05 thockin

I could create documentation for creating a testing app. Automation might be tricky since there's no GitHub API endpoints or the like for creating apps but it's worth looking into further

risset avatar May 14 '24 18:05 risset

Update from me on this, I've got the change working now and there's a couple things I'm unsure about before opening a PR (should have that done by the weekend)

  • Where should documentation for creating a github app for testing go? The docs directory seemed more user-facing
  • I've been testing my change by running a git-sync container in a similar way to the current github https e2e test, but with the addition of the new github app args which need to be supplied. Any ideas on how this can be integrated cleanly alongside the existing tests?

Thanks :)

risset avatar May 16 '24 09:05 risset

Doc could go in "docs/dev/testing_github_app_auth.md" or something like that?

Tests: I am not sure I follow the question. I don't have a way to write an e2e test that doesn't always run, but that seems plausibly useful. If you write a PR which treats it like a normal e2e function, I will find a way to let you designate it as "skipped by default" or something

thockin avatar May 16 '24 16:05 thockin

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot avatar Aug 14 '24 17:08 k8s-triage-robot

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle rotten
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot avatar Sep 13 '24 17:09 k8s-triage-robot