adding proxy parameters to git pipeline resource #2215

pritidesai · 2020-03-11T19:05:27Z

Changes

Git resource parameters now support, httpProxy, httpsProxy, and noProxy.
All three parameters are optional, no validation done on any.
Its user's responsibility to specify valid values and specify whichever is needed based on their use case.

e.g.:

  inputs:
    resources:
      - name: skaffold
        resourceSpec:
          type: git
          params:
            - name: revision
              value: master
            - name: url
              value: https://github.com/GoogleContainerTools/skaffold
            - name: httpProxy
              value: "http.proxy.com"
            - name: httpsProxy
              value: "https.proxy.com"
            - name: noProxy
              value: "no.proxy.com"

Fixes: #1663

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

Describe any user facing changes here, or delete this block.

Examples of user facing changes:
- API changes
- Bug fixes
- Any changes in behavior

tekton-robot · 2020-03-11T19:08:02Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/resource/v1alpha1/git/git_resource.go	80.0%	80.5%	0.5
test/builder/pipeline.go	87.0%	85.9%	-1.1

tekton-robot · 2020-03-11T19:18:03Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/resource/v1alpha1/git/git_resource.go	80.0%	80.5%	0.5
test/builder/pipeline.go	87.0%	85.9%	-1.1

tekton-robot · 2020-03-11T19:20:33Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/resource/v1alpha1/git/git_resource.go	80.0%	80.5%	0.5
test/builder/pipeline.go	87.0%	85.9%	-1.1

pritidesai · 2020-03-11T19:28:57Z

Couple of thoughts here:

I need a feedback whether this is a reasonable solution to add env section in pipeline resource in addition to params.
I couldn't foresee any use case to have same env section in image pipeline resource, I have limited env to only git pipeline resource while resource binding at https://github.com/tektoncd/pipeline/pull/2215/files#diff-d35ed297a7a57be3ca8e05f1adc8c11dR82
stdout and stderr does not appear in the container logs, we might want to enable them when GIT_CURL_VERBOSE is set 🤔

tekton-robot · 2020-03-11T20:12:02Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/resource/v1alpha1/git/git_resource.go	80.0%	80.5%	0.5
test/builder/pipeline.go	87.0%	85.9%	-1.1

tekton-robot · 2020-03-11T20:47:31Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/resource/v1alpha1/git/git_resource.go	80.0%	80.5%	0.5
test/builder/pipeline.go	87.0%	87.1%	0.2

pritidesai · 2020-03-11T21:47:37Z

/test pull-tekton-pipeline-integration-tests

pritidesai · 2020-03-11T23:26:10Z

Should this be added to git-init as well here ?

ghost · 2020-03-12T12:10:33Z

I'm a little wary of adding arbitrary env variable support for a single use case of the git resource. Task authors can already do this through the StepTemplate of a Task I think. Is there a way to scope the change down to just those fields which are most relevant for corporate proxies?

For git-init my hunch is that we shouldn't make changes to it unless it's necessary to support this feature. In the git-clone catalog task we are actively moving away from git-init to an inline Script, which will make the behaviour a little simpler for users to inspect. So keeping git-init in a fixed state would be preferable (at least from my POV - totally understand if there are other reasons to make changes).

pritidesai · 2020-03-12T16:52:40Z

My understanding is StepTemplate Env. section is applied to the container where that step is executed, git resource is creating its own container and Env from StepTemplate is not available to git container running /ko-app/git-init here:

https://github.com/tektoncd/pipeline/blob/master/pkg/apis/resource/v1alpha1/git/git_resource.go#L151

apiVersion: tekton.dev/v1beta1
kind: Task
metadata:
  name: task-git-env
spec:
  resources:
    inputs:
      - name: git-repo
        type: git
        description: "The input is code from a git repository"
  steps:
    - name: check-git-repo
      image: ubuntu
      env:
        - name: HTTPS_PROXY
          value: "invalid.git.com"
      script: |
        #!/usr/bin/env bash
        if [ -d $(resources.inputs.git-repo.path) ]; then
          echo "Git repo was cloned at $(resources.inputs.git-repo.path)"
        else
          echo "Git repo was not cloned at $(resources.inputs.git-repo.path)"
        fi
---

apiVersion: tekton.dev/v1beta1
kind: TaskRun
metadata:
  name: demo-git-env
spec:
  resources:
    inputs:
      - name: git-repo
        resourceSpec:
          type: git
          params:
            - name: url
              value: https://github.com/tektoncd/pipeline.git
  taskRef:
    name: task-git-env

Results in:

kubectl logs pod/demo-git-env-pod-9bc2z --all-containers
{"level":"info","ts":1584031541.875279,"caller":"git/git.go:102","msg":"Successfully cloned https://github.com/tektoncd/pipeline.git @ master in path /workspace/git-repo"}
{"level":"warn","ts":1584031541.8754478,"caller":"git/git.go:149","msg":"Unexpected error: creating symlink: symlink /tekton/home/.ssh /root/.ssh: file exists"}
{"level":"info","ts":1584031541.9178064,"caller":"git/git.go:130","msg":"Successfully initialized and updated submodules in path /workspace/git-repo"}
Git repo was cloned at /workspace/git-repo

