From d8f657aed17d0134fd00fe45a6a8cd3aa0d1d70a Mon Sep 17 00:00:00 2001 From: Ryan McKinley Date: Fri, 13 Dec 2024 10:18:43 +0300 Subject: [PATCH] POC/Provisioning: Allow saving valid, but unhealthy configurations (#97899) Co-authored-by: Mariell Hoversholm --- pkg/registry/apis/provisioning/register.go | 11 ++-- .../apis/provisioning/repository/local.go | 50 ++++++++--------- .../provisioning/repository/local_test.go | 54 +++++++++++++++++++ pkg/registry/apis/provisioning/types.go | 2 + 4 files changed, 85 insertions(+), 32 deletions(-) create mode 100644 pkg/registry/apis/provisioning/repository/local_test.go diff --git a/pkg/registry/apis/provisioning/register.go b/pkg/registry/apis/provisioning/register.go index 5b582ba8858..67d37b9a1ac 100644 --- a/pkg/registry/apis/provisioning/register.go +++ b/pkg/registry/apis/provisioning/register.go @@ -5,7 +5,6 @@ import ( "crypto/hmac" "crypto/sha256" "encoding/hex" - "errors" "fmt" "log/slog" "net/url" @@ -48,9 +47,8 @@ const ( ) var ( - _ builder.APIGroupBuilder = (*ProvisioningAPIBuilder)(nil) - _ RepoGetter = (*ProvisioningAPIBuilder)(nil) - ErrLocalRepositoryDisabled = errors.New("the local repository type has been disabled due to having no permitted local paths") + _ builder.APIGroupBuilder = (*ProvisioningAPIBuilder)(nil) + _ RepoGetter = (*ProvisioningAPIBuilder)(nil) ) // This is used just so wire has something unique to return @@ -247,10 +245,7 @@ func (b *ProvisioningAPIBuilder) asRepository(ctx context.Context, obj runtime.O func (b *ProvisioningAPIBuilder) AsRepository(ctx context.Context, r *provisioning.Repository) (repository.Repository, error) { switch r.Spec.Type { case provisioning.LocalRepositoryType: - if len(b.localFileResolver.PermittedPrefixes) == 0 { - return nil, ErrLocalRepositoryDisabled - } - return repository.NewLocal(r, b.localFileResolver) + return repository.NewLocal(r, b.localFileResolver), nil case provisioning.GitHubRepositoryType: baseURL, err := url.Parse(b.urlProvider(r.GetNamespace())) if err != nil { diff --git a/pkg/registry/apis/provisioning/repository/local.go b/pkg/registry/apis/provisioning/repository/local.go index 4acb9137cb1..4ea653e4e20 100644 --- a/pkg/registry/apis/provisioning/repository/local.go +++ b/pkg/registry/apis/provisioning/repository/local.go @@ -81,24 +81,22 @@ func (r *LocalFolderResolver) LocalPath(p string) (string, error) { var _ Repository = (*localRepository)(nil) type localRepository struct { - config *provisioning.Repository + config *provisioning.Repository + resolver *LocalFolderResolver // validated path that can be read if not empty path string } -func NewLocal(config *provisioning.Repository, resolver *LocalFolderResolver) (*localRepository, error) { +func NewLocal(config *provisioning.Repository, resolver *LocalFolderResolver) *localRepository { r := &localRepository{ - config: config, + config: config, + resolver: resolver, } if config.Spec.Local != nil { - var err error - r.path, err = resolver.LocalPath(config.Spec.Local.Path) - if err != nil { - return nil, err - } + r.path, _ = resolver.LocalPath(config.Spec.Local.Path) } - return r, nil + return r } func (r *localRepository) Config() *provisioning.Repository { @@ -107,14 +105,6 @@ func (r *localRepository) Config() *provisioning.Repository { // Validate implements provisioning.Repository. func (r *localRepository) Validate() (fields field.ErrorList) { - if r.config.Spec.Type != provisioning.LocalRepositoryType { - fields = append(fields, &field.Error{ - Type: field.ErrorTypeInvalid, - Field: "spec.type", - Detail: "Local repository requires spec.type=local", - }) - } - cfg := r.config.Spec.Local if cfg == nil { fields = append(fields, &field.Error{ @@ -128,9 +118,6 @@ func (r *localRepository) Validate() (fields field.ErrorList) { if cfg.Path == "" { fields = append(fields, field.Required(field.NewPath("spec", "local", "path"), "must enter a path to local file")) - } else if r.path == "" { - fields = append(fields, field.Invalid(field.NewPath("spec", "local", "path"), - cfg.Path, "configured path is not allowed, see system allow list")) } return fields @@ -139,23 +126,34 @@ func (r *localRepository) Validate() (fields field.ErrorList) { // Test implements provisioning.Repository. // NOTE: Validate has been called (and passed) before this function should be called func (r *localRepository) Test(ctx context.Context, logger *slog.Logger) (*provisioning.TestResults, error) { - if r.path == "" { + if r.config.Spec.Local.Path == "" { return &provisioning.TestResults{ Code: http.StatusBadRequest, Success: false, Errors: []string{ - fmt.Sprintf("invalid path: %s", r.config.Spec.Local.Path), + "no path is configured", }, }, nil } - _, err := os.Stat(r.path) + _, err := r.resolver.LocalPath(r.config.Spec.Local.Path) + if err != nil { + return &provisioning.TestResults{ + Code: http.StatusBadRequest, + Success: false, + Errors: []string{ + err.Error(), + }, + }, nil + } + + _, err = os.Stat(r.path) if errors.Is(err, os.ErrNotExist) { return &provisioning.TestResults{ Code: http.StatusBadRequest, Success: false, Errors: []string{ - fmt.Sprintf("file not found: %s", r.config.Spec.Local.Path), + fmt.Sprintf("directory not found: %s", r.config.Spec.Local.Path), }, }, nil } @@ -172,6 +170,10 @@ func (r *localRepository) validateRequest(ref string) error { return apierrors.NewBadRequest("local repository does not support ref") } if r.path == "" { + _, err := r.resolver.LocalPath(r.config.Spec.Local.Path) + if err != nil { + return err + } return &apierrors.StatusError{ ErrStatus: metav1.Status{ Message: "the service is missing a root path", diff --git a/pkg/registry/apis/provisioning/repository/local_test.go b/pkg/registry/apis/provisioning/repository/local_test.go new file mode 100644 index 00000000000..098ea6809d5 --- /dev/null +++ b/pkg/registry/apis/provisioning/repository/local_test.go @@ -0,0 +1,54 @@ +package repository + +import ( + "context" + "log/slog" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/grafana/grafana/pkg/apis/provisioning/v0alpha1" +) + +func TestLocalResolver(t *testing.T) { + resolver := &LocalFolderResolver{ + PermittedPrefixes: []string{ + "github/testdata", + }, + HomePath: "", + } + _, err := resolver.LocalPath("github/testdata") + require.NoError(t, err) + + _, err = resolver.LocalPath("something") + require.Error(t, err) +} + +func TestLocal(t *testing.T) { + t.Run("invalid path", func(t *testing.T) { + r := NewLocal(&v0alpha1.Repository{ + Spec: v0alpha1.RepositorySpec{ + Local: &v0alpha1.LocalRepositoryConfig{ + Path: "invalid/path", + }, + }, + }, &LocalFolderResolver{}) + + // Did not resolve a local path + require.Equal(t, "", r.path) + + // The correct fields are set + require.Nil(t, r.Validate()) + + expected := "the path given ('invalid/path') is invalid for a local repository (no permitted prefixes were configured)" + + rsp, err := r.Test(context.Background(), slog.Default()) + require.NoError(t, err) + require.Equal(t, false, rsp.Success) + require.Equal(t, []string{expected}, rsp.Errors) + + // We get the same error when trying to read a file + _, err = r.Read(context.Background(), slog.Default(), "path/to/file", "") + require.Equal(t, expected, err.Error()) + }) +} diff --git a/pkg/registry/apis/provisioning/types.go b/pkg/registry/apis/provisioning/types.go index 366b8393698..fb1db327bdd 100644 --- a/pkg/registry/apis/provisioning/types.go +++ b/pkg/registry/apis/provisioning/types.go @@ -12,5 +12,7 @@ type RepoGetter interface { GetRepository(ctx context.Context, name string) (repository.Repository, error) // Given a repository configuration, return it as a repository instance + // This will only error for un-recoverable system errors + // the repository instance may or may not be valid/healthy AsRepository(ctx context.Context, cfg *provisioning.Repository) (repository.Repository, error) }