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

core v1: deprecate the gitRepo volume type #63445

Merged

Conversation

ericchiang
Copy link
Contributor

@ericchiang ericchiang commented May 4, 2018

gitRepo stopped accepting new features nearly 2 years ago #17676 (comment) and today this behavior can easily be achieved through an init container. The kubelet shelling out to git in the host namespace can also be a security issue on un-trusted repos, as was demonstrated by CVE-2017-1000117. Our own documentation even alludes to this volume type being removed in the future:

In the future, such volumes may be moved to an even more decoupled model, rather than extending the Kubernetes API for every such use case.

https://kubernetes.io/docs/concepts/storage/volumes/#gitrepo

Edit: CVE 2018-11235 which also impacts this volume type was announced while this PR was open.

Closes #60999

The GitRepo volume type is deprecated. To provision a container with a git repo, mount an EmptyDir into an InitContainer that clones the repo using git, then mount the EmptyDir into the Pod's container.

/release-note-action-required

Instead of this:

apiVersion: v1
kind: Pod
metadata:
  name: server
spec:
  containers:
  - image: nginx
    name: nginx
    volumeMounts:
    - mountPath: /mypath
      name: git-volume
  volumes:
  - name: git-volume
    gitRepo:
      repository: "git@somewhere:me/my-git-repository.git"
      revision: "22f1d8406d464b0c0874075539c1f2e96c253775"

Do this:

apiVersion: v1
kind: ConfigMap
metadata:
  name: git-clone
data:
  git-clone.sh: |
    #!/bin/sh -e
    REPO=$1
    REF=$2
    DIR=$3
    # Init Containers will re-run on Pod restart. Remove the directory's contents
    # and reprovision when this happens.
    if [ -d "$DIR" ]; then
        rm -rf $( find $DIR -mindepth 1 )
    fi
    git clone $REPO $DIR
    cd $DIR
    git reset --hard $REF
---
apiVersion: v1
kind: Pod
metadata:
  name: server
spec:
  initContainers:
  - name: git-clone
    image: alpine/git # Any image with git will do
    command:
    - /usr/local/git/git-clone.sh
    args:
    - "https://somewhere/me/my-git-repository.git"
    - "22f1d8406d464b0c0874075539c1f2e96c253775"
    - "/mypath"
    volumeMounts:
    - name: git-clone
      mountPath: /usr/local/git
    - name: git-repo
      mountPath: /mypath
  containers:
  - image: nginx
    name: nginx
    volumeMounts:
    - mountPath: /mypath
      name: git-volume
  volumes:
  - name: git-volume
    emptyDir: {}
  - name: git-clone
    configMap:
      name: git-clone
      defaultMode: 0755

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 4, 2018
@k8s-github-robot k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label May 4, 2018
@ericchiang ericchiang force-pushed the deprecate-git-repo-volume branch 2 times, most recently from 5d912f9 to b493c9e Compare May 4, 2018 21:10
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 4, 2018
@ericchiang ericchiang added the release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. label May 4, 2018
@ericchiang
Copy link
Contributor Author

cc @kubernetes/sig-storage-pr-reviews
cc @tallclair

@k8s-ci-robot k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. labels May 4, 2018
@msau42
Copy link
Member

msau42 commented May 7, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 7, 2018
@liggitt
Copy link
Member

liggitt commented May 8, 2018

as much as I'd be happy to see this deprecated/removed, just dropping the field from v1 would fall outside of kube deprecation policies.

in additional to deciding if this is an acceptable deviation (@kubernetes/sig-storage-api-reviews @kubernetes/api-approvers), and documenting why, we should think through how this removal would actually be accomplished without breaking existing data (simply dropping the field would leave previously valid objects as invalid pod templates with volumes with no source)

@ericchiang
Copy link
Contributor Author

Sent a docs only PR for the meantime kubernetes/website#8429

@tallclair tallclair added this to the v1.11 milestone May 30, 2018
@tallclair tallclair added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label May 30, 2018
@tallclair tallclair removed this from the v1.11 milestone May 30, 2018
@tallclair tallclair removed priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. milestone/incomplete-labels labels May 30, 2018
@tallclair
Copy link
Member

What do you think about at least marking this as deprecated in 1.11, even if it takes a long time to get it removed?

@ericchiang
Copy link
Contributor Author

@tallclair I still have a PR open to strike it from the docs. kubernetes/website#8429

I think that's good enough.

@bgrant0607
Copy link
Member

This will also need an "action required" release note.

@ericchiang ericchiang added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 31, 2018
@ericchiang
Copy link
Contributor Author

/test pull-kubernetes-kubemark-e2e-gce-big

@ericchiang
Copy link
Contributor Author

/retest

@ericchiang
Copy link
Contributor Author

/test pull-kubernetes-kubemark-e2e-gce-big

@msau42
Copy link
Member

msau42 commented May 31, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ericchiang, liggitt, msau42

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:

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

@ericchiang ericchiang added this to the v1.11 milestone May 31, 2018
@ericchiang ericchiang added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label May 31, 2018
@ericchiang ericchiang added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label May 31, 2018
@k8s-github-robot
Copy link

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@ericchiang @liggitt @msau42

Pull Request Labels
  • sig/storage: Pull Request will be escalated to these SIGs if needed.
  • priority/important-longterm: Escalate to the pull request owners; move out of the milestone after 1 attempt.
  • kind/cleanup: Adding tests, refactoring, fixing old bugs.
Help

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 63445, 63820). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 26caa84 into kubernetes:master May 31, 2018
@k8s-ci-robot
Copy link
Contributor

@ericchiang: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce f8f5f04 link /test pull-kubernetes-e2e-gce
pull-kubernetes-kubemark-e2e-gce-big f8f5f04 link /test pull-kubernetes-kubemark-e2e-gce-big

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@ericchiang ericchiang deleted the deprecate-git-repo-volume branch June 1, 2018 06:29
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. labels Jun 1, 2018
@sebgoa
Copy link
Contributor

sebgoa commented Jun 8, 2018

fwiw in @ericchiang example at the top, git-repo reference in initContainers should be git-volume

@gsemet
Copy link

gsemet commented Jun 12, 2018

Typo fixed:

apiVersion: v1
kind: ConfigMap
metadata:
  name: git-clone
data:
  git-clone.sh: |
    #!/bin/sh -e
    REPO=$1
    REF=$2
    DIR=$3
    # Init Containers will re-run on Pod restart. Remove the directory's contents
    # and reprovision when this happens.
    if [ -d "$DIR" ]; then
        rm -rf $( find $DIR -mindepth 1 )
    fi
    git clone $REPO $DIR
    cd $DIR
    git reset --hard $REF
---
apiVersion: v1
kind: Pod
metadata:
  name: server
spec:
  initContainers:
  - name: git-clone
    image: alpine/git # Any image with git will do
    command:
    - /usr/local/git/git-clone.sh
    args:
    - "https://somewhere/me/my-git-repository.git"
    - "22f1d8406d464b0c0874075539c1f2e96c253775"
    - "/repo"
    volumeMounts:
    - name: git-clone
      mountPath: /usr/local/git
    - name: git-repo
      mountPath: /repo
  containers:
  - image: nginx
    name: nginx
    volumeMounts:
    - mountPath: /mypath/to/my/repo
      name: git-repo
  volumes:
  - name: git-repo
    emptyDir: {}
  - name: git-clone
    configMap:
      name: git-clone
      defaultMode: 0755

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants