From 4a0967200a6bfea056ed7a635f560dee0d6ecf99 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Tue, 27 Nov 2018 22:20:14 -0800 Subject: [PATCH 1/2] data/aws/vpc: Add an S3 endpoint to new VPCs 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 8a37f72b (modules/aws/bootstrap: Pull AWS bootstrap setup into a module, 2018-09-05, #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 --- data/data/aws/main.tf | 1 + data/data/aws/vpc/variables.tf | 5 +++++ data/data/aws/vpc/vpc.tf | 6 ++++++ 3 files changed, 12 insertions(+) diff --git a/data/data/aws/main.tf b/data/data/aws/main.tf index 428ad87a751..59ce2ea3f0f 100644 --- a/data/data/aws/main.tf +++ b/data/data/aws/main.tf @@ -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)}" diff --git a/data/data/aws/vpc/variables.tf b/data/data/aws/vpc/variables.tf index 73b39e627ad..cfce7db467a 100644 --- a/data/data/aws/vpc/variables.tf +++ b/data/data/aws/vpc/variables.tf @@ -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." +} diff --git a/data/data/aws/vpc/vpc.tf b/data/data/aws/vpc/vpc.tf index 5ef3c9402ee..4b30178efe5 100644 --- a/data/data/aws/vpc/vpc.tf +++ b/data/data/aws/vpc/vpc.tf @@ -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)}"] +} From cdcaeb2aa4ac0a33ad048d8294b234e4b3be2a41 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Wed, 12 Dec 2018 23:35:06 -0800 Subject: [PATCH 2/2] vendor: Bump hive to 802db5420da Pulling in openshift/hive@b7d71518 (remove VPC endpoints, 2018-12-03, openshift/hive#122). Generated with: $ sed -i s/2349f175d3e4fc6542dec79add881a59f2d7b1b8/802db5420da6a88f034fc2501081e2ab12e8463e/ Gopkg.toml $ dep ensure using: $ dep version dep: version : v0.5.0 build date : git hash : 22125cf go version : go1.10.3 go compiler : gc platform : linux/amd64 features : ImportDuringSolve=false --- Gopkg.lock | 6 +- Gopkg.toml | 2 +- .../awstagdeprovision/awstagdeprovision.go | 65 ++++++++++--------- 3 files changed, 41 insertions(+), 32 deletions(-) diff --git a/Gopkg.lock b/Gopkg.lock index 8a36f01cd8a..45378cf55d8 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -392,11 +392,11 @@ revision = "fb8b55a1072436a51b153de9acf5bf5525efcf83" [[projects]] - digest = "1:c2edaeee2250d3886f1e2223a8c6698498ecdc467a72949888f89dc1dde89045" + digest = "1:fff47662f79ac206b6c6c466afb79f0458fbdf1390f527cad9255e07f8de1369" name = "github.com/openshift/hive" packages = ["contrib/pkg/awstagdeprovision"] pruneopts = "NUT" - revision = "2349f175d3e4fc6542dec79add881a59f2d7b1b8" + revision = "802db5420da6a88f034fc2501081e2ab12e8463e" [[projects]] digest = "1:93b1d84c5fa6d1ea52f4114c37714cddd84d5b78f151b62bb101128dd51399bf" @@ -825,6 +825,8 @@ "github.com/apparentlymart/go-cidr/cidr", "github.com/awalterschulze/gographviz", "github.com/aws/aws-sdk-go/aws", + "github.com/aws/aws-sdk-go/aws/credentials", + "github.com/aws/aws-sdk-go/aws/defaults", "github.com/aws/aws-sdk-go/aws/session", "github.com/aws/aws-sdk-go/service/ec2", "github.com/coreos/ignition/config/util", diff --git a/Gopkg.toml b/Gopkg.toml index 38f956821b9..8c2604c9987 100644 --- a/Gopkg.toml +++ b/Gopkg.toml @@ -70,7 +70,7 @@ ignored = [ [[constraint]] name = "github.com/openshift/hive" - revision = "2349f175d3e4fc6542dec79add881a59f2d7b1b8" + revision = "802db5420da6a88f034fc2501081e2ab12e8463e" [[constraint]] name = "k8s.io/utils" diff --git a/vendor/github.com/openshift/hive/contrib/pkg/awstagdeprovision/awstagdeprovision.go b/vendor/github.com/openshift/hive/contrib/pkg/awstagdeprovision/awstagdeprovision.go index f99d82981dd..de80aa0e1bf 100644 --- a/vendor/github.com/openshift/hive/contrib/pkg/awstagdeprovision/awstagdeprovision.go +++ b/vendor/github.com/openshift/hive/contrib/pkg/awstagdeprovision/awstagdeprovision.go @@ -377,6 +377,34 @@ func rtHasMainAssociation(rt *ec2.RouteTable) bool { return false } +// deleteVPCEndpoints will find all VPC endpoints associated with the passed in VPC and attempt to delete them +func deleteVPCEndpoints(vpc *ec2.Vpc, ec2Client *ec2.EC2, logger log.FieldLogger) error { + describeEndpointsInput := ec2.DescribeVpcEndpointsInput{} + describeEndpointsInput.Filters = []*ec2.Filter{ + { + Name: aws.String("vpc-id"), + Values: []*string{vpc.VpcId}, + }, + } + + results, err := ec2Client.DescribeVpcEndpoints(&describeEndpointsInput) + if err != nil { + logger.Debugf("error describing VPC endpoints: %v", err) + return err + } + for _, ep := range results.VpcEndpoints { + _, err := ec2Client.DeleteVpcEndpoints(&ec2.DeleteVpcEndpointsInput{ + VpcEndpointIds: []*string{ep.VpcEndpointId}, + }) + if err != nil { + logger.Debugf("error deleting VPC endpoint: %v", err) + return err + } + logger.WithField("id", *ep.VpcEndpointId).Info("Deleted VPC endpoint") + } + return nil +} + // deleteRouteTablesWithVPC will attempt to delete all route tables associated with a given VPC func deleteRouteTablesWithVPC(vpc *ec2.Vpc, ec2Client *ec2.EC2, logger log.FieldLogger) error { var anyError error @@ -400,12 +428,6 @@ func deleteRouteTablesWithVPC(vpc *ec2.Vpc, ec2Client *ec2.EC2, logger log.Field return err } - err = deleteRoutesFromTable(rt, ec2Client, logger) - if err != nil { - logger.Debugf("error deleting routes from route table: %v", err) - return err - } - if rtHasMainAssociation(rt) { // can't delete route table with the 'Main' association // it will get cleaned up as part of deleting the VPC @@ -456,8 +478,15 @@ func deleteVPCs(awsSession *session.Session, filters AWSFilter, clusterName stri return false, nil } + // next delete any VPC endpoints associated with the VPC (they are not taggable) + err := deleteVPCEndpoints(vpc, ec2Client, logger) + if err != nil { + logger.Debugf("error deleting VPC endpoint: %v", err) + return false, nil + } + // next delete route tables associated with the VPC (not all of them are tagged) - err := deleteRouteTablesWithVPC(vpc, ec2Client, logger) + err = deleteRouteTablesWithVPC(vpc, ec2Client, logger) if err != nil { logger.Debugf("error deleting route tables: %v", err) return false, nil @@ -1017,28 +1046,6 @@ func disassociateRouteTable(rt *ec2.RouteTable, ec2Client *ec2.EC2, logger log.F return nil } -// deleteRoutesFromTable will attempt to remove all routes defined in a given route table -func deleteRoutesFromTable(rt *ec2.RouteTable, ec2Client *ec2.EC2, logger log.FieldLogger) error { - for _, route := range rt.Routes { - // can't delete the 'local' route - if route.GatewayId != nil && *route.GatewayId == "local" { - continue - } - logger.Debugf("deleting route %v from RT %v", *route.DestinationCidrBlock, *rt.RouteTableId) - _, err := ec2Client.DeleteRoute(&ec2.DeleteRouteInput{ - RouteTableId: rt.RouteTableId, - DestinationCidrBlock: route.DestinationCidrBlock, - }) - if err != nil { - logger.Debugf("error deleting route from route table: %v", err) - return err - } - - logger.Infof("Deleted route %v from route table %v", *route.DestinationCidrBlock, *rt.RouteTableId) - } - return nil -} - // deleteSubnets will attempt to delete all Subnets matching the given filter func deleteSubnets(session *session.Session, filter AWSFilter, clusterName string, logger log.FieldLogger) (bool, error) { logger.Debugf("Deleting subnets (%s)", filter)