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

Continuation of event support PR #490

Merged
merged 1 commit into from
Dec 29, 2021

Conversation

terev
Copy link
Contributor

@terev terev commented Apr 28, 2021

This pr is a continuation of PR #370 created by @isaldana . I hope its okay that I picked up this work. This would be a really great feature to have available. If this pr is accepted I want to make sure the work is attributed to @isaldana. I was sure to rebase that work to retain their commits.

I've addressed the comment left on his pr about moving event options to a separate struct. I've also made a few fixes including:

  • a typo in an option name
  • the event generated from object creations didn't include the metadata from the created object
  • the workaround for a gsutil cp issue discovered by @StephenWithPH here gsutil cp bad request #217 (comment) . i have done some digging on the next error but have yet to discover a solution
  • Always add localhost route rule to ensure requests via another network (like docker bridge) succeed. addresses issue encountered in [go-example] object doesn't exist #280

@isaldana please feel free to veto this pr if you prefer i dont submit it on your behalf

@isaldana
Copy link
Contributor

@terev Thanks so much for addressing the comments on the PR and sorry for the late reply. This looks good to me. Thanks again!

@fsouza
Copy link
Owner

fsouza commented Jul 21, 2021

Hey @terev, I'm sorry I neglected your PR. I'll find some time this week to look into it.

@terev
Copy link
Contributor Author

terev commented Jul 21, 2021

no worries!

fakestorage/upload.go Outdated Show resolved Hide resolved
Copy link
Owner

@fsouza fsouza left a comment

Choose a reason for hiding this comment

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

Event changes looks good, but can you rebase your branch to make it mergeable, move the event files to a dedicated (and internal) package, and split the fixes for #217 and #280 into separate PRs?

Thank you very much for working on this!

fakestorage/event.go Outdated Show resolved Hide resolved
@fsouza
Copy link
Owner

fsouza commented Jul 28, 2021

(when you rebase, can you make sure that events to any new endpoints, such as compose?)

@mlhamel
Copy link

mlhamel commented Dec 14, 2021

Any updates on this one? That would be awesome to have this branch being part of an official release 🙏

@terev
Copy link
Contributor Author

terev commented Dec 15, 2021

@mlhamel sorry i've neglected this for too long. i'll get this pr ready for review sometime this week

Co-authored-by: francisco souza <108725+fsouza@users.noreply.github.com>
@terev
Copy link
Contributor Author

terev commented Dec 18, 2021

@fsouza I've completed the following:

  • rebased
  • moved event.go to dedicated package (notifications)
  • added support for archive events
  • added support for compose endpoint
  • added support for update metadata endpoint
  • removed extraneous changes

@terev terev requested a review from fsouza December 18, 2021 08:48
Copy link
Owner

@fsouza fsouza left a comment

Choose a reason for hiding this comment

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

Nice, did a quick review and everything looks ok! I'll give it another review later today and likely merge and release it before EOY 😁

Happy to see this coming through :D

@fsouza
Copy link
Owner

fsouza commented Dec 29, 2021

@terev btw, can you fix the lint failure? (just need goimports -w I think). Not a problem if you're busy or off for the holidays, I can definitely do it before merging.

@fsouza fsouza changed the base branch from main to event-support-wip December 29, 2021 23:38
@fsouza fsouza merged commit 884a46e into fsouza:event-support-wip Dec 29, 2021
@Ortega-Dan
Copy link

Looks like this allows fakegcs to publish notifications to PubSub ? How can this be used, or configured ? Is there some documentation somewhere. Sorry I did my search but couldn't find anything.
I actually tried to use it surely with the regular configuration from GCP API, but what if the notification or PubSub server needs to be changed to notify to a local PubSub emulator ? Is this possible ?
(cc @fsouza @isaldana )

@Ortega-Dan
Copy link

I just realized it is possibly achievable by adding the PUBSUB_EMULATOR_HOST env variable to the fage-gcs-server. I am going to try it right now and share if it worked.

@terev
Copy link
Contributor Author

terev commented May 13, 2023

@Ortega-Dan Yeah that's the way to get the server to call an emulated pubsub server. But you'll also need to set a few pubsub config options. You can find the flags added by this mr here https://github.com/fsouza/fake-gcs-server/pull/490/files#diff-54c7c1af5fa8d5db4dc49f0e8e80e93ba2b1183ba4d5c9e2e5729e6deae6a3cdR66-R69 .

@Ortega-Dan
Copy link

Ortega-Dan commented May 13, 2023

Thank you very much @terev

I was able to successfully get the two emulators to communicate with each other with:

docker network create testnetwork

then
docker run --rm -it -p 8085:8085 --network=testnetwork --name pubsub google/cloud-sdk:emulators gcloud beta emulators pubsub start --host-port=0.0.0.0:8085

