kots icon indicating copy to clipboard operation
kots copied to clipboard

Add Alpha support for Amazon S3 as BYO object store

Open dexhorthy opened this issue 5 years ago • 3 comments

Implements the proposal at https://github.com/replicatedhq/kots/blob/master/docs/proposals/object-store.md, adding alpha support for #457

For reviewing sources, I'd recommend to start reading at pkg/kotsadm/types/objectstoreoptions.go

KOTS CLI Changes

  • added flags described here https://github.com/replicatedhq/kots/blob/master/docs/proposals/object-store.md#proposed-new-changes
  • added validation described here https://github.com/replicatedhq/kots/blob/master/docs/proposals/object-store.md#validating-configuration
  • added a struct, ObjectStoreOptions, to track flags for deployment
type ObjectStoreOptions struct {
	ObjectStoreType string
	AccessKeyID     string
	SecretAccessKey string
	BucketName      string
	Endpoint        string
	Region          string
	BucketInPath    bool
}
  • expanded kotsadm-minio secret with the following fields:
"type"
"accesskey"
"secretkey"
"endpoint"
"bucketname"
"region"
"skip-ensure-bucket"
"bucket-in-path"
  • updated the kots pull command to read from an already written kots secret on the local filesystem, expanding it to read the new fields in addition to secretkey and accesskey that were already being read. This function and its caller have been updated to return an error in the case that the loaded object store config does not pass validation. I don't see this as liability although I could see an argument for having a flag to swallow this error. I'd rather not add that, since the kots pull functionality is not in heavy use and folks who have modified admin_console in upstream should be coached to use kustomize patches instead.

  • updated the kotsadm and kotsadm-api deployment to load values from these secrets. Skipped any sort of "re-deploy when the secret changes" behavior since we don't do that today.

  • only deploy minio if ObjectStoreType is "internal"

  • move generation of default minio S3 AccessKey/Secrets with uuid.New() to the hydration/validation step

Kotsadm (go) changes

  • add debug logging to s3 health check
  • add exponential backoff to s3 bucket startup check in kotsadm
  • add support for a region env var

Kotsadm API (node) changes

  • add an env var to disable bucket creation
  • add support for a region env var
  • tweak the health check in /healthz to properly respect both AWS S3 and Minio errors

Other small Changes

  • add logging to dependency checks in kotsadm and kotsadm-api
  • add LOG_LEVEL env var to kotsadm-api default YAML objects created byt he kots CLI to help document how to change the log level

Open Questions

Right now, kots install will not patch any existing secret, which means that while install may in the past have worked on subsequent runs, running install on an older version of kotsadm with different object store flags may now fail. We might want to consider blocking install if kotsadm is already deployed, and prompt a user to upgrade instead.

dexhorthy avatar May 23 '20 19:05 dexhorthy

@marccampbell I'm resolving merge conflicts and I'm a little concerned that there's a lot of overlap with this change https://github.com/replicatedhq/kots/pull/610/files -- is there a design doc for the OCI registry stuff? These feel to be fairly in conflict as far as design goes and I think we might need a redesign to reconcile.

I primarily want to understand what you expect the validation rules to be when different combinations of

--deploy-minio --deploy-dockerdistribution --object-store-type=internal --object-store-type=external

We could remove --object-store-type= entirely and then just read in

--object-store-access-key
--object-store-secret-key
--object-store-bucket

and other flags, but I feel like we're missing an opportunity to do validation and help folks avoid misconfigurations.

I don't really feel comfortable proceeding here without a design that

  • tackles all four scenarios (2 external, 2 internal)
  • has explicit validation rules for each scenario

dexhorthy avatar Jun 05 '20 14:06 dexhorthy

@marccampbell @divolgin I took a stab at some consolidation logic for this, but I didn't dig deep on #610

dexhorthy avatar Jun 05 '20 15:06 dexhorthy

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Jan 15 '21 17:01 CLAassistant