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

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dipti-pai
Copy link
Member

  • 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
Copy link

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

Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

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.

docs/spec/v1/gitrepositories.md Outdated Show resolved Hide resolved
docs/spec/v1/gitrepositories.md Outdated Show resolved Hide resolved
docs/spec/v1/gitrepositories.md Outdated Show resolved Hide resolved
docs/spec/v1/gitrepositories.md Outdated Show resolved Hide resolved
docs/spec/v1/gitrepositories.md Outdated Show resolved Hide resolved
@stefanprodan
Copy link
Member

stefanprodan commented Dec 6, 2024

@dipti-pai please rebase with main, it contains the latest auth and git packages with GitHub App support.

- 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.

Signed-off-by: Dipti Pai <[email protected]>
if obj.Spec.SecretRef == nil {
e := serror.NewStalling(
fmt.Errorf("secretRef with github app data must be specified when provider is set to github"),
sourcev1.AuthenticationFailedReason,
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the way we use AuthenticationFailedReason today is for actual authentication failures which are retried. Using AuthenticationFailedReason with stalled condition seems incorrect. The problem is a misconfiguration, invalid configuration. We have URLInvalidReason, InvalidSTSConfigurationReason, etc which are explicit about the type of failure. In this case, we haven't even attempted authentication but we know that authentication won't work. I think it would be better to introduce a new reason for this, similar to STS configuration, say InvalidProviderConfigurationReason.

I think the same can be used for the case below where the secret contains github app data but the provider is not set, also invalid provider configuration.

Copy link
Contributor

@darkowlzz darkowlzz Dec 19, 2024

Choose a reason for hiding this comment

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

I pushed a separate commit introducing InvalidProviderConfigurationReason and using it above. I'll change it in case of any objection.

Also, added a few tests for checking this reason on failure in the status conditions.
Also, slightly modified and rearranged a test case where secret ref is provided but the secret doesn't exist. This is not specific to github provider, but generic in getAuthOpts().

Introduce InvalidProviderConfigurationReason for Git provider github
related misconfiguration.

Add github provider related tests to check the status conditions reason.

Rearrange and modify a test case for getAuthOpts() for provider test
where a referred secret doesn't exist. This scenario is not specific to
any provider.

Signed-off-by: Sunny <[email protected]>
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.

4 participants