and
docker run --rm -it -p 4443:4443 --network=testnetwork -e PUBSUB_EMULATOR_HOST=pubsub:8085 --name fakegcs fsouza/fake-gcs-server:latest -scheme http -event.pubsub-project-id example-name -event.pubsub-topic projects/example-name/topics/NEW_FILE_UPLOAD

and I have even made sure I created the project and topic first in the PubSub emulator, and tested the PubSub emulator independently ... but when I create a file with:

   WriteChannel channel = storage
                    .writer(BlobInfo.newBuilder(bucketName, "some_file2.txt").build());
            channel.write(ByteBuffer.wrap("line3\n".getBytes()));
            channel.write(ByteBuffer.wrap("line4\n".getBytes()));
            channel.close();

I am able to confirm the file gets created but the notification never gets there.

I am also able to confirm that the PubSub server is definitely reachable by the fakegcs because the PubSub at least detects the connection but only when the fakegcs starts.

[pubsub] May 13, 2023 4:17:57 AM io.gapi.emulators.netty.HttpVersionRoutingHandler channelRead
[pubsub] INFO: Detected HTTP/2 connection.

but when the file is created there is no PubSub message delivered.

Could there be something else missing ?

@terev
Copy link
Contributor Author

terev commented May 13, 2023

@Ortega-Dan The pubsub emulator takes a --project flag which may be what you're missing. It should match the project id you pass to gcs emulator. Additionally I don't think the topic won't be automatically created. So you'll need to call the emulator's api to create it https://cloud.google.com/pubsub/docs/reference/rest/v1/projects.topics/create .

@terev terev deleted the event-support branch May 13, 2023 07:25
@Ortega-Dan
Copy link

Ortega-Dan commented May 13, 2023

Thanks @terev . I included the project to the pubsub emulator like:
docker run --rm -it -p 8085:8085 --network=testnetwork --name pubsub google/cloud-sdk:emulators gcloud beta emulators pubsub start --project=example-name --host-port=0.0.0.0:8085
and the topic was originally created for the specific project and even a subscription to that topic created and tested from a Java Client, and it works when any other client pushes a notification to that topic, but not getting the notification from fakegcs.

(Just noting: After identifying the real issue shown in the previous message, It was shown that the --project flag for the pubsub emulator was not the real problem, even when it is recommended)
(Also for the record, I was glad to find out that PubSub doesn't really have to be up for the time FakeGCS starts, it can even attempt several failed notifications, and if the PubSub comes online after, it will succeed sending the notifications for other files later)

@Ortega-Dan
Copy link

Ortega-Dan commented May 13, 2023

While I was writing the previous message, I added some additional code that I had removed before from the cloud storage client, that was just a read instruction of the previously uploaded bucket.
And only when running that additional instruction after the closing of the file:

  WriteChannel channel = storage
                   .writer(BlobInfo.newBuilder(bucketName, "veryNewFile.txt").build());
           channel.write(ByteBuffer.wrap("line3\n".getBytes()));
           channel.write(ByteBuffer.wrap("line4\n".getBytes()));
           channel.close();

// ... BY ADDING THE FOLLOWING LINES AGAIN ...

           Blob someFile2 = storage.get(bucketName, "veryNewFile.txt");
           String fileContent = new String(someFile2.getContent());

And when that happened, and only after that additional instruction (even when the previous instruction already closed the file by closing the writechannel), I saw the following error:

INFO[0021] error publishing event: rpc error: code = InvalidArgument desc = Invalid [topics] name: (name=projects/example-name/topics/projects/example-name/topics/NEW_FILE_UPLOAD)172.20.0.1 - - [13/May/2023:17:53:01 +0000] "GET /storage/v1/b/bucketNameNew/o/veryNewFile.txt?projection=full HTTP/1.1" 200 508

Which was solved then by starting fakegcs with the notification flags as:

-event.pubsub-project-id example-name -event.pubsub-topic NEW_FILE_UPLOAD

instead of:

-event.pubsub-project-id example-name -event.pubsub-topic projects/example-name/topics/NEW_FILE_UPLOAD

I was providing the full topic name, because that is what other tools like PubSub have required.
I am also not sure why the error message was not shown by just closing the writechannel, and needed and additional instruction to show up.

From this we could: improve the help flag description, to note that only the final portion of the topic name is required for that flag, and possibly error out from the beginning if a full topic name is provided.
And see why the error doesn't show immediately after the file is written to the bucket.

@terev
Copy link
Contributor Author

terev commented May 15, 2023

@Ortega-Dan Ah I see. Glad you got it working! It seems the error message didn't get flushed right away as the writer is line buffered and that specific write doesn't include a newline. Those are great change suggestions.

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.

5 participants