Skip to content
This repository has been archived by the owner on Jun 24, 2020. It is now read-only.

Remove the newly generated key if sc add-gcp-broker command fails #133

Merged
merged 1 commit into from
Feb 12, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
28 changes: 27 additions & 1 deletion installer/pkg/cmd/gcp_broker.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"crypto/md5"
"encoding/base32"
"encoding/base64"
"encoding/json"
"fmt"
"io/ioutil"
Expand Down Expand Up @@ -116,6 +117,8 @@ func addGCPBroker() error {

vb, err := getOrCreateVirtualBroker(projectID, "default", "Default Broker")
if err != nil {
// Clean up the newly generated key if the command failed.
cleanupNewKey(brokerSAEmail, key)
return fmt.Errorf("error retrieving or creating default broker : %v", err)
}

Expand All @@ -126,11 +129,15 @@ func addGCPBroker() error {

err = generateGCPBrokerConfigs(dir, data)
if err != nil {
// Clean up the newly generated key if the command failed.
cleanupNewKey(brokerSAEmail, key)
return fmt.Errorf("error generating configs for GCP :: %v", err)
}

err = deployGCPBrokerConfigs(dir)
if err != nil {
// Clean up the newly generated key if the command failed.
cleanupNewKey(brokerSAEmail, key)
return fmt.Errorf("error deploying GCP broker configs :%v", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Group as a helper function, and then do something like:

if err := foo(a, b, c, ..); err != nil {
  cleanupNewKey(brokerSAEmail, key)
  return ...
}

Copy link
Contributor Author

@maqiuyujoyce maqiuyujoyce Feb 11, 2018

Choose a reason for hiding this comment

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

Do you mean to group getOrCreateVirtualBroker(), generateGCPBrokerConfigs(), and deployGCPBrokerConfigs() together?

In that case, I'd prefer to return the key to the NewXXXCommand method and let it handle the cleanup when the key is not empty.

Copy link
Contributor

Choose a reason for hiding this comment

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

Calling cleanupNewKey in multiple places makes further change prone to introduce a bug. Some simple option of avoiding it is creating one helper function where you put all the cases that should call cleanupNewKey(). If you have other way to achieve it, please feel free to try that out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will keep as is and clean up/refactor the entire tool when we have time. Tracked in #149.


Expand Down Expand Up @@ -280,6 +287,25 @@ func httpAdapterFromAuthKey(keyFile string) (*adapter.HttpAdapter, error) {
return adapter.NewHttpAdapter(client), nil
}

// cleanupNewKey removes the newly generated service account key.
func cleanupNewKey(email, key string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

s/the newly generated/a given/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one is more than cleanup the "given key". It is to cleanup the given keyfile content in base64 encoding. I'll update the comment to be more specific.

keyBytes, err := base64.StdEncoding.DecodeString(key)
if err != nil {
// Silently return if there is an error decoding the newly generated key.
return
}

keyJson := make(map[string]interface{})
err = json.Unmarshal(keyBytes, &keyJson)
if err != nil {
// Silently return if there is an error unmarshalling the key.
return
}

keyID := keyJson["private_key_id"].(string)
Copy link
Contributor

Choose a reason for hiding this comment

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

a chance of panic due to ".(string)" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tracked in #149.

gcp.RemoveServiceAccountKey(email, keyID)
}

func NewRemoveGCPBrokerCmd() *cobra.Command {
return &cobra.Command{
Use: "remove-gcp-broker",
Expand Down Expand Up @@ -348,7 +374,7 @@ func removeGCPBroker() error {
}

// Clean up all the associated keys.
err = gcp.RemoveServiceAccountKeys(brokerSAEmail)
err = gcp.RemoveAllServiceAccountKeys(brokerSAEmail)
if err != nil {
return err
}
Expand Down
21 changes: 14 additions & 7 deletions installer/pkg/gcp/broker.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"encoding/json"
"fmt"
"os/exec"
"strconv"
"strings"
"time"
)
Expand Down Expand Up @@ -153,7 +154,8 @@ type saKey struct {
ValidBeforeTime string `json:"validBeforeTime"`
}

func RemoveServiceAccountKeys(email string) error {
// RemoveAllServiceAccountKeys removes all the keys associated with the service account.
func RemoveAllServiceAccountKeys(email string) error {
cmd := exec.Command("gcloud", "iam", "service-accounts", "keys", "list", "--iam-account", email, "--format=json")
out, err := cmd.CombinedOutput()
if err != nil {
Expand Down Expand Up @@ -182,18 +184,23 @@ func RemoveServiceAccountKeys(email string) error {

life := et.Sub(bt)
if life > 365*24*time.Hour {
cmd := exec.Command("gcloud", "iam", "service-accounts", "keys", "delete", k.Name, "--iam-account", email, "--quiet" /*disable interactive mode*/)
out, err := cmd.CombinedOutput()
if err != nil {
fmt.Printf("failed to delete service account key: %s : %v\n", string(out), err)
fmt.Printf("WARNING: Please clean up the key from service account %s", email)
}
RemoveServiceAccountKey(email, k.Name)
}
}

return nil
}

// RemoveServiceAccountKey removes the given key from the service account.
func RemoveServiceAccountKey(email, keyID string) {
cmd := exec.Command("gcloud", "iam", "service-accounts", "keys", "delete", keyID, "--iam-account", email, "--quiet" /*disable interactive mode*/)
out, err := cmd.CombinedOutput()
if err != nil {
fmt.Printf("failed to delete service account key: %s : %v\n", string(out), err)
fmt.Printf("WARNING: Please clean up the key from service account %s", email)
Copy link
Contributor

Choose a reason for hiding this comment

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

"Error: failed to delete ..."
"Warning: please ..." ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current error message is actually "freeform" and based on the experience of each developer. We need to do a bit design and clean it up. Tracked by #150.

}
}

// GetConfigValue returns a property value from given section of gcloud's
// default config.
func GetConfigValue(section, property string) (string, error) {
Expand Down