Init Process for AWS for Fluent Bit on ECS

[Galaoaoa]

Description

Users can build the new image amazon/aws-for-fluent-bit-init:latest and original image amazon/aws-for-fluent-bit:latest as the same time by using the command make release

---

Changes from last PR

Code Style

• Change the original code style to use a consistent code style and naming style with the team
• Change code structure for modularization and code reuse

Unit Test

• Add code for unit test in fluent_bit_init_process_test.go

Dockerfile

• Merge two dockerfiles into Dockerfile.init

---

Details of comparison with aws/aws-for-fluent-bit @ mainline

• Change Makefile
  Change the make release command, add docker build -t amazon/aws-for-fluent-bit-init:latest -f Dockerfile.init . After executing the make release command, will also build the image amazon/aws-for-fluent-bit-init:latest

• Create Dockerfile.init
  This Dockerfile will build the new image amazon/aws-for-fluent-bit-init:latest which include init process and finally execute fluent_bit_init_entrypoint.sh

• Create init folder
  This folder is used to store files related to the init process (following three files)

• Create fluent_bit_init_entrypoint.sh
  This file is used to run init process binary and run fluent-bit-invoker.sh, which will execute export command to set ECS Metadata, and finally invoke Fluent Bit

• Create fluent_bit_init_process.go
  This file is the code which used to achieve the desired init process functions

• Create fluent_bit_init_process_test.go
  This file is the code of Unit Test for fluent_bit_init_process.go

---

Tests already performed

Run image on ECS

• Without any user specified environment variables. Same effect as using the original image

• Add a Dummy Input config file via setting environment variables. Main config file is changed as expected

• Add a Dummy Input config file, a Parser config file and a related Filter config file via setting environment variables. Main config file is changed as expected, the Fluent Bit Command is changed as expected

• Add a Dummy Input config file, a Parser config file, a related Filter config file, a S3 Output config file, a stdout Output config file via setting environment variables. Main config file is changed as expected, the Fluent Bit Command is changed as expected. Can receive the log in the container and S3

Run image locally

• Without ECS Task Metadata. Can invoke Fluent Bit and print out a warning [FluentBit Init Process] Unable to get ECS Metadata, ignore this warning if not running on ECS

[zhonghui12]

Much better than last PR. good work

[zhonghui12]

I don't think we need to fully copy Dockerfile file right? We can reuse the aws-for-fluent-bit container and just copy our init process into it. Same thing as copying aws-for-fluent-bit-plugin into aws-for-fluent-bit container : https://github.com/aws/aws-for-fluent-bit/blob/mainline/load_tests/load_test.py#L117.

[zhonghui12]

Oh is it because of the entrypoint.sh?

[zhonghui12]

but we can still skip most of the code here right? like we don't need to build fluent bit, we can just copy the binary for aws-for-fluent-bit container

[Galaoaoa]

Yea make sense. I also plan to continue to improve this part and will try again

[zhonghui12]

let's keep fluent-bit-init-xx into one subfolder like init

[Galaoaoa]

Do you think it's a good idea to put all init-related files and folders in one subfolder like fluent-bit-init?

[zhonghui12]

I think we can just create a folder called init and put all related files under it

[zhonghui12]

why not do fluent-bit-init-s3-files/ if it is a path?

[Galaoaoa]

Will modify this with above comment. Actually I will modify all paths of files and folders to make them more reasonable

[zhonghui12]

nit: I think mainConfigFile and invokerFile is enough

[Galaoaoa]

Yeah will modify

[zhonghui12]

you don't need to comment to an obvious item.. it just seems to write the same thing twice..

[Galaoaoa]

Will delete unnecessary comments..

[zhonghui12]

same as above

[zhonghui12]

same as above

[zhonghui12]

when autoclose will be set to false?

[Galaoaoa]

here
Because s3Downloader need to download content to this file, so it needs to be kept open

[zhonghui12]

then we will not close it after downloading content?

[zhonghui12]

we can change the name to be more generate or official rather than using our alias. Like Cluster:ecs-cluster

[Galaoaoa]

Yea make sense!

[zhonghui12]

for name, let' do all - or _ to keep consistent

[Galaoaoa]

Hmm.. it's because Go unit test file must end with _test.go

[zhonghui12]

I see. Then I would prefer all _

[Galaoaoa]

Much better than last PR. good work

Thank you for your comments on this PR!
Some details need to be confirmed by your reply again :)

[Galaoaoa]

All file permissions like this will be reconfirmed

[zhonghui12]

I am not sure if we need this file. Need to confirm with @PettitWesley . Normally I just don't git add these files

