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

performance improvements on mount fetch #2152

Merged
merged 11 commits into from
Feb 26, 2024
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ FEATURES:
* Add support for `skip_import_rotation` and `skip_static_role_import_rotation` in `ldap_secret_backend_static_role` and `ldap_secret_backend` respectively. Requires Vault 1.16+ ([#2128](https://github.com/hashicorp/terraform-provider-vault/pull/2128)).
* Improve logging to track full API exchanges between the provider and Vault ([#2139](https://github.com/hashicorp/terraform-provider-vault/pull/2139))

IMPROVEMENTS:
* Improve performance of READ operations across many resources: ([#2145](https://github.com/hashicorp/terraform-provider-vault/pull/2145)), ([#2152](https://github.com/hashicorp/terraform-provider-vault/pull/2152))

## 3.25.0 (Feb 14, 2024)

FEATURES:
Expand Down
81 changes: 81 additions & 0 deletions util/mountutil/mountutil.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
package mountutil

import (
"context"
"errors"
"fmt"
"strings"

"github.com/hashicorp/terraform-provider-vault/internal/consts"
"github.com/hashicorp/vault/api"
)

// Error strings that are returned by the Vault API.
const (
ErrVaultSecretMountNotFound = "No secret engine mount at"
ErrVaultAuthMountNotFound = "No auth engine at"
)

// Error strings that are used internally by TFVP
var (
// ErrMountNotFound is used to signal to resources that a secret or auth
// mount does not exist and should be removed from state.
ErrMountNotFound = errors.New("mount not found")
)

// GetMount will fetch the secret mount at the given path.
func GetMount(ctx context.Context, client *api.Client, path string) (*api.MountOutput, error) {
mount, err := client.Sys().GetMountWithContext(ctx, path)
// Hardcoding the error string check is not ideal, but Vault does not
// return 404 in this case
if err != nil && strings.Contains(err.Error(), ErrVaultSecretMountNotFound) || mount == nil {
return nil, fmt.Errorf("%w: %s", ErrMountNotFound, err)
}
if err != nil {
return nil, fmt.Errorf("error reading from Vault: %s", err)
}
return mount, nil
}

// GetAuthMount will fetch the auth mount at the given path.
func GetAuthMount(ctx context.Context, client *api.Client, path string) (*api.MountOutput, error) {
mount, err := client.Sys().GetAuthWithContext(ctx, path)
// Hardcoding the error string check is not ideal, but Vault does not
// return 404 in this case
if err != nil && strings.Contains(err.Error(), ErrVaultAuthMountNotFound) || mount == nil {
return nil, fmt.Errorf("%w: %s", ErrMountNotFound, err)
}
if err != nil {
return nil, fmt.Errorf("error reading from Vault: %s", err)
}
return mount, nil
}

// NormalizeMountPath to be in a form valid for accessing values from api.MountOutput
func NormalizeMountPath(path string) string {
return TrimSlashes(path) + consts.PathDelim
}

// TrimSlashes from path.
func TrimSlashes(path string) string {
return strings.Trim(path, consts.PathDelim)
}

// CheckMountEnabledWithContext in Vault
func CheckMountEnabledWithContext(ctx context.Context, client *api.Client, path string) (bool, error) {
_, err := GetMount(ctx, client, path)
if errors.Is(err, ErrMountNotFound) {
return false, err
}

if err != nil {
return false, err
}

return true, nil
}

// CheckMountEnabled in Vault
func CheckMountEnabled(client *api.Client, path string) (bool, error) {
return CheckMountEnabledWithContext(context.Background(), client, path)
}
24 changes: 0 additions & 24 deletions util/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@ import (
"github.com/cenkalti/backoff/v4"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/hashicorp/vault/api"

"github.com/hashicorp/terraform-provider-vault/internal/consts"
)

type (
Expand Down Expand Up @@ -281,28 +279,6 @@ func SetResourceData(d *schema.ResourceData, data map[string]interface{}) error
return nil
}

// NormalizeMountPath to be in a form valid for accessing values from api.MountOutput
func NormalizeMountPath(path string) string {
return TrimSlashes(path) + consts.PathDelim
}

// TrimSlashes from path.
func TrimSlashes(path string) string {
return strings.Trim(path, consts.PathDelim)
}

// CheckMountEnabled in Vault, path must contain a trailing '/',
func CheckMountEnabled(client *api.Client, path string) (bool, error) {
mounts, err := client.Sys().ListMounts()
if err != nil {
return false, err
}

_, ok := mounts[NormalizeMountPath(path)]

return ok, nil
}

// GetAPIRequestDataWithMap to pass to Vault from schema.ResourceData.
// The fieldMap specifies the schema field to its vault constituent.
// If the vault field is empty, then two fields are mapped 1:1.
Expand Down
26 changes: 0 additions & 26 deletions vault/auth_mount.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ package vault
import (
"fmt"
"log"
"strings"

"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
Expand Down Expand Up @@ -77,19 +76,6 @@ func authMountTuneSchema() *schema.Schema {
}
}

func authMountInfoGet(client *api.Client, path string) (*api.AuthMount, error) {
auths, err := client.Sys().ListAuth()
if err != nil {
return nil, fmt.Errorf("error reading from auth mounts: %s", err)
}

authMount := auths[strings.Trim(path, "/")+"/"]
if authMount == nil {
return nil, fmt.Errorf("auth mount %s not present", path)
}
return authMount, nil
}

func authMountTune(client *api.Client, path string, configured interface{}) error {
input := expandAuthMethodTune(configured.(*schema.Set).List())

Expand Down Expand Up @@ -124,15 +110,3 @@ func authMountDisable(client *api.Client, path string) error {

return nil
}

// getAuthMountIfPresent will fetch the auth mount at the given path.
func getAuthMountIfPresent(client *api.Client, path string) (*api.AuthMount, error) {
auth, err := client.Sys().GetAuth(path)
if err != nil {
return nil, fmt.Errorf("error reading from Vault: %s", err)
}
if auth.Accessor == "" {
return nil, fmt.Errorf("mount not found: %s", err)
}
return auth, nil
}
44 changes: 27 additions & 17 deletions vault/data_source_auth_backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,14 @@
package vault

import (
"context"
"fmt"
"strings"

"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"

"github.com/hashicorp/terraform-provider-vault/internal/provider"
"github.com/hashicorp/terraform-provider-vault/util/mountutil"
)

func authBackendDataSource() *schema.Resource {
Expand Down Expand Up @@ -66,29 +68,37 @@ func authBackendDataSourceRead(d *schema.ResourceData, meta interface{}) error {
return e
}

targetPath := d.Get("path").(string)
path := d.Get("path").(string)

auths, err := client.Sys().ListAuth()
auth, err := mountutil.GetAuthMount(context.Background(), client, path)
if err != nil {
return fmt.Errorf("error reading from Vault: %s", err)
}

for path, auth := range auths {
path = strings.TrimSuffix(path, "/")
if path == targetPath {
// Compatibility with resource_auth_backend id
d.SetId(path)
d.Set("type", auth.Type)
d.Set("description", auth.Description)
d.Set("accessor", auth.Accessor)
d.Set("default_lease_ttl_seconds", auth.Config.DefaultLeaseTTL)
d.Set("max_lease_ttl_seconds", auth.Config.MaxLeaseTTL)
d.Set("listing_visibility", auth.Config.ListingVisibility)
d.Set("local", auth.Local)
return nil
}
path = strings.TrimSuffix(path, "/")
d.SetId(path)

if err := d.Set("type", auth.Type); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we want the error checks for d.Set to be ignored, it seems like we can set up an exclusion: https://github.com/golangci/golangci-lint/blob/master/.golangci.reference.yml#L266-L299

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I think we should probably enforce error checking in this case just to be safe.

return err
}
if err := d.Set("description", auth.Description); err != nil {
return err
}
if err := d.Set("accessor", auth.Accessor); err != nil {
return err
}
if err := d.Set("default_lease_ttl_seconds", auth.Config.DefaultLeaseTTL); err != nil {
return err
}
if err := d.Set("max_lease_ttl_seconds", auth.Config.MaxLeaseTTL); err != nil {
return err
}
if err := d.Set("listing_visibility", auth.Config.ListingVisibility); err != nil {
return err
}
if err := d.Set("local", auth.Local); err != nil {
return err
}

// If we fell out here then we didn't find our Auth in the list.
return nil
}
20 changes: 0 additions & 20 deletions vault/okta.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ func listOktaUsers(client *api.Client, path string) ([]string, error) {

func readOktaUser(client *api.Client, path string, username string) (*oktaUser, error) {
secret, err := client.Logical().Read(oktaUserEndpoint(path, username))

if err != nil {
return nil, err
}
Expand All @@ -76,24 +75,6 @@ func deleteOktaUser(client *api.Client, path, username string) error {
return err
}

func isOktaAuthBackendPresent(client *api.Client, path string) (bool, error) {
auths, err := client.Sys().ListAuth()
if err != nil {
return false, fmt.Errorf("error reading from Vault: %s", err)
}

configuredPath := path + "/"

for authBackendPath, auth := range auths {

if auth.Type == "okta" && authBackendPath == configuredPath {
return true, nil
}
}

return false, nil
}

func isOktaGroupPresent(client *api.Client, path, name string) (bool, error) {
secret, err := client.Logical().Read(oktaGroupEndpoint(path, name))
if err != nil {
Expand Down Expand Up @@ -122,7 +103,6 @@ func listOktaGroups(client *api.Client, path string) ([]string, error) {

func readOktaGroup(client *api.Client, path string, name string) (*oktaGroup, error) {
secret, err := client.Logical().Read(oktaGroupEndpoint(path, name))

if err != nil {
return nil, err
}
Expand Down
23 changes: 13 additions & 10 deletions vault/resource_ad_secret_backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,16 @@
package vault

import (
"context"
"errors"
"fmt"
"log"
"strings"

"github.com/hashicorp/terraform-provider-vault/internal/consts"
"github.com/hashicorp/terraform-provider-vault/internal/provider"
"github.com/hashicorp/terraform-provider-vault/util"
"github.com/hashicorp/terraform-provider-vault/util/mountutil"

"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/hashicorp/vault/api"
Expand Down Expand Up @@ -362,19 +365,21 @@
path := d.Id()
log.Printf("[DEBUG] Reading %q", path)

mountResp, err := client.Sys().MountConfig(path)
if err != nil && util.Is404(err) {
log.Printf("[WARN] %q not found, removing from state", path)
mount, err := mountutil.GetMount(context.Background(), client, path)
if errors.Is(err, mountutil.ErrMountNotFound) {
log.Printf("[WARN] Mount %q not found, removing from state.", path)
d.SetId("")
return nil
} else if err != nil {
return fmt.Errorf("error reading %q: %s", path, err)
}

if err != nil {
return err
}

d.Set("backend", d.Id())

d.Set("default_lease_ttl_seconds", mountResp.DefaultLeaseTTL)
d.Set("max_lease_ttl_seconds", mountResp.MaxLeaseTTL)
d.Set("default_lease_ttl_seconds", mount.Config.DefaultLeaseTTL)

Check failure on line 381 in vault/resource_ad_secret_backend.go

View workflow job for this annotation

GitHub Actions / lint

Error return value of `d.Set` is not checked (errcheck)
d.Set("max_lease_ttl_seconds", mount.Config.MaxLeaseTTL)

Check failure on line 382 in vault/resource_ad_secret_backend.go

View workflow job for this annotation

GitHub Actions / lint

Error return value of `d.Set` is not checked (errcheck)

configPath := fmt.Sprintf("%s/config", d.Id())
log.Printf("[DEBUG] Reading %q", configPath)
Expand Down Expand Up @@ -605,9 +610,6 @@
if raw, ok := d.GetOk("groupfilter"); ok {
data["groupfilter"] = raw
}
if raw, ok := d.GetOk("insecure_tls"); ok {
data["insecure_tls"] = raw
}
if raw, ok := d.GetOk("last_rotation_tolerance"); ok {
data["last_rotation_tolerance"] = raw
}
Expand Down Expand Up @@ -653,6 +655,7 @@
if raw, ok := d.GetOk("userdn"); ok {
data["userdn"] = raw
}
data["insecure_tls"] = d.Get("insecure_tls")
if _, err := client.Logical().Write(vaultPath, data); err != nil {
return fmt.Errorf("error updating template auth backend role %q: %s", vaultPath, err)
}
Expand Down
16 changes: 10 additions & 6 deletions vault/resource_auth_backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
package vault

import (
"context"
"errors"
"fmt"
"log"

Expand All @@ -13,6 +15,7 @@ import (
"github.com/hashicorp/terraform-provider-vault/internal/consts"
"github.com/hashicorp/terraform-provider-vault/internal/provider"
"github.com/hashicorp/terraform-provider-vault/util"
"github.com/hashicorp/terraform-provider-vault/util/mountutil"
)

func AuthBackendResource() *schema.Resource {
Expand Down Expand Up @@ -126,16 +129,17 @@ func authBackendRead(d *schema.ResourceData, meta interface{}) error {

path := d.Id()

mount, err := getAuthMountIfPresent(client, path)
if err != nil {
return err
}

if mount == nil {
mount, err := mountutil.GetAuthMount(context.Background(), client, path)
if errors.Is(err, mountutil.ErrMountNotFound) {
log.Printf("[WARN] Mount %q not found, removing from state.", path)
d.SetId("")
return nil
}

if err != nil {
return err
}

if err := d.Set("type", mount.Type); err != nil {
return err
}
Expand Down
Loading
Loading