fix: add warning for long secret names #359

Merged
decentral1se merged 14 commits from rix/abra:add-secret-length-linting into main 2024-04-06 21:41:37 +00:00
Showing only changes of commit 22f1ffacaa - Show all commits

View File

@ -126,7 +126,7 @@ var appNewCommand = cli.Command{
logrus.Fatal(err)
}
if err := ensureSecretLengths(secrets, internal.Domain); err != nil {
if err := ensureSecretLengths(secrets, sanitisedAppName); err != nil {
logrus.Fatal(err)
}
@ -262,20 +262,35 @@ func ensureServerFlag() error {
return nil
}
func ensureSecretLengths(secrets AppSecrets, domain string) error {
domainLength := len(domain)
func ensureSecretLengths(secrets AppSecrets, sanitisedAppName string) error {
if len(sanitisedAppName) > 45 {
decentral1se marked this conversation as resolved Outdated

Maybe it's time to abstract this 45 to a const in pkg/config/env.go? We're starting to use it in several places and this will probably lead to confusion at best and bugs at worst as things change over time.

Maybe it's time to abstract this `45` to a `const` in `pkg/config/env.go`? We're starting to use it in several places and this will probably lead to confusion at best and bugs at worst as things change over time.
Outdated
Review

Done : )

Done : )
sanitisedAppName = sanitisedAppName[:45]
}
domainLength := len(sanitisedAppName)
failingSecrets := []string{}
maxSecretLength := 0
for secretName := range secrets {
if len(secretName)+domainLength > 64 {
decentral1se marked this conversation as resolved Outdated

nitpick: drop extra newline?

nitpick: drop extra newline?
Outdated
Review

Think I've got this one now but let me know if I misinterpreted which line you were talking about : )

Think I've got this one now but let me know if I misinterpreted which line you were talking about : )
if len(secretName) > maxSecretLength {
maxSecretLength = len(secretName)
}
if len(secretName+"_v1")+domainLength > 64 {
decentral1se marked this conversation as resolved Outdated

Maybe 64 can also go in pkg/config/env.go while we're at it?

Maybe `64` can also go in `pkg/config/env.go` while we're at it?
Outdated
Review

Done

Done
failingSecrets = append(failingSecrets, secretName)
decentral1se marked this conversation as resolved Outdated

I think it would be simpler to just error out here. Also a simpler/clearer error message to write?

return fmt.Errorf("%s is too long (> 64 chars when combined with %s)", secretName, sanitisedAppName)
I think it would be simpler to just error out here. Also a simpler/clearer error message to write? ```go return fmt.Errorf("%s is too long (> 64 chars when combined with %s)", secretName, sanitisedAppName) ```
Outdated
Review

I've made it drop out after the first error now but I guess the side effect is that if there's more than one long secret it will be a bit of a whack-a-mole to find them all but I guess it's a pretty rare occurence so the trade off for cleaner code is probably worth it : )

I've made it drop out after the first error now but I guess the side effect is that if there's more than one long secret it will be a bit of a whack-a-mole to find them all but I guess it's a pretty rare occurence so the trade off for cleaner code is probably worth it : )
}
}
if len(failingSecrets) > 0 {
failedSecretsString := strings.Join(failingSecrets, ", ")
return fmt.Errorf("The following secrets are too long to work with the domain name %s, change their length or use a shorter domain name: %s", domain, failedSecretsString)
return fmt.Errorf("the following secrets are too long to work with the domain name %s, change their length or use a shorter domain name: %s", sanitisedAppName, failedSecretsString)
}
logrus.Debugf(
decentral1se marked this conversation as resolved Outdated

Unsure if this debug logging is necessary? It would also simplify the logic above and we could drop maxSecretLength and the accompanying if statement?

Unsure if this debug logging is necessary? It would also simplify the logic above and we could drop `maxSecretLength` and the accompanying `if` statement?
Outdated
Review

Removed, it helped me with debugging and I thought that maybe it might help explain the logic but you're right that's it's probably not helpful for the day to day experience.

Removed, it helped me with debugging and I thought that maybe it might help explain the logic but you're right that's it's probably not helpful for the day to day experience.
`The longest secret name is %d
including 4 extra characters for format %s_<name>_v1
fits with domain length of %d for max docker secret length of %d`,
maxSecretLength, sanitisedAppName, domainLength, domainLength+maxSecretLength+4)
return nil
}