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

feat: Automount git credentials if they exist #564

Merged
merged 6 commits into from
Sep 6, 2021

Conversation

JPinkney
Copy link
Contributor

@JPinkney JPinkney commented Aug 27, 2021

What does this PR do?

This PR enables the ability to automount git credentials if they exist. The way it works is fundamentally the same as Che.

  1. Users provide a secret containing their credentials with a label of "controller.devfile.io/git-credential": "true"
  2. When a devworkspace is created the devworkspace controller looks for all secrets with that label, combines them into a new secret (so that you can have multiple credentials referenced in one credential file. See Ability to mount more than one Git credential secret eclipse-che/che#18721) and then mounts both that new combined secret and a system wide gitconfig that points to the secret so that user credentials are already picked up when cloning
  3. Devworkspace now has access to those credentials and can use them for cloning inside of project clone

What issues does this PR fix or reference?

#506

Is it tested? How?

  1. Create a secret on the cluster containing:
kind: Secret
apiVersion: v1
metadata:
  name: git-credentials-secret
  namespace: devworkspace-controller
  annotations:
    controller.devfile.io/mount-path: /home/theia/.git-credentials/
  labels:
    controller.devfile.io/git-credential: 'true'
data:
  credentials: >- ${your base64 credentials here}
type: Opaque

${your base64 credentials here} should be base64 encoded in the format of "https://{USERNAME}:{PERSONAL_ACCESS_TOKEN}@github.com". E.g. echo -n "https://{USERNAME}:{PERSONAL_ACCESS_TOKEN}@github.com" | base64
2. Create a private repo on github
3. Create a devworkspace that uses your private repo in the same namespace as your secret
4. See that project cloned correctly

PR Checklist

  • E2E tests pass (when PR is ready, comment /test v7-devworkspaces-operator-e2e, v7-devworkspace-happy-path to trigger)
    • v7-devworkspaces-operator-e2e: DevWorkspace e2e test
    • v7-devworkspace-happy-path: DevWorkspace e2e test

@JPinkney JPinkney changed the title Automount git credentials if they exist feat: Automount git credentials if they exist Aug 27, 2021
Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

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

I haven't done good review yet but from the first look, from the testing instructions:

    controller.devfile.io/mount-path: /home/theia/.git-credentials/

I would expect project clone to be using it but not theia.

@JPinkney
Copy link
Contributor Author

Technically the mount path can be anywhere and both project clone and theia will pick it up automatically. The only reason the mount path was:

 controller.devfile.io/mount-path: /home/theia/.git-credentials/

was so that I could test that the project clone was working and the credentials were also successfully mounted to theia to make sure that I could clone a private repo that wasn't in the devfile

@JPinkney
Copy link
Contributor Author

/test v7-devworkspaces-operator-e2e

@sleshchenko
Copy link
Member

/test v8-devworkspaces-operator-e2e

1 similar comment
@JPinkney
Copy link
Contributor Author

JPinkney commented Sep 1, 2021

/test v8-devworkspaces-operator-e2e

Signed-off-by: Josh Pinkney <joshpinkney@gmail.com>
@JPinkney
Copy link
Contributor Author

JPinkney commented Sep 2, 2021

/test v8-devworkspaces-operator-e2e

Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

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

I haven't had a chance to test it but changes LGTM now

@openshift-ci
Copy link

openshift-ci bot commented Sep 3, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JPinkney, sleshchenko

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

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [JPinkney,sleshchenko]

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

// Create the gitconfig volume mount and set it's location as /etc/gitconfig so it's automatically picked up by git
gitConfigMapVolumeMount := GetAutoMountConfigMapVolumeMount(gitConfigLocation, configMapName)
gitConfigMapVolumeMount.SubPath = gitConfigName
gitConfigMapVolumeMount.ReadOnly = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is readonly: false required here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not technically no, I just followed what Che is currently doing


podAdditions := &v1alpha1.PodAdditions{}
if len(credentials) > 0 {
gitCredsName := devworkspace.Status.DevWorkspaceId + "-" + gitConfigName
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm concerned that will effectively take a fixed number of secrets and result in a variable number of secrets based on the number of workspaces -- this could cause issues if a quota is in place.

Since all workspaces in a namespace should be fine sharing the combined credentials secret, we should be okay defining a fixed name (that's unlikely to collide with others, e.g. devworkspace-merged-git-credentials, if that fits in the max length) and using that.

Created #580 for follow-up

(Sorry to bump an already-merged PR, just got back from PTO)

Copy link
Member

Choose a reason for hiding this comment

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

effectively take a fixed number of secrets

I just wonder where this requirement is really coming from.
Like I understand limiting deployment/pods, they are heavy...
But configmap is like 1Mb at maximum right? Then we somebody should limit you to use only 10,20... )
But good point. We should save objects if it's possible and does not hurt anyone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants