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

Add system tests for topic/subscription IAM policy get/set methods. #1654

Merged
merged 4 commits into from
Mar 29, 2016
Merged

Add system tests for topic/subscription IAM policy get/set methods. #1654

merged 4 commits into from
Mar 29, 2016

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented Mar 23, 2016

The tests uncover a wart in the API: setIamPermissions takes an extra wrapper element (policy) around the actual Policy resource. I don't know where to report that issue.

The new system tests fail repeatedly for my system account with 503s: adding retries (interspersed with time.sleep(1)) doesn't seem to help. @tmatsuo can you comment? (Note that I have made that account an owner of my project).

@tseaver tseaver added testing api: pubsub Issues related to the Pub/Sub API. labels Mar 23, 2016
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 23, 2016
@@ -305,8 +305,10 @@ def set_iam_policy(self, policy, client=None):
client = self._require_client(client)
path = '%s:setIamPolicy' % (self.path,)
resource = policy.to_api_repr()
# 'set_iam_policy' API requires an extra wrapper. :(

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@dhermes dhermes assigned tmatsuo and unassigned dhermes Mar 24, 2016
@dhermes
Copy link
Contributor

dhermes commented Mar 24, 2016

Mostly looks fine though we should resolve the 503s before I gave an LGTM. Assigned to @tmatsuo for now.

@dhermes
Copy link
Contributor

dhermes commented Mar 28, 2016

@tmatsuo Bump

@tmatsuo
Copy link
Contributor

tmatsuo commented Mar 28, 2016

Which test and API are repeatedly failing? Any logs? Detailed message? Does it always fail or sometimes succeed? Can you show the retry code?

@tseaver
Copy link
Contributor Author

tseaver commented Mar 28, 2016

@tmatsuo the new system tests being added in this PR fail with 503s at the following points.

Wrapping the set_iam_policy calls with retry logic didn't help, so I removed it.

@tmatsuo
Copy link
Contributor

tmatsuo commented Mar 28, 2016

Does it always fail? If so, is it possible to show the actual JSON request?

@tseaver
Copy link
Contributor Author

tseaver commented Mar 28, 2016

@tmatsuo Just before the projects.topics.setIamPolicy API call:

(Pdb) l
298             """
299             client = self._require_client(client)
300             path = '%s:setIamPolicy' % (self.path,)
301             resource = policy.to_api_repr()
302             wrapped = {'policy': resource}
303  ->         resp = client.connection.api_request(
304                 method='POST', path=path, data=wrapped)
305             return Policy.from_api_repr(resp)
(pdb) pp path
u'/projects/citric-celerity-697/topics/test-iam-policy-topic1459191212607:setIamPolicy'
(Pdb) pp wrapped
{'policy': {'bindings': [{'members': ['user:jgeewax@google.com'],
                          'role': 'roles/reader'}],
            'etag': u'ACAB'}}

At the return:

(Pdb) pp response
{'-content-encoding': 'gzip',
 'alt-svc': 'quic=":443"; ma=2592000; v="31,30,29,28,27,26,25"',
 'alternate-protocol': '443:quic,p=1',
 'cache-control': 'private',
 'content-length': '162',
 'content-type': 'application/json; charset=UTF-8',
 'date': 'Mon, 28 Mar 2016 18:55:54 GMT',
 'server': 'ESF',
 'status': '503',
 'transfer-encoding': 'chunked',
 'vary': 'Origin, X-Origin, Referer',
 'x-content-type-options': 'nosniff',
 'x-frame-options': 'SAMEORIGIN',
 'x-xss-protection': '1; mode=block'}
(Pdb) pp content
'{\n  "error": {\n    "code": 503,\n    "message": "The service was unable to fulfill your request. Please try again. [code=8a75]",\n    "status": "UNAVAILABLE"\n  }\n}\n'
(Pdb) pp url
'https://pubsub.googleapis.com/v1/projects/citric-celerity-697/topics/test-iam-policy-topic1459191212607:setIamPolicy'

@tseaver
Copy link
Contributor Author

tseaver commented Mar 28, 2016

@tmatsuo the projects.subscriptions.getIamPolicy call is now repeatable returning a 404 (waiting is not helping):

gcloud.exceptions.NotFound: 404 Resource not found (resource=test-iam-policy-sub-1459192208068). (GET https://pubsub.googleapis.com/v1/projects/citric-celerity-697/subscriptions/test-iam-policy-sub-1459192208068:getIamPolicy)

@tmatsuo
Copy link
Contributor

tmatsuo commented Mar 28, 2016

According to https://cloud.google.com/pubsub/access_control

I don't think roles/reader is a valid role. Can you try it with roles/viewer or roles/pubsub.viewer ?
The error message is not helpful at all, which should be improved though.

@tseaver
Copy link
Contributor Author

tseaver commented Mar 28, 2016

@tmatsuo After updating the role strings, I'm still getting the 503 from projects.topics.setIamPolicy. I have checked that the payload for the request now has the correct role:

-> resp = client.connection.api_request(
(Pdb) pp wrapped
{'policy': {'bindings': [{'members': ['user:jgeewax@google.com'],
                          'role': 'roles/viewer'}],
            'etag': u'ACAB'}}

FWIW: A 503 is a terrible status code for "you gave me bad data"; that status is supposed to mean "my backend went away unexpectedly, try again later." A better status would be a 40x (probably just 400 "Bad Request").

@tmatsuo
Copy link
Contributor

tmatsuo commented Mar 29, 2016

@tseaver

Indeed, the HTTP status code is terrible. I think the product team is working on it.

Maybe I found out the cause. I suspect the account jgeewax@google.com doesn't exist. Can you try using an existing account like tmatsuo@google.com?

@tseaver
Copy link
Contributor Author

tseaver commented Mar 29, 2016

@tmatsuo Indeed, fixing the e-mail address makes the setIamPolicy calls succeed. Now I just need to figure out why the teardown code blows up for the subscription case.

@tseaver tseaver assigned dhermes and unassigned tmatsuo Mar 29, 2016
@tseaver
Copy link
Contributor Author

tseaver commented Mar 29, 2016

@dhermes the two new system tests now pass for me. PTAL

@tmatsuo thanks for the help!

@@ -127,9 +127,9 @@ def from_api_repr(cls, resource):
members = set(binding['members'])
if role == OWNER_ROLE:
policy.owners = members
elif role == WRITER_ROLE:
elif role == EDITOR_ROLE:
policy.writers = members

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

policy = topic.get_iam_policy()
policy.readers.add(policy.user('jjg@google.com'))
new_policy = topic.set_iam_policy(policy)
self.assertEqual(new_policy.readers, policy.readers)

This comment was marked as spam.

This comment was marked as spam.

@dhermes
Copy link
Contributor

dhermes commented Mar 29, 2016

LGTM

@tseaver tseaver merged commit 7aafecd into googleapis:master Mar 29, 2016
@tseaver tseaver deleted the pubsub-system_tests-iam_policy branch March 29, 2016 18:57
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants