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

Update GCS dependencies and fix GCS driver #3702

Closed
wants to merge 1 commit into from

Conversation

jmontleon
Copy link

@jmontleon jmontleon commented Jul 29, 2022

This change updates google cloud dependency as the current version dates back to a 2015 version and does not compile with the latest Go version.

@jmontleon jmontleon force-pushed the update-fix-gcs branch 2 times, most recently from 7f04198 to 9d41ebb Compare July 29, 2022 01:16
@jmontleon jmontleon force-pushed the update-fix-gcs branch 6 times, most recently from ca64a79 to 2ea2d29 Compare July 29, 2022 03:59
@@ -914,10 +921,6 @@ func putChunk(client *http.Client, sessionURI string, chunk []byte, from int64,
return bytesPut, err
}

func (d *driver) context(context context.Context) context.Context {
Copy link
Author

Choose a reason for hiding this comment

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

googleapis/google-cloud-go@a7a5fa8

The WithContext and NewContext functions were only needed for this
legacy API pattern; the new Client pattern can use regular contexts from
the golang.org/x/net/context package directly.

}
for _, subpath := range objects.Prefixes {
subpath = d.keyToPath(subpath)
if object.Name == "" && object.Prefix != "" {
Copy link
Author

Choose a reason for hiding this comment

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

It looks lik the old version of the iterator kept prefixes separate. In the new version:
https://pkg.go.dev/cloud.google.com/go/storage#ObjectIterator.Next

If Query.Delimiter is non-empty, some of the ObjectAttrs returned by Next will have a non-empty Prefix field, and a zero value for all other fields. These represent prefixes.

@jmontleon jmontleon marked this pull request as ready for review July 29, 2022 04:09
@codecov-commenter
Copy link

codecov-commenter commented Jul 29, 2022

Codecov Report

Patch and project coverage have no change.

Comparison is base (e5d5810) 56.58% compared to head (0a3d3ff) 56.58%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3702   +/-   ##
=======================================
  Coverage   56.58%   56.58%           
=======================================
  Files         105      105           
  Lines       10636    10636           
=======================================
  Hits         6018     6018           
  Misses       3946     3946           
  Partials      672      672           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@milosgajdos
Copy link
Member

Great effort @jmontleon ❤️

@jmontleon jmontleon force-pushed the update-fix-gcs branch 2 times, most recently from 984e877 to a47e556 Compare July 29, 2022 17:02
@jmontleon
Copy link
Author

Thank you! When satisfied, I'm happy if you want to squash this, or merge #3676 (I think a go vendor will delete one more file if I'm not mistaken) then rebase this and merge on top of it. Please let me know how you want to handle it - I didn't want to steal anyone's thunder.

@milosgajdos
Copy link
Member

Let's wait and see if we can merge #3676 . The author needs to reply :)

@bshaaban
Copy link

bshaaban commented Aug 2, 2022

Let's wait and see if we can merge #3676 . The author needs to reply :)

my appologies for the delay, i had other urgent work to finalize which took my time out of this effort, we can definitely merge this PR, then i can rebase and update #3676 to complete the dep updates.

@bshaaban
Copy link

bshaaban commented Aug 8, 2022

@milosgajdos @thaJeztah @Jamstah any maintainer who can approve & merge this pr? Or what is the process to get it merged? I know verifying and merging 457 files is not a simple task (this is why I'm so glad go.mod and go.sum files were introduced, it just avoids vendoring altogether, it would be awesome if we eventually can delete the vendor folder altogether).

It's blocking dep updates for many projects depending on this one.

Thanks.

@kaovilai
Copy link
Contributor

kaovilai commented Aug 8, 2022

@bshaaban meanwhile you can do what we did :D
https://github.com/openshift/openshift-velero-plugin/pull/151/files#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6R40
which uses commit from https://github.com/konveyor/distribution/tree/notv3

@milosgajdos
Copy link
Member

@bshaaban I've allocated some time for a review on Friday -- I also want to give heads up that I'm not a GCP user so I'll be checking the API docs a lot and assuming @jmontleon's expertise on the topic :) Bear with me till then 🙇

@jmontleon
Copy link
Author

jmontleon commented Aug 8, 2022

I also want to give heads up that I'm not a GCP user so I'll be checking the API docs a lot and assuming @jmontleon's expertise on the topic

For the record, I am not an expert on GCP/GCS. I looked through the API docs and made an effort to restore functionality in order to help one of the teams that I work closely with. I would welcome any corrections from someone with more knowledge.

@kaovilai
Copy link
Contributor

kaovilai commented Aug 8, 2022

Other than the PR's content.. will this repository have any valid GCS credentials to supply to the CI for e2e testing?

@Jamstah
Copy link
Collaborator

Jamstah commented Aug 8, 2022

Other than the PR's content.. will this repository have any valid GCS credentials to supply to the CI for e2e testing?

I expect not, wouldn't it be better to mock the gcp SDK instead?

Although... How is S3 tested?

Copy link
Collaborator

@corhere corhere left a comment

Choose a reason for hiding this comment

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

As another reviewer unfamiliar with GCS, the changes generally look good to me. I have a few nits to pick about style and the like, though.

For posterity, could you please document in the PR description what was broken about the GCS driver that is being fixed?

}

