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

pkg/gcp/gcs: Improve GCS path construction/retrieval logic #1738

Merged
merged 11 commits into from
Nov 15, 2020

Conversation

justaugustus
Copy link
Member

@justaugustus justaugustus commented Nov 14, 2020

What type of PR is this?

/kind bug cleanup

What this PR does / why we need it:

Continuation of #1711.

  • pkg/gcp/gcs: Improve path normalization logic

    • Ensure constructed paths are prefixed with gs://

    • Use flag vars instead of strings for RsyncRecursive

    • Add isPathNormalized function

      Use this function as pre-check for any gsutil/GCS functions that
      manipulate GCS bucket contents.

    • Use isPathNormalized in RsyncRecursive and PathExists

    • Clarify when we are constructing a release path or version marker path

  • pkg/gcp/gcs: NormalizeGCSPath now handles multiple path elements

    This allows any caller to pass an arbitrary GCS path elements without
    having to handle any filepath.Join() logic.

    Should handle the following cases:

    • single path element
    • multiple path elements
    • path element already contains gs://
    • path element was the result of a filepath.Join() and contains gs:/
    • path element contains a leading /

    Should error if:

    • there were no path elements
    • the only path element is an empty string
  • pkg: Update usages of gcs.NormalizeGCSPath()

  • pkg/release: Respect the GCS suffix when pushing version markers

  • pkg/gcp/gcs: Add a few test cases for GCS paths

  • pkg/build: Fixup filepath.Join() bug when pushing release artifacts

  • pkg/build: Deprecate the GCSSuffix option and introduce GCSRoot option

    Testing with the --gcs-suffix flag has been frustrating.

    It's possible the original shell library never properly handled this
    option and it was never noticed because the option isn't exercised in
    our releases or CI jobs.

    Here we replace its usage with --gcs-root.

    If specified, it will override BuildType.

    When unset:

    • BuildType: "ci"
    • final path: gs:///ci

    When set:

    • BuildType: "ci"
    • GCSRoot: "new-root"
    • final path: gs:///new-root
  • pkg/build: Drop references to GCSSuffix

  • pkg/gcp/gcs: RsyncRecursive doesn't need to run against normalized paths

    gsutil rsync doesn't actual require that either of the specified
    directories be GCS paths, so here we remove that requirement.

  • pkg/release: Fix prefix check to determine bucket permissions

  • dependencies.yaml: Add entry for skopeo

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

- pkg/gcp/gcs: Improve path normalization logic
  
  - Ensure constructed paths are prefixed with `gs://`
  - Use flag vars instead of strings for RsyncRecursive
  - Add isPathNormalized function
  
    Use this function as pre-check for any gsutil/GCS functions that
    manipulate GCS bucket contents.
  
  - Use isPathNormalized in RsyncRecursive and PathExists
  - Clarify when we are constructing a release path or version marker path

- pkg/gcp/gcs: NormalizeGCSPath now handles multiple path elements
  
  This allows any caller to pass an arbitrary GCS path elements without
  having to handle any filepath.Join() logic.
  
  Should handle the following cases:
  - single path element
  - multiple path elements
  - path element already contains `gs://`
  - path element was the result of a filepath.Join() and contains `gs:/`
  - path element contains a leading `/`
  
  Should error if:
  - there were no path elements
  - the only path element is an empty string

- pkg: Update usages of gcs.NormalizeGCSPath()

- pkg/release: Respect the GCS suffix when pushing version markers

- pkg/gcp/gcs: Add a few test cases for GCS paths

- pkg/build: Fixup filepath.Join() bug when pushing release artifacts

- pkg/build: Deprecate the GCSSuffix option and introduce GCSRoot option
  
  Testing with the `--gcs-suffix` flag has been frustrating.
  
  It's possible the original shell library never properly handled this
  option and it was never noticed because the option isn't exercised in
  our releases or CI jobs.
  
  Here we replace its usage with `--gcs-root`.
  
  If specified, it will override BuildType.
  
  When unset:
    - BuildType: "ci"
    - final path: gs://<bucket>/ci
  
  When set:
    - BuildType: "ci"
    - GCSRoot: "new-root"
    - final path: gs://<bucket>/new-root

- pkg/build: Drop references to GCSSuffix

- pkg/gcp/gcs: RsyncRecursive doesn't need to run against normalized paths
  
  `gsutil rsync` doesn't actual require that either of the specified
  directories be GCS paths, so here we remove that requirement.

- pkg/release: Fix prefix check to determine bucket permissions

- Ensure constructed paths are prefixed with `gs://`
- Use flag vars instead of strings for RsyncRecursive
- Add isPathNormalized function

  Use this function as pre-check for any gsutil/GCS functions that
  manipulate GCS bucket contents.

- Use isPathNormalized in RsyncRecursive and PathExists
- Clarify when we are constructing a release path or version marker path

Signed-off-by: Stephen Augustus <[email protected]>
This allows any caller to pass an arbitrary GCS path elements without
having to handle any filepath.Join() logic.

Should handle the following cases:
- single path element
- multiple path elements
- path element already contains `gs://`
- path element was the result of a filepath.Join() and contains `gs:/`
- path element contains a leading `/`

Should error if:
- there were no path elements
- the only path element is an empty string

Signed-off-by: Stephen Augustus <[email protected]>
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/bug Categorizes issue or PR as related to a bug. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 14, 2020
@k8s-ci-robot k8s-ci-robot added the area/release-eng Issues or PRs related to the Release Engineering subproject label Nov 14, 2020
@k8s-ci-robot k8s-ci-robot added sig/release Categorizes an issue or PR as relevant to SIG Release. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 14, 2020
@k8s-ci-robot k8s-ci-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 Nov 15, 2020
Testing with the `--gcs-suffix` flag has been frustrating.

It's possible the original shell library never properly handled this
option and it was never noticed because the option isn't exercised in
our releases or CI jobs.

Here we replace its usage with `--gcs-root`.

If specified, it will override BuildType.

When unset:
  - BuildType: "ci"
  - final path: gs://<bucket>/ci

When set:
  - BuildType: "ci"
  - GCSRoot: "new-root"
  - final path: gs://<bucket>/new-root

Signed-off-by: Stephen Augustus <[email protected]>
`gsutil rsync` doesn't actual require that either of the specified
directories be GCS paths, so here we remove that requirement.

Signed-off-by: Stephen Augustus <[email protected]>
@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 do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 15, 2020
@justaugustus justaugustus changed the title [WIP] pkg/gcp/gcs: Improve GCS path construction/retrieval logic pkg/gcp/gcs: Improve GCS path construction/retrieval logic Nov 15, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 15, 2020
@justaugustus
Copy link
Member Author

/assign @saschagrunert @hasheddan @cpanato
cc: @kubernetes/release-engineering

Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justaugustus, saschagrunert

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 [justaugustus,saschagrunert]

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

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. area/release-eng Issues or PRs related to the Release Engineering subproject cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. 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. needs-priority release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/release Categorizes an issue or PR as relevant to SIG Release. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants