Skip to content
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

Conditionally check S3 artifacts #175

Merged
merged 3 commits into from
Dec 20, 2016
Merged

Conditionally check S3 artifacts #175

merged 3 commits into from
Dec 20, 2016

Conversation

chillaxd
Copy link
Contributor

@chillaxd chillaxd commented Nov 14, 2016

Condition to skip S3 bucket and object existence during runtime, so that more tight and granular AWS policy can be provided on the instance where fluentd is running.

With existing one, we need minimum S3:ListBucket, S3:GetObjectVersion, and S3:PutObject policy. But if we apply this two configure parameter, fluentd will work with only S3:PutObject policy.

For this to happen the below mentioned config parameters must be set:
check_bucket => false
check_object => false

@keshavab
Copy link

+1
Such conditional checks help put less restrictive policy when using S3 buckets.

@repeatedly
Copy link
Member

Please add tests.

When it is false,
s3_object_key_format will be %{path}/events/%{date_slice}/%{time_slice}.%{file_extension}
where, time_slice will be in hhmmss format, so that each object will be unique.

Copy link
Member

Choose a reason for hiding this comment

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

lack check_bucket parameter

When it is false,
s3_object_key_format will be %{path}/events/%{date_slice}/%{time_slice}.%{file_extension}
where, time_slice will be in hhmmss format, so that each object will be unique.

Copy link
Member

Choose a reason for hiding this comment

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

And adding s3 policy example is better for this mode.

@chillaxd
Copy link
Contributor Author

After I added the test cases, build got failed. So I delete those test case. Even it is failing. Can you please give some light on this?

@repeatedly
Copy link
Member

Maybe v0.14.9 changes affect it. I will check it later.

Copy link
Contributor Author

@chillaxd chillaxd left a comment

Choose a reason for hiding this comment

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

It's Ok,

@chillaxd
Copy link
Contributor Author

Thanks Masahiro

@chillaxd
Copy link
Contributor Author

Hi Masahiro, did you got the time to look into the issue?

@repeatedly
Copy link
Member

I found the cause and I have already opened an PR on fluentd: fluent/fluentd#1319
I will re-check your patch later.

@repeatedly
Copy link
Member

The test failure are not related your test.
Please add tests again.

@chillaxd chillaxd closed this Dec 6, 2016
@chillaxd chillaxd reopened this Dec 6, 2016
@chillaxd chillaxd closed this Dec 6, 2016
@chillaxd chillaxd reopened this Dec 6, 2016
@chillaxd chillaxd closed this Dec 13, 2016
@chillaxd chillaxd reopened this Dec 13, 2016
@chillaxd
Copy link
Contributor Author

Hi Masahiro, I have added test cases and introduced the mock function to work. Please check

@@ -137,6 +147,14 @@ def test_path_slicing_utc
slice = path_slicer.call(path)
assert_equal slice, Time.now.utc.strftime("log/%Y/%m/%d")
end

def test_hms_slicing
Copy link
Member

Choose a reason for hiding this comment

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

This tests Ruby's class, not plugin feature. Not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

assert_equal '131415', Time.parse("2011-01-02 13:14:15").strftime("%H%M%S")
end

def test_hms_slicing_utc
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

previous_path = s3path
end while @bucket.object(s3path).exists?
else
@s3_object_key_format = "%{path}/%{date_slice}_%{hms_slice}.%{file_extension}"
Copy link
Member

Choose a reason for hiding this comment

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

Don't update instance variable in write.
Update should be in configure or start.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -142,10 +146,12 @@ def configure(conf)
@path_slicer = Proc.new {|path|
Time.now.strftime(path)
}
@hms_slicer = Time.now.strftime("%H%M%S")
Copy link
Member

@repeatedly repeatedly Dec 15, 2016

Choose a reason for hiding this comment

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

This value is never updated in write, so all stored objects share same @hms_slicer value in the path.
Is this expected?

Copy link
Contributor Author

@chillaxd chillaxd Dec 15, 2016

Choose a reason for hiding this comment

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

That's my fault. To refactor I missed that portion. Corrected

else
@path_slicer = Proc.new {|path|
Time.now.utc.strftime(path)
}
@hms_slicer = Time.now.utc.strftime("%H%M%S")
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected

@@ -118,6 +120,14 @@ def test_configure_with_hex_random_length
create_driver(conf + "\nhex_random_length 16\n")
end
end

def test_configure_with_no_check_on_s3
conf = CONFIG.clone
Copy link
Member

Choose a reason for hiding this comment

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

@repeatedly
Copy link
Member

repeatedly commented Dec 15, 2016

After updated tests, please squash fix/fixes like commits into one.

@repeatedly
Copy link
Member

Could you merge "fix" commits into one?
Afte that, I will merge this.

@chillaxd
Copy link
Contributor Author

Hi Masahiro, The previous fixes is what when I try to fix the test cases. And all those changes are in the current commit itself. Do I still need to merge previous "fix" commits into one?

@repeatedly
Copy link
Member

Yes because if merge current commits into master, master branch has lots of useless fix commits.
This is not good for other users, e.g. hard to check changes.

…licy

Condition to skip S3 bucket and object existence during runtime,
so that more tight and granular AWS policy can be provided on the instance where fluentd is running.

With existing one, we need minimum S3:ListBucket, S3:GetObjectVersion, and S3:PutObject policy.
But if we apply this two configure parameter, fluentd will work with only S3:PutObject policy.
For this to happen the below mentioned config parameters must be set:
check_bucket => false
check_object => false
@chillaxd
Copy link
Contributor Author

Done

@repeatedly
Copy link
Member

repeatedly commented Dec 20, 2016

Where is the changes of plugin?
Your last commit includes only README update.

Condition to skip S3 bucket and object existence during runtime,
so that more tight and granular AWS policy can be provided on the instance where fluentd is running.

With existing one, we need minimum S3:ListBucket, S3:GetObjectVersion, and S3:PutObject policy.
But if we apply this two configure parameter, fluentd will work with only S3:PutObject policy.

For this to happen the below mentioned config parameters must be set:
check_bucket => false
check_object => false
@repeatedly repeatedly merged commit 350f2c3 into fluent:master Dec 20, 2016
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