-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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.
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
Approle local secret IDs #4427
Approle local secret IDs #4427
Conversation
7db0def
to
4ee66b5
Compare
@@ -163,6 +167,12 @@ TTL will be set to the value of this parameter.`, | |||
Type: framework.TypeString, | |||
Description: "Identifier of the role. Defaults to a UUID.", | |||
}, | |||
"enable_local_secret_ids": &framework.FieldSchema{ | |||
Type: framework.TypeBool, | |||
Description: ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the newline here intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. I did it just so I could linewrap-format the description properly using gq
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed it though as it was inconsistent with other descriptions. 👍
} | ||
for _, secretIDHMAC := range secretIDHMACs { | ||
// In order to avoid lock swroleing in case there is need to delete, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: swroleing
.
entryIndex := fmt.Sprintf("%s%s%s", secretIDPrefixToUse, roleNameHMAC, secretIDHMAC) | ||
secretIDEntry, err := s.Get(ctx, entryIndex) | ||
if err != nil { | ||
lock.Unlock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative way to deal with unlocking would be to put the interior of the loop into its own function, and then to defer unlocking within that method.
@@ -163,6 +167,11 @@ TTL will be set to the value of this parameter.`, | |||
Type: framework.TypeString, | |||
Description: "Identifier of the role. Defaults to a UUID.", | |||
}, | |||
"enable_local_secret_ids": &framework.FieldSchema{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this just be local_secret_ids
instead of with enable
in front?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was local_secret_ids
before. I changed it to better indicate that its a boolean and not expecting any secret IDs to be supplied. I don't have a strong preference to have enable_
in the front. Let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed back to local_secret_ids
.
|
||
// SecretIDPrefix is the storage prefix for persisting secret IDs. This | ||
// differs based on whether the secret IDs are cluster local or not. | ||
SecretIDPrefix string `json:"secret_id_prefix" mapstructure:"secret_id_prefix" structs:"secret_id_prefix"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like this should be a bool, or an iota, rather than storing the prefix over and over.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see why you did this.
No description provided.