Add support for GitHub app authentication #878

risset · 2024-05-19T19:31:41Z

Related issue: #877

Currently the e2e::github_app_auth test should work locally, if the correct env vars are set. I've tested the feature that way before creating the PR. Ideally it would be possible to run it in the GHA workflow. One way I thought that might be feasible is if an app was created and installed to a particular (private) repo, and then its private key was stored as a git-sync repository secret.

Some information about GitHub app authentication: https://docs.github.com/en/apps/creating-github-apps/authenticating-with-a-github-app

linux-foundation-easycla · 2024-05-19T19:31:48Z

The committers listed above are authorized under a signed CLA.

✅ login: risset / name: Liam Wyllie (5822b73)
✅ login: nan-yu / name: Nan Yu (ba2fe67)

k8s-ci-robot · 2024-05-19T19:31:50Z

Welcome @risset!

It looks like this is your first PR to kubernetes/git-sync 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/git-sync has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

thockin

I'll have to think more about this when I get a second. It looks pretty clean overall.

Also need to add flags to the manual (at the bottom of the file).

I'll think about how to make e2e tests optional.

thockin · 2024-05-20T16:09:47Z

main.go

+		_ = resp.Body.Close()
+	}()
+	if resp.StatusCode != 201 {
+		errMessage, err := io.ReadAll(resp.Body)


Shouldn't we bound the read here to 1K or 4K or 32K or something?

That would make sense, should the same apply to the CallAskPassURL usage of io.ReadAll?

Missed this before, yeah, it should :)

You know what, this is fine. I'm just being paranoid. Doing a bounded copy is nice, but I won't require it here.

thockin · 2024-05-20T16:11:21Z

main.go

@@ -1851,6 +1878,72 @@ func (git *repoSync) CallAskPassURL(ctx context.Context) error {
 	return nil
 }

