feat(storage/gcs): add support for Google Cloud Storage

[erka]

fixes #2288

Reference for docs with auth options https://cloud.google.com/docs/authentication#service-accounts

I would suggest that object config should be changed in more flat variant (no s3/azblob/gc object). Probably there will be one store and some configuration switch/builder of each object type.

[codecov]

Codecov Report

Attention: 42 lines in your changes are missing coverage. Please review.

Comparison is base (c2e602a) 71.78% compared to head (8f27806) 71.57%.

Files	Patch %	Lines
internal/storage/fs/object/store.go	71.84%	21 Missing and 8 partials ⚠️
internal/storage/fs/index.go	85.24%	6 Missing and 3 partials ⚠️
internal/storage/fs/snapshot.go	77.77%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2589      +/-   ##
==========================================
- Coverage   71.78%   71.57%   -0.21%     
==========================================
  Files          87       84       -3     
  Lines        8271     8085     -186     
==========================================
- Hits         5937     5787     -150     
+ Misses       1978     1947      -31     
+ Partials      356      351       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[markphelps]

thank you @erka !! Amazing as always

couple minor naming convention updates requested. I think since gs isnt as well known/used as like s3, then maybe we should use something more clear like googlecloud. wdyt? /cc @GeorgeMac

also really love the refactor to use gocloud across the board!

[GeorgeMac]

This is awesome as ever. One thing from me around slashes in prefixes.

One last take it or leave it: I think you could reduce all the different stores down to just one store implementation with different configuration, as you have with FS (I could be missing something). This could also be a later refactor.

[markphelps]

one question and one suggested fix

[markphelps]

looks great! thank you @erka ! I can put up a docs PR and we can try to get this out today or tomorrow in a release

[erka]

looks great! thank you @erka ! I can put up a docs PR and we can try to get this out today or tomorrow in a release

@markphelps It looks like this could take some time to review this PR. Probably it could be included in the next release.

[erka]

@GeorgeMac could you please suggest how to modify this PR? Should I drop go-cloud and just create Google Cloud Storage implementation?

[erka]

@markphelps @GeorgeMac This prefix is a wild bit. After going back and forth it looks like s3 prefix never works. If you check grpc.NewObjectStore (v1.32) or store.newObjectStore (main) it doesn't have s3.WithPrefix option applied to the s3 store... Right? Could you please double check that?

[GeorgeMac]

It looks like you're right @erka 🤦 It was never threaded through. Missed that in review.

It has been this way since day zero.

[GeorgeMac]

@erka I am looking at how the test is setup here for the bug you're experience and realizing that there is another bug in the old S3 implementation, which means it should never have worked in the first place.

The old implementation appears to not be picking up the .flipt.yml file, and your change has made it so it does pick it up.
Because the .flipt.yml file contains an index which only trusts files named default.yml and production.yml, not the file added during the test features.yml which is added during the test.

This is the testdata pushed to s3 (minio): https://github.com/flipt-io/flipt/tree/main/build/testing/integration/readonly/testdata

Apologies, you've dug up some skeletons in the closet 😂

Update: see my mad rant below, I was barking up the wrong tree.

[GeorgeMac]

@erka I got the following changes to work as expected for me 👇

What do you think about those. I think they're roughly inline with how things worked previously. Though we will never know as we never actually wired it up properly.

[erka]

@erka I got the following changes to work as expected for me 👇

What do you think about those. I think they're roughly inline with how things worked previously. Though we will never know as we never actually wired it up properly.

@GeorgeMac thank you for the hint.

Probably this is a good time to think how prefix should work. That missing WithPrefix option give the change to make it right.

[GeorgeMac]

@erka yeah I think even my suggestion here might be subtly broken. If you add a case here:

flipt/internal/storage/fs/object/s3/store_test.go

Lines 96 to 100 in 386fc85

	// namespace shouldn't exist as it has been filtered out by the prefix
	require.Error(t, store.View(func(s storage.ReadOnlyStore) error {
	_, err := s.GetNamespace(context.TODO(), "production")
	return err
	}), "production namespace shouldn't be retrieavable")

