all: define a meta-interface for IAM

[broady]

Many APIs (pubsub and storage are probably the most prominent) need to modify IAM roles.

Someone needs to define a meta-interface so that the IAM functionality is consistent across packages.

Maybe something similar to storage.ACLHandle?

[broady]

cc @rakyll @jba @light

[jba]

The way the IAM methods are factored into a separate service that takes an arbitrary resource name makes it easy for service implementors, but not so much for users. We probably don't want to expose that directly, because specifying a resource name correctly can be a challenge.

I think that for clients that represent each entity as a separate type, like pubsub, each such type (e.g. pubsub.Topic) should have its own set of methods:

• GetIAMPolicy(ctx) (*Policy, error)
• SetIAMPolicy(ctx, *Policy) (*Policy, error)
• TestIAMPermissions(ctx, perms []string) ([]string, error)

The resource name will be handled entirely by the client.

Other clients may add a string field to those methods denoting a suffix of the full resource name. For example, the new logging client uses logIDs (not full log resource names), so it could have IAM methods that take log IDs.

We may want to wrap the Policy proto and put it in a package like cloud.google.com/go/iam, or we may just want to leave it as a proto. It translates into a pretty straightforward Go type.

[okdave]

Instead of adding those methods to each type, could we have a central IAM package which can deal with arbitrary types? That would keep the signatures for these basic types much less polluted.

I imagine we could do something like:

package iam

func (c *Client?) GetPolicy(x interface{}) (*Policy, error) {
  id := internal.Lookup(x)  // Say.
  if id == "" { return nil, fmt.Errorf("unknown type %T", x) }
}

package pubsub

func init() {
  internal.RegisterType(func(t *Topic) string {return t.id() })
  etc.
}

That's a very, very rough sketch – would something like that work?

[okdave]

One interesting thing which might influence the design: IIUC the IAM service has it's own endpoint with specific scopes (https://iam.googleapis.com/v1/* and iam.*) – that might indicate we need a separate Client type anyway?

[shantuo]

Yes, I realized that we probably need another Client for iam when I looked into the code.

While from a user's point of view, I'd love to see a simple and straightforward interface like what @jba mentioned. Maybe we can have an iam package for the client and put the iam.Client into each major API (storage and pubsub)? In that way user can use all the iam methods without directly use the iam package.

[broady]

The iam package is actually a little different - it's for managing service accounts and keys only.

Each API has its own methods for IAM operations.

I'm a bit biased, but I liked what I did with the storage package.

Something like

pubsubTopic.IAM() // type cloud.IAMPolicy (or something else)

policy, err := pubsubTopic.IAM().Get()

policy, err := pubsubTopic.IAM().SetRole(...) // Handles Get/Modify atomically
policy, err := pubsubTopic.IAM().RemoveRole(...) // Handles Get/Modify atomically

package cloud

type IAMPolicy interface {
  Get()
  Set(...)
  SetRole(...)
  RemoveRole(...)
}

This way, we don't have three+ (more in future) methods for IAM for each resource in our packages.

[jba]

I like @broady's idea because it's clear which things have IAM policies.

We could also consider a variant of @okdave's idea:

type IAMer interface { iamResourceName() string }

func (c *Client) GetIAMPolicy(x IAMer) ...

We'd repeat this in each client.

There is no additional user-visible surface for each type that supports IAM, but the IAM calls are checked at compile-time. The downside is that you'd have to read the docs to know which types are IAMers. We already have precedent for this: the bigquery Copy method, and Source and Destination interfaces.

[elibixby]

FYI you're also going to have to implement testIAMPermissions and queryGrantableRoles as they are part of the IAM meta interface. Although they aren't super important right now they will be come very important once Custom Roles drop Soon (TM).

[jba]

This is now a priority for early Q4 so let's find a design we can agree on. I know @pongad is also thinking about this.

I think @broady has the best idea so far: each IAM-enabled resource has one additional method, IAM, which returns a value that handles all IAM functionality for that resource.

Pros:

• Clear from the doc what resources support IAM.
• Only one method per resource.
• IAM support can live in a single package common to all clients. Not only do we factor out the implementation, we also factor out the doc--the only added doc weight is the per-resource IAM method.

[jba]

See https://code-review.googlesource.com/#/c/8130.

[jba]

One complication: it looks like QueryGrantableRoles is defined on the IAM API itself, not the per-resource meta-API. That means we'll need a separate client for that RPC.

