Merge pull request #357 from mumoshu/fix-s3-inconsistent-object-prefix

Fix the inconsistent S3 object prefix issue

cfnstack/assets.go

@@ -2,33 +2,47 @@ package cfnstack
 
 import (
 	"fmt"
-	"regexp"
+	"path/filepath"
+	"strings"
 )
 
 type Assets interface {
 	Merge(Assets) Assets
-	AsMap() map[assetID]Asset
-	FindAssetByStackAndFileName(string, string) Asset
+	AsMap() map[AssetID]Asset
+	FindAssetByStackAndFileName(string, string) (Asset, error)
 }
 
 type assetsImpl struct {
-	underlying map[assetID]Asset
+	underlying map[AssetID]Asset
 }
 
-type assetID struct {
-	StackName string
-	Filename  string
+type AssetID interface {
+	StackName() string
+	Filename() string
 }
 
-func NewAssetID(stack string, file string) assetID {
-	return assetID{
-		StackName: stack,
-		Filename:  file,
+type assetIDImpl struct {
+	stackName string
+	filename  string
+}
+
+func (i assetIDImpl) StackName() string {
+	return i.stackName
+}
+
+func (i assetIDImpl) Filename() string {
+	return i.filename
+}
+
+func NewAssetID(stack string, file string) AssetID {
+	return assetIDImpl{
+		stackName: stack,
+		filename:  file,
 	}
 }
 
 func (a assetsImpl) Merge(other Assets) Assets {
-	merged := map[assetID]Asset{}
+	merged := map[AssetID]Asset{}
 
 	for k, v := range a.underlying {
 		merged[k] = v
@@ -42,19 +56,19 @@ func (a assetsImpl) Merge(other Assets) Assets {
 	}
 }
 
-func (a assetsImpl) AsMap() map[assetID]Asset {
+func (a assetsImpl) AsMap() map[AssetID]Asset {
 	return a.underlying
 }
 
-func (a assetsImpl) findAssetByID(id assetID) Asset {
+func (a assetsImpl) findAssetByID(id AssetID) (Asset, error) {
 	asset, ok := a.underlying[id]
 	if !ok {
-		panic(fmt.Sprintf("[bug] failed to get the asset for the id \"%s\"", id))
+		return asset, fmt.Errorf("[bug] failed to get the asset for the id \"%s\"", id)
 	}
-	return asset
+	return asset, nil
 }
 
-func (a assetsImpl) FindAssetByStackAndFileName(stack string, file string) Asset {
+func (a assetsImpl) FindAssetByStackAndFileName(stack string, file string) (Asset, error) {
 	return a.findAssetByID(NewAssetID(stack, file))
 }
 
@@ -65,7 +79,7 @@ type AssetsBuilder interface {
 
 type assetsBuilderImpl struct {
 	locProvider AssetLocationProvider
-	assets      map[assetID]Asset
+	assets      map[AssetID]Asset
 }
 
 func (b *assetsBuilderImpl) Add(filename string, content string) AssetsBuilder {
@@ -92,7 +106,7 @@ func NewAssetsBuilder(stackName string, s3URI string) AssetsBuilder {
 			s3URI:     s3URI,
 			stackName: stackName,
 		},
-		assets: map[assetID]Asset{},
+		assets: map[AssetID]Asset{},
 	}
 }
 
@@ -107,11 +121,14 @@ type AssetLocationProvider struct {
 }
 
 type AssetLocation struct {
-	ID     assetID
+	ID     AssetID
 	Key    string
 	Bucket string
 	Path   string
-	URL    string
+}
+
+func (l AssetLocation) URL() string {
+	return fmt.Sprintf("https://s3.amazonaws.com/%s/%s", l.Bucket, l.Key)
 }
 
 func newAssetLocationProvider(stackName string, s3URI string) AssetLocationProvider {
@@ -124,38 +141,28 @@ func newAssetLocationProvider(stackName string, s3URI string) AssetLocationProvi
 func (p AssetLocationProvider) locationFor(filename string) (*AssetLocation, error) {
 	s3URI := p.s3URI
 
-	re := regexp.MustCompile("s3://(?P<bucket>[^/]+)/(?P<directory>.+[^/])/*$")
-	matches := re.FindStringSubmatch(s3URI)
-
-	path := fmt.Sprintf("%s/%s", p.stackName, filename)
-
-	var bucket string
-	var key string
-	if len(matches) == 3 {
-		bucket = matches[1]
-		directory := matches[2]
+	uri, err := S3URIFromString(s3URI)
 
-		key = fmt.Sprintf("%s/%s", directory, path)
-	} else {
-		re := regexp.MustCompile("s3://(?P<bucket>[^/]+)/*$")
-		matches := re.FindStringSubmatch(s3URI)
+	if err != nil {
+		return nil, fmt.Errorf("failed to determin location for %s: %v", filename, err)
+	}
 
-		if len(matches) == 2 {
-			bucket = matches[1]
-			key = path
-		} else {
-			return nil, fmt.Errorf("failed to parse s3 uri(=%s): The valid uri pattern for it is s3://mybucket/mydir or s3://mybucket", s3URI)
-		}
+	relativePathComponents := []string{
+		p.stackName,
+		filename,
 	}
 
-	url := fmt.Sprintf("https://s3.amazonaws.com/%s/%s", bucket, key)
-	id := assetID{StackName: p.stackName, Filename: filename}
+	key := strings.Join(
+		append(uri.PathComponents(), relativePathComponents...),
+		"/",
+	)
+
+	id := NewAssetID(p.stackName, filename)
 
 	return &AssetLocation{
 		ID:     id,
 		Key:    key,
-		Bucket: bucket,
-		Path:   path,
-		URL:    url,
+		Bucket: uri.Bucket(),
+		Path:   filepath.Join(relativePathComponents...),
 	}, nil
 }

cfnstack/provisioner.go

@@ -53,7 +53,7 @@ func (c *Provisioner) uploadFile(s3Svc S3ObjectPutterService, content string, fi
 		return "", err
 	}
 
-	return loc.URL, nil
+	return loc.URL(), nil
 }
 
 func (c *Provisioner) uploadAsset(s3Svc S3ObjectPutterService, asset Asset) error {

cfnstack/s3_uri.go

@@ -0,0 +1,53 @@
+package cfnstack
+
+import (
+	"fmt"
+	"regexp"
+)
+
+type S3URI interface {
+	Bucket() string
+	PathComponents() []string
+}
+
+type s3URIImpl struct {
+	bucket    string
+	directory string
+}
+
+func (u s3URIImpl) Bucket() string {
+	return u.bucket
+}
+
+func (u s3URIImpl) PathComponents() []string {
+	if u.directory != "" {
+		return []string{
+			u.directory,
+		}
+	}
+	return []string{}
+}
+
+func S3URIFromString(s3URI string) (S3URI, error) {
+	re := regexp.MustCompile("s3://(?P<bucket>[^/]+)/(?P<directory>.+[^/])/*$")
+	matches := re.FindStringSubmatch(s3URI)
+	var bucket string
+	var directory string
+	if len(matches) == 3 {
+		bucket = matches[1]
+		directory = matches[2]
+	} else {
+		re := regexp.MustCompile("s3://(?P<bucket>[^/]+)/*$")
+		matches := re.FindStringSubmatch(s3URI)
+
+		if len(matches) == 2 {
+			bucket = matches[1]
+		} else {
+			return nil, fmt.Errorf("failed to parse s3 uri(=%s): The valid uri pattern for it is s3://mybucket/mydir or s3://mybucket", s3URI)
+		}
+	}
+	return s3URIImpl{
+		bucket:    bucket,
+		directory: directory,
+	}, nil
+}

core/controlplane/cluster/cluster.go

@@ -148,7 +148,12 @@ func (c *Cluster) TemplateURL() (string, error) {
 	if err != nil {
 		return "", err
 	}
-	return assets.FindAssetByStackAndFileName(c.StackName(), STACK_TEMPLATE_FILENAME).URL, nil
+	asset, err := assets.FindAssetByStackAndFileName(c.StackName(), STACK_TEMPLATE_FILENAME)
+	if err != nil {
+		return "", fmt.Errorf("failed to get template URL: %v", err)
+	}
+
+	return asset.URL(), nil
 }
 
 func (c *Cluster) ValidateStack() (string, error) {

core/nodepool/cluster/cluster.go

@@ -96,12 +96,16 @@ func (c *Cluster) Assets() (cfnstack.Assets, error) {
 		Build(), nil
 }
 
-func (c *Cluster) TemplateURL() string {
+func (c *Cluster) TemplateURL() (string, error) {
 	assets, err := c.Assets()
 	if err != nil {
-		panic(err)
+		return "", fmt.Errorf("failed to get template url: %v", err)
 	}
-	return assets.FindAssetByStackAndFileName(c.StackName(), STACK_TEMPLATE_FILENAME).URL
+	asset, err := assets.FindAssetByStackAndFileName(c.StackName(), STACK_TEMPLATE_FILENAME)
+	if err != nil {
+		return "", fmt.Errorf("failed to get template url: %v", err)
+	}
+	return asset.URL(), nil
 }
 
 func (c *Cluster) stackProvisioner() *cfnstack.Provisioner {

core/root/cluster.go

@@ -26,7 +26,7 @@ const (
 )
 
 func (c clusterImpl) Export() error {
-	assets, err := c.assets()
+	assets, err := c.Assets()
 
 	if err != nil {
 		return err
@@ -94,6 +94,7 @@ func (c clusterImpl) EstimateCost() ([]string, error) {
 }
 
 type Cluster interface {
+	Assets() (cfnstack.Assets, error)
 	Create() error
 	Export() error
 	EstimateCost() ([]string, error)
@@ -186,7 +187,7 @@ func (c clusterImpl) Info() (*Info, error) {
 }
 
 func (c clusterImpl) prepareTemplateWithAssets() (string, error) {
-	assets, err := c.assets()
+	assets, err := c.Assets()
 
 	if err != nil {
 		return "", err
@@ -198,17 +199,28 @@ func (c clusterImpl) prepareTemplateWithAssets() (string, error) {
 		return "", err
 	}
 
-	url := assets.FindAssetByStackAndFileName(c.stackName(), REMOTE_STACK_TEMPLATE_FILENAME).URL
+	asset, err := assets.FindAssetByStackAndFileName(c.stackName(), REMOTE_STACK_TEMPLATE_FILENAME)
+
+	if err != nil {
+		return "", fmt.Errorf("failed to prepare template with assets: %v", err)
+	}
+
+	url := asset.URL()
 
 	return url, nil
 }
 
-func (c clusterImpl) assets() (cfnstack.Assets, error) {
+func (c clusterImpl) Assets() (cfnstack.Assets, error) {
 	stackTemplate, err := c.renderTemplateAsString()
 	if err != nil {
 		return nil, fmt.Errorf("Error while rendering template : %v", err)
 	}
-	assets := cfnstack.NewAssetsBuilder(c.stackName(), c.opts.S3URI).Add(REMOTE_STACK_TEMPLATE_FILENAME, stackTemplate).Build()
+	s3URI := fmt.Sprintf("%s/kube-aws/clusters/%s/exported/stacks",
+		strings.TrimSuffix(c.opts.S3URI, "/"),
+		c.controlPlane.ClusterName,
+	)
+
+	assets := cfnstack.NewAssetsBuilder(c.stackName(), s3URI).Add(REMOTE_STACK_TEMPLATE_FILENAME, stackTemplate).Build()
 
 	cpAssets, err := c.controlPlane.Assets()
 	if err != nil {

core/root/template_params.go

@@ -66,10 +66,10 @@ func (p nodePool) Tags() map[string]string {
 }
 
 func (p nodePool) TemplateURL() (string, error) {
-	u := p.nodePool.TemplateURL()
+	u, err := p.nodePool.TemplateURL()
 
-	if u == "" {
-		return "", fmt.Errorf("failed to get TemplateURL: %+v", *p.nodePool)
+	if err != nil || u == "" {
+		return "", fmt.Errorf("failed to get template url: %v", err)
 	}
 
 	return u, nil

test/integration/aws_test.go

@@ -23,13 +23,6 @@ func (r testEnv) get(name string) string {
 	return v
 }
 
-const minimalMainClusterYaml = `clusterName: test-cluster-name
-externalDNSName: test.staging.core-os.net
-keyName: test-key-name
-kmsKeyArn: "arn:aws:kms:us-west-1:xxxxxxxxx:key/xxxxxxxxxxxxxxxxxxx"
-region: us-west-1
-`
-
 func useRealAWS() bool {
 	return os.Getenv("KUBE_AWS_INTEGRATION_TEST") != ""
 }
@@ -47,8 +40,14 @@ type kubeAwsSettings struct {
 func newKubeAwsSettingsFromEnv(t *testing.T) kubeAwsSettings {
 	env := testEnv{t: t}
 
+	clusterName, clusterNameExists := os.LookupEnv("KUBE_AWS_CLUSTER_NAME")
+
+	if !clusterNameExists || clusterName == "" {
+		clusterName = "kubeaws-it"
+		t.Logf(`Falling back clusterName to a stub value "%s" for tests of validating stack templates. No assets will actually be uploaded to S3 and no clusters will be created with CloudFormation`, clusterName)
+	}
+
 	if useRealAWS() {
-		clusterName := env.get("KUBE_AWS_CLUSTER_NAME")
 		externalDnsName := fmt.Sprintf("%s.%s", clusterName, env.get("KUBE_AWS_DOMAIN"))
 		keyName := env.get("KUBE_AWS_KEY_NAME")
 		kmsKeyArn := env.get("KUBE_AWS_KMS_KEY_ARN")
@@ -75,8 +74,14 @@ region: "%s"
 		}
 	} else {
 		return kubeAwsSettings{
-			mainClusterYaml: minimalMainClusterYaml,
-			encryptService:  helper.DummyEncryptService{},
+			clusterName: clusterName,
+			mainClusterYaml: fmt.Sprintf(`clusterName: %s
+externalDNSName: test.staging.core-os.net
+keyName: test-key-name
+kmsKeyArn: "arn:aws:kms:us-west-1:xxxxxxxxx:key/xxxxxxxxxxxxxxxxxxx"
+region: us-west-1
+`, clusterName),
+			encryptService: helper.DummyEncryptService{},
 		}
 	}
 }