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

init: fix S3 ARN parsing #617 #619

Merged
merged 1 commit into from
Apr 3, 2023

Conversation

PettitWesley
Copy link
Contributor

Issue #, if available:

Fixes #617

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@PettitWesley PettitWesley requested a review from a team as a code owner March 31, 2023 02:18
bucketAndFile := strings.SplitN(arnBucketFile, "/", 2)
s3ARN, err := arn.Parse(userInput)
if err != nil {
logrus.Fatalf("[FluentBit Init Process] Unrecognizable arn: %s\n", userInput)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it indicate that this is the s3 arn, similar to the task arn failure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It'll print the arn tho, so the user will see its the S3 arn they passed in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'll change "unrecognizable" to "failure to parse" or something like that

@PettitWesley
Copy link
Contributor Author

I need to test this with the init integ test before we can merge and release it.

Signed-off-by: Wesley Pettit <[email protected]>
@dalgibbard
Copy link

I need to test this with the init integ test before we can merge and release it.

Have tested it on my previously broken deployment in GovCloud and can confirm this is working as expected now 👍

@PettitWesley
Copy link
Contributor Author

PettitWesley commented Apr 2, 2023

So I am struggling to test this myself due issues that I don't fully understand with working in gov cloud.

I ran the integ test we have with an S3 bucket in the main partition, it worked, so no regression. I then tried running the init with gov cloud credentials and I'm getting:

FATA[0000] [FluentBit Init Process] Cannot get bucket region of *****-gov-cloud-test-bucket + dummy-s3-output.conf, you must be the bucket owner to implement this operation

It got my bucket name and key correct so the parsing definitely does work.

@dalgibbard
Copy link

dalgibbard commented Apr 3, 2023

get bucket region of *****-gov-cloud-test-bucket

I noticed that a lot of the tests have the region hardcoded to us-west-2 - you may need to rewrite those?

cd integ/
for fname in `grep -rl "us-west-2" ./*`; do sed -i 's/us-west-2/us-gov-east-1/g' ${fname}; done

I also noticed that the tests are using legacy docker-compose which is deprecated in favour of docker compose (creating /usr/local/bin/docker-compose containing docker compose $@ was enough to trick it though)

Other than that, I was having trouble with it expecting various values which I haven't seen documented, like S3_BUCKET_NAME, KINESIS_STREAM, and probably something around S3 actions (causing errors of: caused by: failed to perform batch operation on "" to "":)
I gave up at this point, but seems like some extra docs/configurability around this might be handy too 😅 (at least some notice as to which envvars are expected perhaps).

That said; i'm actively using this in GovCloud right now, so i'm happy that the changes are working anyway :D

@PettitWesley PettitWesley merged commit 35f563b into aws:mainline Apr 3, 2023
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.

Init doesn't work in GovCloud
3 participants