From ff12c4b2d42b910eb3d73740d6f6e351521305b9 Mon Sep 17 00:00:00 2001 From: Vinoth Kannan Date: Wed, 6 Feb 2019 19:16:08 +0530 Subject: [PATCH] FIX: Bucket name is missing in S3 inventory data path --- lib/s3_inventory.rb | 27 +++++++++---- spec/components/s3_inventory_spec.rb | 57 +++++++++++++++------------ spec/jobs/update_s3_inventory_spec.rb | 2 +- 3 files changed, 51 insertions(+), 35 deletions(-) diff --git a/lib/s3_inventory.rb b/lib/s3_inventory.rb index 321182f3d6a..3f069673b35 100644 --- a/lib/s3_inventory.rb +++ b/lib/s3_inventory.rb @@ -10,6 +10,7 @@ class S3Inventory CSV_KEY_INDEX ||= 1 CSV_ETAG_INDEX ||= 2 INVENTORY_PREFIX ||= "inventory" + INVENTORY_VERSION ||= "1" def initialize(s3_helper, type) @s3_helper = s3_helper @@ -96,10 +97,10 @@ class S3Inventory "Effect": "Allow", "Principal": { "Service": "s3.amazonaws.com" }, "Action": ["s3:PutObject"], - "Resource": ["arn:aws:s3:::#{inventory_root_path}/*"], + "Resource": ["#{inventory_path_arn}/*"], "Condition": { "ArnLike": { - "aws:SourceArn": "arn:aws:s3:::#{bucket_name}" + "aws:SourceArn": bucket_arn }, "StringEquals": { "s3:x-amz-acl": "bucket-owner-full-control" @@ -134,7 +135,7 @@ class S3Inventory { destination: { s3_bucket_destination: { - bucket: "arn:aws:s3:::#{bucket_name}", + bucket: bucket_arn, prefix: destination_prefix, format: "CSV" } @@ -163,7 +164,7 @@ class S3Inventory def unsorted_files objects = [] - @s3_helper.list(File.join(inventory_path, "data")).each do |obj| + @s3_helper.list(inventory_data_path).each do |obj| if obj.key.match?(/\.csv\.gz$/i) objects << obj end @@ -174,12 +175,22 @@ class S3Inventory log("Failed to list inventory from S3", e) end - def inventory_path - File.join(inventory_root_path, inventory_id) + def inventory_data_path + File.join(inventory_path, bucket_name, inventory_id, "data") end - def inventory_root_path - File.join(bucket_name, bucket_folder_path || "", INVENTORY_PREFIX) + def inventory_path_arn + File.join(bucket_arn, inventory_path) + end + + def inventory_path + path = File.join(INVENTORY_PREFIX, INVENTORY_VERSION) + path = File.join(bucket_folder_path, path) if bucket_folder_path.present? + path + end + + def bucket_arn + "arn:aws:s3:::#{bucket_name}" end def log(message, ex = nil) diff --git a/spec/components/s3_inventory_spec.rb b/spec/components/s3_inventory_spec.rb index 5f147f370f4..fdb4a793a6f 100644 --- a/spec/components/s3_inventory_spec.rb +++ b/spec/components/s3_inventory_spec.rb @@ -16,33 +16,38 @@ describe "S3Inventory" do SiteSetting.s3_secret_access_key = "def" SiteSetting.enable_s3_inventory = true - client.stub_responses(:list_objects, - contents: [ - { - etag: "\"70ee1738b6b21e2c8a43f3a5ab0eee71\"", - key: "example1.csv.gz", - last_modified: Time.parse("2014-11-21T19:40:05.000Z"), - owner: { - display_name: "myname", - id: "12345example25102679df27bb0ae12b3f85be6f290b936c4393484be31bebcc", + client.stub_responses(:list_objects, -> (context) { + inventory_data_path = "#{S3Inventory::INVENTORY_PREFIX}/#{S3Inventory::INVENTORY_VERSION}/bucket/original/data" + expect(context.params[:prefix]).to eq(inventory_data_path) + + { + contents: [ + { + etag: "\"70ee1738b6b21e2c8a43f3a5ab0eee71\"", + key: "example1.csv.gz", + last_modified: Time.parse("2014-11-21T19:40:05.000Z"), + owner: { + display_name: "myname", + id: "12345example25102679df27bb0ae12b3f85be6f290b936c4393484be31bebcc", + }, + size: 11, + storage_class: "STANDARD", }, - size: 11, - storage_class: "STANDARD", - }, - { - etag: "\"9c8af9a76df052144598c115ef33e511\"", - key: "example2.csv.gz", - last_modified: Time.parse("2013-11-15T01:10:49.000Z"), - owner: { - display_name: "myname", - id: "12345example25102679df27bb0ae12b3f85be6f290b936c4393484be31bebcc", - }, - size: 713193, - storage_class: "STANDARD", - } - ], - next_marker: "eyJNYXJrZXIiOiBudWxsLCAiYm90b190cnVuY2F0ZV9hbW91bnQiOiAyfQ==" - ) + { + etag: "\"9c8af9a76df052144598c115ef33e511\"", + key: "example2.csv.gz", + last_modified: Time.parse("2013-11-15T01:10:49.000Z"), + owner: { + display_name: "myname", + id: "12345example25102679df27bb0ae12b3f85be6f290b936c4393484be31bebcc", + }, + size: 713193, + storage_class: "STANDARD", + } + ], + next_marker: "eyJNYXJrZXIiOiBudWxsLCAiYm90b190cnVuY2F0ZV9hbW91bnQiOiAyfQ==" + } + }) end it "should return the latest inventory file name" do diff --git a/spec/jobs/update_s3_inventory_spec.rb b/spec/jobs/update_s3_inventory_spec.rb index 1bcb8985a79..ec26f950b15 100644 --- a/spec/jobs/update_s3_inventory_spec.rb +++ b/spec/jobs/update_s3_inventory_spec.rb @@ -18,7 +18,7 @@ describe Jobs::UpdateS3Inventory do id = "original" @client.expects(:put_bucket_policy).with( bucket: "bucket", - policy: "{\"Version\":\"2012-10-17\",\"Statement\":[{\"Sid\":\"InventoryAndAnalyticsPolicy\",\"Effect\":\"Allow\",\"Principal\":{\"Service\":\"s3.amazonaws.com\"},\"Action\":[\"s3:PutObject\"],\"Resource\":[\"arn:aws:s3:::bucket/inventory/*\"],\"Condition\":{\"ArnLike\":{\"aws:SourceArn\":\"arn:aws:s3:::bucket\"},\"StringEquals\":{\"s3:x-amz-acl\":\"bucket-owner-full-control\"}}}]}" + policy: "{\"Version\":\"2012-10-17\",\"Statement\":[{\"Sid\":\"InventoryAndAnalyticsPolicy\",\"Effect\":\"Allow\",\"Principal\":{\"Service\":\"s3.amazonaws.com\"},\"Action\":[\"s3:PutObject\"],\"Resource\":[\"arn:aws:s3:::bucket/#{S3Inventory::INVENTORY_PREFIX}/#{S3Inventory::INVENTORY_VERSION}/*\"],\"Condition\":{\"ArnLike\":{\"aws:SourceArn\":\"arn:aws:s3:::bucket\"},\"StringEquals\":{\"s3:x-amz-acl\":\"bucket-owner-full-control\"}}}]}" ) @client.expects(:put_bucket_inventory_configuration) @client.expects(:put_bucket_inventory_configuration).with(