V/S.

With these changes:

apiVersion: tekton.dev/v1beta1
kind: Task
metadata:
  name: task-git-env
spec:
  resources:
    inputs:
      - name: git-repo
        type: git
        description: "The input is code from a git repository"
  steps:
    - name: check-git-repo
      image: ubuntu
      script: |
        #!/usr/bin/env bash
        if [ -d $(resources.inputs.git-repo.path) ]; then
          echo "Git repo was cloned at $(resources.inputs.git-repo.path)"
        else
          echo "Git repo was not cloned at $(resources.inputs.git-repo.path)"
        fi
---

apiVersion: tekton.dev/v1beta1
kind: TaskRun
metadata:
  name: demo-git-env
spec:
  resources:
    inputs:
      - name: git-repo
        resourceSpec:
          type: git
          params:
            - name: url
              value: https://github.com/tektoncd/pipeline.git
          env:
            - name: HTTPS_PROXY
              value: "invalid.git.com"
  taskRef:
    name: task-git-env

Results in expected failure since PROXY server is invalid:

 kubectl logs pod/demo-git-env-pod-wz2cc --all-containers
{"level":"error","ts":1584031884.1245286,"caller":"git/git.go:41","msg":"Error running git [fetch --recurse-submodules=yes --depth=1 origin master]: exit status 128\nfatal: unable to access 'https://github.com/tektoncd/pipeline.git/': Failed to connect to invalid.git.com port 1080: Connection refused\n","stacktrace":"github.com/tektoncd/pipeline/pkg/git.run\n\t/Users/pdesai/Go/src/github.com/tektoncd/pipeline/pkg/git/git.go:41\ngithub.com/tektoncd/pipeline/pkg/git.Fetch\n\t/Users/pdesai/Go/src/github.com/tektoncd/pipeline/pkg/git/git.go:90\nmain.main\n\t/Users/pdesai/Go/src/github.com/tektoncd/pipeline/cmd/git-init/main.go:51\nruntime.main\n\t/usr/local/go/src/runtime/proc.go:203"}

We can certainly limit the scope and accept only proxy relevant env. settings. I will get that going.

ghost · 2020-03-12T17:23:21Z

Ah I see, I should have checked the code that implements the StepTemplate before commenting, thanks for clarifying!

tekton-robot · 2020-03-12T18:13:59Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/resource/v1alpha1/git/git_resource.go	80.0%	73.7%	-6.3

tekton-robot · 2020-03-12T21:33:58Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/resource/v1alpha1/git/git_resource.go	80.0%	77.3%	-2.7

tekton-robot · 2020-03-15T05:54:10Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/resource/v1alpha1/git/git_resource.go	80.0%	84.4%	4.4

Git resource parameters now support, httpProxy, httpsProxy, and noProxy. All three parameters are optional, no validation done on any. Its user's responsibility to specify valid values and specify whichever is needed based on their use case. e.g.: ``` inputs: resources: - name: skaffold resourceSpec: type: git params: - name: revision value: master - name: url value: https://github.com/GoogleContainerTools/skaffold - name: httpProxy value: "http.proxy.com" - name: httpsProxy value: "https.proxy.com" - name: noProxy value: "no.proxy.com" ```

tekton-robot · 2020-03-15T06:55:40Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/resource/v1alpha1/git/git_resource.go	80.0%	84.4%	4.4

pritidesai · 2020-03-15T07:33:00Z

/test pull-tekton-pipeline-integration-tests

pritidesai · 2020-03-15T15:41:33Z

@sbwsg its ready for review again, thanks 🙏

pritidesai · 2020-03-16T17:20:59Z

pkg/apis/resource/v1alpha1/git/git_resource.go

