Skip to content

Fix setting service accounts and scopes on GCE instances#305

Merged
icco merged 2 commits intofog:masterfrom
KohlsTechnology:fix-server-service-accounts
Feb 26, 2018
Merged

Fix setting service accounts and scopes on GCE instances#305
icco merged 2 commits intofog:masterfrom
KohlsTechnology:fix-server-service-accounts

Conversation

@seanmalloy
Copy link
Copy Markdown
Contributor

Setting service accounts should use an array of hashes instead
of a hash. See the resource.serviceAccounts[] field in the below
Google API document for details.

https://cloud.google.com/compute/docs/reference/rest/v1/instances/insert

Setting service accounts should use an array of hashes instead
of a hash. See the resource.serviceAccounts[] field in the below
Google API document for details.

https://cloud.google.com/compute/docs/reference/rest/v1/instances/insert
:scopes => map_scopes(service_accounts[:scopes])
)
if service_accounts && service_accounts[0]
service_accounts[0].merge!(:scopes => map_scopes(service_accounts[0][:scopes]))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use service_accounts[0][:scopes] = map_scopes(service_accounts[0][:scopes]) instead of service_accounts[0].merge!(:scopes => map_scopes(service_accounts[0][:scopes])).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

)
if service_accounts && service_accounts[0]
service_accounts[0][:scopes] = map_scopes(service_accounts[0][:scopes])
options[:service_accounts] = service_accounts
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since people are possibly already passing in a scopes hash, could we still support that? Maybe modify the request to make sure only one service account is passed in and its in the right format?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will test this tonight and see if it works. But from what I recall it seemed like setting the service accounts and scopes was completely broken on the master branch.

I'll dig into it and let you know.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will not get to this until this weekend. Sorry for the delay.

Copy link
Copy Markdown
Contributor Author

@seanmalloy seanmalloy Feb 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried out the latest code from master to see how it behaves. My initial test seems to indicate that simply setting scopes like this does not actually set the service account scopes. When I go into the Google Cloud Console no service account scopes are set on the GCE instance.

 {:scopes => ["storage/full"] }

Some screen shots from my pry session trying to see how this currently works.

screenshot from 2018-02-26 00-02-25

screenshot from 2018-02-26 00-03-51

I will need to dig a little bit deeper on this to make sure I am understanding how this currently works. Current plan it to have more details in the next few days.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think actually I misread this, what I was asking doesn't make sense. I was thinking you were throwing away an option a user was passing in, which is not what is happening here. This actually looks great and I'm going to merge.

)
if service_accounts && service_accounts[0]
service_accounts[0][:scopes] = map_scopes(service_accounts[0][:scopes])
options[:service_accounts] = service_accounts
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think actually I misread this, what I was asking doesn't make sense. I was thinking you were throwing away an option a user was passing in, which is not what is happening here. This actually looks great and I'm going to merge.

@icco icco merged commit 5a5ac9c into fog:master Feb 26, 2018
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.

3 participants