Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove CodePipeline GitHub environment variable #14175

Merged
merged 10 commits into from
Jul 31, 2020
Merged

Remove CodePipeline GitHub environment variable #14175

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
48083f3
Code cleanup
gdavison Jul 13, 2020
0468506
Removes `GITHUB_TOKEN` environment variable requirement for CodePipel…
gdavison Jul 14, 2020
2b2a2d1
Updates usage example in documentation for Terraform 0.12
gdavison Jul 14, 2020
857c35d
Adds disappears test and cleans up test configurations
gdavison Jul 14, 2020
1e90b47
Updates tests for GovCloud
gdavison Jul 14, 2020
438deca
Updates `PreCheck` to try API call in acceptance test regions
gdavison Jul 15, 2020
22e5d58
Adds Version 3 Upgrade documentation
gdavison Jul 15, 2020
fa9628f
Removes hashing from GitHub `OAuthToken` and marks `configuration` se…
gdavison Jul 30, 2020
2f0704c
Revert "Removes hashing from GitHub `OAuthToken` and marks `configura…
gdavison Jul 31, 2020
c2e885f
Updates provider for multiregion tests and adds OAuthToken to CodePip…
gdavison Jul 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
128 changes: 79 additions & 49 deletions aws/resource_aws_codepipeline.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
package aws