[zhonghui12]

nit: initer sounds weird to me. I prefer builder or init-builder

[Galaoaoa]

Will modify all these names

[zhonghui12]

nit: extra line

[PettitWesley]

you should not run go mod commands inside of a build script IMO/AFAIK. The go.mod and go.sum files are generally committed to the source control in most projects I have seen and this helps ensure all build use the same versions of dependencies.

[Galaoaoa]

Yea, I changed it to copy go.mod and go.sum from our repo instead of creating a new one

[zhonghui12]

nit: invoker sounds weird to me. I prefer /init/init.sh

[zhonghui12]

nit: naming issue as above

[zhonghui12]

Based on discussion, let's change this to aws_fluent_bit_init_s3 and aws_fluent_bit_init_file

[Galaoaoa]

And both aws_fluent_bit_init_s3 and aws_fluent_bit_init_S3 will be accepted as config files from S3.
I think it would be more helpful here not to use case sensitivity, customers may not be concerned about the way s3 or S3 is written

[Galaoaoa]

Update

• Changed the prefix from aws_fluent_bit_s3|file to aws_fluent_bit_init_s3|file

• Modified some of the naming that was not good enough

• Modified file permissions

[DrewZhang13]

Overall LGTM, code in good format, nice work!

[DrewZhang13]

Can we try not to hardcode the go version since This always need to be upgraded.
Not 100% if it works but can you verify if it's possible to use

gimme stable

to install go?
Official doc: https://github.com/travis-ci/gimme

[Galaoaoa]

I've tried switching to the stable version, but this prevents me from setting the PATH for the go command in the Dockerfile..
Using 1.17.9 is because other Dockerfiles in our repo used this version

[DrewZhang13]

useless comment, could remove

[PettitWesley]

actually go lint will require a comment for structures... preferably more descriptive than this but this is the minimum needed to pass the linter

[Galaoaoa]

For these interfaces, I think they are useless comments. And maybe we can discuss a standard for our team's code comments, although this is not a very important thing for this moment..

[DrewZhang13]

useless comment, could remove.

[PettitWesley]

See my comment on HTTPClient

[DrewZhang13]

Do we need to consider retry is download from S3 fail for once?

[PettitWesley]

Check the docs, I think the downloader may have some setting to auto-add retries.

[Galaoaoa]

Already modified. Thanks!

[DrewZhang13]

I like the format in main(), it's clean and in good shape.
These three lines could be optimized to only one function like others in main() as well.

and move these three lines into that function outside of main().

[Galaoaoa]

Writing them out for unit testing, has removed metadataReigon from the main().
But I think I still have to keep the other two..

[zhonghui12]

do we need all of these packages?

[Galaoaoa]

After verification, tar gzip git are required. Already removed make gcc. Thanks for this reminder~

[zhonghui12]

you can just make it case-insensitive here: https://stackoverflow.com/questions/15326421/how-do-i-do-a-case-insensitive-regular-expression-in-go. as well as file/File maybe

[Galaoaoa]

Already modified. Thank you for this advice

[zhonghui12]

what does content_b mean? Avoid to use meaningless name

[PettitWesley]

I think its always better to do full names if needed: contentBytes

[zhonghui12]

metadataRegion

[zhonghui12]

nit: existS3Client

[PettitWesley]

Actually, s3ClientExists or s3ClientCreated would be more idiomatic I think... this fits what I've seem from other go programmers, if you have a bool, you use works like has or is or something else, but you put it in the variable name in a way that fits how you'd say it out loud in english, so hass3Client but that sounds weird to me... I prefer s3ClientCreated since its readable is exactly what you mean- has the S3 client been created or not yet?

[Galaoaoa]

Already modified this variable name. Thanks for advice

[zhonghui12]

I don't think it is needed. In function getAllConfigFiles(), from line 166, you can directly do:

if s3 {
 download S3;
return filePath;
}

processConfigFile(filePath); 

This function is very similar to processConfigFile

[zhonghui12]

Then you don't need to use one extra function to create a s3Folder, you can just save all S3 files in one path which shares the same prefix. I remember the command will automatically create one path or find that path and create the file.

[Galaoaoa]

For the first comment, thanks for this advice. Reuse processConfigFile() and delete processS3ConfigFiles() makes my code more concise

For the second comment, after trying, os.Create cannot create a file in a non-existent folder, which means I need create this folder first..

[zhonghui12]

for #2, I think you can have a function for it.

// create a file from a specific path
func create(p string) (*os.File, error) {
	if err := os.MkdirAll(filepath.Dir(p), 0600); err != nil {
		return nil, err
	}
	return os.Create(p)
}

