From 4847882ee76d819c5342ab8cc24ebda28d9ff4b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Jim=C3=A9nez=20S=C3=A1nchez?= Date: Fri, 18 Jul 2025 13:25:21 +0200 Subject: [PATCH] Provisioning: handle .git extension more gracefully (#108213) * Move .git to repository packages * Bump nanogit 2025-07-17 This version handles `.git` extension internally so that the client doesn't have to worry about it * Put back mutation for Github * Mutate also git URL for clarity --- go.mod | 2 +- go.sum | 4 +- pkg/registry/apis/provisioning/register.go | 35 ----- .../provisioning/repository/git/mutator.go | 21 ++- .../repository/git/mutator_test.go | 127 +++++++++++++++++- .../provisioning/repository/github/mutator.go | 10 ++ .../repository/github/mutator_test.go | 68 ++++++++++ .../repository/github/repository.go | 5 +- 8 files changed, 231 insertions(+), 41 deletions(-) diff --git a/go.mod b/go.mod index b922613a0a8..b3a1af0ab63 100644 --- a/go.mod +++ b/go.mod @@ -104,7 +104,7 @@ require ( github.com/grafana/grafana-openapi-client-go v0.0.0-20231213163343-bd475d63fb79 // @grafana/grafana-backend-group github.com/grafana/grafana-plugin-sdk-go v0.278.0 // @grafana/plugins-platform-backend github.com/grafana/loki/v3 v3.2.1 // @grafana/observability-logs - github.com/grafana/nanogit v0.0.0-20250709085038-55508a6a9f40 // @grafana-app-platform-squad + github.com/grafana/nanogit v0.0.0-20250717084510-7027b3f0138e // @grafana-app-platform-squad github.com/grafana/otel-profiling-go v0.5.1 // @grafana/grafana-backend-group github.com/grafana/pyroscope-go/godeltaprof v0.1.8 // @grafana/observability-traces-and-profiling github.com/grafana/pyroscope/api v1.2.1-0.20250415190842-3ff7247547ae // @grafana/observability-traces-and-profiling diff --git a/go.sum b/go.sum index f13cfc2dc32..292d12d19c4 100644 --- a/go.sum +++ b/go.sum @@ -1641,8 +1641,8 @@ github.com/grafana/loki/pkg/push v0.0.0-20231124142027-e52380921608 h1:ZYk42718k github.com/grafana/loki/pkg/push v0.0.0-20231124142027-e52380921608/go.mod h1:f3JSoxBTPXX5ec4FxxeC19nTBSxoTz+cBgS3cYLMcr0= github.com/grafana/loki/v3 v3.2.1 h1:VB7u+KHfvL5aHAxgoVBvz5wVhsdGuqKC7uuOFOOe7jw= github.com/grafana/loki/v3 v3.2.1/go.mod h1:WvdLl6wOS+yahaeQY+xhD2m2XzkHDfKr5FZaX7D/X2Y= -github.com/grafana/nanogit v0.0.0-20250709085038-55508a6a9f40 h1:wsIgOI4Ou1o/UtxtJlemLufpVBpMdcXVJxedk0wLoCM= -github.com/grafana/nanogit v0.0.0-20250709085038-55508a6a9f40/go.mod h1:ToqLjIdvV3AZQa3K6e5m9hy/nsGaUByc2dWQlctB9iA= +github.com/grafana/nanogit v0.0.0-20250717084510-7027b3f0138e h1:IcrC8SqJcNbGlNq0nqptJ4X8pyy21smMaFrhA7DrfYg= +github.com/grafana/nanogit v0.0.0-20250717084510-7027b3f0138e/go.mod h1:ToqLjIdvV3AZQa3K6e5m9hy/nsGaUByc2dWQlctB9iA= github.com/grafana/otel-profiling-go v0.5.1 h1:stVPKAFZSa7eGiqbYuG25VcqYksR6iWvF3YH66t4qL8= github.com/grafana/otel-profiling-go v0.5.1/go.mod h1:ftN/t5A/4gQI19/8MoWurBEtC6gFw8Dns1sJZ9W4Tls= github.com/grafana/prometheus-alertmanager v0.25.1-0.20250620093340-be61a673dee6 h1:oJnbhG6ZNy10AjsgNeAtAKeGHogIGOMfAsBH6fYYa5M= diff --git a/pkg/registry/apis/provisioning/register.go b/pkg/registry/apis/provisioning/register.go index ba114092b66..e3209fa7026 100644 --- a/pkg/registry/apis/provisioning/register.go +++ b/pkg/registry/apis/provisioning/register.go @@ -438,41 +438,6 @@ func (b *APIBuilder) Mutate(ctx context.Context, a admission.Attributes, o admis r.Spec.Sync.IntervalSeconds = 60 } - // TODO: move this logic into github repository concrete implementation. - if r.Spec.Type == provisioning.GitHubRepositoryType { - if r.Spec.GitHub == nil { - return fmt.Errorf("github configuration is required") - } - - // Trim trailing slash or .git - if len(r.Spec.GitHub.URL) > 5 { - r.Spec.GitHub.URL = strings.TrimSuffix(r.Spec.GitHub.URL, ".git") - r.Spec.GitHub.URL = strings.TrimSuffix(r.Spec.GitHub.URL, "/") - } - } - if r.Spec.Type == provisioning.GitRepositoryType { - if r.Spec.Git == nil { - return fmt.Errorf("git configuration is required") - } - - if r.Spec.GitHub != nil { - return fmt.Errorf("git and github cannot be used together") - } - - if r.Spec.Local != nil { - return fmt.Errorf("git and local cannot be used together") - } - - // Trim trailing slash and ensure .git is present - if len(r.Spec.Git.URL) > 5 { - r.Spec.Git.URL = strings.TrimSuffix(r.Spec.Git.URL, "/") - - if !strings.HasSuffix(r.Spec.Git.URL, ".git") { - r.Spec.Git.URL = r.Spec.Git.URL + ".git" - } - } - } - if r.Spec.Workflows == nil { r.Spec.Workflows = []provisioning.Workflow{} } diff --git a/pkg/registry/apis/provisioning/repository/git/mutator.go b/pkg/registry/apis/provisioning/repository/git/mutator.go index a67f473b82d..e68940fafb8 100644 --- a/pkg/registry/apis/provisioning/repository/git/mutator.go +++ b/pkg/registry/apis/provisioning/repository/git/mutator.go @@ -2,6 +2,8 @@ package git import ( "context" + "fmt" + "strings" "k8s.io/apimachinery/pkg/runtime" @@ -17,10 +19,27 @@ func Mutator(secrets secrets.RepositorySecrets) controller.Mutator { return nil } - if repo.Spec.Git == nil { + if repo.Spec.Type != provisioning.GitRepositoryType { return nil } + if repo.Spec.Git == nil { + return fmt.Errorf("git configuration is required for git repository type") + } + + if repo.Spec.Git.URL != "" { + url := strings.TrimSpace(repo.Spec.Git.URL) + if url != "" { + // Remove any trailing slashes + url = strings.TrimRight(url, "/") + // Only add .git if it's not already present + if !strings.HasSuffix(url, ".git") { + url = url + ".git" + } + repo.Spec.Git.URL = url + } + } + if repo.Spec.Git.Token != "" { secretName := repo.Name + gitTokenSecretSuffix nameOrValue, err := secrets.Encrypt(ctx, repo, secretName, repo.Spec.Git.Token) diff --git a/pkg/registry/apis/provisioning/repository/git/mutator_test.go b/pkg/registry/apis/provisioning/repository/git/mutator_test.go index f924b1b2146..87196d6bcb0 100644 --- a/pkg/registry/apis/provisioning/repository/git/mutator_test.go +++ b/pkg/registry/apis/provisioning/repository/git/mutator_test.go @@ -21,6 +21,7 @@ func TestMutator(t *testing.T) { expectedToken string expectedEncryptedToken string expectedError string + expectedURL string }{ { name: "successful token encryption", @@ -30,6 +31,7 @@ func TestMutator(t *testing.T) { Namespace: "default", }, Spec: provisioning.RepositorySpec{ + Type: provisioning.GitRepositoryType, Git: &provisioning.GitRepositoryConfig{ Token: "secret-token", }, @@ -44,6 +46,7 @@ func TestMutator(t *testing.T) { Namespace: "default", }, Spec: provisioning.RepositorySpec{ + Type: provisioning.GitRepositoryType, Git: &provisioning.GitRepositoryConfig{ Token: "secret-token", }, @@ -64,6 +67,7 @@ func TestMutator(t *testing.T) { Namespace: "default", }, Spec: provisioning.RepositorySpec{ + Type: provisioning.GitRepositoryType, Git: &provisioning.GitRepositoryConfig{ Token: "secret-token", }, @@ -78,6 +82,7 @@ func TestMutator(t *testing.T) { Namespace: "default", }, Spec: provisioning.RepositorySpec{ + Type: provisioning.GitRepositoryType, Git: &provisioning.GitRepositoryConfig{ Token: "secret-token", }, @@ -97,13 +102,32 @@ func TestMutator(t *testing.T) { Namespace: "default", }, Spec: provisioning.RepositorySpec{ - Git: nil, + Type: provisioning.LocalRepositoryType, + Git: nil, }, }, setupMocks: func(mockSecrets *secrets.MockRepositorySecrets) { // No expectations }, }, + + { + name: "no git spec for git repository type", + obj: &provisioning.Repository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-repo", + Namespace: "default", + }, + Spec: provisioning.RepositorySpec{ + Type: provisioning.GitRepositoryType, + Git: nil, + }, + }, + setupMocks: func(mockSecrets *secrets.MockRepositorySecrets) { + // No expectations + }, + expectedError: "git configuration is required for git repository type", + }, { name: "empty token", obj: &provisioning.Repository{ @@ -112,6 +136,7 @@ func TestMutator(t *testing.T) { Namespace: "default", }, Spec: provisioning.RepositorySpec{ + Type: provisioning.GitRepositoryType, Git: &provisioning.GitRepositoryConfig{ Token: "", }, @@ -128,6 +153,101 @@ func TestMutator(t *testing.T) { // No expectations }, }, + { + name: "URL normalization - add .git suffix", + obj: &provisioning.Repository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-repo", + Namespace: "default", + }, + Spec: provisioning.RepositorySpec{ + Type: provisioning.GitRepositoryType, + Git: &provisioning.GitRepositoryConfig{ + URL: "https://github.com/grafana/grafana", + }, + }, + }, + setupMocks: func(mockSecrets *secrets.MockRepositorySecrets) { + // No expectations + }, + expectedURL: "https://github.com/grafana/grafana.git", + }, + { + name: "URL normalization - keep existing .git suffix", + obj: &provisioning.Repository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-repo", + Namespace: "default", + }, + Spec: provisioning.RepositorySpec{ + Type: provisioning.GitRepositoryType, + Git: &provisioning.GitRepositoryConfig{ + URL: "https://github.com/grafana/grafana.git", + }, + }, + }, + setupMocks: func(mockSecrets *secrets.MockRepositorySecrets) { + // No expectations + }, + expectedURL: "https://github.com/grafana/grafana.git", + }, + { + name: "URL normalization - remove trailing slash and add .git", + obj: &provisioning.Repository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-repo", + Namespace: "default", + }, + Spec: provisioning.RepositorySpec{ + Type: provisioning.GitRepositoryType, + Git: &provisioning.GitRepositoryConfig{ + URL: "https://github.com/grafana/grafana/", + }, + }, + }, + setupMocks: func(mockSecrets *secrets.MockRepositorySecrets) { + // No expectations + }, + expectedURL: "https://github.com/grafana/grafana.git", + }, + { + name: "URL normalization - trim whitespace and add .git", + obj: &provisioning.Repository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-repo", + Namespace: "default", + }, + Spec: provisioning.RepositorySpec{ + Type: provisioning.GitRepositoryType, + Git: &provisioning.GitRepositoryConfig{ + URL: " https://github.com/grafana/grafana ", + }, + }, + }, + setupMocks: func(mockSecrets *secrets.MockRepositorySecrets) { + // No expectations + }, + expectedURL: "https://github.com/grafana/grafana.git", + }, + { + name: "URL normalization - empty URL after trim", + obj: &provisioning.Repository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-repo", + Namespace: "default", + }, + Spec: provisioning.RepositorySpec{ + Type: provisioning.GitRepositoryType, + Git: &provisioning.GitRepositoryConfig{ + URL: " ", + }, + }, + }, + setupMocks: func(mockSecrets *secrets.MockRepositorySecrets) { + // No expectations + }, + expectedURL: "", + }, } for _, tt := range tests { @@ -152,6 +272,11 @@ func TestMutator(t *testing.T) { // EncryptedToken should be set to the expected value assert.Equal(t, tt.expectedEncryptedToken, string(repo.Spec.Git.EncryptedToken), "EncryptedToken should match expected value") } + + // Check URL normalization + if tt.expectedURL != "" { + assert.Equal(t, tt.expectedURL, repo.Spec.Git.URL, "URL should be normalized correctly") + } } } }) diff --git a/pkg/registry/apis/provisioning/repository/github/mutator.go b/pkg/registry/apis/provisioning/repository/github/mutator.go index 2c7c4a57827..4aec95aa7d6 100644 --- a/pkg/registry/apis/provisioning/repository/github/mutator.go +++ b/pkg/registry/apis/provisioning/repository/github/mutator.go @@ -2,6 +2,7 @@ package github import ( "context" + "strings" "k8s.io/apimachinery/pkg/runtime" @@ -21,6 +22,15 @@ func Mutator(secrets secrets.RepositorySecrets) controller.Mutator { return nil } + // Trim trailing ".git" and any trailing slash from the GitHub URL, if present, using the strings package. + if repo.Spec.GitHub.URL != "" { + url := repo.Spec.GitHub.URL + url = strings.TrimRight(url, "/") + url = strings.TrimSuffix(url, ".git") + url = strings.TrimRight(url, "/") + repo.Spec.GitHub.URL = url + } + if repo.Spec.GitHub.Token != "" { secretName := repo.Name + githubTokenSecretSuffix nameOrValue, err := secrets.Encrypt(ctx, repo, secretName, repo.Spec.GitHub.Token) diff --git a/pkg/registry/apis/provisioning/repository/github/mutator_test.go b/pkg/registry/apis/provisioning/repository/github/mutator_test.go index c60d53b5e88..a6bbbe22c11 100644 --- a/pkg/registry/apis/provisioning/repository/github/mutator_test.go +++ b/pkg/registry/apis/provisioning/repository/github/mutator_test.go @@ -22,6 +22,74 @@ func TestMutator(t *testing.T) { expectedEncryptedToken string expectedError string }{ + { + name: "trims trailing .git and slash from GitHub URL", + obj: &provisioning.Repository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "repo1", + Namespace: "default", + }, + Spec: provisioning.RepositorySpec{ + GitHub: &provisioning.GitHubRepositoryConfig{ + URL: "https://github.com/org/repo.git/", + }, + }, + }, + setupMocks: func(mockSecrets *secrets.MockRepositorySecrets) {}, + expectedToken: "", + expectedEncryptedToken: "", + }, + { + name: "trims only trailing slash from GitHub URL", + obj: &provisioning.Repository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "repo2", + Namespace: "default", + }, + Spec: provisioning.RepositorySpec{ + GitHub: &provisioning.GitHubRepositoryConfig{ + URL: "https://github.com/org/repo/", + }, + }, + }, + setupMocks: func(mockSecrets *secrets.MockRepositorySecrets) {}, + expectedToken: "", + expectedEncryptedToken: "", + }, + { + name: "trims only trailing .git from GitHub URL", + obj: &provisioning.Repository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "repo3", + Namespace: "default", + }, + Spec: provisioning.RepositorySpec{ + GitHub: &provisioning.GitHubRepositoryConfig{ + URL: "https://github.com/org/repo.git", + }, + }, + }, + setupMocks: func(mockSecrets *secrets.MockRepositorySecrets) {}, + expectedToken: "", + expectedEncryptedToken: "", + }, + { + name: "does not trim if no .git or slash", + obj: &provisioning.Repository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "repo4", + Namespace: "default", + }, + Spec: provisioning.RepositorySpec{ + GitHub: &provisioning.GitHubRepositoryConfig{ + URL: "https://github.com/org/repo", + }, + }, + }, + setupMocks: func(mockSecrets *secrets.MockRepositorySecrets) {}, + expectedToken: "", + expectedEncryptedToken: "", + }, { name: "successful token encryption", obj: &provisioning.Repository{ diff --git a/pkg/registry/apis/provisioning/repository/github/repository.go b/pkg/registry/apis/provisioning/repository/github/repository.go index e7aed375242..e6cc5aa304d 100644 --- a/pkg/registry/apis/provisioning/repository/github/repository.go +++ b/pkg/registry/apis/provisioning/repository/github/repository.go @@ -114,7 +114,10 @@ func (r *githubRepository) Validate() (list field.ErrorList) { } func ParseOwnerRepoGithub(giturl string) (owner string, repo string, err error) { - parsed, e := url.Parse(strings.TrimSuffix(giturl, ".git")) + giturl = strings.TrimSuffix(giturl, ".git") + giturl = strings.TrimSuffix(giturl, "/") + + parsed, e := url.Parse(giturl) if e != nil { err = e return