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: Native Google Cloud Storage support for artifact. Closes #1911 #2484

Merged
merged 12 commits into from
Mar 25, 2020

Conversation

whynowy
Copy link
Member

@whynowy whynowy commented Mar 19, 2020

Closes #1911 and #1067

This commit implements the all the features of using GCS as artifact repository with Argo.

  • Use GCS as artifact input, output, can be either a file or directory
  • Configure GCS as default artifact repository in Argo configmap
  • Hardwire GCS artifact, file or directory

A GCS artifact config looks like following:

        artifacts:
          - name: my-art
            path: /my-artifact
            gcs:
              bucket: my-bucket-name
              # key could be either a file or a directory.
              key: path/in/bucket
              # serviceAccountKeySecret is a secret selector.
              # It references the k8s secret named 'my-gcs-credentials'.
              # This secret is expected to have have the key 'serviceAccountKey',
              # containing the base64 encoded Google Cloud Service Account Key (json)
              # to access the bucket.
              #
              # If it's running on GKE and Workload Identity is configured,
              # serviceAccountKeySecret is not needed.
              serviceAccountKeySecret:
                name: my-gcs-credentials
                key: serviceAccountKey

Example default GCS artifact configuration in the Configmap:

data:
  config: |
    artifactRepository:
      gcs:
        bucket: my-bucket
        #keyFormat is optional, it could reference workflow variables, such as "{{workflow.name}}/{{pod.name}}"
        keyFormat: prefix/in/bucket
        # serviceAccountKeySecret is not needed if Workload Identity is configured
        serviceAccountKeySecret:
          name: my-gcs-credentials
          key: serviceAccountKey

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
  • The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
  • I have written unit and/or e2e tests for my change. PRs without these are unlike to be merged.
  • Optional. I've added My organization is added to the USERS.md.
  • I've signed the CLA and required builds are green.

@whynowy whynowy changed the title feat: Native Google Cloud Storage support for artifact feat: Native Google Cloud Storage support for artifact. Closes #1911 Mar 19, 2020
@whynowy
Copy link
Member Author

whynowy commented Mar 19, 2020

A separate PR to address the comment #1067 (comment), which is about Workload Identity.

@codecov
Copy link

codecov bot commented Mar 19, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@999b1e1). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2484   +/-   ##
=========================================
  Coverage          ?   11.21%           
=========================================
  Files             ?       75           
  Lines             ?    31690           
  Branches          ?        0           
=========================================
  Hits              ?     3555           
  Misses            ?    27657           
  Partials          ?      478
Impacted Files Coverage Δ
config/config.go 33.33% <ø> (ø)
pkg/apis/workflow/v1alpha1/generated.pb.go 0.46% <ø> (ø)
...kg/apis/workflow/v1alpha1/zz_generated.deepcopy.go 0% <0%> (ø)
pkg/apis/workflow/v1alpha1/workflow_types.go 11.45% <0%> (ø)
workflow/controller/workflowpod.go 70.66% <0%> (ø)
workflow/executor/executor.go 4.45% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 999b1e1...cf89e04. Read the comment docs.

@whynowy
Copy link
Member Author

whynowy commented Mar 22, 2020

A separate PR to address the comment #1067 (comment), which is about Workload Identity.

I did it in this PR, serviceAccountKeySecret is optional, if it's absent, it assumes workload Identity is configured, and will use a default client, which will call metadata API underneath to get a token.

@sarabala1979 sarabala1979 self-assigned this Mar 23, 2020
Copy link
Member

@simster7 simster7 left a comment

Choose a reason for hiding this comment

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

This LGTM. Having tests would be ideal, but given the nature of the PR they might be difficult to produce. However, since this PR is mostly orthogonal, I think it should be fine.

I would suggest also getting @alexec or @sarabala1979 to take a look before merging.

@whynowy
Copy link
Member Author

whynowy commented Mar 24, 2020

This LGTM. Having tests would be ideal, but given the nature of the PR they might be difficult to produce. However, since this PR is mostly orthogonal, I think it should be fine.

I would suggest also getting @alexec or @sarabala1979 to take a look before merging.

Thanks @simster7 !

@whynowy
Copy link
Member Author

whynowy commented Mar 24, 2020

@alexec @sarabala1979 , do you have any comments?

Copy link
Contributor

@alexec alexec left a comment

Choose a reason for hiding this comment

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

I trust Simon as much as myself on this one.

@whynowy whynowy merged commit 06cfc12 into argoproj:master Mar 25, 2020
@whynowy whynowy deleted the gcs_support branch March 25, 2020 00:32
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.

Support GCS natively
4 participants