+	}}
+
+	if len(s.HTTPProxy) != 0 {
+		env = append(env, corev1.EnvVar{Name: "HTTP_PROXY", Value: s.HTTPProxy})


Please note that here, respective proxy env. variables are being set while creating a container instead of running git config --global like sslVerify in git.go

The reason for implementing this way is, for some reason, git proxy configuration was ignored and cloning a repo was not failing even after setting HTTPS_PROXY to an invalid server, see the failure test at
https://github.com/tektoncd/pipeline/pull/2215/files#diff-6e725a312c3f914d42e49f045db0bde5R185

ghost · 2020-03-17T12:10:57Z

We should also add these fields to the git-clone Task in the Catalog so that they have matching params. @pritidesai are you ok to do that once this is merged? If not let me know and I'll pick it up, cheers!

pritidesai · 2020-03-17T16:54:38Z

yup, sure, will add these new fields to catalog 🙏

vdemeester

/lgtm

@sbwsg do we want to cherry-pick this ?

tekton-robot · 2020-03-18T15:53:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg, vdemeester

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~~ [sbwsg,vdemeester]

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

ghost · 2020-03-18T15:55:42Z

yup, good thinking!

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 11, 2020

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Mar 11, 2020

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 11, 2020

tekton-robot requested review from abayer and afrittoli March 11, 2020 19:05

pritidesai force-pushed the git-env branch from 2d7d891 to a7e2624 Compare March 11, 2020 19:15

pritidesai force-pushed the git-env branch from a7e2624 to fa90d4f Compare March 11, 2020 19:18

pritidesai mentioned this pull request Mar 11, 2020

PipelineResources git sslVerify parameter causing issues #2164

Closed

pritidesai changed the title ~~WIP - introducing env to git pipeline resource~~ introducing env to git pipeline resource Mar 11, 2020

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 11, 2020

pritidesai force-pushed the git-env branch from fa90d4f to 37af7eb Compare March 11, 2020 20:09

pritidesai force-pushed the git-env branch from 37af7eb to b4b844d Compare March 11, 2020 20:45

pritidesai changed the title ~~introducing env to git pipeline resource~~ WIP - introducing env to git pipeline resource Mar 12, 2020

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 12, 2020

pritidesai force-pushed the git-env branch from b4b844d to bf83a8a Compare March 12, 2020 18:11

pritidesai force-pushed the git-env branch from bf83a8a to e21c080 Compare March 12, 2020 21:31

pritidesai changed the title ~~WIP - introducing env to git pipeline resource~~ WIP - adding proxy parameters to git pipeline resource Mar 12, 2020

pritidesai force-pushed the git-env branch from e21c080 to 814f7f0 Compare March 15, 2020 05:51

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 15, 2020

pritidesai force-pushed the git-env branch from 814f7f0 to 09216fb Compare March 15, 2020 06:53

pritidesai changed the title ~~WIP - adding proxy parameters to git pipeline resource~~ adding proxy parameters to git pipeline resource Mar 15, 2020

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 15, 2020

ghost approved these changes Mar 16, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 16, 2020

pritidesai commented Mar 16, 2020

View reviewed changes

vdemeester approved these changes Mar 18, 2020

View reviewed changes

tekton-robot assigned vdemeester Mar 18, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 18, 2020

ghost added the needs-cherry-pick Indicates a PR needs to be cherry-pick to a release branch label Mar 18, 2020

tekton-robot merged commit a10e779 into tektoncd:master Mar 18, 2020

ghost removed the needs-cherry-pick Indicates a PR needs to be cherry-pick to a release branch label Mar 24, 2020

bobcatfish mentioned this pull request Mar 25, 2020

Add proxy support to git resource tektoncd/catalog#238

Closed

gabemontero mentioned this pull request Mar 25, 2020

facilitate mount of certs for HTTPS Proxy #2288

Closed

kenplusplus mentioned this pull request May 1, 2020

adding proxy parameters to pull request pipeline resource #2520

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding proxy parameters to git pipeline resource #2215

adding proxy parameters to git pipeline resource #2215

pritidesai commented Mar 11, 2020 •

edited

Loading

tekton-robot commented Mar 11, 2020

tekton-robot commented Mar 11, 2020

tekton-robot commented Mar 11, 2020

pritidesai commented Mar 11, 2020

tekton-robot commented Mar 11, 2020

tekton-robot commented Mar 11, 2020

pritidesai commented Mar 11, 2020

pritidesai commented Mar 11, 2020

ghost commented Mar 12, 2020

pritidesai commented Mar 12, 2020

ghost commented Mar 12, 2020

tekton-robot commented Mar 12, 2020

tekton-robot commented Mar 12, 2020

tekton-robot commented Mar 15, 2020

tekton-robot commented Mar 15, 2020

pritidesai commented Mar 15, 2020

pritidesai commented Mar 15, 2020

pritidesai Mar 16, 2020

ghost commented Mar 17, 2020

pritidesai commented Mar 17, 2020

vdemeester left a comment

tekton-robot commented Mar 18, 2020

ghost commented Mar 18, 2020

adding proxy parameters to git pipeline resource #2215

adding proxy parameters to git pipeline resource #2215

Conversation

pritidesai commented Mar 11, 2020 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented Mar 11, 2020

tekton-robot commented Mar 11, 2020

tekton-robot commented Mar 11, 2020

pritidesai commented Mar 11, 2020

tekton-robot commented Mar 11, 2020

tekton-robot commented Mar 11, 2020

pritidesai commented Mar 11, 2020

pritidesai commented Mar 11, 2020

ghost commented Mar 12, 2020

pritidesai commented Mar 12, 2020

ghost commented Mar 12, 2020

tekton-robot commented Mar 12, 2020

tekton-robot commented Mar 12, 2020

tekton-robot commented Mar 15, 2020

tekton-robot commented Mar 15, 2020

pritidesai commented Mar 15, 2020

pritidesai commented Mar 15, 2020

pritidesai Mar 16, 2020

Choose a reason for hiding this comment

ghost commented Mar 17, 2020

pritidesai commented Mar 17, 2020

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Mar 18, 2020

ghost commented Mar 18, 2020

pritidesai commented Mar 11, 2020 •

edited

Loading