-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add support for GCS managed folders #10786
Merged
melinath
merged 19 commits into
GoogleCloudPlatform:main
from
juliocc:jccb/managed-folders
Jun 26, 2024
Merged
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
1800c33
Add support for GCS managed folders
juliocc e62c642
Add force_destroy to managed folder nested example
juliocc 2593fb4
Update mmv1/products/storage/ManagedFolder.yaml
juliocc 170610c
Apply suggestions from code review
juliocc 1ad5b70
Apply review comments
juliocc 1aa31e1
Map force_destroy to allowNonEmpty query param on delete
juliocc 281228d
Removed second example
juliocc 4f33c63
Move storage ManagedFolder IAM to handwritten resource
juliocc fe3fd01
Removing need for trailing slash
juliocc f4cff4c
Fix IAM tests
juliocc cf3b187
Fix example
juliocc 1d8dde1
Remove header from handwritten docs
juliocc a223650
Validate IAM managed folder syntax
juliocc 52b1998
Merge remote-tracking branch 'upstream/main' into jccb/managed-folders
juliocc f487704
Fixed indentation (use tabs)
juliocc 44bd723
Remove force_destroy virtual field
juliocc b08494c
Update IAM import format
juliocc 393e204
Merge remote-tracking branch 'upstream/main' into jccb/managed-folders
juliocc 2066073
Update mmv1/third_party/terraform/website/docs/r/storage_managed_fold…
melinath File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next
Next commit
Add support for GCS managed folders
- Loading branch information
commit 1800c33f0669c54f2f124ae753ee020d6f8d3c65
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,104 @@ | ||
# Copyright 2024 Google Inc. | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
--- !ruby/object:Api::Resource | ||
name: 'ManagedFolder' | ||
kind: 'storage#managedFolder' | ||
base_url: 'b/{{bucket}}/managedFolders' | ||
self_link: 'b/{{bucket}}/managedFolders/{{%name}}' | ||
id_format: '{{bucket}}/{{name}}' | ||
has_self_link: true | ||
immutable: true | ||
skip_sweeper: true # Skipping sweeper since this is a child resource. | ||
description: | | ||
A Google Cloud Storage Managed Folder. | ||
|
||
You can apply Identity and Access Management (IAM) policies to | ||
managed folders to grant principals access only to the objects | ||
within the managed folder, which lets you more finely control access | ||
for specific data sets and tables within a bucket. You can nest | ||
managed folders up to 15 levels deep, including the parent managed | ||
folder. | ||
|
||
Managed folders can only be created in buckets that have uniform | ||
bucket-level access enabled. | ||
references: !ruby/object:Api::Resource::ReferenceLinks | ||
guides: | ||
'Official Documentation': 'https://cloud.google.com/storage/docs/managed-folders' | ||
api: 'https://cloud.google.com/storage/docs/json_api/v1/managedFolder' | ||
iam_policy: !ruby/object:Api::Resource::IamPolicy | ||
allowed_iam_role: 'roles/storage.objectViewer' | ||
admin_iam_role: 'roles/storage.admin' | ||
parent_resource_attribute: 'managed_folder' | ||
base_url: 'b/{{bucket}}/managedFolders/{{%managed_folder}}' | ||
import_format: | ||
- '{{bucket}}/managedFolders/{{%managed_folder}}' | ||
- '{{managed_folder}}' | ||
juliocc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
iam_conditions_request_type: :QUERY_PARAM | ||
method_name_separator: '/' | ||
fetch_iam_policy_method: 'iam' | ||
fetch_iam_policy_verb: :GET | ||
set_iam_policy_method: 'iam' | ||
set_iam_policy_verb: :PUT | ||
wrapped_policy_obj: false | ||
skip_import_test: true | ||
juliocc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
custom_diff_suppress: 'templates/terraform/iam/storage_managed_folder_diff_suppress.go.erb' | ||
example_config_body: 'templates/terraform/iam/example_config_body/storage_managed_folder.tf.erb' | ||
import_format: | ||
- '{{bucket}}/managedFolders/{{%name}}' | ||
- '{{bucket}}/{{%name}}' | ||
examples: | ||
- !ruby/object:Provider::Terraform::Examples | ||
name: 'storage_managed_folder_basic' | ||
primary_resource_id: 'folder' | ||
juliocc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
vars: | ||
bucket_name: 'my-bucket' | ||
melinath marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- !ruby/object:Provider::Terraform::Examples | ||
name: 'storage_managed_folder_nested' | ||
primary_resource_id: 'folder' | ||
vars: | ||
bucket_name: 'my-bucket' | ||
parameters: | ||
- !ruby/object:Api::Type::ResourceRef | ||
name: 'bucket' | ||
resource: 'Bucket' | ||
juliocc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
imports: 'name' | ||
description: 'The name of the bucket that contains the managed folder.' | ||
required: true | ||
- !ruby/object:Api::Type::String | ||
name: 'name' | ||
description: | | ||
The name of the managed folder expressed as a path. For example, `example_dir/example_dir2/`. | ||
Must include trailing `/`. | ||
required: true | ||
# API returns values with trailing slashes, even if not provided. | ||
# Enforcing trailing slashes prevents diffs and ensures consistent | ||
# output. | ||
validation: !ruby/object:Provider::Terraform::Validation | ||
regex: '/$' | ||
properties: | ||
- !ruby/object:Api::Type::String | ||
name: createTime | ||
description: | | ||
Output only. The timestamp at which this managed folder was created. | ||
juliocc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
output: true | ||
- !ruby/object:Api::Type::String | ||
name: updateTime | ||
description: | | ||
Output only. The timestamp at which this managed folder was most recently updated. | ||
juliocc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
output: true | ||
- !ruby/object:Api::Type::String | ||
name: metageneration | ||
description: | | ||
Output only. The metadata generation of the managed folder. | ||
juliocc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
output: true |
10 changes: 10 additions & 0 deletions
10
mmv1/templates/terraform/examples/storage_managed_folder_basic.tf.erb
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
resource "google_storage_bucket" "bucket" { | ||
name = "<%= ctx[:vars]['bucket_name'] %>" | ||
location = "EU" | ||
uniform_bucket_level_access = true | ||
} | ||
|
||
resource "google_storage_managed_folder" "<%= ctx[:primary_resource_id] %>" { | ||
bucket = google_storage_bucket.bucket.name | ||
name = "managed-folder-1/" | ||
} |
15 changes: 15 additions & 0 deletions
15
mmv1/templates/terraform/examples/storage_managed_folder_nested.tf.erb
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
resource "google_storage_bucket" "bucket" { | ||
name = "<%= ctx[:vars]['bucket_name'] %>" | ||
location = "EU" | ||
uniform_bucket_level_access = true | ||
juliocc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
resource "google_storage_managed_folder" "parent" { | ||
juliocc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
bucket = google_storage_bucket.bucket.name | ||
name = "parent/" | ||
} | ||
|
||
resource "google_storage_managed_folder" "<%= ctx[:primary_resource_id] %>" { | ||
bucket = google_storage_bucket.bucket.name | ||
name = "${google_storage_managed_folder.parent.name}nested/" | ||
} |
2 changes: 2 additions & 0 deletions
2
mmv1/templates/terraform/iam/example_config_body/storage_managed_folder.tf.erb
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
bucket = google_storage_managed_folder.folder.bucket | ||
managed_folder = google_storage_managed_folder.folder.name |
14 changes: 14 additions & 0 deletions
14
mmv1/templates/terraform/iam/storage_managed_folder_diff_suppress.go.erb
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
func <%= object.resource_name -%>DiffSuppress(_, old, new string, _ *schema.ResourceData) bool { | ||
juliocc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// Standard IAM policies use the expanded base URL for the | ||
// resource's last parameter. Managed folders have a unique | ||
// endpoint format | ||
// ('b/{{bucket}}/managedFolders/{{%managed_folder}}'), so we | ||
// adjust extraction to correctly identify the folder name. | ||
|
||
oldParts := strings.SplitN(old, "/managedFolders/", 2) | ||
if len(oldParts) == 2 { | ||
oldFolderName := oldParts[1] + "/" | ||
juliocc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return oldFolderName == new | ||
} | ||
return false | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Hi @melinath, @trodge. Just wanted to confirm that Here path escape character(%) works or not?
I looked at some generated IAM files and found that url is not being constructed with Encoding of path parameters. For example, this code, I can see that
urlTemplate
is being constructed directly from the parameters value without any url encoding.I do see all the generated tests passed but It might be because of "managed-folder-1/" resource name. We basically don't need any encoding here as there is only one slash here which is at the end. It may fail for nested managed folders like "a/b/".
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.
+1 that we should test the IAM behavior with nested folders to be safe. The code looks correct to me but also I don't see any difference from the google_sourcerepo_repository_iam code you linked (and that doesn't override the base_url for IAM at all.) It could be that the % is a no-op here.
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.
Actually I took this url constructor reference just to ask that I don't see any url encoding happening in the IAM resource. As you show that MMV1 yaml is not having any path escaper for IAM, it won't have any logic in generated resource but I tried manually with
%
character and No difference. It is generating urlTemplate like{{StorageBasePath}}b/a/managedFolders/b//iam
which I think doesn't make any sense and results in 404 not found error.It looks like we have to move IAM resource in handwritten to construct url with proper encoding.
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 just tested this locally and you're absolutely right. The current approach doesn't work for nested folders. I don't see any hooks in iam_policy.erb to modify the URL, so it looks like we'll have to write it manually.
Is there any documentation on how create a handwritten IAM policy resource? Should I start from the generated code and update it manually?
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.
So if I understand correctly, the problem is that the managed folder path has to end with a "/" (ideally, if we could set up validation for the parent resource field), but when you join that with the IAM methods it ends up having an extra slash? If that's correct, you could try adding setting the method separator to an empty string, since the trailing "/" would act as the separator on its own.
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.
Unfortunately, as far as I know, the
%
just indicates that the given part of the path can contain slashes; it doesn't mean that we urlencode that part of the path. If that's necessary here, then either users need to provide the path already escaped (for example using the Terraformurlencode
function might work?) or the IAM support for this resource needs to be handwritten.Adding support for url escaping as a feature of the generator would be a pretty significant lift, I think, so it is not something I'd want to commit to in the context of this PR.
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.
Asking the user to urlencode might break other things. Let me give it a try and I'll get back to you
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.
+1. I think, we should do url encoding from provider side. As it appears This is the special case for IAM auto generation, We can solve this by moving this resource to Handwritten and applying custom encoding logic.
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.
If It requires significant effort to modify generator then we should move IAM to handwritten by following guidelines given here.
In HandWritten resource, I think we need to only change
qualifyManagedFolderUrl
function with path escaping logic. Probably Something like this should work,urlTemplate := fmt.Sprintf("{{StorageBasePath}}%s/%s", fmt.Sprintf("b/%s/managedFolders/%s", u.bucket, url.PathEscape(u.managedFolder)), methodIdentifier)
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.
Yeah, we probably don't want to handle this specially for IAM right now. If there are additional resources that need it we should discuss that separately (so this resources isn't blocked) and you can look into converting back to generated later.