From cb72f89dfb06e857449df81ebd91c21377464e4b Mon Sep 17 00:00:00 2001 From: Mariell Hoversholm Date: Mon, 25 Nov 2024 13:02:01 +0100 Subject: [PATCH] feat: add more logs --- pkg/registry/apis/provisioning/export.go | 7 ++++- pkg/registry/apis/provisioning/files.go | 26 ++++++++++++++----- pkg/registry/apis/provisioning/parser.go | 7 +++-- pkg/registry/apis/provisioning/register.go | 12 +++++++++ .../apis/provisioning/repository/github.go | 2 ++ pkg/registry/apis/provisioning/utils.go | 13 +++++++++- pkg/registry/apis/provisioning/utils_test.go | 4 ++- 7 files changed, 59 insertions(+), 12 deletions(-) diff --git a/pkg/registry/apis/provisioning/export.go b/pkg/registry/apis/provisioning/export.go index a10b3ca55f0..954cc6c0ffc 100644 --- a/pkg/registry/apis/provisioning/export.go +++ b/pkg/registry/apis/provisioning/export.go @@ -147,9 +147,14 @@ func (c *exportConnector) Connect( return } + var ref string + if repo.Config().Spec.Type == provisioning.GitHubRepositoryType { + ref = repo.Config().Spec.GitHub.Branch + } + fileName := filepath.Join(folder.CreatePath(), baseFileName) // TODO: Upsert - if err := repo.Create(ctx, fileName, "", marshalledBody, "export of dashboard "+name+" in namespace "+ns); err != nil { + if err := repo.Create(ctx, fileName, ref, marshalledBody, "export of dashboard "+name+" in namespace "+ns); err != nil { slog.ErrorContext(ctx, "failed to write dashboard model to repository", "err", err, "repository", repo.Config().GetName(), diff --git a/pkg/registry/apis/provisioning/files.go b/pkg/registry/apis/provisioning/files.go index 2631c1e9652..e2d5a91c70a 100644 --- a/pkg/registry/apis/provisioning/files.go +++ b/pkg/registry/apis/provisioning/files.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "io" + "log/slog" "net/http" "path/filepath" "strings" @@ -21,6 +22,7 @@ import ( type filesConnector struct { getter RepoGetter client *resourceClient + logger *slog.Logger } func (*filesConnector) New() runtime.Object { @@ -55,26 +57,32 @@ func (*filesConnector) NewConnectOptions() (runtime.Object, bool, string) { } func (s *filesConnector) Connect(ctx context.Context, name string, opts runtime.Object, responder rest.Responder) (http.Handler, error) { + logger := s.logger.With("repository_name", name) repo, err := s.getter.GetRepository(ctx, name) if err != nil { + logger.DebugContext(ctx, "failed to find repository", "error", err) return nil, err } return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + logger := logger.With("url", r.URL.Path) prefix := fmt.Sprintf("/%s/files/", name) idx := strings.Index(r.URL.Path, prefix) if idx == -1 { + logger.DebugContext(r.Context(), "failed to find a file path in the URL") responder.Error(apierrors.NewBadRequest("invalid request path")) return } filePath := r.URL.Path[idx+len(prefix):] if filePath == "" { + logger.DebugContext(r.Context(), "got an empty file path") responder.Error(apierrors.NewBadRequest("missing path")) return } if strings.Contains(filePath, "..") { + logger.DebugContext(r.Context(), "got a file path including '..'; failing the request for security reasons") responder.Error(apierrors.NewBadRequest("invalid path navigation")) return } @@ -83,6 +91,7 @@ func (s *filesConnector) Connect(ctx context.Context, name string, opts runtime. case ".json", ".yaml", ".yml": // ok default: + logger.DebugContext(r.Context(), "got a file extension that was not JSON or YAML", "extension", filepath.Ext(filePath)) responder.Error(apierrors.NewBadRequest("only yaml and json files supported")) return } @@ -90,15 +99,16 @@ func (s *filesConnector) Connect(ctx context.Context, name string, opts runtime. var obj *provisioning.ResourceWrapper ref := r.URL.Query().Get("ref") message := r.URL.Query().Get("message") + logger = logger.With("ref", ref, "message", message) code := http.StatusOK switch r.Method { case http.MethodGet: - code, obj, err = s.doRead(r.Context(), repo, filePath, ref) + code, obj, err = s.doRead(r.Context(), logger, repo, filePath, ref) case http.MethodPost: - obj, err = s.doWrite(r.Context(), false, repo, filePath, ref, message, r) + obj, err = s.doWrite(r.Context(), logger, false, repo, filePath, ref, message, r) case http.MethodPut: - obj, err = s.doWrite(r.Context(), true, repo, filePath, ref, message, r) + obj, err = s.doWrite(r.Context(), logger, true, repo, filePath, ref, message, r) case http.MethodDelete: obj, err = s.doDelete(r.Context(), repo, filePath, ref, message) default: @@ -106,9 +116,11 @@ func (s *filesConnector) Connect(ctx context.Context, name string, opts runtime. } if err != nil { + logger.DebugContext(ctx, "got an error after processing request", "error", err) responder.Error(err) return } + logger.DebugContext(ctx, "request resulted in valid object", "object", obj) responder.Object(code, obj) }), nil } @@ -130,7 +142,7 @@ func (s *filesConnector) getParser(repo repository.Repository) (*fileParser, err }, nil } -func (s *filesConnector) doRead(ctx context.Context, repo repository.Repository, path string, ref string) (int, *provisioning.ResourceWrapper, error) { +func (s *filesConnector) doRead(ctx context.Context, logger *slog.Logger, repo repository.Repository, path string, ref string) (int, *provisioning.ResourceWrapper, error) { info, err := repo.Read(ctx, path, ref) if err != nil { return 0, nil, err @@ -141,7 +153,7 @@ func (s *filesConnector) doRead(ctx context.Context, repo repository.Repository, return 0, nil, err } - parsed, err := parser.parse(ctx, info, true) + parsed, err := parser.parse(ctx, logger, info, true) if err != nil { return 0, nil, err } @@ -153,7 +165,7 @@ func (s *filesConnector) doRead(ctx context.Context, repo repository.Repository, return code, parsed.AsResourceWrapper(), nil } -func (s *filesConnector) doWrite(ctx context.Context, update bool, repo repository.Repository, path string, ref string, message string, req *http.Request) (*provisioning.ResourceWrapper, error) { +func (s *filesConnector) doWrite(ctx context.Context, logger *slog.Logger, update bool, repo repository.Repository, path string, ref string, message string, req *http.Request) (*provisioning.ResourceWrapper, error) { settings := repo.Config().Spec.Editing if update && !settings.Update { return nil, apierrors.NewForbidden(provisioning.RepositoryResourceInfo.GroupResource(), "updating files not enabled", nil) @@ -178,7 +190,7 @@ func (s *filesConnector) doWrite(ctx context.Context, update bool, repo reposito return nil, err } - parsed, err := parser.parse(ctx, info, true) + parsed, err := parser.parse(ctx, logger, info, true) if err != nil { if errors.Is(err, ErrUnableToReadResourceBytes) { return nil, apierrors.NewBadRequest("unable to read the request as a resource") diff --git a/pkg/registry/apis/provisioning/parser.go b/pkg/registry/apis/provisioning/parser.go index 48d04a0f3f4..6886f456ab0 100644 --- a/pkg/registry/apis/provisioning/parser.go +++ b/pkg/registry/apis/provisioning/parser.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "fmt" + "log/slog" "path/filepath" "strings" @@ -55,11 +56,13 @@ type parsedFile struct { errors []error } -func (r *fileParser) parse(ctx context.Context, info *repository.FileInfo, validate bool) (*parsedFile, error) { +func (r *fileParser) parse(ctx context.Context, logger *slog.Logger, info *repository.FileInfo, validate bool) (*parsedFile, error) { obj, gvk, err := LoadYAMLOrJSON(bytes.NewBuffer(info.Data)) if err != nil { - obj, gvk, err = FallbackResourceLoader(info.Data) + logger.DebugContext(ctx, "failed to find GVK of the input data", "error", err) + obj, gvk, err = FallbackResourceLoader(ctx, logger, info.Data) if err != nil { + logger.DebugContext(ctx, "also failed to get GVK from fallback loader?", "error", err) return nil, err } } diff --git a/pkg/registry/apis/provisioning/register.go b/pkg/registry/apis/provisioning/register.go index 538f880b6bd..e9fed2d23fd 100644 --- a/pkg/registry/apis/provisioning/register.go +++ b/pkg/registry/apis/provisioning/register.go @@ -168,6 +168,7 @@ func (b *ProvisioningAPIBuilder) UpdateAPIGroupInfo(apiGroupInfo *genericapiserv storage[provisioning.RepositoryResourceInfo.StoragePath("files")] = &filesConnector{ getter: b, client: b.client, + logger: b.logger.With("connector", "files"), } storage[provisioning.RepositoryResourceInfo.StoragePath("export")] = exportConnector apiGroupInfo.VersionedResourcesStorageMap[provisioning.VERSION] = storage @@ -482,6 +483,17 @@ func (b *ProvisioningAPIBuilder) PostProcessOpenAPI(oas *spec3.OpenAPI) (*spec3. // update the version with a path sub = oas.Paths.Paths[repoprefix+"/files/{path}"] if sub != nil { + sub.Parameters = append(sub.Parameters, &spec3.Parameter{ + ParameterProps: spec3.ParameterProps{ + Name: "ref", + In: "query", + Example: "the ref name of the branch", + Description: "the commit hash or branch name to look at (writes must be branch names)", + Schema: spec.StringProperty(), + Required: false, + }, + }) + sub.Get.Description = "Read value from upstream repository" sub.Get.Parameters = []*spec3.Parameter{ { diff --git a/pkg/registry/apis/provisioning/repository/github.go b/pkg/registry/apis/provisioning/repository/github.go index 4893ff401e3..b32f8b41e70 100644 --- a/pkg/registry/apis/provisioning/repository/github.go +++ b/pkg/registry/apis/provisioning/repository/github.go @@ -229,6 +229,8 @@ func (r *githubRepository) onPushEvent(ctx context.Context, event *github.PushEv // Skip silently if the event is not for the main/master branch // as we cannot configure the webhook to only publish events for the main branch if event.GetRef() != fmt.Sprintf("refs/heads/%s", r.config.Spec.GitHub.Branch) { + r.logger.DebugContext(ctx, "ignoring push event as it is not for the configured branch", + "ref", event.GetRef()) return nil } diff --git a/pkg/registry/apis/provisioning/utils.go b/pkg/registry/apis/provisioning/utils.go index e36cf9a52e8..1c2cd05ba8a 100644 --- a/pkg/registry/apis/provisioning/utils.go +++ b/pkg/registry/apis/provisioning/utils.go @@ -1,9 +1,11 @@ package provisioning import ( + "context" "encoding/json" "fmt" "io" + "log/slog" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" @@ -14,7 +16,10 @@ import ( "github.com/grafana/grafana/pkg/apis/dashboard" ) -func FallbackResourceLoader(data []byte) (*unstructured.Unstructured, *schema.GroupVersionKind, error) { +// Tries to figure out what kind this data is. +// +// The context and logger are both only used for logging purposes. They do not control any logic. +func FallbackResourceLoader(ctx context.Context, logger *slog.Logger, data []byte) (*unstructured.Unstructured, *schema.GroupVersionKind, error) { // Try parsing as JSON if data[0] == '{' { var value map[string]any @@ -24,7 +29,11 @@ func FallbackResourceLoader(data []byte) (*unstructured.Unstructured, *schema.Gr } // regular version headers exist + // TODO: do we intend on this checking Kind or kind? document reasoning. if value["apiVersion"] != nil && value["Kind"] != nil { + logger.DebugContext(ctx, "found that the object had K8s definitions already", + "apiVersion", value["apiVersion"], + "kind", value["Kind"]) gv, err := schema.ParseGroupVersion(value["apiVersion"].(string)) if err != nil { return nil, nil, fmt.Errorf("invalid apiVersion") @@ -52,6 +61,8 @@ func FallbackResourceLoader(data []byte) (*unstructured.Unstructured, *schema.Gr }, }, gvk, nil } + } else { + logger.DebugContext(ctx, "failed to get") } return nil, nil, ErrUnableToReadResourceBytes diff --git a/pkg/registry/apis/provisioning/utils_test.go b/pkg/registry/apis/provisioning/utils_test.go index ebf9ff72f30..3dba9fa8162 100644 --- a/pkg/registry/apis/provisioning/utils_test.go +++ b/pkg/registry/apis/provisioning/utils_test.go @@ -2,6 +2,8 @@ package provisioning import ( "bytes" + "context" + "log/slog" "testing" "github.com/stretchr/testify/require" @@ -63,7 +65,7 @@ spec: t.Run("load dashboard json", func(t *testing.T) { // Support dashboard conversion - obj, gvk, err := FallbackResourceLoader([]byte(`{ + obj, gvk, err := FallbackResourceLoader(context.Background(), slog.Default(), []byte(`{ "schemaVersion": 7, "panels": [], "tags": []