Add IAM support to APIs which support it

(copied from googleapis/google-cloud-node#1192, which is funnily-enough exactly true for gcloud-python, too).

IAM is currently buried inside of our Pub/Sub code, but should be abstracted to be used with any API that supports it:

A gentle ping on this, given that IAM is now GA: is there an IAM API coming? Any chance the Pub/Sub IAM API might be brought up to date.

@kir-titievsky

@kir-titievsky PR #2031 seems to be related but has not seen updates for a few months.

@lukesneeringer

@lukesneeringer How would you prefer that I factor out the existing google.cloud.pubsub IAM support?

Shared bits move into a new google.clooud.iam module inside the core/ subproject?
Shared bits move into a new iam/ subproject?

Good question. My instinct is that I would prefer not have an excessive number of sub-projects, and to put it in a google.cloud.iam module in core. My feeling is that if we later have six or seven projects like that, it will become hard to know what is an API and what is a shared module.

That is a loose preference though and I am happy to be persuaded otherwise.

(If this were day 1, I would have preferred it have been google.cloud.core.* namespaced.)

I'm find with putting it in core/: I was just worried that we wanted to be more conservative about such changes.

@jonparrott

What about putting it all in google-auth-library-python? @jonparrott WDYT?

@dhermes

@dhermes I'd prefer to keep code that hits any non-auth API endpoint outside of google-auth (I was actually really wary of adding IAMSigner). But I may be able to be convinced if you think it's the best place for this.

gax/core seems like the right spot for this IMO.

I'd vote for core at this point: the APIs of interest (pubsub, storage) don't expose any IAM-related methods over gRPC: they are REST-only. There really isn't very much code involved for the API-specific bits (mostly knowing how to formulate the request URLs, which aren't uniform).

Most of the code will be in the "common" bits: schlepping the policy resource into / out of the Python object, creating various principal iDs, etc. See #3188.

@tseaver

@tseaver To answer a prior question, I am fine with additive changes to core. What I want to move away from is sweeping refactors like what we did with ._connection that basically threw you into situations where you could not run certain juxtapositions of APIs simultaneously.

FYI looks like most of the IAM implementations (not just pubsub) will break with the introduction of custom roles which is happening within the next month (already some customers in alpha).

Fortunately the solution is simple. Stop maintaining individual sets for each role and instead maintain one dict of sets which maps from roles -> members. Then make property shortcuts use the new "bindings" property to maintain backwards compat.

@bjwatson

FYI looks like most of the IAM implementations (not just pubsub) will break with the introduction of custom roles which is happening within the next month (already some customers in alpha).

Wait, what? This is pretty critical information.
Has the IAM team reviewed the implementations that just went in for Node and PHP to see if they are forward-compatible with your changes?

Tagging @bjwatson, @omaray for awareness.

Fortunately the solution is simple. Stop maintaining individual sets for each role and instead maintain one dict of sets which maps from roles -> members. Then make property shortcuts use the new "bindings" property to maintain backwards compat.

Could we see an example?

@lukesneeringer

@lukesneeringer I have provided an example on python on @tseaver 's CL, which is how I had it set up in my proposal for #2031 . I don't know about Node or PHP since I don't know those languages, but we should make sure of that. In python it could be done backwards compat like this:

class Policy:
  _LEGACY_OWNERS_ROLE = 'roles/owner'

  def __init__(self, version, etag, bindings=None):
     if bindings is None:
       self.bindings = {}
     self.version = version
     self.etag = etag

  def from_api_repr(cls, resource):
    return cls(
      resource.get('version'),
      resource.get('etag'),
      bindings= {binding['role']: set(binding['members'])
                        for binding in resource.get('bindings')}
    )
  
  @property
  def owners(self):
    return self.bindings.setdefault(type(self)._LEGACY_OWNERS_ROLE, set())

  @owners.setter
  def set_owners(self, values):
     """Probably shouldn't use this, but backwards compat"""
     assert isintance(values, set)
     return self.bindings[type(self)._LEGACY_OWNERS_ROLE] = values

  ... # Repeat for other legacy roles

  def to_api_repr(self):
    return {
      'etag': self.etag,
      'version': self.version,
      'bindings': [{'role': role, 'members': members} 
                         for role, members in six.iteritems(self.bindings)
                         if members] # Sending empty members sets will result in an error.
    }

