From 5bd2c4d3b8217d72293d9f1bc952d4da0d0ed2d7 Mon Sep 17 00:00:00 2001 From: Sergey Lanzman Date: Wed, 7 Feb 2018 03:04:28 +0200 Subject: [PATCH 1/3] improve-s3 impove --- .gitignore | 1 + lib/terraforming/resource/s3.rb | 67 ++++++++++++++---- lib/terraforming/template/tf/s3.erb | 105 +++++++++++++++++++++++++++- lib/terraforming/version.rb | 2 +- 4 files changed, 160 insertions(+), 15 deletions(-) diff --git a/.gitignore b/.gitignore index 0cb6eeb0..0eb7708c 100644 --- a/.gitignore +++ b/.gitignore @@ -1,5 +1,6 @@ /.bundle/ /.yardoc +.idea /Gemfile.lock /_yardoc/ /coverage/ diff --git a/lib/terraforming/resource/s3.rb b/lib/terraforming/resource/s3.rb index 0cdd1132..f9bd5895 100644 --- a/lib/terraforming/resource/s3.rb +++ b/lib/terraforming/resource/s3.rb @@ -16,24 +16,24 @@ def initialize(client) end def tf - apply_template(@client, "tf/s3") + apply_template(@client, 'tf/s3').gsub(/^[\s]*$\n/, '') end def tfstate buckets.inject({}) do |resources, bucket| bucket_policy = bucket_policy_of(bucket) resources["aws_s3_bucket.#{module_name_of(bucket)}"] = { - "type" => "aws_s3_bucket", - "primary" => { - "id" => bucket.name, - "attributes" => { - "acl" => "private", - "bucket" => bucket.name, - "force_destroy" => "false", - "id" => bucket.name, - "policy" => bucket_policy ? bucket_policy.policy.read : "", + "type" => 'aws_s3_bucket', + "primary" => { + "id" => bucket.name, + "attributes" => { + "acl" => 'private', + "bucket" => bucket.name, + "force_destroy" => 'false', + "id" => bucket.name, + "policy" => bucket_policy ? bucket_policy : '', + } } - } } resources @@ -47,19 +47,60 @@ def bucket_location_of(bucket) end def bucket_policy_of(bucket) - @client.get_bucket_policy(bucket: bucket.name) + bucket.policy.policy.read rescue Aws::S3::Errors::NoSuchBucketPolicy nil end def buckets - @client.list_buckets.map(&:buckets).flatten.select { |bucket| same_region?(bucket) } + return @buckets unless @buckets.nil? + @buckets = [] + @client.list_buckets.map(&:buckets).flatten.each do |bucket| + @buckets << Aws::S3::Bucket.new(bucket.name, client: @client) if same_region?(bucket) + end + @buckets + end + + def region(bucket) + bucket_location_of(bucket) end def module_name_of(bucket) normalize_module_name(bucket.name) end + def tagging?(bucket) + return false if bucket.tagging.tag_set.nil? + true + rescue Aws::S3::Errors::NoSuchTagSet + false + end + + def cors?(bucket) + return false if bucket.cors.cors_rules.nil? + true + rescue Aws::S3::Errors::NoSuchCORSConfiguration + false + end + + def lifecycle?(bucket) + return false if bucket.lifecycle_configuration.rules.nil? + true + rescue Aws::S3::Errors::NoSuchLifecycleConfiguration + false + end + + def website_configuation?(bucket) + return false if bucket.website.index_document.nil? + true + rescue Aws::S3::Errors::NoSuchWebsiteConfiguration + false + end + + def prettify_website_routing_rules(bucket) + prettify_policy(bucket.website.routing_rules.map { |t| t.to_h.to_json }.to_json.gsub('"{', '{').gsub('\"', '"').gsub('}"', '}')) + end + def same_region?(bucket) bucket_location = bucket_location_of(bucket) (bucket_location == @client.config.region) || (bucket_location == "" && @client.config.region == "us-east-1") diff --git a/lib/terraforming/template/tf/s3.erb b/lib/terraforming/template/tf/s3.erb index 73ebf77a..e8db5f1e 100644 --- a/lib/terraforming/template/tf/s3.erb +++ b/lib/terraforming/template/tf/s3.erb @@ -1,12 +1,115 @@ <% buckets.each do |bucket| -%> + resource "aws_s3_bucket" "<%= module_name_of(bucket) %>" { bucket = "<%= bucket.name %>" acl = "private" + region = "<%=region(bucket)%>" <%- unless (policy = bucket_policy_of(bucket)).nil? -%> policy = < +<%= prettify_policy(policy) %> POLICY <%- end -%> + +<%- if tagging?(bucket)-%> + tags { + <% bucket.tagging.tag_set.each do |t| -%> + "<%=t[:key]%>" = "<%=t[:value]%>" + <%end-%> +} +<%- end -%> + +<%- if cors?(bucket) -%> + + <% bucket.cors.cors_rules.each do |rule| -%> +cors_rule { + allowed_methods = <%=rule.allowed_methods%> + allowed_origins = <%=rule.allowed_origins%> + <%unless rule.allowed_headers.nil? %> + allowed_headers = <%=rule.allowed_headers%> + <%end%> + <%unless rule.expose_headers.nil? %> + expose_headers = <%=rule.expose_headers%> + <%end%> + <%unless rule.max_age_seconds.nil? %> + max_age_seconds = <%=rule.max_age_seconds%> + <%end%> + } + <%end-%> +<%- end -%> + +<%- unless bucket.versioning.status.nil? -%> + versioning { + enabled = <%=bucket.versioning.status == 'Enabled'%> + } +<%- end -%> + +<%- unless bucket.logging.logging_enabled.nil? -%> + logging { + target_bucket = "<%=bucket.logging.logging_enabled.target_bucket%>" + target_prefix = "<%=bucket.logging.logging_enabled.target_prefix%>" + } +<%- end -%> + +<%- if lifecycle?(bucket) -%> + <% bucket.lifecycle_configuration.rules.each do |rule| -%> +lifecycle_rule { + id = "<%=rule.id%>" + enabled = <%=rule.status == 'Enabled'%> + prefix = "<%=rule.prefix%>" + <% rule.transitions.each do |transition| -%> + transition { + storage_class = "<%=transition.storage_class%>" + <%unless transition.days.nil? %> + days = <%=transition.days%> + <%end%> + <%unless transition.date.nil? %> + date = "<%=transition.date%>" + <%end%> + } +<%end-%> + <%unless rule.expiration.nil? %> + expiration { + <%unless rule.expiration.days.nil? %> + days = <%=rule.expiration.days%> + <%end%> + <%unless rule.expiration.date.nil? %> + date = "<%=rule.expiration.date%>" + <%end%> + <%unless rule.expiration.expired_object_delete_marker.nil? %> + expired_object_delete_marker = "<%=rule.expiration.expired_object_delete_marker%>" + <%end%> + } + <%end%> + <% rule.noncurrent_version_transitions.each do |transition| -%> + noncurrent_version_transition { + days = <%=transition.noncurrent_days%> + storage_class = "<%=transition.storage_class%>" + } + <%end-%> + <%unless rule.noncurrent_version_expiration.nil? %> + noncurrent_version_expiration { + days = <%=rule.noncurrent_version_expiration.noncurrent_days%> + } + <%end%> + <%unless rule.abort_incomplete_multipart_upload.nil? %> + abort_incomplete_multipart_upload_days = <%=rule.abort_incomplete_multipart_upload.days_after_initiation%> + <%end%> + } + <%if website_configuation?(bucket) %> + website { + index_document = "<%=bucket.website.index_document.suffix%>" + <%unless bucket.website.error_document.nil? %> + error_document = "<%=bucket.website.error_document.key%>" + <%end%> + <%unless bucket.website.routing_rules.nil? %> + routing_rules = < + EOF + <%end%> + } +<%end%> +<%end-%> +<%- end -%> } <% end -%> diff --git a/lib/terraforming/version.rb b/lib/terraforming/version.rb index 5703ada0..0fc34bd1 100644 --- a/lib/terraforming/version.rb +++ b/lib/terraforming/version.rb @@ -1,3 +1,3 @@ module Terraforming - VERSION = "0.16.0" + VERSION = "0.16.1" end From 0799641f003615772a03d57629be8dd5fbd60962 Mon Sep 17 00:00:00 2001 From: Alex Mirkhaydarov Date: Wed, 3 Jul 2019 22:03:51 +0100 Subject: [PATCH 2/3] Fixed tests for S3 improvements --- spec/lib/terraforming/resource/s3_spec.rb | 33 ++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/spec/lib/terraforming/resource/s3_spec.rb b/spec/lib/terraforming/resource/s3_spec.rb index 755ce5b8..ac948a09 100644 --- a/spec/lib/terraforming/resource/s3_spec.rb +++ b/spec/lib/terraforming/resource/s3_spec.rb @@ -63,6 +63,7 @@ module Resource resource "aws_s3_bucket" "hoge" { bucket = "hoge" acl = "private" + region = "" policy = < Date: Sun, 7 Jul 2019 17:49:07 +0100 Subject: [PATCH 3/3] Added more tests coverage --- lib/terraforming/template/tf/s3.erb | 191 +++++++++++----------- spec/lib/terraforming/resource/s3_spec.rb | 162 ++++++++++++++++-- 2 files changed, 247 insertions(+), 106 deletions(-) diff --git a/lib/terraforming/template/tf/s3.erb b/lib/terraforming/template/tf/s3.erb index e8db5f1e..1d2b00ff 100644 --- a/lib/terraforming/template/tf/s3.erb +++ b/lib/terraforming/template/tf/s3.erb @@ -1,115 +1,116 @@ <% buckets.each do |bucket| -%> - resource "aws_s3_bucket" "<%= module_name_of(bucket) %>" { bucket = "<%= bucket.name %>" acl = "private" region = "<%=region(bucket)%>" -<%- unless (policy = bucket_policy_of(bucket)).nil? -%> + <%- unless (policy = bucket_policy_of(bucket)).nil? -%> policy = < POLICY -<%- end -%> + <%- end -%> -<%- if tagging?(bucket)-%> + <%- if tagging?(bucket) -%> tags { - <% bucket.tagging.tag_set.each do |t| -%> - "<%=t[:key]%>" = "<%=t[:value]%>" - <%end-%> -} -<%- end -%> - -<%- if cors?(bucket) -%> + <%- bucket.tagging.tag_set.each do |tag| -%> + <%= tag[:key] %> = "<%= tag[:value] %>" + <%- end -%> + } + <%- end -%> - <% bucket.cors.cors_rules.each do |rule| -%> -cors_rule { - allowed_methods = <%=rule.allowed_methods%> - allowed_origins = <%=rule.allowed_origins%> - <%unless rule.allowed_headers.nil? %> - allowed_headers = <%=rule.allowed_headers%> - <%end%> - <%unless rule.expose_headers.nil? %> - expose_headers = <%=rule.expose_headers%> - <%end%> - <%unless rule.max_age_seconds.nil? %> - max_age_seconds = <%=rule.max_age_seconds%> - <%end%> + <%- if cors?(bucket) -%> + <%- bucket.cors.cors_rules.each do |rule| -%> + cors_rule { + allowed_methods = <%= rule.allowed_methods %> + allowed_origins = <%= rule.allowed_origins %> + <%- unless rule.allowed_headers.nil? -%> + allowed_headers = <%= rule.allowed_headers %> + <%- end -%> + <%- unless rule.expose_headers.nil? -%> + expose_headers = <%= rule.expose_headers %> + <%- end -%> + <%- unless rule.max_age_seconds.nil? -%> + max_age_seconds = <%= rule.max_age_seconds %> + <%- end -%> } - <%end-%> -<%- end -%> + <%- end -%> + <%- end -%> -<%- unless bucket.versioning.status.nil? -%> + <%- unless bucket.versioning.status.nil? -%> versioning { - enabled = <%=bucket.versioning.status == 'Enabled'%> + enabled = <%= bucket.versioning.status == 'Enabled' %> } -<%- end -%> + <%- end -%> -<%- unless bucket.logging.logging_enabled.nil? -%> + <%- unless bucket.logging.logging_enabled.nil? -%> logging { - target_bucket = "<%=bucket.logging.logging_enabled.target_bucket%>" - target_prefix = "<%=bucket.logging.logging_enabled.target_prefix%>" + target_bucket = "<%= bucket.logging.logging_enabled.target_bucket %>" + target_prefix = "<%= bucket.logging.logging_enabled.target_prefix %>" } -<%- end -%> + <%- end -%> -<%- if lifecycle?(bucket) -%> - <% bucket.lifecycle_configuration.rules.each do |rule| -%> -lifecycle_rule { - id = "<%=rule.id%>" - enabled = <%=rule.status == 'Enabled'%> - prefix = "<%=rule.prefix%>" - <% rule.transitions.each do |transition| -%> - transition { - storage_class = "<%=transition.storage_class%>" - <%unless transition.days.nil? %> - days = <%=transition.days%> - <%end%> - <%unless transition.date.nil? %> - date = "<%=transition.date%>" - <%end%> - } -<%end-%> - <%unless rule.expiration.nil? %> - expiration { - <%unless rule.expiration.days.nil? %> - days = <%=rule.expiration.days%> - <%end%> - <%unless rule.expiration.date.nil? %> - date = "<%=rule.expiration.date%>" - <%end%> - <%unless rule.expiration.expired_object_delete_marker.nil? %> - expired_object_delete_marker = "<%=rule.expiration.expired_object_delete_marker%>" - <%end%> - } - <%end%> - <% rule.noncurrent_version_transitions.each do |transition| -%> - noncurrent_version_transition { - days = <%=transition.noncurrent_days%> - storage_class = "<%=transition.storage_class%>" - } - <%end-%> - <%unless rule.noncurrent_version_expiration.nil? %> - noncurrent_version_expiration { - days = <%=rule.noncurrent_version_expiration.noncurrent_days%> - } - <%end%> - <%unless rule.abort_incomplete_multipart_upload.nil? %> - abort_incomplete_multipart_upload_days = <%=rule.abort_incomplete_multipart_upload.days_after_initiation%> - <%end%> - } - <%if website_configuation?(bucket) %> - website { - index_document = "<%=bucket.website.index_document.suffix%>" - <%unless bucket.website.error_document.nil? %> - error_document = "<%=bucket.website.error_document.key%>" - <%end%> - <%unless bucket.website.routing_rules.nil? %> - routing_rules = < - EOF - <%end%> - } -<%end%> -<%end-%> -<%- end -%> -} + <%- if lifecycle?(bucket) -%> + <%- bucket.lifecycle_configuration.rules.each do |rule| -%> + lifecycle_rule { + id = "<%= rule.id %>" + prefix = "<%= rule.prefix %>" + enabled = <%= rule.status == 'Enabled' %> + <%- rule.transitions.each do |transition| -%> + transition { + storage_class = "<%= transition.storage_class %>" + <%- unless transition.days.nil? %> + days = <%= transition.days %> + <%- end -%> + <%- unless transition.date.nil? -%> + date = "<%= transition.date %>" + <%- end -%> + } + <%- end -%> + + <%- unless rule.expiration.nil? -%> + expiration { + <%- unless rule.expiration.days.nil? -%> + days = <%= rule.expiration.days %> + <%- end -%> + <%- unless rule.expiration.date.nil? -%> + date = "<%= rule.expiration.date %>" + <%- end -%> + <%- unless rule.expiration.expired_object_delete_marker.nil? -%> + expired_object_delete_marker = "<%= rule.expiration.expired_object_delete_marker %>" + <%- end -%> + } + <%- end -%> + + <%- rule.noncurrent_version_transitions.each do |transition| -%> + noncurrent_version_transition { + days = <%= transition.noncurrent_days %> + storage_class = "<%= transition.storage_class %>" + } + <%- end-%> + + <%- unless rule.noncurrent_version_expiration.nil? -%> + noncurrent_version_expiration { + days = <%= rule.noncurrent_version_expiration.noncurrent_days %> + } + <%- end -%> + + <%- unless rule.abort_incomplete_multipart_upload.nil? -%> + abort_incomplete_multipart_upload_days = <%= rule.abort_incomplete_multipart_upload.days_after_initiation %> + <%- end -%> + } + <%- end %> + <%- end -%> + + <%- if website_configuation?(bucket) %> + website { + index_document = "<%= bucket.website.index_document.suffix %>" + <%- unless bucket.website.error_document.nil? %> + error_document = "<%= bucket.website.error_document.key %>" + <%- end -%> + <%- unless bucket.website.routing_rules.nil? %> + routing_rules = <%= prettify_website_routing_rules(bucket) %> + <%- end -%> + } + <%- end -%> -<% end -%> +} +<%- end -%> diff --git a/spec/lib/terraforming/resource/s3_spec.rb b/spec/lib/terraforming/resource/s3_spec.rb index ac948a09..505299b4 100644 --- a/spec/lib/terraforming/resource/s3_spec.rb +++ b/spec/lib/terraforming/resource/s3_spec.rb @@ -43,6 +43,66 @@ module Resource { location_constraint: "" } end + let(:bucket_tags) do + [ + { + key: "marketing", + value: "organization" + } + ] + end + + let(:bucket_cors) do + [ + { + allowed_headers: [ + "Authorization", + ], + allowed_methods: [ + "GET", + ], + allowed_origins: [ + "*", + ], + max_age_seconds: 3000 + } + ] + end + + let(:bucket_lifecycle_configuration) do + [ + { + id: "Move rotated logs to Glacier", + status: "Enabled", + prefix: "rotated/", + transitions: [ + { + days: 365, + storage_class: "GLACIER" + } + ] + } + ] + end + + let(:routing_rules) do + [ + { + condition: { + http_error_code_returned_equals: "404", + key_prefix_equals: "index.html" + }, + redirect: { + host_name: "www.example.com", + http_redirect_code: "", + protocol: "http", + replace_key_prefix_with: "", + replace_key_with: "" + } + } + ] + end + context "from ap-northeast-1" do let(:client) do Aws::S3::Client.new(region: "ap-northeast-1", stub_responses: true) @@ -55,6 +115,14 @@ module Resource "NoSuchBucketPolicy", ]) client.stub_responses(:get_bucket_location, [hoge_location, fuga_location, piyo_location]) + client.stub_responses(:get_bucket_tagging, { tag_set: bucket_tags }) + client.stub_responses(:get_bucket_cors, { cors_rules: bucket_cors }) + client.stub_responses(:get_bucket_lifecycle_configuration, { rules: bucket_lifecycle_configuration }) + client.stub_responses(:get_bucket_website, { + error_document: { key: "error.html" }, + index_document: { suffix: "index.html" }, + routing_rules: routing_rules + }) end describe ".tf" do @@ -82,13 +150,48 @@ module Resource } POLICY tags { + marketing = "organization" + } + cors_rule { + allowed_methods = ["GET"] + allowed_origins = ["*"] + allowed_headers = ["Authorization"] + max_age_seconds = 3000 } versioning { - enabled = false + enabled = false } logging { - target_bucket = "TargetBucket" - target_prefix = "TargetPrefix" + target_bucket = "TargetBucket" + target_prefix = "TargetPrefix" + } + lifecycle_rule { + id = "Move rotated logs to Glacier" + prefix = "rotated/" + enabled = true + transition { + storage_class = "GLACIER" + days = 365 + } + } + website { + index_document = "index.html" + error_document = "error.html" + routing_rules = [ + { + "condition": { + "http_error_code_returned_equals": "404", + "key_prefix_equals": "index.html" + }, + "redirect": { + "host_name": "www.example.com", + "http_redirect_code": "", + "protocol": "http", + "replace_key_prefix_with": "", + "replace_key_with": "" + } + } +] } } resource "aws_s3_bucket" "fuga" { @@ -96,13 +199,48 @@ module Resource acl = "private" region = "" tags { + marketing = "organization" + } + cors_rule { + allowed_methods = ["GET"] + allowed_origins = ["*"] + allowed_headers = ["Authorization"] + max_age_seconds = 3000 } versioning { - enabled = false + enabled = false } logging { - target_bucket = "TargetBucket" - target_prefix = "TargetPrefix" + target_bucket = "TargetBucket" + target_prefix = "TargetPrefix" + } + lifecycle_rule { + id = "Move rotated logs to Glacier" + prefix = "rotated/" + enabled = true + transition { + storage_class = "GLACIER" + days = 365 + } + } + website { + index_document = "index.html" + error_document = "error.html" + routing_rules = [ + { + "condition": { + "http_error_code_returned_equals": "404", + "key_prefix_equals": "index.html" + }, + "redirect": { + "host_name": "www.example.com", + "http_redirect_code": "", + "protocol": "http", + "replace_key_prefix_with": "", + "replace_key_with": "" + } + } +] } } EOS @@ -154,6 +292,10 @@ module Resource "NoSuchBucketPolicy", ]) client.stub_responses(:get_bucket_location, [hoge_location, fuga_location, piyo_location]) + client.stub_responses(:get_bucket_tagging, "NoSuchTagSet") + client.stub_responses(:get_bucket_cors, "NoSuchCORSConfiguration") + client.stub_responses(:get_bucket_lifecycle_configuration, "NoSuchLifecycleConfiguration") + client.stub_responses(:get_bucket_website, "NoSuchWebsiteConfiguration") end describe ".tf" do @@ -163,14 +305,12 @@ module Resource bucket = "piyo" acl = "private" region = "" - tags { - } versioning { - enabled = false + enabled = false } logging { - target_bucket = "TargetBucket" - target_prefix = "TargetPrefix" + target_bucket = "TargetBucket" + target_prefix = "TargetPrefix" } } EOS