Skip to content

feat: provide clusterName when available#22

Closed
ofrobots wants to merge 1 commit intostephenplusplus:masterfrom
ofrobots:clusterName
Closed

feat: provide clusterName when available#22
ofrobots wants to merge 1 commit intostephenplusplus:masterfrom
ofrobots:clusterName

Conversation

@ofrobots
Copy link
Copy Markdown
Collaborator

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

isContainerEngine is already acquiring the clusterName when running on
GKE. Provide this value on the environment.
@ofrobots
Copy link
Copy Markdown
Collaborator Author

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

@stephenplusplus
Copy link
Copy Markdown
Owner

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
Copy link
Copy Markdown
Collaborator Author

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

@stephenplusplus
Copy link
Copy Markdown
Owner

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
Copy link
Copy Markdown
Collaborator Author

ofrobots commented Jul 27, 2017

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:

  • Whether we should cache the attributes only for GKE, or should this be done for other environments too to provide a consistent interface to have instance attributes?
  • Why just the attributes, why not do /computeMetadata/v1/instance?recursive=true?
  • Instance attributes are user defined and can be rather large (512KB). We don't really have the need to hold onto that amount of memory.
  • This module doesn't really have the need to download the ssh keys (or one of the other sensitive the 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
Copy link
Copy Markdown
Owner

stephenplusplus commented Jul 27, 2017

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:

  • A) useful, concise (we want this)
  • B) everything (we don't want this)

Using that system,

  • Which category does "GET /attributes" fit?

  • Is there a parallel endpoint for the other environments, or a non-controversial way to define "useful, concise" from a response that has "everything"?

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
Copy link
Copy Markdown
Collaborator Author

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
Copy link
Copy Markdown
Collaborator Author

ofrobots commented Aug 2, 2017

Superceded by googleapis/google-cloud-node#2483.

@ofrobots ofrobots closed this Aug 2, 2017
@ofrobots ofrobots deleted the clusterName branch August 2, 2017 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants