[RFC-007] Implement GitHub app authentication for git repositories.

[dipti-pai]

• API change to add new github provider field in GitRepository spec.
• Controller change to use the GitHub authentication information specified in .spec.secretRef to create the auth options to authenticate to git repositories when the provider field is set to github
• Tests for new github provider field
• Updated docs to use GitHub Apps for authentication in source-controller.

[dschniepp]

Thank you for picking up this topic. Can we also add documentation for the use with GitHub Enterprise?

[darkowlzz]

Did some basic testing and it worked as expected for the happy path

spec:
  interval: 5m0s
  provider: github
  ref:
    branch: main
  secretRef:
    name: github-sa
  timeout: 60s
  url: https://github.com/darkowlzz/test-1
status:
  artifact:
    digest: sha256:4166a3310c4f57471a5cfa2910475e4d47874920d7c156f656b7e70c01adcd2e
    lastUpdateTime: "2024-12-03T21:09:21Z"
    path: gitrepository/default/test-1/a4f6eca644dadd9112ff3d93c99eda63eeca949a.tar.gz
    revision: main@sha1:a4f6eca644dadd9112ff3d93c99eda63eeca949a
    size: 4083
    url: http://:0/gitrepository/default/test-1/a4f6eca644dadd9112ff3d93c99eda63eeca949a.tar.gz
  conditions:
  - lastTransitionTime: "2024-12-03T21:09:21Z"
    message: stored artifact for revision 'main@sha1:a4f6eca644dadd9112ff3d93c99eda63eeca949a'
    observedGeneration: 1
    reason: Succeeded
    status: "True"
    type: Ready
  - lastTransitionTime: "2024-12-03T21:09:21Z"
    message: stored artifact for revision 'main@sha1:a4f6eca644dadd9112ff3d93c99eda63eeca949a'
    observedGeneration: 1
    reason: Succeeded
    status: "True"
    type: ArtifactInStorage
  observedGeneration: 1

Left some comments, suggestions a few improvements.

[darkowlzz]

For completeness, we should also mention about the optional githubAppBaseURL that should be required for GitHub enterprise users.

[darkowlzz]

Suggested change

	The `github` provider can be used to authenticate to git repositories using
	The `github` provider can be used to authenticate to Git repositories using

[darkowlzz]

Not that it matters much, but all the lists in this document use - instead of *. Let's keep it consistent.

[darkowlzz]

This link is broken

Suggested change

	- [Register]((https://docs.github.com/en/apps/creating-github-apps/registering-a-github-app/registering-a-github-app))
	- [Register](https://docs.github.com/en/apps/creating-github-apps/registering-a-github-app/registering-a-github-app)

[darkowlzz]

Surrounding these values with "" would help indicate that these are to be strictly strings when used with Secret stringData. API validation on Secret v1 won't accept non-string value here. But since these IDs are just numbers, it's likely that the users will just paste the number value.

[darkowlzz]

Unlike Azure, GitHub provider requires a secret reference containing the identity details. Currently, not providing any secret ref results in the following state:

status:
  conditions:
  - lastTransitionTime: "2024-12-03T21:11:55Z"
    message: building artifact
    observedGeneration: 1
    reason: ProgressingWithRetry
    status: "True"
    type: Reconciling
  - lastTransitionTime: "2024-12-03T21:11:55Z"
    message: 'failed to checkout and determine revision: invalid app id, err: strconv.Atoi:
      parsing "": invalid syntax'
    observedGeneration: 1
    reason: GitOperationFailed
    status: "False"
    type: Ready
  - lastTransitionTime: "2024-12-03T21:11:50Z"
    message: 'failed to checkout and determine revision: invalid app id, err: strconv.Atoi:
      parsing "": invalid syntax'
    observedGeneration: 1
    reason: GitOperationFailed
    status: "True"
    type: FetchFailed
  observedGeneration: -1

and similar logs

{"level":"error","ts":"2024-12-04T02:42:13.583+0530","msg":"Reconciler error","controller":"gitrepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"GitRepository","GitRepository":{"name":"test-1","namespace":"default"},"namespace":"default","name":"test-1","reconcileID":"eaa78267-8c4a-4d85-9779-05226fff0b2b","error":"failed to checkout and determine revision: invalid app id, err: strconv.Atoi: parsing \"\": invalid syntax"}

I don't think these are friendly enough for the new users. Also, this is a bad configuration and should result in Stalled state.

❗ NOTE: The following suggestion can be ignored. See the next comment for another suggestion with considerations of more scenarios.

getAuthOpts() returns error that's interpreted as a generic error to attempt a retry. Rather than trying to return a typed error from getAuthOpts() and then try to return the appropriate error, it may be better to create a new function for just validating the provider related configuration (just for GitHub for now, but GitLab too in the future) and any error returned from it should result in a stalling error. We can call this function just before

source-controller/internal/controller/gitrepository_controller.go

Line 507 in 3b6896f

authOpts, err := r.getAuthOpts(ctx, obj, *u)

.

[darkowlzz]

This was discussed in the dev meeting today and we have two more scenarios related to this that should have validation:

• When a secret ref is provided but the provider is not set, and the secret contains github app data, it should result in a stalling failure with error saying that the secret contains github app data, please set the provider to github. It may be better to first check if the repository host is github.com before checking the secret.
• When a secret ref is provided and the provider is set to github, but the secret does not contain github app data fields, it should result in a retryable failure (generic error) with error saying that the secret must have the github app data. This is a validation of the provided secret content. Because we don't watch the secret, we don't have a way to get notified when the secret is fixed. We have to fail and retry, hoping that eventually the secret will be fixed.

With this, I'd backtrack a bit. Initially, I was recommending to validate that the secret ref is provided when github is the provider before getAuthOpts() as there was no secret involved. But with the two scenarios above, the secret content needs to be analyzed. It would be better to get the secret just once, which we already have in getAuthOpts().
I think we can change getAuthOpts() similar to the other functions, like verifySignature(), fetchIncludes() and others where the errors and conditions are set within these functions. The caller just returns the result. That way, we remain consistent with what's done in other functions.

[stefanprodan]

We need to implement flux create secret githubapp and mention it here as a way to generate this secret.