+func (git *repoSync) RefreshAppToken(ctx context.Context, githubBaseURL, privateKey string, appID, installationID int) error {


I'd like the entire set of function names to reflect "GitHubApp" rather than just app, so it's easier to read. Could also do with a nice comment, please

thockin · 2024-05-20T16:14:43Z

main.go

@@ -254,6 +257,19 @@ func main() {
 		envString("", "GITSYNC_ASKPASS_URL", "GIT_SYNC_ASKPASS_URL", "GIT_ASKPASS_URL"),
 		"a URL to query for git credentials (username=<value> and password=<value>)")

+	flGithubAPIURL := pflag.String("github-api-url",


it's not really a URL is it? Usually a URL includes "scheme" or "protocol", but later you prepend "https://". Also - is this allowed to have paths? E.g. could I say "notgithub.51.al/mytenant/path" ?

I propose to either make this a URL ("https://host:port/path") or rename it to "host" or "domain".

What do you think?

Yeah you're right that

I think having it as an actual base URL is better, I'll make this change

thockin · 2024-05-20T16:16:58Z

main.go

@@ -254,6 +257,19 @@ func main() {
 		envString("", "GITSYNC_ASKPASS_URL", "GIT_SYNC_ASKPASS_URL", "GIT_ASKPASS_URL"),
 		"a URL to query for git credentials (username=<value> and password=<value>)")

+	flGithubAPIURL := pflag.String("github-api-url",
+		envString("api.github.com", "GITHUB_API_URL"),


Is this env name special (e.g. used somewhere deep in a library)? I don't think so, right?

All the othe env vars are named "GITSYNC_xyz", so these should probably be "GITSYNC_GITHUB_APP_xyz"

thockin · 2024-05-20T16:19:12Z

main.go

+		envString("", "GITHUB_APP_PRIVATE_KEY"),
+		"the GitHub app private key to use for GitHub app auth")
+	flGithubAppApplicationID := pflag.Int("github-app-application-id",
+		envInt(-1, "GITHUB_APP_APPLICATION_ID"),


why not 0 for default? Is that a valid value?

I wasn't 100% sure that it couldn't be a valid value, but now you mention it, it doesn't seem likely that the IDs would start at 0 anyway. I'll have them default to 0

thockin · 2024-05-20T16:22:09Z

main.go

+	flGithubAPIURL := pflag.String("github-api-url",
+		envString("api.github.com", "GITHUB_API_URL"),
+		"the API URL to use when making requests to GitHub when using app auth")
+	flGithubAppPrivateKey := pflag.String("github-app-private-key",


We need to validate these (see ~ L493). Specifically:

You can't use these and some other form of auth at the same time

If user sets this flag, they also need to set the app ID and installation ID

Actually, we should not put private keys as flags, they are listable via /proc. The --password flag is bad for the same reason.

We should have a way to declare a non-flag which can be set by env but not via CLI. I will think about that. It's going to be a busy few weeks, so please hang tight.

Also, we should be able to pass this in via a file, like --password-file. For all of these, I'd like it to be possible to use a kubernetes secret and mount a volume, rather than flags/env vars. I'll have to refresh myself on formatting, i think we can only demand one value per file. So maybe we want an alternative --github-app-config=<dir> with documented files within it, e.g. api-url (optional), private-key (required), application-id (required), and installation-id (required). That way a user could make a single k8s Secret object with those key, and use a Secret-type volume.

It's going to be a busy few weeks, so please hang tight.

No worries! thanks for taking the time for the detailed review, I will address the simple things you brought up in the meantime

+1 for using a single k8s Secret object with these keys and add validation for them. It would be much easier for the downstream service to provide these values.

thockin · 2024-05-20T16:24:54Z

main.go

@@ -789,6 +806,16 @@ func main() {
 			}
 			metricAskpassCount.WithLabelValues(metricKeySuccess).Inc()
 		}
+
+		if *flGithubAppPrivateKey != "" && *flGithubAppInstallationID != -1 && *flGithubAppApplicationID != -1 {
+			if git.appTokenExpiry.Before(time.Now()) {


Shouldn't there be some buffer here, so we don't fail TOCTOU ? E.g. require max(20% of the token's lifespan, 30s) or something?

I don't want tokens to be valid here but fail to sync 1 second later, and some of the ops we do can take O(seconds).

That makes sense

thockin · 2024-05-20T16:25:48Z

main.go

+		if *flGithubAppPrivateKey != "" && *flGithubAppInstallationID != -1 && *flGithubAppApplicationID != -1 {
+			if git.appTokenExpiry.Before(time.Now()) {
+				if err := git.RefreshAppToken(ctx, *flGithubAPIURL, *flGithubAppPrivateKey, *flGithubAppApplicationID, *flGithubAppInstallationID); err != nil {
+					metricAskpassCount.WithLabelValues(metricKeyError).Inc()


I think we should have a new metric for this

This should be a different metric - it's not askpass

thockin · 2024-05-20T16:39:40Z

test_e2e.sh

+##############################################
+# Test github app auth
+##############################################
+function e2e::github_app_auth() {


Put this with the other e2e::auth_* tests and name it e2e::auth_github_app.

k8s-ci-robot · 2024-05-24T15:19:33Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: risset
Once this PR has been reviewed and has the lgtm label, please ask for approval from thockin. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

nan-yu · 2024-06-04T18:05:39Z

main.go

+		if *flGithubAppInstallationID != 0 {
+			handleConfigError(log, true, "ERROR: --github-app-installation-id may only be specified when --github-app-private-key-file is specified")
+		}
+	}


This makes the Github app auth mutually exclusive with username/password. What about other auth types, like ssh, cookiefile, or etc?

I think I omitted those because I didn't see them being checked in the existing validations, but I can add them sure

Yeah, this is a mess. Was OK when it was 3 mutually exclusive modes, but it's getting out of hand. OK for now, but we should plan a followup which is more declarative feeling and handles the mutual exclusion more automatically.

nan-yu · 2024-06-04T18:07:52Z

main.go

+		return err
+	}
+
+	url, err := url.JoinPath(githubBaseURL, fmt.Sprintf("installations/%d/access_tokens", installationID))


The path should be app/installations/%d/access_tokens.

nan-yu · 2024-06-04T18:11:47Z

main.go

@@ -254,6 +257,19 @@ func main() {
 		envString("", "GITSYNC_ASKPASS_URL", "GIT_SYNC_ASKPASS_URL", "GIT_ASKPASS_URL"),
 		"a URL to query for git credentials (username=<value> and password=<value>)")

+	flGithubBaseURL := pflag.String("github-base-url",
+		envString("https://api.github.com/", "GITSYNC_GITHUB_BASE_URL", "GIT_SYNC_GITHUB_BASE_URL"),


These flags are new, so you don't need the alternative keys with the GIT_SYNC prefix for backward compatibility.

Thanks, addressed this

nan-yu · 2024-06-04T18:12:50Z

docs/dev/testing_github_app_auth.md

+## Step 2: Export the necessary environment variables
+
+The following environment variables are *required* to run the git-sync github app auth test:
+- `GITHUB_APP_PRIVATE_KEY`


These env variables need to have the GITSYNC_ prefix.

These ones don't correspond to the actual application env vars though, they're the values that get passed in via flags to the GIT_SYNC function in the test

nan-yu · 2024-06-04T18:14:42Z

docs/dev/testing_github_app_auth.md

+Should have been saved when creating the app
+
+### GITHUB_APP_APPLICATION_ID
+The value after "App ID" in the app's settings page


Suggest adding the page URL: https://github.com/settings/apps/<unique_app_name>

nan-yu · 2024-06-04T18:52:19Z

While setting up a Github app, I noticed instead of using App ID to get installation tokens, it also supports using the Client ID.

I'm wondering have you experimented with the client ID?

risset · 2024-06-09T17:00:34Z

While setting up a Github app, I noticed instead of using App ID to get installation tokens, it also supports using the Client ID.

I'm wondering have you experimented with the client ID?

I didn't know about this, thanks. It appears to be a recent feature: https://github.blog/changelog/2024-05-01-github-apps-can-now-use-the-client-id-to-fetch-installation-tokens/

So the App ID and Client ID can be used interchangeably in this case.

The application ID is not being deprecated at this time, nor are their plans to remove it. However, compatibility with future features will rely on use of the client ID, so updating is recommended.

Should we have both the app ID and client ID as parameters and accept either of them, or just replace app ID with client ID?

nan-yu · 2024-06-10T20:38:22Z

Should we have both the app ID and client ID as parameters and accept either of them, or just replace app ID with client ID?

I would vote for supporting both scenarios.

main.go

nan-yu · 2024-06-25T04:38:21Z

Hi @risset , any recent update on addressing the comments in this PR?

I'm working on Config Sync. If possible, we would like to leverage this feature in our next minor release in July.

thockin · 2024-06-25T05:13:44Z

Hi Nan, Before we can merge this we need to make sure we can test it properly, which involves a private key. I have not had the time to play and see how private it really needs to be, or if maybe we can get it to test via GH actions. If you have time, that's where we need to spend it.

…

On Mon, Jun 24, 2024, 9:38 PM Nan Yu ***@***.***> wrote: Hi @risset <https://github.com/risset> , any recent update on addressing the comments in this PR? I'm working on Config Sync <https://cloud.google.com/kubernetes-engine/enterprise/config-sync/docs/overview>. If possible, we would like to leverage this feature in our next minor release in July. — Reply to this email directly, view it on GitHub <#878 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKWAVAJOQGHD6LXYS6QTZDZJDX5HAVCNFSM6AAAAABH6TO2F6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBXHE3DANBRG4> . You are receiving this because you were assigned.Message ID: ***@***.***>

risset · 2024-06-25T18:51:53Z

Hi @risset , any recent update on addressing the comments in this PR?

Hi, I will be working on this some more over the next few days! I've been having some issues with running test_e2e.sh on my current (macOS) machine, but I'm able to test my changes on another Linux machine at least.

nan-yu · 2024-07-01T20:55:34Z

Hi Liam Wyllie , any recent update on addressing the comments in this PR?

Hi, I will be working on this some more over the next few days! I've been having some issues with running test_e2e.sh on my current (macOS) machine, but I'm able to test my changes on another Linux machine at least.

Thanks for the update. Did you run the e2e test with a public repo or a private repo?
I followed the instruction with a private repo, and git-sync returned a repo not found error. Not sure if that works for you.

risset · 2024-07-03T14:33:29Z

Hi Liam Wyllie , any recent update on addressing the comments in this PR?

Hi, I will be working on this some more over the next few days! I've been having some issues with running test_e2e.sh on my current (macOS) machine, but I'm able to test my changes on another Linux machine at least.

Thanks for the update. Did you run the e2e test with a public repo or a private repo? I followed the instruction with a private repo, and git-sync returned a repo not found error. Not sure if that works for you.

I've been testing with a private repo. I'll double check the instructions and how I've set the app up etc. in case I missed something

risset · 2024-07-08T20:14:21Z

Hi Liam Wyllie , any recent update on addressing the comments in this PR?

Hi, I will be working on this some more over the next few days! I've been having some issues with running test_e2e.sh on my current (macOS) machine, but I'm able to test my changes on another Linux machine at least.

Thanks for the update. Did you run the e2e test with a public repo or a private repo? I followed the instruction with a private repo, and git-sync returned a repo not found error. Not sure if that works for you.

I've been testing with a private repo. I'll double check the instructions and how I've set the app up etc. in case I missed something

Does the app you created definitely have read-only or read-write access for Contents under Repository permissions, and is installed to the private repository itself (only select repositories) under Install App?

When I re-run the tests with either of those not true, I also get the repo not found error.

nan-yu · 2024-07-08T23:05:09Z

Hi Liam Wyllie , any recent update on addressing the comments in this PR?

Hi, I will be working on this some more over the next few days! I've been having some issues with running test_e2e.sh on my current (macOS) machine, but I'm able to test my changes on another Linux machine at least.

Thanks for the update. Did you run the e2e test with a public repo or a private repo? I followed the instruction with a private repo, and git-sync returned a repo not found error. Not sure if that works for you.

I've been testing with a private repo. I'll double check the instructions and how I've set the app up etc. in case I missed something

Does the app you created definitely have read-only or read-write access for Contents under Repository permissions, and is installed to the private repository itself (only select repositories) under Install App?

When I re-run the tests with either of those not true, I also get the repo not found error.

No worries. I think it was the issue with my testing account. I created a robot Github account. The account is flagged and requires reinstatement.

It works with my personal account.

risset · 2024-07-11T23:14:47Z

No worries. I think it was the issue with my testing account. I created a robot Github account. The account is flagged and requires reinstatement.

It works with my personal account.

Nice! Hopefully we can get this over the line. I'm still not quite sure how getting the tests to run properly via actions will work.

nan-yu · 2024-07-15T22:48:39Z

Regarding the e2e test for the github app integration, I created a robot GitHub account, a GitHub app, along with a private repository under the account. After that, I set up the required environment variables in GitHub Secret in my personal fork. All e2e test passed with changes in risset#1: https://github.com/nan-yu/git-sync/actions/runs/9947634472.

Can we use the robot account for testing, @thockin ?

thockin

Can you also update the README? Basically just delete the last section, build the binary and run it with --man >> README.md, then add the last triple-ticks.

Other than my own automated and manual testing, it looks good.

thockin · 2024-07-27T02:26:55Z

main.go

+		envString("", "GITSYNC_GITHUB_APP_PRIVATE_KEY_FILE"),
+		"the file from which the private key for GitHub app auth will be sourced")
+	flGithubAppClientID := pflag.String("github-app-client-id",
+		envString("", "GTSYNC_GITHUB_APP_CLIENT_ID"),


Typo: GTSYNC should be GITSYNC.

Same on the next one, too

main.go

thockin · 2024-07-27T02:34:36Z

main.go

+		if *flGithubAppInstallationID != 0 {
+			handleConfigError(log, true, "ERROR: --github-app-installation-id may only be specified when --github-app-private-key-file is specified")
+		}
+	}


Yeah, this is a mess. Was OK when it was 3 mutually exclusive modes, but it's getting out of hand. OK for now, but we should plan a followup which is more declarative feeling and handles the mutual exclusion more automatically.

thockin · 2024-07-27T02:38:43Z

main.go

+		if *flGithubAppPrivateKey != "" && *flGithubAppInstallationID != -1 && *flGithubAppApplicationID != -1 {
+			if git.appTokenExpiry.Before(time.Now()) {
+				if err := git.RefreshAppToken(ctx, *flGithubAPIURL, *flGithubAppPrivateKey, *flGithubAppApplicationID, *flGithubAppInstallationID); err != nil {
+					metricAskpassCount.WithLabelValues(metricKeyError).Inc()


This should be a different metric - it's not askpass

thockin · 2024-07-27T02:41:19Z

main.go

+		_ = resp.Body.Close()
+	}()
+	if resp.StatusCode != 201 {
+		errMessage, err := io.ReadAll(resp.Body)


Missed this before, yeah, it should :)

main.go

risset · 2024-07-27T17:48:31Z

Have updated to address recent feedback !

Pardon my ignorance here but I'm still a bit unsure on how best to address your comment on the usage of io.ReadAll to read the HTTP response body. Is reading with io.Copy to a bytes.Buffer preferable here, or is there a more efficient approach to that?

Also I've added one, but not sure if you feel a metric is actually needed for this feature, it was just sloppiness from me having the askpass code fragment left in there before ...

nan-yu

It looks mostly good to me. I would like to enable the e2e tests for this new feature.
Feel free to port over changes in https://github.com/risset/git-sync/pull/1/files.

nan-yu · 2024-07-27T18:43:48Z

main.go

@@ -2492,6 +2665,22 @@ AUTHENTICATION
            When --cookie-file ($GITSYNC_COOKIE_FILE) is specified, the
            associated cookies can contain authentication information.

+    github app
+           When --github-app-private-key-file ($GITSYNC_GITHUB_APP_PRIVATE_KEY_FILE),
+           --github-app-application-id ($GITSYNC_GITHUB_APP_APPLICATION_ID)


--github-app-application-id or --github-app-client-id

nan-yu · 2024-07-27T18:49:07Z

test_e2e.sh

@@ -295,6 +295,7 @@ function GIT_SYNC() {
        -v "$DOT_SSH/1/id_test":"/ssh/secret.1":ro \
        -v "$DOT_SSH/2/id_test":"/ssh/secret.2":ro \
        -v "$DOT_SSH/3/id_test":"/ssh/secret.3":ro \
+	-v "$(pwd)/$GITHUB_APP_PRIVATE_KEY_FILE":"/github_app_private_key.pem":ro \


This requires the github app private key file to be located at the same directory. It won't work with the e2e test in the GitHub Actions Workflow.

I'm not sure if you get a chance to take a look at risset#1. Basically, it loads the private key from GitHub Secrets and stored it in a temp file. It would be good to incorporate it in this PR and enables the e2e test.

I will take a look at that

Thanks. I updated risset#1 to make the github app auth test skippable.

Merged this in now

thockin

I am AFK for a couple weeks, and don't want to merge until I can run some.manual tests, but it looks great

thockin · 2024-07-27T20:35:02Z

main.go

+
+	metricRefreshGitHubAppTokenCount = prometheus.NewCounterVec(prometheus.CounterOpts{
+		Name: "git_sync_refresh_github_app_token_count",
+		Help: "How many times github app token was refreshed, partitioned by state (success, error)",


Super nit: "the GitHub app token"

thockin · 2024-07-27T20:42:01Z

main.go

@@ -838,6 +896,17 @@ func main() {
 			}
 			metricAskpassCount.WithLabelValues(metricKeySuccess).Inc()
 		}
+
+		if *flGithubAppPrivateKeyFile != "" && *flGithubAppInstallationID != 0 && (*flGithubAppApplicationID != 0 || *flGithubAppClientID != "") {


Should this be considering the newly added environment also?

thockin · 2024-07-27T20:43:00Z

main.go

+func (git *repoSync) RefreshGitHubAppToken(ctx context.Context, githubBaseURL, privateKeyFile, clientID string, appID, installationID int) error {
+	git.log.V(3).Info("refreshing GitHub app token")
+
+	privateKeyBytes, err := os.ReadFile(privateKeyFile)


Should this consider the environment var key?

Forgot about this, added it now

thockin · 2024-07-27T20:44:34Z

main.go

+		_ = resp.Body.Close()
+	}()
+	if resp.StatusCode != 201 {
+		errMessage, err := io.ReadAll(resp.Body)


You know what, this is fine. I'm just being paranoid. Doing a bounded copy is nice, but I won't require it here.

thockin · 2024-07-27T20:46:39Z

main.go

+
+    --github-app-application-id <int>, $GITSYNC_GITHUB_APP_APPLICATION_ID
+            The app ID of the GitHub app used for GitHub app authentication.
+            One of --github-app-application-id or --github-app-client-id is required.


...when GitHub app authentication is used.

Same below

Signed-off-by: Liam Wyllie <risset@mailbox.org>

The GitHub app e2e test requires a GitHub app to be created and installed, and also requires a few environment variables to be set. This commit updates the GitHub action workflow by providing the environment variables which can be set via GitHub Secret. GitHub Secrests cannot start with `GITHUB_`. Hence, this commit prepends `TEST_` to the env variables. It also updates how GitHub app private key file is set. It can be set by either `TEST_GITHUB_APP_PRIVATE_KEY` or `TEST_GITHUB_APP_PRIVATE_KEY_FILE`.

sdowell · 2024-07-31T20:45:01Z

test_e2e.sh

+# Set the trap to call the final_cleanup function on exit.
+trap final_cleanup EXIT
+
+skip_github_app_test="${SKIP_GITHUB_APP_TEST:-false}"


nit: I think skipping may be a more user friendly default, since in most cases a developer won't have github app credentials ready to use

That makes sense as long as the github action workflow doesn't skip the github app test.

sdowell · 2024-08-01T22:45:33Z

test_e2e.sh

+skip_github_app_test="${SKIP_GITHUB_APP_TEST:-false}"
+required_env_vars=()
+LOCAL_GITHUB_APP_PRIVATE_KEY_FILE="github_app_private_key.pem"
+GITHUB_APP_PRIVATE_KEY_MOUNT=""


This should be declared as an empty array so that it is properly expanded below when the volume mount is not added.

e.g.

GITHUB_APP_PRIVATE_KEY_MOUNT=() ... # Mount the GitHub App private key file to the git-sync container GITHUB_APP_PRIVATE_KEY_MOUNT+=(-v "${TEST_GITHUB_APP_PRIVATE_KEY_FILE}:/${LOCAL_GITHUB_APP_PRIVATE_KEY_FILE}:ro") ... "${GITHUB_APP_PRIVATE_KEY_MOUNT[@]}" \ "${GIT_SYNC_E2E_IMAGE}" \

Changing it to an empty array looks good to me. Thanks for pointing it out.

sdowell · 2024-09-05T22:45:10Z

@risset @thockin Is this PR still being worked on?

risset · 2024-09-05T23:47:17Z

@risset @thockin Is this PR still being worked on?

Yeah I was just waiting for @thockin as he said he'd be away for a while. Also I think your review comments make sense but since @nan-yu wrote those parts of test_e2e.sh, I thought they might have some thoughts on it before I change those things. @nan-yu what do you think?

thockin · 2024-09-05T23:49:12Z

Yes, the ball is in my court to evaluate testing. I apologize, it has been a chaotic few weeks since I got back from holiday. I have not forgotten it.

thockin · 2024-09-06T17:02:04Z

test_e2e.sh

+    if [[ ! -v TEST_GITHUB_APP_PRIVATE_KEY_FILE || -z "${TEST_GITHUB_APP_PRIVATE_KEY_FILE}" ]]; then
+      TEST_GITHUB_APP_PRIVATE_KEY_FILE="${DIR}/${LOCAL_GITHUB_APP_PRIVATE_KEY_FILE}"
+    fi
+    echo "${TEST_GITHUB_APP_PRIVATE_KEY}" > "${TEST_GITHUB_APP_PRIVATE_KEY_FILE}"


I'm worried about this path, seems like an opportunity to blow away a file we shouldn't. Maybe we can make a temp file and use that?

TEST_GITHUB_APP_PRIVATE_KEY_FILE points to a temp file when it is either unset or blank. Another option is to always use the temp file regardless of whether the variable is set, i.e., remove the if condition in line 222.

if [[ -v TEST_GITHUB_APP_PRIVATE_KEY && -n "${TEST_GITHUB_APP_PRIVATE_KEY}" ]]; then TEST_GITHUB_APP_PRIVATE_KEY_FILE="${DIR}/${LOCAL_GITHUB_APP_PRIVATE_KEY_FILE}" echo "${TEST_GITHUB_APP_PRIVATE_KEY}" > "${TEST_GITHUB_APP_PRIVATE_KEY_FILE}" fi

I'm making a few edits as I look at this, will post them here. May not finish today.

OK, I dug into the testing in depth and set up my own app. With the below diff I am happy with this PR, but I have not yet looked at making CI work (with GH secrets)

diff --git a/test_e2e.sh b/test_e2e.sh index 9b2023f..142d7c1 100755 --- a/test_e2e.sh +++ b/test_e2e.sh @@ -41,6 +41,11 @@ function fail() { return 42 } +function skip() { + echo "SKIP" >&3 + return 43 +} + function pass() { echo "PASS" } @@ -204,37 +209,43 @@ function final_cleanup() { # Set the trap to call the final_cleanup function on exit. trap final_cleanup EXIT -skip_github_app_test="${SKIP_GITHUB_APP_TEST:-false}" +skip_github_app_test="${SKIP_GITHUB_APP_TEST:-true}" required_env_vars=() LOCAL_GITHUB_APP_PRIVATE_KEY_FILE="github_app_private_key.pem" -GITHUB_APP_PRIVATE_KEY_MOUNT="" +GITHUB_APP_PRIVATE_KEY_MOUNT=() if [[ "${skip_github_app_test}" != "true" ]]; then required_env_vars=( "TEST_GITHUB_APP_AUTH_TEST_REPO" "TEST_GITHUB_APP_APPLICATION_ID" "TEST_GITHUB_APP_INSTALLATION_ID" "TEST_GITHUB_APP_CLIENT_ID" - "TEST_GITHUB_APP_PRIVATE_KEY_FILE" ) - # TEST_GITHUB_APP_PRIVATE_KEY, if set, overrides TEST_GITHUB_APP_PRIVATE_KEY_FILE - if [[ -v TEST_GITHUB_APP_PRIVATE_KEY && -n "${TEST_GITHUB_APP_PRIVATE_KEY}" ]]; then - if [[ ! -v TEST_GITHUB_APP_PRIVATE_KEY_FILE || -z "${TEST_GITHUB_APP_PRIVATE_KEY_FILE}" ]]; then - TEST_GITHUB_APP_PRIVATE_KEY_FILE="${DIR}/${LOCAL_GITHUB_APP_PRIVATE_KEY_FILE}" - fi - echo "${TEST_GITHUB_APP_PRIVATE_KEY}" > "${TEST_GITHUB_APP_PRIVATE_KEY_FILE}" + if [[ -n "${TEST_GITHUB_APP_PRIVATE_KEY_FILE:-}" && -n "${TEST_GITHUB_APP_PRIVATE_KEY:-}" ]]; then + echo "ERROR: Both TEST_GITHUB_APP_PRIVATE_KEY_FILE and TEST_GITHUB_APP_PRIVATE_KEY were specified." + exit 1 + fi + if [[ -n "${TEST_GITHUB_APP_PRIVATE_KEY_FILE:-}" ]]; then + cp "${TEST_GITHUB_APP_PRIVATE_KEY_FILE}" "${DIR}/${LOCAL_GITHUB_APP_PRIVATE_KEY_FILE}" + elif [[ -n "${TEST_GITHUB_APP_PRIVATE_KEY:-}" ]]; then + echo "${TEST_GITHUB_APP_PRIVATE_KEY}" > "${DIR}/${LOCAL_GITHUB_APP_PRIVATE_KEY_FILE}" + else + echo "ERROR: Neither TEST_GITHUB_APP_PRIVATE_KEY_FILE nor TEST_GITHUB_APP_PRIVATE_KEY was specified." + echo " Either provide a value or skip this test (SKIP_GITHUB_APP_TEST=true)." + exit 1 fi # Validate all required environment variables for the github-app-auth tests are provided. for var in "${required_env_vars[@]}"; do if [[ ! -v "${var}" ]]; then - echo "Error: Required environment variable '${var}' is not set or empty. Either provide a value or skip the GitHub App test by setting SKIP_GITHUB_APP_TEST to 'true'." + echo "ERROR: Required environment variable '${var}' is not set." + echo " Either provide a value or skip this test (SKIP_GITHUB_APP_TEST=true)." exit 1 fi done # Mount the GitHub App private key file to the git-sync container - GITHUB_APP_PRIVATE_KEY_MOUNT=(-v "${TEST_GITHUB_APP_PRIVATE_KEY_FILE}":"/${LOCAL_GITHUB_APP_PRIVATE_KEY_FILE}":ro) + GITHUB_APP_PRIVATE_KEY_MOUNT=(-v "${DIR}/${LOCAL_GITHUB_APP_PRIVATE_KEY_FILE}":"/${LOCAL_GITHUB_APP_PRIVATE_KEY_FILE}":ro) fi # WORK is temp space and in reset for each testcase. @@ -2232,7 +2243,7 @@ function e2e::auth_askpass_url_slow_start() { ############################################## function e2e::auth_github_app_application_id() { if [[ "${skip_github_app_test}" == "true" ]]; then - return + skip fi GIT_SYNC \ --one-time \ @@ -2247,7 +2258,7 @@ function e2e::auth_github_app_application_id() { function e2e::auth_github_app_client_id() { if [[ "${skip_github_app_test}" == "true" ]]; then - return + skip fi GIT_SYNC \ --one-time \ @@ -3658,6 +3669,8 @@ for t; do run_test RUN_RET "${TEST_FN}" >"${LOG}.${RUN}" 2>&1 if [[ "$RUN_RET" == 0 ]]; then pass + elif [[ "$RUN_RET" == 43 ]]; then + true # do nothing else TEST_RET=1 if [[ "$RUN_RET" != 42 ]]; then

thockin · 2024-09-06T17:51:24Z

Since this is a kubernetes repo, I'll have to see about making the kubernetes org own the app

HorusTheSonOfOsiris · 2024-09-13T22:05:05Z

Since this is a kubernetes repo, I'll have to see about making the kubernetes org own the app

Any plans on releasing this soon? This could help our team a lot.

thockin · 2024-09-25T18:51:35Z

I'm going to merge this and followup. I Still don't have CI for GH apps up, though.

k8s-ci-robot requested review from nan-yu and stp-ip May 19, 2024 19:31

thockin reviewed May 20, 2024

View reviewed changes

thockin self-assigned this May 20, 2024

risset force-pushed the add-support-for-github-app-authentication branch from 7e947b5 to 834bc50 Compare May 24, 2024 15:19

nan-yu reviewed Jun 4, 2024

View reviewed changes

risset force-pushed the add-support-for-github-app-authentication branch from 834bc50 to 05e4771 Compare June 9, 2024 16:52

risset force-pushed the add-support-for-github-app-authentication branch from 261b10d to 10f7d70 Compare June 10, 2024 16:12

thockin reviewed Jun 13, 2024

View reviewed changes

main.go Show resolved Hide resolved

risset force-pushed the add-support-for-github-app-authentication branch from 10f7d70 to 8aab47c Compare June 26, 2024 23:33

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 17, 2024

risset force-pushed the add-support-for-github-app-authentication branch from 8aab47c to d7ecc0b Compare July 18, 2024 08:11

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2024

thockin reviewed Jul 27, 2024

View reviewed changes

risset force-pushed the add-support-for-github-app-authentication branch from d7ecc0b to 3ea1801 Compare July 27, 2024 17:33

nan-yu reviewed Jul 27, 2024

View reviewed changes

thockin reviewed Jul 27, 2024

View reviewed changes

risset force-pushed the add-support-for-github-app-authentication branch from 10d816d to 12d712e Compare July 31, 2024 20:02

risset and others added 2 commits July 31, 2024 21:08

Add support for GitHub app authentication

5822b73

Signed-off-by: Liam Wyllie <risset@mailbox.org>

risset force-pushed the add-support-for-github-app-authentication branch from 12d712e to ba2fe67 Compare July 31, 2024 20:08

sdowell reviewed Jul 31, 2024

View reviewed changes

sdowell reviewed Aug 1, 2024

View reviewed changes

thockin reviewed Sep 6, 2024

View reviewed changes

thockin merged commit 8441240 into kubernetes:master Sep 25, 2024
1 of 2 checks passed

BrewTestBot mentioned this pull request Sep 26, 2024

git-sync 4.3.0 Homebrew/homebrew-core#191921

Merged

Add support for GitHub app authentication #878

Add support for GitHub app authentication #878

Conversation

risset commented May 19, 2024

linux-foundation-easycla bot commented May 19, 2024 • edited Loading

k8s-ci-robot commented May 19, 2024

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin May 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

risset May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nan-yu commented Jun 4, 2024

risset commented Jun 9, 2024

nan-yu commented Jun 10, 2024

nan-yu commented Jun 25, 2024

thockin commented Jun 25, 2024 via email

risset commented Jun 25, 2024

nan-yu commented Jul 1, 2024

risset commented Jul 3, 2024

risset commented Jul 8, 2024

nan-yu commented Jul 8, 2024

risset commented Jul 11, 2024

nan-yu commented Jul 15, 2024

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

risset commented Jul 27, 2024

nan-yu left a comment • edited Loading

Choose a reason for hiding this comment

nan-yu Jul 27, 2024 • edited Loading

Choose a reason for hiding this comment

nan-yu Jul 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented May 19, 2024 •

edited

Loading

thockin May 20, 2024 •

edited

Loading

risset May 21, 2024 •

edited

Loading

nan-yu left a comment •

edited

Loading

nan-yu Jul 27, 2024 •

edited

Loading

nan-yu Jul 27, 2024 •

edited

Loading