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.

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

Merged
merged 23 commits into from
Apr 24, 2018
Merged

Approle local secret IDs #4427

merged 23 commits into from
Apr 24, 2018

Conversation

vishalnayak
Copy link
Member

No description provided.

@vishalnayak vishalnayak added this to the 0.10.1 milestone Apr 23, 2018
@vishalnayak vishalnayak changed the title [WIP] Approle local secret IDs Approle local secret IDs Apr 23, 2018
@@ -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: `
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Member Author

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,
Copy link
Contributor

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()
Copy link
Contributor

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{
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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"`
Copy link
Member

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.

Copy link
Member

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.

@vishalnayak vishalnayak merged commit 39b6186 into master Apr 24, 2018
@vishalnayak vishalnayak deleted the approle-local-secretid branch April 24, 2018 23:08
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants