Support S3 Virtual Hosted Style #8787

johanneswuerbach · 2020-03-23T17:06:00Z

Support S3 URLs written in Virtual Hosted Style, which seems to be only recommended notation nowadays, but isn't supported in kops.

https://docs.aws.amazon.com/AmazonS3/latest/dev/UsingBucket.html#virtual-host-style-url-ex

All the other options are more or less deprecated path style https://docs.aws.amazon.com/AmazonS3/latest/dev/VirtualHosting.html#path-style-access, old virtual styles https://docs.aws.amazon.com/AmazonS3/latest/dev/VirtualHosting.html#VirtualHostingBackwardsCompatibility

k8s-ci-robot · 2020-03-23T17:06:08Z

Hi @johanneswuerbach. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

rifelpet · 2020-03-23T17:07:48Z

/ok-to-test

johngmyers · 2020-03-26T03:58:46Z

/lgtm

johngmyers · 2020-03-26T04:01:56Z

util/pkg/vfs/s3context.go

@@ -42,7 +42,7 @@ var (
 	// https://docs.aws.amazon.com/general/latest/gr/rande.html#s3_region
 	// TODO: perhaps make region regex more specific, i.e. (us|eu|ap|cn|ca|sa), to prevent matching bucket names that match region format?
 	//       but that will mean updating this list when AWS introduces new regions
-	s3UrlRegexp = regexp.MustCompile(`s3([-.](?P<region>\w{2}-\w+-\d{1})|[-.](?P<bucket>[\w.\-\_]+)|)?.amazonaws.com(.cn)?(?P<path>.*)?`)
+	s3UrlRegexp = regexp.MustCompile(`(s3([-.](?P<region>\w{2}-\w+-\d{1})|[-.](?P<bucket>[\w.\-\_]+)|)?|(?P<bucket>[\w\-\_]+).s3.(?P<region>\w{2}-\w+-\d{1})).amazonaws.com(.cn)?(?P<path>.*)?`)


Why excluding '.' in bucket names in virtual hosted style?

Having a dot in the bucket name causes https connection to fail:

https://docs.aws.amazon.com/AmazonS3/latest/dev/BucketRestrictions.html#bucketnamingrules

When you use virtual hosted–style buckets with Secure Sockets Layer (SSL), the SSL wildcard certificate only matches buckets that don't contain periods. To work around this, use HTTP or write your own certificate verification logic. We recommend that you do not use periods (".") in bucket names when using virtual hosted–style buckets.

While mirroring assets should work (as kops uses an aws sdk s3 client), the user data script is likely to fail as kops tries to fetch assets from https://bucket

That would be a reason to avoid writing out virtual hosted style URIs for such buckets, but doesn't seem a reason to avoid recognizing such URIs.

Ah - there's actually a bug underlying this difference of opinion :-) VFSPath is used in two different places for two quite different things.

Can I suggest that we rename VFSPath to S3URLForHTTPURL or similar? (If you can find something that doesn't look like shouting in the go naming rules, that's even better!)

I started going down this little rabbit hole and it looks like we actually need a bigger refactor... I think one of the usages (storage.go) is really ParseS3Path and the other one (copyfile.go) is really MapToUploadLocation (and should probably just be inlined in that function.

I'm happy to do that refactor if you want, but I don't want to "steal" the PR from you two that have actually discovered the problem :-)

@justinsb thanks for the feedback, I did a small refactoring, but not entirely sure whether it goes in the imaging direction.

Scratch that, I reverted my attempt as I was starting to duplicate a lot of logic and in the end those functions are fairly similar.

Instead I added the . as asked for by @johngmyers and as a customer your could also specify http://my-bucket, which would still work, but is less secure obviously.

johngmyers · 2020-03-26T04:02:06Z

/lgtm cancel

johngmyers · 2020-05-03T00:45:25Z

/lgtm
Someone with repo access will need to retry the failed Travis job, or you could rebase.

geojaz · 2020-05-03T01:37:45Z

i poked travis for ya

geojaz

/lgtm
/approve
I think we can probable clean up that TODO comment in 43, but let's punt on that one. thanks!

k8s-ci-robot · 2020-05-03T01:45:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: geojaz, johanneswuerbach

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [geojaz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fejta-bot · 2020-05-03T03:46:34Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

johngmyers · 2020-05-03T05:34:39Z

Oh, that Travis failure looks like an actual Go 1.12 incompatibility.

Please rebase, as the Travis config has since changed to no longer run on Go 1.12.

johanneswuerbach · 2020-05-03T06:01:29Z

@johngmyers rebased :-)

johngmyers · 2020-05-03T06:51:20Z

/lgtm

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 23, 2020

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 23, 2020

k8s-ci-robot requested review from joshbranham and robinpercy March 23, 2020 17:06

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 23, 2020

k8s-ci-robot assigned johngmyers Mar 26, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 26, 2020

johngmyers reviewed Mar 26, 2020

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 26, 2020

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 9, 2020

johanneswuerbach force-pushed the s3-virtual-host branch 2 times, most recently from 959edb0 to 4939aea Compare April 9, 2020 15:47

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 9, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 3, 2020

geojaz approved these changes May 3, 2020

View reviewed changes

k8s-ci-robot assigned geojaz May 3, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 3, 2020

Support S3 Virtual Hosted Style

b92ef68

johanneswuerbach force-pushed the s3-virtual-host branch from 4939aea to b92ef68 Compare May 3, 2020 06:00

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 3, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 3, 2020

k8s-ci-robot merged commit c91c7cc into kubernetes:master May 3, 2020

k8s-ci-robot added this to the v1.18 milestone May 3, 2020

johanneswuerbach deleted the s3-virtual-host branch May 4, 2020 07:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support S3 Virtual Hosted Style #8787

Support S3 Virtual Hosted Style #8787

johanneswuerbach commented Mar 23, 2020 •

edited

Loading

k8s-ci-robot commented Mar 23, 2020

rifelpet commented Mar 23, 2020

johngmyers commented Mar 26, 2020

johngmyers Mar 26, 2020

johanneswuerbach Mar 26, 2020

johngmyers Mar 27, 2020

justinsb Apr 9, 2020

johanneswuerbach Apr 9, 2020

johanneswuerbach Apr 9, 2020

johngmyers commented Mar 26, 2020

johngmyers commented May 3, 2020

geojaz commented May 3, 2020

geojaz left a comment

k8s-ci-robot commented May 3, 2020

fejta-bot commented May 3, 2020

johngmyers commented May 3, 2020 •

edited

Loading

johanneswuerbach commented May 3, 2020

johngmyers commented May 3, 2020

Support S3 Virtual Hosted Style #8787

Support S3 Virtual Hosted Style #8787

Conversation

johanneswuerbach commented Mar 23, 2020 • edited Loading

k8s-ci-robot commented Mar 23, 2020

rifelpet commented Mar 23, 2020

johngmyers commented Mar 26, 2020

johngmyers Mar 26, 2020

Choose a reason for hiding this comment

johanneswuerbach Mar 26, 2020

Choose a reason for hiding this comment

johngmyers Mar 27, 2020

Choose a reason for hiding this comment

justinsb Apr 9, 2020

Choose a reason for hiding this comment

johanneswuerbach Apr 9, 2020

Choose a reason for hiding this comment

johanneswuerbach Apr 9, 2020

Choose a reason for hiding this comment

johngmyers commented Mar 26, 2020

johngmyers commented May 3, 2020

geojaz commented May 3, 2020

geojaz left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 3, 2020

fejta-bot commented May 3, 2020

johngmyers commented May 3, 2020 • edited Loading

johanneswuerbach commented May 3, 2020

johngmyers commented May 3, 2020

johanneswuerbach commented Mar 23, 2020 •

edited

Loading

johngmyers commented May 3, 2020 •

edited

Loading