[jba]

Here is where the design now stands. Unfortunately, it has a significant problem.

The design follows the split built in to IAM, which separates common per-resource operations that are implemented by each Google API (Get/Set Policy and TestPermissions) from administrative actions supported by a separate IAM service.

• There is an iam package (CL) that defines a Handle. Each API client will provide one method named IAM on each IAM-enable resource that returns a Handle. Handles support GetPolicy, SetPolicy and TestPermissions. For example, pubsub topics support IAM, so pubsub.Topic has an IAM method that returns an *iam.Handle.
• There is a separate package for the IAM admin API. That package will be generated (CL).

The problem is that both IAM packages refer to Policy, but in the handwritten package this is the handwritten iam.Policy type, while in the generated code it is the Policy proto message. The Policy message appears in the admin API because one job of the admin API is to manage policies for service accounts. In other words, service accounts are an IAM-enabled resource like pubsub topics, and the service that manages them is the IAM admin service.

As I see it, we have these options:

1. Do nothing. The few (?) users who deal with service account policies will have to work with the generated code and the raw proto. The handwritten Policy type has a few convenience methods for adding and removing members and the like, so working with the raw proto is mildly unpleasant by comparison. But if it's rare, then that is not a big deal.
2. Have the generated code refer to the handwritten Policy type instead of the proto. This is a nice solution if it's feasible. @pongad, is it?
3. Export conversions between the two Policy types from the iam package. This clutters the package with raw protos, but is easy to do and will allow users of service account policies to have a slightly more pleasant time.
4. Write a handwritten IAM admin client. This is the most work and the gain is unclear. @jgeewax, @omaray, do we want handwritten IAM admin clients?

[s-mang]

I vote for #1. If we do not have a hand-written client for service-account-IAM b/c of lack of a use case, I don't see a need to support service-account-IAM in any other hand-written clients.
If we later decide there is a need for a hand-written client for the service-account-IAM proto, then I think we should incorporate it into our other hand-written clients.

We should make the decision to incorporate or not incorporate service accounts in hand-written IAM packages across the board.

[jba]

I'm OK with (1), but one correction: we would only implement one handwritten client for service accounts: the IAM admin client. All service accounts are managed from that one API.

[s-mang]

Oh ok, I misunderstood then.
So we are writing:

1. a handwritten client for service account IAM with all of the extensive functionality that its proto supports
2. another handwritten IAM package for non-service accounts with the three bits of functionality supported in the proto for general IAM
3. IAM accessor methods on top of the existing handwritten clients for each resource for each GCP service

and the question is whether or not we need to have support for service account IAM in (3).

Is that correct?

[jba]

Not quite. We are doing (2) and (3), but your (1) is my original (4) -- in other words, it's a possibility, but not definite (and is actually my least favorite choice).

[s-mang]

Ok. So when you say "we would only implement one handwritten client for service accounts" in your last comment, what does that mean if not my (1)?

[jba]

I was responding to your " I don't see a need to support service-account-IAM in any other hand-written clients." I was pointing out that if we supported service-account IAM, we would do so by writing only one handwritten client, the one for IAM admin. In other words, I was correcting your impression of multiple service-account clients, when there would be only one. I wasn't saying we would actually write it.

[s-mang]

Oh! Sorry for the confusion.

Ok, I'm still for (1) then until we get user requests for something better.

[pongad]

@adams-sarah @jba 2. is a little tricky. Our generator is built around protobuf messages, and so making it refer to hand-written code in this way might require some dark magic.

Another solution is to tell the generator to not generate code for the 3 methods that refers to Policy. Then, in a separate file, hand-write just those 3 methods. (Go allows methods of one type to span multiple files right?) If we want to do this, it would be the first time we generate/write this kind of "cyborg" code. There's a bit of unknown here, but it might be worth exploring.

cc @garrettjonesgoogle

[jba]

The cyborg idea is something we've discussed before, in the abstract. It could definitely work here. @pongad, can you generate such a client and mail me the CL? I'll take it from there.

[pongad]

@jba googleapis/gapic-generator#599 should provide the support needed to do this kind of stuff. I'll work on this after the PR lands. If you want it sooner, please let me know.

[pongad]

Update: https://code-review.googlesource.com/#/c/8491/ has landed. The IAM admin should be ready to get cyborgified now.

[jba]

This particular issue was addressed in fc7a628.