To support custom roles at the very least you need to support arbitrary role strings and not limit the role strings that you parse out of policy bindings.

To fully support custom roles, you'll need to support several more API endpoints, see here: https://cloud.google.com/iam/docs/creating-custom-roles they unfortunately aren't listed in the discovery doc yet, I suspect because they are still under whitelist.

There's a design questions about whether or not these endpoints belong on a separate IAM object, or within the Resource object. On the one hand, these endpoints already take an arbitrary resource path, which every resource should track, and might make it easier for users if it was on the Resource object. On the other, these are technically against the iam.googleapis.com endpoint, and not each individual resource's endpoint. Frankly this is a design discussion that already should have been had for queryGrantableRoles which exists in the same state.

Additionally it doesn't look like there's any support for testIamPermissions at all in this package? This is part of the meta-API and should definitely be on every Resource that supports IAM.

@elibixby

@elibixby google.cloud.pubsub has implemented get_iam_policy, set_iam_policy, and test_iam_permissions for two years. PR #3188 is only factoring out the "shared" bits of google.cloud.pubsub.iam into a new google.cloud.iam module. I have updated it to address your issues concerning custom roles.

The bits of your proposal which add a common subclass for the implementation of those API methods are trickier to address: the endpoints are not consistent across resource types: storage appends /iam and uses GET / PUT, while pubsub uses GET against ':getIamPolicy/POSTagainst:setIamPolicy`. The implementations are so minimal that I plan to just fork them, rather than attempting to framework away those differences.

@tseaver

@tseaver This makes it difficult/slow to implement IAM for a new API. One platform should standardize the implementations. In this case it is Pub/Sub which is doing things correctly and storage which is the oddball since the former is a OP API and the latter is not. So new APIs should follow the pattern of Pub/Sub.

As a potential user of this library I have avoided it for a while because it didn't have IAM on APIs that supported it, and I needed, and used apiary instead. For example resource manager: also a OP API that follows the same implementation as pubsub.

It's really critical to reduce the amount of time it takes to add support for IAM to new APIs, since all new OP APIs that rollout must have such support now, and I think a Mixin is the best way of doing this.

That said it can definitely be addressed in a follow-up PR. I just wouldn't consider this issue closed until such a strategy is in place.

@tseaver

@tseaver What would be your thoughts on implementing generalistic class the "pubsub way" (which @elibixby says is the correct way) and subclassing for storage?

Clarity: I am not mandating this. Honestly asking.

@elibixby

What would be your thoughts on implementing generalistic class the "pubsub way" (which @elibixby says is the correct way) and subclassing for storage?

Maybe it would work out, if storage is truly the only exception. I don't think there will be any point in subclassing for storage, rather than just re-implementing the same set of API methods.

@elibixby

@elibixby I want to follow up on something you said earlier.

FYI looks like most of the IAM implementations (not just pubsub) will break with the introduction of custom roles which is happening within the next month (already some customers in alpha).

I am worried about the potential that something is coming out that will break our libraries, and I still do not understand what the change is or how it will break the library.

Can we set up a meeting (internally) to discuss? I want to figure out what, if anything, cross-language action items are.

chore: consolidate PyarrowVersions helpers (#1679)

refactor: add uid generator and encasualate query as cte in SQLGlotCo…

Navigation Menu

Repository navigation

Add IAM support to APIs which support it #1679

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Add IAM support to APIs which support it #1679

Description

Activity

kir-titievsky commented on Nov 2, 2016

ncouture commented on Jan 1, 2017

tseaver commented on Mar 22, 2017

lukesneeringer commented on Mar 22, 2017

tseaver commented on Mar 22, 2017

dhermes commented on Mar 22, 2017

theacodes commented on Mar 22, 2017

theacodes commented on Mar 22, 2017

tseaver commented on Mar 22, 2017

tseaver commented on Mar 22, 2017

lukesneeringer commented on Mar 22, 2017

elibixby commented on Mar 22, 2017

lukesneeringer commented on Mar 23, 2017

elibixby commented on Mar 23, 2017

tseaver commented on Mar 23, 2017

elibixby commented on Mar 23, 2017

lukesneeringer commented on Mar 23, 2017

tseaver commented on Mar 23, 2017

lukesneeringer commented on Mar 23, 2017

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions