Skip to content
This repository has been archived by the owner on Mar 16, 2024. It is now read-only.

Commit

Permalink
fix: credential secrets: avoid the race condition better (#2410) (#2431)
Browse files Browse the repository at this point in the history 
Signed-off-by: Grant Linville <[email protected]>
  • Loading branch information
@g-linville
g-linville authored Jan 18, 2024
1 parent 3cf6c10 commit 994b468
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 46 deletions.
3 changes: 2 additions & 1 deletion pkg/cli/credential_login.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ func (a *CredentialLogin) Run(cmd *cobra.Command, args []string) error {
if c, err := a.client.CreateDefault(); err == nil {
app, err := c.AppGet(cmd.Context(), args[0])
if err == nil {
return login.Secrets(cmd.Context(), c, app)
_, err = login.Secrets(cmd.Context(), c, app)
return err
} else if !(apierrors.IsNotFound(err) || apierrors.IsForbidden(err)) {
return err
}
Expand Down
37 changes: 11 additions & 26 deletions pkg/log/default_log.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@ func NewDefaultLogger(ctx context.Context, c client.Client) *DefaultLoggerImpl {
}

type DefaultLoggerImpl struct {
lock sync.Mutex
ctx context.Context
client client.Client
containerColors map[string]pterm.Color
lastLogin int64
lock sync.Mutex
ctx context.Context
client client.Client
containerColors map[string]pterm.Color
lastLoginGeneration int64
}

func (d *DefaultLoggerImpl) Errorf(format string, args ...interface{}) {
Expand All @@ -46,27 +46,12 @@ func (d *DefaultLoggerImpl) AppStatus(ready bool, msg string, app *apiv1.App) {
} else {
d.lock.Lock()
defer d.lock.Unlock()
if app.Status.AppStatus.LoginRequired && app.Status.ObservedGeneration > d.lastLogin {
// Wait until all containers in the app are defined in order to avoid a race condition.
// When deploying acorns to the SaaS, a race condition can occur where the user is prompted
// to enter the credentials for a secret multiple times, as the app changes upstream while the
// user is typing into the prompt. If we wait until all containers are defined before prompting
// the user, then the race condition is avoided.
allDefined := true
for _, c := range app.Status.AppStatus.Containers {
if !c.CommonStatus.Defined {
allDefined = false
break
}
}

if allDefined {
err := login.Secrets(d.ctx, d.client, app)
if err != nil {
go d.Errorf(err.Error())
} else {
d.lastLogin = app.Generation
}
if app.Status.AppStatus.LoginRequired && app.Status.ObservedGeneration > d.lastLoginGeneration {
updatedApp, err := login.Secrets(d.ctx, d.client, app)
if err != nil {
go d.Errorf(err.Error())
} else {
d.lastLoginGeneration = updatedApp.Generation
}
}
pterm.Println(pterm.LightYellow(msg))
Expand Down
42 changes: 23 additions & 19 deletions pkg/login/login.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,32 +14,34 @@ import (
apierror "k8s.io/apimachinery/pkg/api/errors"
)

func Secrets(ctx context.Context, c client.Client, app *apiv1.App) error {
func Secrets(ctx context.Context, c client.Client, app *apiv1.App) (*apiv1.App, error) {
for secretName, secret := range app.Status.AppSpec.Secrets {
if strings.HasPrefix(secret.Type, v1.SecretTypeCredentialPrefix) {
if err := loginSecret(ctx, c, app, secretName); err != nil {
return err
updatedApp, err := loginSecret(ctx, c, app, secretName)
if err != nil {
return nil, err
}
app = updatedApp
}
}

for _, acorn := range app.Status.AppStatus.Acorns {
if acorn.AcornName != "" {
if err := loginApp(ctx, c, acorn.AcornName); err != nil {
return err
return nil, err
}
}
}

for _, service := range app.Status.AppStatus.Services {
if service.ServiceAcornName != "" {
if err := loginApp(ctx, c, service.ServiceAcornName); err != nil {
return err
return nil, err
}
}
}

return nil
return app, nil
}

func loginApp(ctx context.Context, c client.Client, appName string) error {
Expand All @@ -49,7 +51,9 @@ func loginApp(ctx context.Context, c client.Client, appName string) error {
} else if err != nil {
return err
}
return Secrets(ctx, c, app)

_, err = Secrets(ctx, c, app)
return err
}

func secretIsOk(app *apiv1.App, secretName string) (string, bool) {
Expand Down Expand Up @@ -79,27 +83,27 @@ func printInstructions(app *apiv1.App, secretName string) error {
return nil
}

func bindSecret(ctx context.Context, c client.Client, app *apiv1.App, targetSecretName, overrideSecretName string) error {
func bindSecret(ctx context.Context, c client.Client, app *apiv1.App, targetSecretName, overrideSecretName string) (*apiv1.App, error) {
parts := append(strings.Split(app.Name, "."), targetSecretName)
appName := parts[0]
targetSecretName = strings.Join(parts[1:], ".")

_, err := c.AppUpdate(ctx, appName, &client.AppUpdateOptions{
updatedApp, err := c.AppUpdate(ctx, appName, &client.AppUpdateOptions{
Secrets: []v1.SecretBinding{
{
Secret: overrideSecretName,
Target: targetSecretName,
},
},
})
return err
return updatedApp, err
}

func createSecret(ctx context.Context, c client.Client, app *apiv1.App, secretName string) error {
func createSecret(ctx context.Context, c client.Client, app *apiv1.App, secretName string) (*apiv1.App, error) {
secretType := app.Status.AppSpec.Secrets[secretName].Type

if err := printInstructions(app, secretName); err != nil {
return err
return nil, err
}

asked := map[string]struct{}{}
Expand All @@ -113,7 +117,7 @@ func createSecret(ctx context.Context, c client.Client, app *apiv1.App, secretNa
}
value, err := prompt.Password(message)
if err != nil {
return err
return nil, err
}
if len(value) == 0 {
value = []byte(def)
Expand All @@ -133,7 +137,7 @@ func createSecret(ctx context.Context, c client.Client, app *apiv1.App, secretNa
}
value, err := prompt.Password(message)
if err != nil {
return err
return nil, err
}
if len(value) == 0 {
value = []byte(def)
Expand All @@ -143,28 +147,28 @@ func createSecret(ctx context.Context, c client.Client, app *apiv1.App, secretNa

secret, err := c.SecretCreate(ctx, secretName+"-", secretType, data)
if err != nil {
return err
return nil, err
}

return bindSecret(ctx, c, app, secretName, secret.Name)
}

func loginSecret(ctx context.Context, c client.Client, app *apiv1.App, secretName string) error {
func loginSecret(ctx context.Context, c client.Client, app *apiv1.App, secretName string) (*apiv1.App, error) {
secretType := app.Status.AppSpec.Secrets[secretName].Type
secretDisplayName := app.Name + "." + secretName

if existing, ok := secretIsOk(app, secretName); ok {
change, err := prompt.Bool(fmt.Sprintf("Credential [%s] is configured to [%s], do you want to change it",
secretDisplayName, existing), false)
if err != nil || !change {
return err
return nil, err
}
fmt.Println()
}

secrets, err := c.SecretList(ctx)
if err != nil {
return err
return nil, err
}

var (
Expand All @@ -183,7 +187,7 @@ func loginSecret(ctx context.Context, c client.Client, app *apiv1.App, secretNam
def := "Enter a new credential"
choice, err := prompt.Choice("Choose an existing credential or enter a new one", append(displayText, def), def)
if err != nil {
return err
return nil, err
}
if choice == def {
return createSecret(ctx, c, app, secretName)
Expand Down

0 comments on commit 994b468

Please sign in to comment.