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

chore(pubsub): add subscriber role test for streaming #9507

Merged
merged 1 commit into from
Oct 31, 2019

Conversation

plamut
Copy link
Contributor

@plamut plamut commented Oct 21, 2019

Fixes #9382.

Pulling the messages using a streaming pull should work with accounts having only the pubsub.subscriber role. This commits adds a test that covers this aspect.

How to test

  • Apply the following patch to the streaming pull manager:
    diff --git pubsub/google/cloud/pubsub_v1/subscriber/_protocol/streaming_pull_manager.py pubsub/google/cloud/pubsub_v1/subscriber/_protocol/streaming_pull_manager.py
    index d3b1d6f51eb..d73a6f9cdd8 100644
    --- pubsub/google/cloud/pubsub_v1/subscriber/_protocol/streaming_pull_manager.py
    +++ pubsub/google/cloud/pubsub_v1/subscriber/_protocol/streaming_pull_manager.py
    @@ -403,6 +403,7 @@ class StreamingPullManager(object):
            # the default stream ACK deadline should again be set based on the
            # historic ACK timing data, i.e. `self.ack_histogram.percentile(99)`.
            stream_ack_deadline_seconds = _DEFAULT_STREAM_ACK_DEADLINE
    +       subscription = self._client.api.get_subscription(self._subscription)
    
            get_initial_request = functools.partial(
                self._get_initial_request, stream_ack_deadline_seconds
  • Run the new system test test_streaming_pull_subscriber_permissions_sufficient.

Expected result:
The test should fail (permission error), proving that it would catch the regression in #9339, and undoing the above patch makes the test pass.

@plamut plamut added testing api: pubsub Issues related to the Pub/Sub API. type: process A process-related concern. May include testing, release, or the like. labels Oct 21, 2019
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 21, 2019

# TODO: A service account granting only the pubsub.subscriber role must
# be used
filename = "/some/service/account/file.json.json"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@busunkim96 Let's talk offline on how to inject such service account into Kokoro environment, and how to do the same for the convenience of developers (we cannot commit the actual service account .json key file, of course).

Copy link
Contributor

Choose a reason for hiding this comment

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

There is now a service account with just Pub/Sub subscriber in ${KOKORO_GFILE_DIR}/pubsub-subscriber-service-account.json

You can set an environment variable in https://github.com/googleapis/google-cloud-python/blob/master/.kokoro/build.sh

I think there may actually be a way to use the existing service account to create a short-lived credential with only the Pub/Sub subscriber role. See https://cloud.google.com/iam/docs/creating-short-lived-service-account-credentials#sa-credentials-permissions and https://google-auth.readthedocs.io/en/latest/reference/google.auth.impersonated_credentials.html#module-google.auth.impersonated_credentials

This would also be nicer for developers trying to run the tests (they don't have to worry about having a separate service account with the correct role)

@plamut plamut added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 29, 2019
Pulling the messages using a streaming pull should work with accounts
having only the pubsub.subscriber role. This commits add a test that
covers this aspect.
@plamut plamut force-pushed the iss-9382 branch 3 times, most recently from 626e057 to 80f051a Compare October 29, 2019 11:55
@plamut
Copy link
Contributor Author

plamut commented Oct 29, 2019

The test now correctly fails with 403 if the regression from #9339 is re-introduced.

I opted to not add the path to the special service account file in .kokoro/build.sh, as adding more test-specific stuff to the common config could eventually bloat it.

I haven't explored the short-lived service accounts for local development yet, I got around it by skipping the test if not inside the Kokoro environment (but that could still be faked locally, if needed).

@plamut plamut removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 29, 2019
Copy link
Contributor

@pradn pradn left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you, Peter!

@plamut
Copy link
Contributor Author

plamut commented Oct 31, 2019

Yay, merging then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the Pub/Sub API. cla: yes This human has signed the Contributor License Agreement. testing type: process A process-related concern. May include testing, release, or the like.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PubSub: Add system test for streaming pull using an account with pubsub.subscriber role
4 participants