func storageStatObject(context context.Context, bucket string, name string) (*storage.Object, error) {
var obj *storage.Object
func storageStatObject(ctx context.Context, bucket string, name string, gcs *storage.Client) (*storage.ObjectAttrs, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

OPTIONAL: Looking at how this function is used, all call sites pass the same arguments. The bucket and gcs arguments could be dropped entirely and the d.pathToKey boilerplate around the name arg could be elided by refactoring this function to a method on *driver.

Suggested change
func storageStatObject(ctx context.Context, bucket string, name string, gcs *storage.Client) (*storage.ObjectAttrs, error) {
func (d *driver) storageStatObject(ctx context.Context, name string) (*storage.ObjectAttrs, error) {

@@ -68,6 +69,7 @@ type driverParameters struct {
client *http.Client
rootDirectory string
chunkSize int
GCS *storage.Client
Copy link
Collaborator

@corhere corhere Apr 27, 2023

Choose a reason for hiding this comment

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

The new field should be private. There are ways in which concrete-typed values of unexported types could escape a package.

Suggested change
GCS *storage.Client
gcs *storage.Client

@@ -97,6 +99,7 @@ type driver struct {
privateKey []byte
rootDirectory string
chunkSize int
GCS *storage.Client
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
GCS *storage.Client
gcs *storage.Client

@@ -369,17 +380,16 @@ type writer struct {
sessionURI string
buffer []byte
buffSize int
GCS *storage.Client
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
GCS *storage.Client
gcs *storage.Client

// try to get as file
gcsContext := d.context(context)
obj, err := storageStatObject(gcsContext, d.bucket, d.pathToKey(path))
//try to get as file
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comments without a leading space are reserved for directives.

Suggested change
//try to get as file
// try to get as file

return objs, err
func storageListObjects(ctx context.Context, bucket string, q *storage.Query, gcs *storage.Client) ([]*storage.ObjectAttrs, error) {
bkt := gcs.Bucket(bucket)
objs := []*storage.ObjectAttrs{}
Copy link
Collaborator

Choose a reason for hiding this comment

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

https://github.com/golang/go/wiki/CodeReviewComments#declaring-empty-slices

Suggested change
objs := []*storage.ObjectAttrs{}
var objs []*storage.ObjectAttrs

dst := gcs.Bucket(destBucket).Object(destName)
attrs, err := dst.CopierFrom(src).Run(ctx)
if err != nil {
if status, ok := err.(*googleapi.Error); ok {
Copy link
Collaborator

Choose a reason for hiding this comment

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

errors.As is generally preferable to type-asserting an error value directly.

Suggested change
if status, ok := err.(*googleapi.Error); ok {
var status *googleapi.Error
if errors.As(err, &status) {

return nil, storagedriver.PathNotFoundError{Path: srcName}
}
}
return nil, fmt.Errorf("Object(%q).CopierFrom(%q).Run: %v", destName, srcName, err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason not to wrap the error?

Suggested change
return nil, fmt.Errorf("Object(%q).CopierFrom(%q).Run: %v", destName, srcName, err)
return nil, fmt.Errorf("Object(%q).CopierFrom(%q).Run: %w", destName, srcName, err)

@@ -42,6 +44,11 @@ func init() {
return
}

jsonKey, err := ioutil.ReadFile(credentials)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The io/ioutil package is deprecated.

Suggested change
jsonKey, err := ioutil.ReadFile(credentials)
jsonKey, err := os.ReadFile(credentials)

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

client: oauth2.NewClient(dcontext.Background(), ts),
chunkSize: defaultChunkSize,
GCS: gcs,
maxConcurrency: uint64(8),
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI: literal numbers in Go don't have a type.

Suggested change
maxConcurrency: uint64(8),
maxConcurrency: 8,

@milosgajdos
Copy link
Member

For posterity, could you please document in the PR description what was broken about the GCS driver that is being fixed?

The main reason was, the original GCS Go module was outdated so there was a legitimate worry should there be a need for a future fix or adding a new feature that would be hard or impossible with the ancient library.

@jmontleon
Copy link
Author

Thank you @corhere, I made the suggested changes.

It's been a long time now @milosgajdos , but my understanding was also that the GCS driver would no longer even build. This PR replaced #3676 which makes a similar claim.

Copy link
Collaborator

@corhere corhere left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the quick turnaround

@corhere
Copy link
Collaborator

corhere commented Apr 27, 2023

@thaJeztah PTAL

@milosgajdos
Copy link
Member

Ping @thaJeztah -- your review is blocking the merge 😄

@kaovilai
Copy link
Contributor

To fix this long term probably wanna add GCS driver back to CI..

@kaovilai
Copy link
Contributor

by adding gcs buildtag to ci and having real creds

@milosgajdos
Copy link
Member

by adding gcs buildtag to ci and having real creds

we need to do a proper review of the existing storage drivers; azure is also missing build tags. It's also a good question how we do the testing of all the storage drivers.

I'd like these to be ripped out to their dedicated repositories and added as go mod dependencies to the main repo -- that was we can delegate ownership of each driver to folks who work with them; for example I've very little knowledge of the GCS driver so my review is a bit of guessing and reading docs, etc.

There is work to be done, indeed.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Thanks! I left some comments / questions (not all blockers, but some of them may be good to look into before merging)

google.golang.org/grpc v1.31.0 // indirect
google.golang.org/protobuf v1.26.0 // indirect
go.opencensus.io v0.24.0 // indirect
golang.org/x/net v0.8.0 // indirect; updated for CVE-2022-27664, CVE-2022-41717
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this comment is no longer relevant; it was added in 345be95, but dependabot updated it in 9594fbc, and looks like it's now automatically bumped as minimum version

(we can fix that in a follow-up)

Comment on lines 119 to +120
func FromParameters(parameters map[string]interface{}) (storagedriver.StorageDriver, error) {
ctx := context.Background()
Copy link
Member

Choose a reason for hiding this comment

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

  • Are these all intentionally context.Background() or should we ultimately pass a context here (and should this be a context.TODO()?
  • Are all intended to share the same context? (previously jwtConf.TokenSource() and oauth2.NewClient() each got their own context.Background(), which is now shared. Just asking because in some cases it's deliberate that a new context is created.

As to "passing the context" (not a new issue, but went looking if there's a context to be passed);

FromParameters is used by the driverfactory.Create(), which is what's used by NewApp()

app.driver, err = factory.Create(config.Storage.Type(), storageParams)

And it looks like NewApp() takes a context (which is now not based on);

func NewApp(ctx context.Context, config *configuration.Configuration) *App {

Copy link
Member

Choose a reason for hiding this comment

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

This would have been a way bigger change than what this PR ships. Currently, no storage driver passes context to FromParameters. IMHO, should we decide to do this it should go into a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, not as part of this PR; the only change here could be to change context.Background() to a context.TODO() to more clearly indicate "this should still be punched through".

As to whether or not they should share the same context, I started reviewing before I noticed @corhere already reviewed; he's all familiar with the ins-and-outs of contexts, so I expect sharing the context is ok (otherwise he'd very likely would've called it out 😂)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Honestly I just skimmed that part of the code. I should have given it more scrutiny.

The returned [http] client is not valid beyond the lifetime of the context.

It looks like golang.org/x/oauth2 captures contexts in struct fields all over the place. oauth2.NewClient and jwtConf.TokenSource not having an error-return should have tipped me off that the passed contexts weren't just being used as cancelation signals for the constructor function calls themselves. I don't see any reasonable alternative to sharing a context, but it does mean that the constructed StorageDriver is not valid beyond the lifetime of the context. That's outside of the norm, so should be documented so as not to trip up people like me who expect the context lifetime to only bound the lifetime of the function call, not the validity of its return values.

Comment on lines -255 to +270
rc, err = storage.NewReader(gcsContext, d.bucket, name)
return err
})
rc, err := d.gcs.Bucket(d.bucket).Object(name).NewReader(ctx)
Copy link
Member

Choose a reason for hiding this comment

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

Was it previously using a new client for each request? (storage.NewReader()) or did that also reuse the same client?

Copy link
Member

Choose a reason for hiding this comment

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

Answering myself; looks like they are designed to be reused. https://github.com/googleapis/google-cloud-go/blob/storage/v1.30.1/storage/storage.go#L126-L134

// Clients should be reused instead of created as needed. The methods of Client
// are safe for concurrent use by multiple goroutines.

The only part I was wondering is if they need a specific scope assigned (but it looks like the old code already requested FullControl);

The default scope is ScopeFullControl. To use a different scope, like ScopeReadOnly, use option.WithScopes.

Would ScopeReadWrite be enough? Would there be benefits to having a separate client for read-operations?
https://github.com/googleapis/google-cloud-go/blob/storage/v1.30.1/storage/storage.go#L77-L87

Comment on lines -707 to -722
for {
objects, err := storageListObjects(d.context(context), d.bucket, query)
if err != nil {
return nil, err
}
for _, obj := range objects.Results {
// GCS does not guarantee strong consistency between
// DELETE and LIST operations. Check that the object is not deleted,
// and filter out any objects with a non-zero time-deleted
if obj.Deleted.IsZero() {
list = append(list, obj.Name)
}
}
query = objects.Next
if query == nil {
break
Copy link
Member

Choose a reason for hiding this comment

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

I see this loop moved into storageListObjects() ..

Comment on lines +779 to +789
it := bkt.Objects(ctx, q)
for {
objAttrs, err := it.Next()
if err == iterator.Done {
break
}
if err != nil {
return nil, err
}
objs = append(objs, objAttrs)
}
Copy link
Member

Choose a reason for hiding this comment

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

.. before this PR, it looks like listAll() and List() would use the .next to iterate. But now all results are appended to objs;

  • Both check for obj.Deleted.IsZero(), and other properties to check if the object must be included
  • Both append the result to another slice
  • ... but storageCopyObject() unconditionally iterates over all objects (which may even involve multiple API calls), and append all of them to objs[]

I'm not sure how many objects will be returned, but from the above, perhaps it would be better to keep the logic (calling it.Next() as part of those functions)?

Comment on lines +778 to +779
var objs []*storage.ObjectAttrs
it := bkt.Objects(ctx, q)
Copy link
Member

Choose a reason for hiding this comment

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

If we do need to keep a single slice (but see my other comment), would it be possible to initialise objs with an initial capacity based on that (to reduce allocations)?

objs := make([]*storage.ObjectAttrs, 0, `<some expected size>`)

Not sure what information we have available about the expected number of results though; I see the iterator has a PageInfo, but that only has the number of items remaining before a new API call has to be made;
https://pkg.go.dev/google.golang.org/api/iterator#PageInfo.Remaining

Comment on lines +669 to 672
if object.Deleted.IsZero() && object.ContentType != uploadSessionContentType && object.Name != "" {

list = append(list, d.keyToPath(object.Name))
}
Copy link
Member

Choose a reason for hiding this comment

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

If we expect a lot of objects, perhaps it's worth trying to continue as early as possible, and handle the more lightweight checks first;

Suggested change
if object.Deleted.IsZero() && object.ContentType != uploadSessionContentType && object.Name != "" {
list = append(list, d.keyToPath(object.Name))
}
if object.Name == "" || object.ContentType == uploadSessionContentType || !object.Deleted.IsZero() {
continue
}
list = append(list, d.keyToPath(object.Name))

for _, subpath := range objects.Prefixes {
subpath = d.keyToPath(subpath)
if object.Name == "" && object.Prefix != "" {
subpath := d.keyToPath(object.Prefix)
Copy link
Member

Choose a reason for hiding this comment

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

From a quick glance, it looks like in most cases we only need a limited set of the Object's attributes, but a lot more attributes are (potentially) available; https://pkg.go.dev/cloud.google.com/go/storage#ObjectAttrs

The documentation calls out that retrieving all attributes may have a performance impact;
https://pkg.go.dev/cloud.google.com/go/storage#hdr-Listing_objects

If only a subset of object attributes is needed when listing, specifying this subset using Query.SetAttrSelection may speed up the listing process:

Perhaps worth looking into if we can limit the attributes that we retrieve.

@@ -640,33 +652,29 @@ func (d *driver) Stat(context context.Context, path string) (storagedriver.FileI

// List returns a list of the objects that are direct descendants of the
// given path.
func (d *driver) List(context context.Context, path string) ([]string, error) {
func (d *driver) List(ctx context.Context, path string) ([]string, error) {
var query *storage.Query
query = &storage.Query{}
query.Delimiter = "/"
query.Prefix = d.pathToDirKey(path)
list := make([]string, 0, 64)
Copy link
Member

Choose a reason for hiding this comment

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

Not new, but (also see my other comment); do we know where the initial 64 capacity comes from? Is that just a randomly picked value, and should this be based on actual (expected number of) results instead?

Copy link
Member

Choose a reason for hiding this comment

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

I vaguely remember this was a bit of a mystery to me in past and now you've revived the memories. It's hard to think of an "expected number of results other than an educated guess here - List lists the storage keys which can be arbitrarily long (or short). 64 is a slice capacity parameter not the length so I see this as a pre-allocated buffer guesstimate that might help us reap some benefits should the number of listed storage keys is >64 (no need to reallocate the slice until we exceed the count of 64); we could discuss performance implications of this for sure, but I don't see this as a major problem at the moment.

@milosgajdos
Copy link
Member

This needs a rebase again @jmontleon 😬

@kaovilai
Copy link
Contributor

Don't maintainers also have push access? that may help push this PR forward IMO.

@milosgajdos
Copy link
Member

milosgajdos commented May 19, 2023

@kaovilai this needs rebasing - I dont think I can force push to @jmontleon fork. IIRC it used to be possible in past.

$ git push origin -f update-fix-gcs
ERROR: Permission to jmontleon/distribution.git denied to milosgajdos.
fatal: Could not read from remote repository.

@kaovilai
Copy link
Contributor

Rebasing can also be done by maintainer.
https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

You can force push to a fork that is not your own. I had just did this one hour ago to a fork PR.

@milosgajdos
Copy link
Member

Yeah, it's not my first rodeo 😄 but as you can see in that output for whatever reason I am unable to push to that fork.

@kaovilai
Copy link
Contributor

I have better luck with ssh remote-url generally. have you tried that? or try github cli gh pr checkout 3702

Signed-off-by: Jason Montleon <jmontleo@redhat.com>
@jmontleon
Copy link
Author

Rebased. Are there any concrete changes required to get this merged?

@milosgajdos
Copy link
Member

Rebased. Are there any concrete changes required to get this merged?

There are some questions by @thaJeztah -- he blocked this PR so chase him, please.

@jmontleon jmontleon closed this May 22, 2023
@kaovilai
Copy link
Contributor

Someone please feel free to take this work and open a new PR as what's in main right now does not build at all.

It's been almost a year of chasing perfection and rebasing so we are abandoning ship at this point.

@flavianmissi
Copy link
Contributor

I've moved the work to a new PR, could y'all help with review?
Tests are passing 💯

milosgajdos added a commit that referenced this pull request Jun 1, 2023
Fix gcs storage driver

Thanks to @jmontleon who laid the first bricks in #3702
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.

None yet