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

Add support for using S3-compatible APIs instead of GCS by passing through a boto configuration file. #1361

Merged
merged 4 commits into from
Oct 10, 2019

Conversation

jbarrick-mesosphere
Copy link
Contributor

Changes

This adds support for using S3-compatible APIs instead of GCS with the GCS storage resource.

Submitter Checklist

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

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

It is now possible to use S3-compatible APIs instead of GCS for GCS storage resources.

@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Sep 27, 2019
@tekton-robot tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 27, 2019
@tekton-robot
Copy link
Collaborator

Hi @jbarrick-mesosphere. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@ghost
Copy link

ghost commented Sep 30, 2019

/ok-to-test
/lgtm

These changes look fine to me but I'm not familiar with the boto SDK. Anyone else have thoughts on this? @bobcatfish @dibyom @vdemeester @afrittoli

@tekton-robot tekton-robot assigned ghost Sep 30, 2019
@tekton-robot tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. lgtm Indicates that a PR is ready to be merged. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 30, 2019
@tekton-robot
Copy link
Collaborator

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/artifacts/artifacts_storage.go 80.9% 81.3% 0.4

@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 30, 2019
@tekton-robot
Copy link
Collaborator

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/artifacts/artifacts_storage.go 80.9% 81.3% 0.4

@dlorenc
Copy link
Contributor

dlorenc commented Oct 7, 2019

Looks good to me, but it needs a rebase.

@tekton-robot
Copy link
Collaborator

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/artifacts/artifacts_storage.go 81.1% 81.5% 0.4

@tekton-robot
Copy link
Collaborator

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/artifacts/artifacts_storage.go 81.1% 81.5% 0.4

@tekton-robot
Copy link
Collaborator

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/artifacts/artifacts_storage.go 81.1% 81.5% 0.4

@jbarrick-mesosphere
Copy link
Contributor Author

Thanks @sbwsg I think the tests should be all set on this one 👍

@jbarrick-mesosphere jbarrick-mesosphere mentioned this pull request Oct 9, 2019
3 tasks
@ghost
Copy link

ghost commented Oct 9, 2019

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 9, 2019
@imjasonh
Copy link
Member

/approve

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ImJasonH

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 10, 2019
@tekton-robot tekton-robot merged commit a6663fb into tektoncd:master Oct 10, 2019
@afrittoli
Copy link
Member

Thank you for this!
Small how to for setting this up with IBM Cloud COS: https://gist.github.com/afrittoli/8440b545a6601f60aa233b8e70fc2b62

@afrittoli
Copy link
Member

BTW, I don't know if this was intentional, but this has the side effect that the GCS Pipeline Resource can also be used now to access an S3 storage:

envVars, secretVolumeMount := getSecretEnvVarsAndVolumeMounts(s.Name, gcsSecretVolumeMountPath, s.Secrets)
step := Step{Container: corev1.Container{
Name: names.SimpleNameGenerator.RestrictLengthWithRandomSuffix(fmt.Sprintf("upload-%s", s.Name)),
Image: s.GsutilImage,
Command: []string{"/ko-app/gsutil"},
Args: args,
VolumeMounts: secretVolumeMount,
Env: envVars},
}

I tried altering the GCS taskrun example to use a COS bucket on IBM Cloud with s3 service credentials enabled, and it works like a charm:

apiVersion: tekton.dev/v1alpha1
kind: TaskRun
metadata:
  generateName: gcs-resource-
spec:
  taskSpec:
    inputs:
      resources:
      - name: source
        type: storage
    steps:
    - image: alpine
      command: ['unzip', 'source/archive.zip']
    - image: alpine
      command: ['cat', 'file.txt']
  inputs:
    resources:
    - name: source
      resourceSpec:
        type: storage
        params:
          - name: location
            value: s3://tektonstorage/archive.zip
          - name: type
            value: gcs
        secrets:
          - fieldName: BOTO_CONFIG
            secretName: pipelines-cos-credentials
            secretKey: boto_config

@oliviabarrick
Copy link

Glad it’s useful for others! As far as I know any of the GCS features that use gsutil should work with any S3 now - I’ve been using the pipeline resource with Minio. I should put up a Minio example, too.

@eddycharly
Copy link
Member

eddycharly commented Nov 6, 2019

I started testing s3 artifacts storage today, and couldn’t get it to work in another region than us-east-1.
I also had to remove `use-sigv4 = Trueˋ, with it downloading artifacts failed. Uploads worked fine though.

Is this a known limitation of gsutil working only in us-east-1 ?

@jbarrick-mesosphere
Copy link
Contributor Author

I started testing s3 artifacts storage today, and couldn’t get it to work in another region than us-east-1.
I also had to remove `use-sigv4 = Trueˋ, with it downloading artifacts failed. Uploads worked fine though.

Is this a known limitation of gsutil working only in us-east-1 ?

I ran into this too when trying to get minio working, it seems to be an upstream issue in gsutil that needs to be fixed there. I worked around it by creating a Kubernetes service called us-east-1 that pointed at my minio instance.

@eddycharly
Copy link
Member

@jbarrick-mesosphere thanks for confirming the issue.
I opened a pull request to improve the doc #1533

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. cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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.

8 participants