feat!: multiple buckets / providers support (#48) #177

hbollon · 2021-06-24T13:35:07Z

As the title shown, I've added the multiple buckets / providers support into Terraboard.
It's now possible to set multiples S3/MinIO buckets or to mix providers (to have AWS, Gitlab and GCP simultaneously for example), Terraboard will fetch them all on db refresh.

However:

To use this functionality, the only configuration method usable is through Yaml config file. Env and flags are still usable for mono bucket configuration (maybe we can use them for multiple providers but it's not appropriated imo)
Atm, Terraboard is dependent of AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY env variables for AWS provider, which prevents being able to have several backend AWS or others compatible providers like MinIO (we can still have several buckets under the same backend). We could consider making these values configurable via the yaml file however (@raphink what do you think?).

I tested that with MinIO/AWS but not with others providers since I don't have any account for them.
The terraboard-dev-multi-providers service in the test/ docker-compose is configured to use two MinIO bucker (just replace terraboard-dev by it in the compose file)
I also updated the ReadMe with some instructions / advices.

Should meets the needs of #48 😄

coveralls · 2021-06-24T13:40:13Z

Coverage decreased (-0.4%) to 57.412% when pulling f3a13f8 on hbollon:multi-bucket-support into ad88878 on camptocamp:master.

README.md

raphink · 2021-06-24T14:52:39Z

README.md

+In the case of AWS, don't forget to set **AWS_ACCESS_KEY_ID** and **AWS_SECRET_ACCESS_KEY** environment variables.
+
+That's it! Terraboard will now fetch these two buckets on DB refresh. You can also mix providers like AWS and Gitlab or anything else.
+You can find a ready-to-use docker example with two *MinIO* buckets in the `test/` sub-folder (just swipe the two terraboard services in the docker-compose file). 


Suggested change

You can find a ready-to-use docker example with two *MinIO* buckets in the `test/` sub-folder (just swipe the two terraboard services in the docker-compose file).

You can find a ready-to-use Docker example with two *MinIO* buckets in the `test/` sub-folder (just swipe the two terraboard services in the docker-compose file).

What do you mean by "swipe the services"?

What do you mean by "swipe the services"?

I added in the docker-compose of /test, a Terraboard container configured to use a YAML config file and two MinIO buckets.
However, I commented it in favor of the Terraboard lambda container to avoid having two applications simultaneously.

services: terraboard-dev: build: context: ../ dockerfile: ./Dockerfile environment: AWS_ACCESS_KEY_ID: root AWS_SECRET_ACCESS_KEY: mypassword AWS_BUCKET: test-bucket AWS_REGION: eu-west-1 AWS_ENDPOINT: http://minio:9000/ AWS_FORCE_PATH_STYLE: "true" TERRABOARD_LOG_LEVEL: debug TERRABOARD_NO_LOCKS: "true" TERRABOARD_NO_VERSIONING: "true" DB_PASSWORD: mypassword DB_SSLMODE: disable GODEBUG: netdns=go depends_on: db: condition: service_healthy minio: condition: service_started volumes: - ../static:/static:ro ports: - "8080:8080" # terraboard-dev-multi-providers: # build: # context: ../ # dockerfile: ./Dockerfile # environment: # AWS_ACCESS_KEY_ID: root # AWS_SECRET_ACCESS_KEY: mypassword # DB_SSLMODE: disable # CONFIG_FILE: config/config.yml # GODEBUG: netdns=go # depends_on: # db: # condition: service_healthy # minio: # condition: service_started # volumes: # - ../static:/static:ro # - ./config.yml:/config/config.yml:ro # ports: # - "8081:8080"

I'm not really happy with that, maybe we can split the test sub-folder into multiple config folders each with a docker-compose 🤔

Ah, so you mean "swap" then, not "swipe" :-)

raphink · 2021-06-24T14:59:24Z

config/config.go

+	c := initDefaultConfig()
+	parseStructFlagsAndEnv(&c)


Why is this necessary?

It's for keeping the env / flags configuration possible with single provider Terraboard.
Indeed, now we have providers array in the config struct:

AWS []AWSConfig `group:"AWS Options" yaml:"aws"` TFE []TFEConfig `group:"Terraform Enterprise Options" yaml:"tfe"` GCP []GCPConfig `group:"Google Cloud Platform Options" yaml:"gcp"` Gitlab []GitlabConfig `group:"GitLab Options" yaml:"gitlab"`

And so they don't have any elements in them on Config instanciation which prevent from using env variables or flags.
c := initDefaultConfig() add an empty struct in each of them and parse go-flags stuff in them. If they're still empty or invalid they will be ignored but with that process we can still use Terraboard with others configuration methods as before (although they are incompatible with multi buckets).

parseStructFlagsAndEnv(interface{}) is just a DRY function for go-flags parsing

raphink · 2021-06-24T15:06:42Z

main.go

-			continue
-		}
+		log.Debugf("Total providers: %d\n", len(sps))
+		for i, sp := range sps {


Looping inside refreshDB() means providers will be parsed sequentially. Maybe we could parallelize providers instead by looping ouside the call to refreshDB() in main() and keep refreshDB() the way it is.

Great idea that I should have thought of!
This will allow to not interrupt all the fetching processes if one of them has an error and reduce the time that the refresh takes
I will do that

+ code review fixes from camptocamp#177

hbollon · 2021-06-25T08:40:53Z

I've done the following changes:

RefreshDB is now parallelized
I added mutex lock security on sensible db operations (FirstOrCreate principally, which lead to errors without locks)
I split test directory into multiples sub-folder containing different configurations
I fixed all ReadMe typo issues

+ code review fixes from camptocamp#177

README.md

db/db.go

raphink · 2021-06-25T10:14:26Z

db/db.go

 	err = db.FirstOrCreate(&lineage, types.Lineage{Value: sf.Lineage}).Error
 	if err != nil || lineage.ID == 0 {
-		log.Error("Unknown error in stateS3toDB during lineage finding")
+		log.Errorf("Unknown error in stateS3toDB during lineage finding: %v", err)


It could be interesting to use a structured log here (with fields).

+ code review fixes from camptocamp#177

…ration

where go-flags ones aren't applicable

+ code review fixes from camptocamp#177

Shocktrooper · 2021-07-01T15:14:19Z

As the title shown, I've added the multiple buckets / providers support into Terraboard.
It's now possible to set multiples S3/MinIO buckets or to mix providers (to have AWS, Gitlab and GCP simultaneously for example), Terraboard will fetch them all on db refresh.

However:

To use this functionality, the only configuration method usable is through Yaml config file. Env and flags are still usable for mono bucket configuration (maybe we can use them for multiple providers but it's not appropriated imo)

Atm, Terraboard is dependent of AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY env variables for AWS provider, which prevents being able to have several backend AWS or others compatible providers like MinIO (we can still have several buckets under the same backend). We could consider making these values configurable via the yaml file however (@raphink what do you think?).

I tested that with MinIO/AWS but not with others providers since I don't have any account for them.
The terraboard-dev-multi-providers service in the test/ docker-compose is configured to use two MinIO bucker (just replace terraboard-dev by it in the compose file)
I also updated the ReadMe with some instructions / advices.

Should meets the needs of #48 😄

With AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY being needed does this prevent multiple cross account S3 buckets from being selected? Would they all have to be in the same account and is there work to migrate over to the AWS SDK or something that can automatically handle credentials automatically instead of relying on hardcoded environment variables

hbollon · 2021-07-01T15:36:12Z

With AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY being needed does this prevent multiple cross account S3 buckets from being selected?

Actually, with this PR, yes all AWS buckets must be on the same account.

is there work to migrate over to the AWS SDK or something that can automatically handle credentials automatically instead of relying on hardcoded environment variables

This is exactly what I proposed to @raphink 😃

* allow aws multiple buckets to be configured directly in s3 yaml field (now an objects array) * add another minio instance to multiple-minio-buckets testing env * update all yaml example config files

hbollon · 2021-07-05T13:40:06Z

I've added:

AWS access key and secret access key config fields and aws sdk appropriate usage to support multiple AWS accounts configuration
AWS Session Token support (should do the trick for feat: support aws_session_token #181)
The ability to directly configure multiple buckets for the same AWS account under the "S3" field of the YAML file which is now an objects array

I also updated unit test and docker testing env to match with new config (I also add a second minio container in the multiple-minio-buckets test env)
Like previously, I made sure that the config via environment variables or by flags is always possible for the instances of Terraboard with a single provider

Example of a YAML config file:

aws:
  - access-key: root
    secret-access-key: mypassword
    endpoint: http://minio:9000/
    region: eu-west-1
    s3:
      - bucket: test-bucket
        force-path-style: true
        file-extension: 
          - .tfstate
      - bucket: test-bucket2
        force-path-style: true
        file-extension: 
          - .tfstate

  - access-key: admin
    secret-access-key: password
    endpoint: http://minio-2:9000/
    region: eu-west-1
    s3:
      - bucket: test-bucket
        force-path-style: true
        file-extension: 
          - .tfstate

config/yaml.go

raphink · 2021-06-25T12:04:08Z

state/aws.go

-	sess := session.Must(session.NewSession())
-
-	awsConfig := aws_sdk.NewConfig()
+func NewAWS(c *config.Config) []*AWS {


I wonder if it'd be easier to keep the existing NewAWS() method as it is and add a NewAWSCollection() wrapper.

To avoid the iteration in NewAWS()?

Yes, and also because generally I'd expect an instantiation method to return a single instance.

Ok so I will do that for AWS but also for all others providers
Each one will have a NewProvider() function wich return a single instance and a NewProviderCollection() one which will iterate and call the other one. (ofc in functions names Provider will be replaced by his name)

README.md

Shocktrooper · 2021-07-08T07:47:57Z

Is there since there was mention of aws sdk usage of credentials is there an example of how to tell terraboard to assume a role in another account or how to tell it to use an instance profile in the case of S3 buckets

raphink · 2021-07-14T14:33:05Z

README.md

-1. Environment variables
-2. CLI parameters
-3. Configuration file (YAML). A configuration file example can be found in the root directory of this repository.
+1. Environment variables **(only usable for mono provider configuration)**


Suggested change

1. Environment variables **(only usable for mono provider configuration)**

1. Environment variables **(only usable for single provider configuration)**

raphink · 2021-07-14T14:33:14Z

README.md

-2. CLI parameters
-3. Configuration file (YAML). A configuration file example can be found in the root directory of this repository.
+1. Environment variables **(only usable for mono provider configuration)**
+2. CLI parameters **(only usable for mono provider configuration)**


Suggested change

2. CLI parameters **(only usable for mono provider configuration)**

2. CLI parameters **(only usable for single provider configuration)**

raphink · 2021-07-14T14:35:08Z

state/aws.go

-	sess := session.Must(session.NewSession())
-
-	awsConfig := aws_sdk.NewConfig()
+func NewAWS(c *config.Config) []*AWS {


Yes, and also because generally I'd expect an instantiation method to return a single instance.

state/aws.go

state/gcp.go

state/gitlab.go

raphink · 2021-07-15T12:03:37Z

state/state.go

+	if len(c.TFE) > 0 {
+		objs, err := NewTFECollection(c)
+		if err != nil {
+			return []Provider{}, err


If you named the return values of the method, you might just use return here iirc

Note, if one provider fails, you get no providers at all at the moment.

Note, if one provider fails, you get no providers at all at the moment.

Do you think it's better to just log an error and continue with the following ones?
Initially, I considered this error as fatal since it would lead to the non-inclusion of an entire provider and because, potentially, it could come from an error in the configuration file which could impact the rest.

Not sure. I say I'd prefer to see some data and get an error somewhere rather than nothing at all.

If you named the return values of the method, you might just use return here iirc

Indeed but in this case I think it's more simple like this since if I name the return values I can't use := anymore with err which will force me to declare the temp variable objs before each time I use it. It's, imo, more complex and furthermore it raise an error on gosimple if I do it.

Not sure. I say I'd prefer to see some data and get an error somewhere rather than nothing at all.

Doesn't the user risk not realizing it if a provider fails but the others still bring states in Terraboard?

mcanevet

I guess you should change one of your commits' commit message with an exclamation mark as it contains a breaking change.

state/tfe.go

hbollon force-pushed the multi-bucket-support branch from 48769bd to 61b162f Compare June 24, 2021 13:38

hbollon force-pushed the multi-bucket-support branch from 61b162f to aeb4893 Compare June 24, 2021 13:46

raphink reviewed Jun 24, 2021

View reviewed changes

README.md Show resolved Hide resolved

raphink reviewed Jun 24, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

raphink reviewed Jun 24, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

raphink reviewed Jun 24, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

raphink reviewed Jun 24, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

raphink reviewed Jun 24, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

raphink reviewed Jun 24, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

raphink reviewed Jun 24, 2021

View reviewed changes

raphink added the breaking label Jun 25, 2021

hbollon added a commit to hbollon/terraboard that referenced this pull request Jun 25, 2021

feat: parallelized db refreshing and add mutex security

39ba18b

+ code review fixes from camptocamp#177

hbollon added a commit to hbollon/terraboard that referenced this pull request Jun 25, 2021

feat: parallelized db refreshing and add mutex security

ff4915a

+ code review fixes from camptocamp#177

hbollon force-pushed the multi-bucket-support branch from 39ba18b to ff4915a Compare June 25, 2021 08:42

raphink reviewed Jun 25, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

raphink reviewed Jun 25, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

raphink reviewed Jun 25, 2021

View reviewed changes

db/db.go Show resolved Hide resolved

raphink reviewed Jun 25, 2021

View reviewed changes

hbollon added a commit to hbollon/terraboard that referenced this pull request Jun 25, 2021

feat: parallelized db refreshing and add mutex security

7b2d2b2

+ code review fixes from camptocamp#177

hbollon force-pushed the multi-bucket-support branch from ff4915a to 7b2d2b2 Compare June 25, 2021 11:30

hbollon added 5 commits June 25, 2021 13:49

feat: multi-provider support (WIP)

1547860

feat: env variables/flags available again for single provider configu…

21a29a7

…ration

feat: add some custom yaml unmarshalling rules to add default values

83f0727

where go-flags ones aren't applicable

docs: update readme with multiple buckets/providers config advices

f719332

feat: parallelized db refreshing and add mutex security

65e6f39

+ code review fixes from camptocamp#177

hbollon force-pushed the multi-bucket-support branch from 7b2d2b2 to 65e6f39 Compare June 25, 2021 11:53

raphink mentioned this pull request Jul 1, 2021

Feature request: multiple s3 buckets #48

Closed

feat: add aws credentials & session token config support

dae5a1e

* allow aws multiple buckets to be configured directly in s3 yaml field (now an objects array) * add another minio instance to multiple-minio-buckets testing env * update all yaml example config files

mcanevet added this to the v2.0.0-alpha1 milestone Jul 7, 2021

raphink reviewed Jul 8, 2021

View reviewed changes

raphink mentioned this pull request Jul 8, 2021

feat: support aws_session_token #181

Closed

raphink reviewed Jul 14, 2021

View reviewed changes

hbollon force-pushed the multi-bucket-support branch 3 times, most recently from 0a4c557 to 6e54c13 Compare July 15, 2021 09:01

raphink reviewed Jul 15, 2021

View reviewed changes

state/aws.go Outdated Show resolved Hide resolved

raphink reviewed Jul 15, 2021

View reviewed changes

state/gcp.go Outdated Show resolved Hide resolved

raphink reviewed Jul 15, 2021

View reviewed changes

state/gitlab.go Outdated Show resolved Hide resolved

raphink reviewed Jul 15, 2021

View reviewed changes

mcanevet suggested changes Jul 15, 2021

View reviewed changes

raphink reviewed Jul 15, 2021

View reviewed changes

state/tfe.go Outdated Show resolved Hide resolved

raphink changed the title ~~feat: multiple buckets / providers support (#48)~~ feat!: multiple buckets / providers support (#48) Jul 15, 2021

docs: update readme with new aws configuration fields

f3a13f8

hbollon force-pushed the multi-bucket-support branch from 6e54c13 to f3a13f8 Compare July 15, 2021 13:32

raphink merged commit e44ebce into camptocamp:master Jul 20, 2021

This was referenced Jul 20, 2021

chore: release 2.0.0-alpha #187

Merged

chore: release 2.0.0-alpha hbollon/terraboard#3

Closed

This was referenced Jul 31, 2021

chore: release 2.0.0-alpha #190

Merged

chore: release 2.0.0-alpha hbollon/terraboard#4

Open

chore: release 2.0.0-alpha #192

Closed

nishkrishnan mentioned this pull request Mar 7, 2022

AWS Default Credentials Chain not fetched in > 2.0.0 #233

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: multiple buckets / providers support (#48) #177

feat!: multiple buckets / providers support (#48) #177

hbollon commented Jun 24, 2021

coveralls commented Jun 24, 2021 •

edited

Loading

raphink Jun 24, 2021

raphink Jun 24, 2021

hbollon Jun 25, 2021

raphink Jun 25, 2021

hbollon Jun 25, 2021

raphink Jun 24, 2021

hbollon Jun 25, 2021

raphink Jun 24, 2021 •

edited

Loading

hbollon Jun 25, 2021

hbollon commented Jun 25, 2021

raphink Jun 25, 2021

hbollon Jun 25, 2021

Shocktrooper commented Jul 1, 2021

hbollon commented Jul 1, 2021

hbollon commented Jul 5, 2021 •

edited

Loading

raphink Jun 25, 2021

hbollon Jul 8, 2021

raphink Jul 14, 2021

hbollon Jul 15, 2021

Shocktrooper commented Jul 8, 2021

raphink Jul 14, 2021

raphink Jul 14, 2021

raphink Jul 14, 2021

raphink Jul 15, 2021

raphink Jul 15, 2021

hbollon Jul 15, 2021 •

edited

Loading

raphink Jul 15, 2021

hbollon Jul 15, 2021

hbollon Jul 15, 2021

mcanevet left a comment

	You can find a ready-to-use docker example with two MinIO buckets in the `test/` sub-folder (just swipe the two terraboard services in the docker-compose file).
	You can find a ready-to-use Docker example with two MinIO buckets in the `test/` sub-folder (just swipe the two terraboard services in the docker-compose file).

	1. Environment variables (only usable for mono provider configuration)
	1. Environment variables (only usable for single provider configuration)

	2. CLI parameters (only usable for mono provider configuration)
	2. CLI parameters (only usable for single provider configuration)

feat!: multiple buckets / providers support (#48) #177

feat!: multiple buckets / providers support (#48) #177

Conversation

hbollon commented Jun 24, 2021

coveralls commented Jun 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphink Jun 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hbollon commented Jun 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shocktrooper commented Jul 1, 2021

hbollon commented Jul 1, 2021

hbollon commented Jul 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shocktrooper commented Jul 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hbollon Jul 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcanevet left a comment

Choose a reason for hiding this comment

coveralls commented Jun 24, 2021 •

edited

Loading

raphink Jun 24, 2021 •

edited

Loading

hbollon commented Jul 5, 2021 •

edited

Loading

hbollon Jul 15, 2021 •

edited

Loading