I believe above should work? it will check if it needs to create a directory and then create the file

[Galaoaoa]

I think it should work, will try it later, thanks

[zhonghui12]

nit: should be -c?

[zhonghui12]

I think this line is too long.. you can skip some useless parts.

[Galaoaoa]

Actually I used ECS Exec specifically to get this real response for testing..

I want to keep this because real test cases are more likely to find potential bugs, and this will be useful for further testing. If we change the Metadate structure future, we can still use this test case to test it

[zhonghui12]

Then I prefer you put the two long responses in one place like in the very beginning as const. It is too long to see here

[Galaoaoa]

Yea already modified~

[zhonghui12]

see above

[zhonghui12]

I don't think line 385 and 394 is needed. See my comment in line 285

[zhonghui12]

#379 (comment)

[PettitWesley]

I thought/think the image should be amazon/aws-for-fluent-bit:latest-init or amazon/aws-for-fluent-bit:init-latest.

Building the image with this tag: amazon/aws-for-fluent-bit-init implies that we have a Docker Hub repo with that name. But we do not. I beleive we are planning to push these images to the same repos as we use for the non-init image right? And thus only the tag is different. So the same hsould be true in our build script IMO.

[Galaoaoa]

Can we create a new Docker Hub repo for init images? Helps to manage new images more clearly and without affecting the original image.

I think if we naming it as amazon/aws-for-fluent-bit:latest-init, we also need to add different version numbers in the future, which can be very confusing like amazon/aws-for-fluent-bit:init-amd64-2.26.0? Tag will be too long and it is a different style from the original image..

[PettitWesley]

So unfortunately creating a new Docker Hub repo is a whole big process... also while I understand the tag becomes very long, I also think its better if all AWS for Fluent Bit images are in one repo if possible.

A good example of long tags is fluentd repo which has tons of tags: https://hub.docker.com/r/fluent/fluentd/tags

[Galaoaoa]

Yea I already discussed this with @zhonghui12 .
Understand to create a new Docker Hub repo will be a big process..

We finally decided to use same repo and these tags:
amazon/aws-for-fluent-bit:init-latest
amazon/aws-for-fluent-bit:init-2.26.0
amazon/aws-for-fluent-bit:init-amd64-2.26.0

[PettitWesley]

Why cat the file in entrypoint? This means it will be printed to the users log... which isn't bad but its also not really needed... and the user can get Fluent Bit to output all registered plugins if htey run it with debug logging...

[Galaoaoa]

This came from a discussion with Bingfeng, he asked me how customer can quickly know which config files they are using.. Maybe using cat here is not suitable. Will delete

[PettitWesley]

nit: I think "base command" is a better way to describe this since you start with it and add to it

[PettitWesley]

Hmm why do we give folks one with just revision, and one with "family:revision", but not just family?? Can we add that please. Since we do not have service name available, family (without the revision, since that changes a lot) is commonly used to replace service name.

[Galaoaoa]

We already have :)

[PettitWesley]

nit: since this function is only one line, it does not really provide for code organization I think... you could replace the one line call to this function with the one line to create the client.

[PettitWesley]

nit: newline at beginning of function

[PettitWesley]

nit: might be more clear to users if its: Failed to read ECS metadata HTTP response.

[PettitWesley]

So this has some limitations which users often run into:

To use this implementation of the operation, you must be the bucket owner.

https://docs.aws.amazon.com/AmazonS3/latest/API/API_GetBucketLocation.html

We should probably put this in our error message

[PettitWesley]

nit: in code, it is preferable IMO to use "directory" instead of folder. Directory is the old unix term, folder came around once we had GUIs.

[Galaoaoa]

Yea, already modified. Will pay attention when writing code in the future

[Galaoaoa]

Update

• Changed the image name from amazon/aws-for-fluent-bit-init:latest to amazon/aws-for-fluent-bit:init-latest

• Deleted processS3ConfigFiles(), changed to reuse processConfigFile()

• Modified the regex to implement case-insensitive of s3/S3, file/File

• Modified some variable names

• Removed two useless packages make gcc in the Dockerfile

[Galaoaoa]

Update

• Set the environment variable FLB_AWS_USER_AGENT=init to get the amazon/aws-for-fluent-bit:init-latest usage

• Deleted the createS3ConfigFileDirectory() function

• Modified the corresponding unit test

[zhonghui12]

Overall looks good to me. Good work!

Also, please rebase all commits into 1 before merge

[zhonghui12]

please update the username here

[Galaoaoa]

already removed Alias in the unit test, thank you!