search

stephenplusplus / google-auto-auth

Making it as easy as possible to authenticate a Google API request
MIT License
34 stars 9 forks source link

feat: provide clusterName when available #22

Closed ofrobots closed 7 years ago

ofrobots commented 7 years ago

isContainerEngine is already acquiring the clusterName when running on GKE. Provide this value on the environment.

ofrobots commented 7 years ago

It seems that the the tests are broken with/without this change. I am looking into that too.

stephenplusplus commented 7 years ago

getEnvironment was only supposed to get the type of environment. It makes sense to return more details as well, though... but could we provide more than just clusterName? Maybe just pull down all of the metadata and attach it to env?

ofrobots commented 7 years ago

Is there anything specific you have in mind that we should add?

stephenplusplus commented 7 years ago

I'm not sure of any specifics that come back, but my thought was that the whole of GET /attributes might be useful, no different than "clusterName" would be useful to the user curious if they're in Container Engine.

resp = GET /attributes

isContainerEngine = resp.clusterName

if isContainerEngine
  extend(self.environment, resp)

callback(null, isContainerEngine)

Putting the extra data into a bucket, such as env.details = resp would be a bit more organized.

WDYT?

ofrobots commented 7 years ago

As you mentioned previously, this is starting to feel more and more out of place. This doesn't have anything to do auth per se. With the cluster name it seemed okay enough because we have to query the cluster-name attribute anyway – might as well cache it and provide it back to the users.

The questions that open up if you cache more attributes:

You do have a good point that adding more and more logic about metadata here isn't going to be scalable. If we need to expand this further in the future, then I think we should spin off the environment logic into a module of its own, or merge it with gcp-metadata.

For now caching just cluster-name (that we are already have) is okay?

stephenplusplus commented 7 years ago

should this be done for other environments too

Yes, but I would be willing to put that on the horizon for a future PR.

For the remaining points, we would need a way to differentiate between:

Using that system,

If we can't find a way to be consistent, then we're not likely to get back to this task, and we'll be stuck with an out of place one-off for "clusterName". So if we just need "clusterName" for GCN, I'd rather GCN uses gcp-metadata and gets the value on its own.

If we can find a way to be consistent, and there's a way to get Category A responses for all environments, then let's put "clusterName" in a "env.details" bucket, for sake of future proofing.

ofrobots commented 7 years ago

Which category does "GET /attributes" fit?

IMO, ...instance/attributes?recursive=true is the 'everything' category. For example, it will include ssh-keys. This module doesn't have a business holding onto that.

Is there a parallel endpoint for the other environments,

It would be the same endpoint. I don't have a concrete use-case for that though.

or a non-controversial way to define "useful, concise" from a response that has "everything"?

I don't really think there is a subset that has business in the 'auth' library. Even cluster_name doesn't really belong here, but it just happens to be the way to distinguish GKE from GCE.

I guess the conclusion would be to close this PR and then add cluster_name acquisition directly into GCN.

ofrobots commented 7 years ago

Superceded by https://github.com/GoogleCloudPlatform/google-cloud-node/pull/2483.