If you change the assertion at the bottom too:

	// namespace shouldn't exist as it has been filtered out by the prefix
	require.NoError(t, store.View(func(s storage.ReadOnlyStore) error {
		_, err := s.GetNamespace(context.TODO(), "production")
		require.Error(err, "production namespace shouldn't be retrieavable")
		
		_, err := s.GetNamespace(context.TODO(), "prefix")
		require.NoError(err, "prefix namespace should be retrieavable")
		
		return nil
	}))

It fails because the prefix namespace didn't actually get read.
I tried a few other permutations and found that fs.WalkDir is doing to some rewriting of paths if you return FileInfo instances with Names that include directories (e.g.prefix/prefix_namespace.yml) is going to get rewritten.

I've got some ideas on how we could get this all in line so that prefix can be arbitrary.
Otherwise, we could change it to be an actually directory.
I do worry though that some users may be depending on this prefix as a simple way to just reduce the number of items listed, not as a directory name. 🤔 hmmmm
I might quickly try out the implementation of gcblob.Bucket as it already implements io/fs.
I wonder if that works with arbitrary prefixes...

Update: the gblob implementation looks promising, but when used with a non slash terminated prefix it goes into an infinite loop on Walk constantly returning the root current dir .. Can't win haha.

[GeorgeMac]

Thanks for being so patient with me on this @erka ! I might need to sleep on it for today, but I have one idea I would like to explore which is to skip making an fs.FS and instead use:

flipt/internal/storage/fs/snapshot.go

Lines 107 to 135 in 386fc85

	// SnapshotFromFiles constructs a StoreSnapshot from the provided slice
	// of fs.File implementations.
	func SnapshotFromFiles(logger *zap.Logger, files ...fs.File) (*Snapshot, error) {
	now := flipt.Now()
	s := Snapshot{
	ns: map[string]*namespace{
	defaultNs: newNamespace("default", "Default", now),
	},
	evalDists: map[string][]*storage.EvaluationDistribution{},
	now: now,
	}
	for _, fi := range files {
	defer fi.Close()
	docs, err := documentsFromFile(fi)
	if err != nil {
	return nil, err
	}
	for _, doc := range docs {
	if err := s.addDoc(doc); err != nil {
	return nil, err
	}
	}
	}
	return &s, nil
	}

My thoughts here is that fs.FS is needlessly overcomplicated for a flat keyspace like a blob store.
The OCI package depends on this, because we just flatten your features yaml files there and just put them in a list.

This just takes a list of fs.File which we can fabricate for each key listed in the blob store with an optional prefix.
The one extra caveat is that we (at-least intended) to support the .flipt.yml too and we would need to filter the list of files with the globs in that file first (might need to refactor the listStateFiles function a bit and export parts of it). All a bit involved, happy to make a PR on your PR tomorrow if you like. Or you're welcome to try that too if you dont mind of course. You've put a lot of hard time into this already :D so no pressure.

[GeorgeMac]

Hey @erka I pushed up a branch over here based on your work:
https://github.com/flipt-io/flipt/compare/gm/gocloud-object?expand=1#diff-d0c365d367f4c2650c25d6810f08164236c3e16a89faa5b86d55bce1c7ccfb5f

What do you think of this? I was going to make a PR onto your branch, but I can't write into your fork.
If you like it, you're always welcome to rebase your branch on the head of this one.
That way we get it back into this PR.

I will open it up as a draft for now just to get CI testing it.

The long and short of the change is it just creates a single store implementation under the object package.
Then it just leans into the fact that gcblob can take the different configurations as URL strings.
It drops trying to make a full fs.FS infavour of using storagefs.NewSnapshotFromFiles() which takes list of files.

[GeorgeMac]

One last thing and I think its good to go. Just need to add this new IT fs/gcs to the GH workflows list:

flipt/.github/workflows/integration-test.yml

Lines 56 to 60 in 770e807

	"fs/git",
	"fs/local",
	"fs/s3",
	"fs/oci",
	"fs/azblob",

[GeorgeMac]

Epic. Thanks so much @erka 🙇‍♂️