import (
"crypto/sha256"
"encoding/hex"
"errors"
"fmt"
"log"
"os"
"strings"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/codepipeline"
Expand All @@ -15,6 +17,12 @@ import (
iamwaiter "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/iam/waiter"
)

const (
CodePipelineProviderGitHub = "GitHub"

CodePipelineGitHubActionConfigurationOAuthToken = "OAuthToken"
)

func resourceAwsCodePipeline() *schema.Resource {
return &schema.Resource{
Create: resourceAwsCodePipelineCreate,
Expand Down Expand Up @@ -101,9 +109,10 @@ func resourceAwsCodePipeline() *schema.Resource {
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"configuration": {
Type: schema.TypeMap,
Optional: true,
Elem: &schema.Schema{Type: schema.TypeString},
Type: schema.TypeMap,
Optional: true,
Elem: &schema.Schema{Type: schema.TypeString},
DiffSuppressFunc: suppressCodePipelineStageActionConfiguration,
},
"category": {
Type: schema.TypeString,
Expand Down Expand Up @@ -209,7 +218,7 @@ func resourceAwsCodePipelineCreate(d *schema.ResourceData, meta interface{}) err
resp, err = conn.CreatePipeline(params)
}
if err != nil {
return fmt.Errorf("Error creating CodePipeline: %s", err)
return fmt.Errorf("Error creating CodePipeline: %w", err)
}
if resp.Pipeline == nil {
return fmt.Errorf("Error creating CodePipeline: invalid response from AWS")
Expand Down Expand Up @@ -297,12 +306,12 @@ func flattenAwsCodePipelineArtifactStore(artifactStore *codepipeline.ArtifactSto
}

values := map[string]interface{}{}
values["type"] = *artifactStore.Type
values["location"] = *artifactStore.Location
values["type"] = aws.StringValue(artifactStore.Type)
values["location"] = aws.StringValue(artifactStore.Location)
if artifactStore.EncryptionKey != nil {
as := map[string]interface{}{
"id": *artifactStore.EncryptionKey.Id,
"type": *artifactStore.EncryptionKey.Type,
"id": aws.StringValue(artifactStore.EncryptionKey.Id),
"type": aws.StringValue(artifactStore.EncryptionKey.Type),
}
values["encryption_key"] = []interface{}{as}
}
Expand All @@ -320,10 +329,10 @@ func flattenAwsCodePipelineArtifactStores(artifactStores map[string]*codepipelin
}

func expandAwsCodePipelineStages(d *schema.ResourceData) []*codepipeline.StageDeclaration {
configs := d.Get("stage").([]interface{})
stages := d.Get("stage").([]interface{})
pipelineStages := []*codepipeline.StageDeclaration{}

for _, stage := range configs {
for _, stage := range stages {
data := stage.(map[string]interface{})
a := data["action"].([]interface{})
actions := expandAwsCodePipelineActions(a)
Expand All @@ -335,31 +344,23 @@ func expandAwsCodePipelineStages(d *schema.ResourceData) []*codepipeline.StageDe
return pipelineStages
}

func flattenAwsCodePipelineStages(stages []*codepipeline.StageDeclaration) []interface{} {
func flattenAwsCodePipelineStages(stages []*codepipeline.StageDeclaration, d *schema.ResourceData) []interface{} {
stagesList := []interface{}{}
for _, stage := range stages {
for si, stage := range stages {
values := map[string]interface{}{}
values["name"] = *stage.Name
values["action"] = flattenAwsCodePipelineStageActions(stage.Actions)
values["name"] = aws.StringValue(stage.Name)
values["action"] = flattenAwsCodePipelineStageActions(si, stage.Actions, d)
stagesList = append(stagesList, values)
}
return stagesList

}

func expandAwsCodePipelineActions(s []interface{}) []*codepipeline.ActionDeclaration {
func expandAwsCodePipelineActions(a []interface{}) []*codepipeline.ActionDeclaration {
actions := []*codepipeline.ActionDeclaration{}
for _, config := range s {
for _, config := range a {
data := config.(map[string]interface{})

conf := expandAwsCodePipelineStageActionConfiguration(data["configuration"].(map[string]interface{}))
if data["provider"].(string) == "GitHub" {
githubToken := os.Getenv("GITHUB_TOKEN")
gdavison marked this conversation as resolved.
Show resolved Hide resolved
if githubToken != "" {
conf["OAuthToken"] = aws.String(githubToken)
}

}

action := codepipeline.ActionDeclaration{
ActionTypeId: &codepipeline.ActionTypeId{
Expand Down Expand Up @@ -406,23 +407,29 @@ func expandAwsCodePipelineActions(s []interface{}) []*codepipeline.ActionDeclara
return actions
}

func flattenAwsCodePipelineStageActions(actions []*codepipeline.ActionDeclaration) []interface{} {
func flattenAwsCodePipelineStageActions(si int, actions []*codepipeline.ActionDeclaration, d *schema.ResourceData) []interface{} {
actionsList := []interface{}{}
for _, action := range actions {
for ai, action := range actions {
values := map[string]interface{}{
"category": *action.ActionTypeId.Category,
"owner": *action.ActionTypeId.Owner,
"provider": *action.ActionTypeId.Provider,
"version": *action.ActionTypeId.Version,
"name": *action.Name,
"category": aws.StringValue(action.ActionTypeId.Category),
"owner": aws.StringValue(action.ActionTypeId.Owner),
"provider": aws.StringValue(action.ActionTypeId.Provider),
"version": aws.StringValue(action.ActionTypeId.Version),
"name": aws.StringValue(action.Name),
}
if action.Configuration != nil {
config := flattenAwsCodePipelineStageActionConfiguration(action.Configuration)
_, ok := config["OAuthToken"]
actionProvider := *action.ActionTypeId.Provider
if ok && actionProvider == "GitHub" {
delete(config, "OAuthToken")

actionProvider := aws.StringValue(action.ActionTypeId.Provider)
if actionProvider == CodePipelineProviderGitHub {
if _, ok := config[CodePipelineGitHubActionConfigurationOAuthToken]; ok {
// The AWS API returns "****" for the OAuthToken value. Pull the value from the configuration.
addr := fmt.Sprintf("stage.%d.action.%d.configuration.OAuthToken", si, ai)
hash := hashCodePipelineGitHubToken(d.Get(addr).(string))
config[CodePipelineGitHubActionConfigurationOAuthToken] = hash
}
}

values["configuration"] = config
}

Expand All @@ -435,15 +442,15 @@ func flattenAwsCodePipelineStageActions(actions []*codepipeline.ActionDeclaratio
}

if action.RoleArn != nil {
values["role_arn"] = *action.RoleArn
values["role_arn"] = aws.StringValue(action.RoleArn)
}

if action.RunOrder != nil {
values["run_order"] = int(*action.RunOrder)
values["run_order"] = int(aws.Int64Value(action.RunOrder))
}

if action.Region != nil {
values["region"] = *action.Region
values["region"] = aws.StringValue(action.Region)
}

if action.Namespace != nil {
Expand Down Expand Up @@ -523,13 +530,13 @@ func resourceAwsCodePipelineRead(d *schema.ResourceData, meta interface{}) error
})

if isAWSErr(err, codepipeline.ErrCodePipelineNotFoundException, "") {
log.Printf("[WARN] Codepipeline (%s) not found, removing from state", d.Id())
log.Printf("[WARN] CodePipeline (%s) not found, removing from state", d.Id())
d.SetId("")
return nil
}

if err != nil {
return fmt.Errorf("error reading Codepipeline: %s", err)
return fmt.Errorf("error reading CodePipeline: %w", err)
}

metadata := resp.Metadata
Expand All @@ -545,7 +552,7 @@ func resourceAwsCodePipelineRead(d *schema.ResourceData, meta interface{}) error
}
}

if err := d.Set("stage", flattenAwsCodePipelineStages(pipeline.Stages)); err != nil {
if err := d.Set("stage", flattenAwsCodePipelineStages(pipeline.Stages, d)); err != nil {
return err
}

Expand All @@ -557,11 +564,11 @@ func resourceAwsCodePipelineRead(d *schema.ResourceData, meta interface{}) error
tags, err := keyvaluetags.CodepipelineListTags(conn, arn)

if err != nil {
return fmt.Errorf("error listing tags for Codepipeline (%s): %s", arn, err)
return fmt.Errorf("error listing tags for CodePipeline (%s): %w", arn, err)
}

if err := d.Set("tags", tags.IgnoreAws().IgnoreConfig(ignoreTagsConfig).Map()); err != nil {
return fmt.Errorf("error setting tags: %s", err)
return fmt.Errorf("error setting tags for CodePipeline (%s): %w", arn, err)
}

return nil
Expand All @@ -580,17 +587,15 @@ func resourceAwsCodePipelineUpdate(d *schema.ResourceData, meta interface{}) err
_, err = conn.UpdatePipeline(params)

if err != nil {
return fmt.Errorf(
"[ERROR] Error updating CodePipeline (%s): %s",
d.Id(), err)
return fmt.Errorf("[ERROR] Error updating CodePipeline (%s): %w", d.Id(), err)
}

arn := d.Get("arn").(string)
if d.HasChange("tags") {
o, n := d.GetChange("tags")

if err := keyvaluetags.CodepipelineUpdateTags(conn, arn, o, n); err != nil {
return fmt.Errorf("error updating Codepipeline (%s) tags: %s", arn, err)
return fmt.Errorf("error updating CodePipeline (%s) tags: %w", arn, err)
}
}

Expand All @@ -609,8 +614,33 @@ func resourceAwsCodePipelineDelete(d *schema.ResourceData, meta interface{}) err
}

if err != nil {
return fmt.Errorf("error deleting Codepipeline (%s): %s", d.Id(), err)
return fmt.Errorf("error deleting CodePipeline (%s): %w", d.Id(), err)
}

return err
}

func suppressCodePipelineStageActionConfiguration(k, old, new string, d *schema.ResourceData) bool {
parts := strings.Split(k, ".")
parts = parts[:len(parts)-2]
providerAddr := strings.Join(append(parts, "provider"), ".")
provider := d.Get(providerAddr).(string)

if provider == CodePipelineProviderGitHub && strings.HasSuffix(k, CodePipelineGitHubActionConfigurationOAuthToken) {
hash := hashCodePipelineGitHubToken(new)
return old == hash
}

return false
}

const codePipelineGitHubTokenHashPrefix = "hash-"

func hashCodePipelineGitHubToken(token string) string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are actively in the process of removing the few other instances of hashed state storage, however Terraform in general doesn't handle TypeMap and Sensitive: true in a meaningful way without making the entire map sensitive, if I remember correctly. 😖

It would be a bummer to introduce this technical debt in, but otherwise we could also be forced down the road of what I think you were suggesting the other day of potentially going against the API design slightly and implementing bespoke Terraform schema so we can control it better (and in this situation, actually implement a single sensitive attribute). We do have some slight precedence of this class of user experience enhancement with resources like aws_sns_platform_application and I believe aws_sns_topic.

I think this leaves us in a tough spot. 🙁 We can try to quickly design something better for this situation to squeeze it in for this major release in the next two weeks, which is fairly risky in my opinion, or punt on this for now and spend later cycles on this issue in general (potentially changing the Core/SDK sensitive map abilities and/or ensuring we get good feedback on a new resource design). What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the environment variable is a huge pain-point for practitioners, and waiting until 4.0 to remove it would be less than ideal. If we don't want to hash the value, would setting the TypeMap to Sensitive be a better option at this point? And then we could come up with a better solution.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our potential path forward will be:

  • Removing the state hashing handling
  • Marking the configuration attribute Sensitive: true and Computed: true
  • Adding a new github/github_configuration configuration block so we can properly show a difference with only the oauth attribute sensitive

These will require apply-time validation to ensure they aren't both defined since ConflictsWith does not support relative references. The other configuration blocks will need to be filled in later due to time. The documentation and upgrade guide should recommend using the new configuration block(s) over the map.

// Without this check, the value was getting encoded twice
if strings.HasPrefix(token, codePipelineGitHubTokenHashPrefix) {
return token
}
sum := sha256.Sum256([]byte(token))
return codePipelineGitHubTokenHashPrefix + hex.EncodeToString(sum[:])
}
Loading