Skip to content

Commit

Permalink
data/aws/vpc: Add an S3 endpoint to new VPCs
Browse files Browse the repository at this point in the history
As suggested by Stephen Cuppett, this allows registry <-> S3 transfers
to bypass the (NAT) gateways.  Traffic over the NAT gateways costs
money, so the new endpoint should make S3 access from the cluster
cheaper (and possibly more reliable).  This also allows for additional
security policy flexibility, although I'm not taking advantage of that
in this commit.  Docs for VPC endpoints are in [1,2,3,4].

Endpoints do not currently support cross-region requests [1].  And
based on discussion with Stephen, adding an endpoint may *break*
access to S3 on other regions.  But I can't find docs to back that up,
and [3] has:

  We use the most specific route that matches the traffic to determine
  how to route the traffic (longest prefix match).  If you have an
  existing route in your route table for all internet traffic
  (0.0.0.0/0) that points to an internet gateway, the endpoint route
  takes precedence for all traffic destined for the service, because
  the IP address range for the service is more specific than
  0.0.0.0/0.  All other internet traffic goes to your internet
  gateway, including traffic that's destined for the service in other
  regions.

which suggests that access to S3 on other regions may be unaffected.
In any case, our registry buckets, and likely any other buckets
associated with the cluster, will be living in the same region.

concat is documented in [5].  The wrapping brackets avoid [6]:

  level=error msg="Error: module.vpc.aws_vpc_endpoint.s3: route_table_ids: should be a list"

although I think that's a Terraform bug.  See also 8a37f72
(modules/aws/bootstrap: Pull AWS bootstrap setup into a module,
2018-09-05, openshift#217), which talks about this same issue.

[1]: https://docs.aws.amazon.com/vpc/latest/userguide/vpc-endpoints-s3.html
[2]: https://docs.aws.amazon.com/vpc/latest/userguide/vpc-endpoints.html
[3]: https://docs.aws.amazon.com/vpc/latest/userguide/vpce-gateway.html
[4]: https://www.terraform.io/docs/providers/aws/r/vpc_endpoint.html
[5]: https://www.terraform.io/docs/configuration/interpolation.html#concat-list1-list2-
[6]: https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_installer/745/pull-ci-openshift-installer-master-e2e-aws/1673/build-log.txt
  • Loading branch information
wking committed Dec 13, 2018
1 parent 61f5064 commit 4a09672
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 0 deletions.
1 change: 1 addition & 0 deletions data/data/aws/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ module "vpc" {
cluster_id = "${var.cluster_id}"
cluster_name = "${var.cluster_name}"
external_vpc_id = "${var.aws_external_vpc_id}"
region = "${var.aws_region}"

external_master_subnet_ids = "${compact(var.aws_external_master_subnet_ids)}"
external_worker_subnet_ids = "${compact(var.aws_external_worker_subnet_ids)}"
Expand Down
5 changes: 5 additions & 0 deletions data/data/aws/vpc/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -51,3 +51,8 @@ variable "public_master_endpoints" {
description = "If set to true, public-facing ingress resources are created."
default = true
}

variable "region" {
type = "string"
description = "The target AWS region for the cluster."
}
6 changes: 6 additions & 0 deletions data/data/aws/vpc/vpc.tf
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,9 @@ resource "aws_vpc" "new_vpc" {
"openshiftClusterID", "${var.cluster_id}"
), var.extra_tags)}"
}

resource "aws_vpc_endpoint" "s3" {
vpc_id = "${aws_vpc.new_vpc.id}"
service_name = "com.amazonaws.${var.region}.s3"
route_table_ids = ["${concat(aws_route_table.private_routes.*.id, aws_route_table.default.*.id)}"]
}

0 comments on commit 4a09672

Please sign in to comment.