From 6413ec0a34472d6afa063146c7416e794bbbfa94 Mon Sep 17 00:00:00 2001 From: Fiachra Corcoran Date: Tue, 14 Jul 2026 18:51:52 +0100 Subject: [PATCH 1/7] Move repo val webhook Signed-off-by: Fiachra Corcoran --- .vscode/launch.json | 4 - .../config/webhook/manifests.yaml | 42 + .../pkg/controllers/repository/config.go | 23 + .../repository/repository_controller.go | 4 +- .../pkg/webhooks/repository_webhook.go | 169 ++++ .../pkg/webhooks/repository_webhook_test.go | 372 +++++++++ deployments/porch/3-porch-server.yaml | 10 - go.mod | 2 +- pkg/apiserver/webhooks.go | 679 ---------------- pkg/apiserver/webhooks_test.go | 754 ------------------ 10 files changed, 610 insertions(+), 1449 deletions(-) create mode 100644 controllers/repositories/config/webhook/manifests.yaml create mode 100644 controllers/repositories/pkg/webhooks/repository_webhook.go create mode 100644 controllers/repositories/pkg/webhooks/repository_webhook_test.go delete mode 100644 pkg/apiserver/webhooks.go delete mode 100644 pkg/apiserver/webhooks_test.go diff --git a/.vscode/launch.json b/.vscode/launch.json index 5d27a1a34..e8557cd75 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -32,8 +32,6 @@ ], "cwd": "${workspaceFolder}", "env": { - "CERT_STORAGE_DIR": "${workspaceFolder}/.build/pki/tmp", - "WEBHOOK_HOST": "localhost", "GOOGLE_API_GO_EXPERIMENTAL_DISABLE_NEW_AUTH_LIB": "true", "OTEL_TRACES_EXPORTER": "none", "OTEL_METRICS_EXPORTER": "none" @@ -56,8 +54,6 @@ ], "cwd": "${workspaceFolder}", "env": { - "CERT_STORAGE_DIR": "${workspaceFolder}/.build/pki/tmp", - "WEBHOOK_HOST": "localhost", "GOOGLE_API_GO_EXPERIMENTAL_DISABLE_NEW_AUTH_LIB": "true", "DB_DRIVER": "pgx", "DB_HOST": "${env:DB_HOST}", diff --git a/controllers/repositories/config/webhook/manifests.yaml b/controllers/repositories/config/webhook/manifests.yaml new file mode 100644 index 000000000..5dd37a456 --- /dev/null +++ b/controllers/repositories/config/webhook/manifests.yaml @@ -0,0 +1,42 @@ +# Copyright 2026 The kpt Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +--- +apiVersion: admissionregistration.k8s.io/v1 +kind: ValidatingWebhookConfiguration +metadata: + name: validating-webhook-configuration +webhooks: +- admissionReviewVersions: + - v1 + clientConfig: + service: + name: porch-controllers + namespace: porch-system + path: /validate-repository + port: 9443 + failurePolicy: Fail + name: repository-validator.porch.kpt.dev + rules: + - apiGroups: + - config.porch.kpt.dev + apiVersions: + - v1alpha1 + operations: + - CREATE + - UPDATE + - DELETE + resources: + - repositories + sideEffects: None + timeoutSeconds: 30 diff --git a/controllers/repositories/pkg/controllers/repository/config.go b/controllers/repositories/pkg/controllers/repository/config.go index ba966a7cf..c57e5d9c6 100644 --- a/controllers/repositories/pkg/controllers/repository/config.go +++ b/controllers/repositories/pkg/controllers/repository/config.go @@ -18,7 +18,10 @@ import ( "flag" "time" + "github.com/kptdev/porch/controllers/repositories/pkg/webhooks" cachetypes "github.com/kptdev/porch/pkg/cache/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" ) const ( @@ -87,6 +90,26 @@ func (r *RepositoryReconciler) validateConfig() { } } +// Init wires runtime dependencies that require the manager. +// It registers the Repository validating webhook on the shared webhook server. +func (r *RepositoryReconciler) Init(mgr ctrl.Manager) error { + log := ctrl.Log.WithName(r.Name()) + + // Register Repository validating webhook. + // The validator implements admission.Handler interface via its Handle method. + // Webhook TLS certificates are mounted from Secret at /etc/webhook/certs (see deployment). + validator := webhooks.NewRepositoryValidator(mgr.GetClient()) + + mgr.GetWebhookServer().Register( + "/validate-repository", + &admission.Webhook{ + Handler: validator, + }) + log.Info("Repository validating webhook registered") + + return nil +} + // LogConfig logs the controller configuration func (r *RepositoryReconciler) LogConfig(log interface { Info(msg string, keysAndValues ...any) diff --git a/controllers/repositories/pkg/controllers/repository/repository_controller.go b/controllers/repositories/pkg/controllers/repository/repository_controller.go index a971c7397..92a444e13 100644 --- a/controllers/repositories/pkg/controllers/repository/repository_controller.go +++ b/controllers/repositories/pkg/controllers/repository/repository_controller.go @@ -74,7 +74,9 @@ type RepositoryReconciler struct { coldStartRepos sync.Map // Tracks repos that have synced since startup } -//go:generate go run sigs.k8s.io/controller-tools/cmd/controller-gen@v0.21.0 rbac:headerFile=../../../../../scripts/boilerplate.yaml.txt,roleName=porch-controllers-repositories,year=$YEAR_GEN webhook paths="." output:rbac:artifacts:config=../../../config/rbac +//go:generate go run sigs.k8s.io/controller-tools/cmd/controller-gen@v0.21.0 rbac:headerFile=../../../../../scripts/boilerplate.yaml.txt,roleName=porch-controllers-repositories,year=$YEAR_GEN webhook:headerFile=../../../../../scripts/boilerplate.yaml.txt,year=$YEAR_GEN paths="." output:rbac:artifacts:config=../../../config/rbac output:webhook:artifacts:config=../../../config/webhook + +//+kubebuilder:webhook:path=/validate-repository,mutating=false,failurePolicy=fail,groups=config.porch.kpt.dev,resources=repositories,verbs=create;update;delete,versions=v1alpha1,name=repository-validator.porch.kpt.dev,admissionReviewVersions=v1,sideEffects=None,serviceName=porch-controllers,serviceNamespace=porch-system,servicePort=9443,timeoutSeconds=30 //+kubebuilder:rbac:groups=config.porch.kpt.dev,resources=repositories,verbs=get;list;watch;create;update;patch;delete //+kubebuilder:rbac:groups=config.porch.kpt.dev,resources=repositories/status,verbs=get;update;patch diff --git a/controllers/repositories/pkg/webhooks/repository_webhook.go b/controllers/repositories/pkg/webhooks/repository_webhook.go new file mode 100644 index 000000000..de35042d7 --- /dev/null +++ b/controllers/repositories/pkg/webhooks/repository_webhook.go @@ -0,0 +1,169 @@ +// Copyright 2026 The kpt Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package webhooks + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "path/filepath" + "strings" + + configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" + admissionv1 "k8s.io/api/admission/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" +) + +// RepositoryValidator validates config.porch.kpt.dev/v1alpha1 Repository CRDs. +// It enforces cross-resource conflict detection that CEL cannot perform: +// - Duplicate git location (URL + branch + directory) in the same namespace +// - Root directory conflicts with subdirectories +// - Nested directory conflicts +type RepositoryValidator struct { + client client.Reader +} + +// NewRepositoryValidator creates a new RepositoryValidator. +func NewRepositoryValidator(client client.Reader) *RepositoryValidator { + return &RepositoryValidator{client: client} +} + +// Handle implements the admission.Handler interface for webhook registration. +func (v *RepositoryValidator) Handle(ctx context.Context, req admission.Request) admission.Response { + switch req.Operation { + case admissionv1.Create, admissionv1.Update: + return v.handleCreateOrUpdate(ctx, req) + case admissionv1.Delete: + return v.handleDelete(ctx, req) + default: + return admission.Allowed("") + } +} + +// handleDelete allows all DELETE operations without validation. +// No conflict detection is needed for deletes. +func (v *RepositoryValidator) handleDelete(_ context.Context, req admission.Request) admission.Response { + log.Log.V(3).Info("repository deletion validated", + "namespace", req.Namespace, "name", req.Name) + return admission.Allowed("Repository deletion validated successfully") +} + +// handleCreateOrUpdate validates CREATE and UPDATE operations for repository conflicts. +func (v *RepositoryValidator) handleCreateOrUpdate(ctx context.Context, req admission.Request) admission.Response { + logger := log.FromContext(ctx) + + if len(req.Object.Raw) == 0 { + return admission.Errored(http.StatusBadRequest, fmt.Errorf("request object is empty")) + } + + var attempted configapi.Repository + if err := json.Unmarshal(req.Object.Raw, &attempted); err != nil { + return admission.Errored(http.StatusBadRequest, + fmt.Errorf("failed to unmarshal repository: %w", err)) + } + + // NOTE: Immutability checks (URL, branch, directory) are handled by CEL validation in the CRD. + // This webhook only performs complex cross-resource conflict detection that CEL cannot do. + + var repoList configapi.RepositoryList + if err := v.client.List(ctx, &repoList); err != nil { + logger.Error(err, "failed to list repositories for conflict check") + return admission.Errored(http.StatusInternalServerError, + fmt.Errorf("could not list repositories: %w", err)) + } + + for i := range repoList.Items { + existing := &repoList.Items[i] + if existing.Name == attempted.Name && existing.Namespace == attempted.Namespace { + continue + } + if IsConflict(existing, &attempted) { + logger.Info("repository conflict detected", + "attempted", fmt.Sprintf("%s/%s", attempted.Namespace, attempted.Name), + "conflictsWith", fmt.Sprintf("%s/%s", existing.Namespace, existing.Name)) + return admission.Denied( + fmt.Sprintf("Repository conflict with existing repository: %s/%s", + existing.Namespace, existing.Name)) + } + } + + logger.V(3).Info("repository validation passed", + "namespace", attempted.Namespace, "name", attempted.Name) + return admission.Allowed("Repository validated successfully") +} + +// NormalizeURL converts a repository URL to a comparable format by replacing +// protocol separators and path characters with dashes. +func NormalizeURL(url string) string { + replace := strings.NewReplacer("://", "---", ":", "-", "/", "-") + return replace.Replace(url) +} + +// IsConflict checks whether two repositories conflict based on their git location. +// Conflict rules: +// 1. Same URL, branch, and directory in the same namespace +// 2. Root directory conflicts with any subdirectory under the same URL and branch +// 3. Nested directory conflicts (one path is a prefix of another) +func IsConflict(existing, attempted *configapi.Repository) bool { + existingURL := NormalizeURL(existing.Spec.Git.Repo) + attemptedURL := NormalizeURL(attempted.Spec.Git.Repo) + + existingDir := strings.Trim(existing.Spec.Git.Directory, "/") + attemptedDir := strings.Trim(attempted.Spec.Git.Directory, "/") + + // Branch defaults to "main" via CRD default, so no need to handle empty values + existingBranch := existing.Spec.Git.Branch + attemptedBranch := attempted.Spec.Git.Branch + + // Different URL or branch → no conflict possible + if existingURL != attemptedURL || existingBranch != attemptedBranch { + return false + } + + // Rule 1: Same URL, branch and directory in same namespace → conflict + if existingDir == attemptedDir && existing.Namespace == attempted.Namespace { + return true + } + + // Rule 2: Root directory conflicts with any other directory + if (existingDir == "" && attemptedDir != "") || (existingDir != "" && attemptedDir == "") { + return true + } + + // Rule 3: Nested directory conflicts + if IsNestedConflict(existingDir, attemptedDir) { + return true + } + + return false +} + +// IsNestedConflict checks if one directory path is nested within the other. +func IsNestedConflict(a, b string) bool { + relAtoB, err1 := filepath.Rel(a, b) + relBtoA, err2 := filepath.Rel(b, a) + + if err1 == nil && !strings.HasPrefix(relAtoB, "../") && relAtoB != "." { + return true // b is nested within a + } + if err2 == nil && !strings.HasPrefix(relBtoA, "../") && relBtoA != "." { + return true // a is nested within b + } + + return false +} diff --git a/controllers/repositories/pkg/webhooks/repository_webhook_test.go b/controllers/repositories/pkg/webhooks/repository_webhook_test.go new file mode 100644 index 000000000..3d155aa12 --- /dev/null +++ b/controllers/repositories/pkg/webhooks/repository_webhook_test.go @@ -0,0 +1,372 @@ +// Copyright 2026 The kpt Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package webhooks + +import ( + "context" + "encoding/json" + "testing" + + configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + admissionv1 "k8s.io/api/admission/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" +) + +func newScheme() *runtime.Scheme { + scheme := runtime.NewScheme() + _ = configapi.AddToScheme(scheme) + return scheme +} + +func makeRepo(name, ns, url, dir, branch string) *configapi.Repository { + return &configapi.Repository{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: ns, + }, + Spec: configapi.RepositorySpec{ + Git: &configapi.GitRepository{ + Repo: url, + Directory: dir, + Branch: branch, + }, + }, + } +} + +func marshalRepo(t *testing.T, repo *configapi.Repository) []byte { + t.Helper() + raw, err := json.Marshal(repo) + require.NoError(t, err) + return raw +} + +// TestHandleDelete verifies DELETE operations are always allowed. +func TestHandleDelete(t *testing.T) { + validator := NewRepositoryValidator(fake.NewClientBuilder().WithScheme(newScheme()).Build()) + + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Delete, + Name: "my-repo", + Namespace: "default", + }, + } + + resp := validator.Handle(context.Background(), req) + assert.True(t, resp.Allowed) +} + +// TestHandleCreateEmptyObject verifies empty CREATE object is rejected. +func TestHandleCreateEmptyObject(t *testing.T) { + validator := NewRepositoryValidator(fake.NewClientBuilder().WithScheme(newScheme()).Build()) + + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Create, + Object: runtime.RawExtension{Raw: []byte{}}, + }, + } + + resp := validator.Handle(context.Background(), req) + assert.False(t, resp.Allowed) +} + +// TestHandleCreateMalformedJSON verifies malformed JSON is rejected. +func TestHandleCreateMalformedJSON(t *testing.T) { + validator := NewRepositoryValidator(fake.NewClientBuilder().WithScheme(newScheme()).Build()) + + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Create, + Object: runtime.RawExtension{Raw: []byte(`{invalid}`)}, + }, + } + + resp := validator.Handle(context.Background(), req) + assert.False(t, resp.Allowed) +} + +// TestHandleUnknownOperation verifies unknown operations are allowed. +func TestHandleUnknownOperation(t *testing.T) { + validator := NewRepositoryValidator(fake.NewClientBuilder().WithScheme(newScheme()).Build()) + + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Connect, + }, + } + + resp := validator.Handle(context.Background(), req) + assert.True(t, resp.Allowed) +} + +// TestHandleCreateNoConflict verifies a valid CREATE without conflicts. +func TestHandleCreateNoConflict(t *testing.T) { + scheme := newScheme() + client := fake.NewClientBuilder().WithScheme(scheme).Build() + validator := NewRepositoryValidator(client) + + repo := makeRepo("new-repo", "ns1", "http://gitea.local/org/repo.git", "dir1", "main") + + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Create, + Object: runtime.RawExtension{Raw: marshalRepo(t, repo)}, + }, + } + + resp := validator.Handle(context.Background(), req) + assert.True(t, resp.Allowed) + assert.Contains(t, resp.Result.Message, "validated successfully") +} + +// TestHandleCreateWithConflict verifies CREATE is rejected when a conflict exists. +func TestHandleCreateWithConflict(t *testing.T) { + scheme := newScheme() + existing := makeRepo("existing-repo", "ns1", "http://gitea.local/org/repo.git", "dir1", "main") + client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(existing).Build() + validator := NewRepositoryValidator(client) + + attempted := makeRepo("new-repo", "ns1", "http://gitea.local/org/repo.git", "dir1", "main") + + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Create, + Object: runtime.RawExtension{Raw: marshalRepo(t, attempted)}, + }, + } + + resp := validator.Handle(context.Background(), req) + assert.False(t, resp.Allowed) + assert.Contains(t, resp.Result.Message, "conflict") +} + +// TestHandleUpdateWithConflict verifies UPDATE is rejected when it introduces a conflict. +func TestHandleUpdateWithConflict(t *testing.T) { + scheme := newScheme() + existing := makeRepo("existing-repo", "ns1", "http://gitea.local/org/repo.git", "", "main") + client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(existing).Build() + validator := NewRepositoryValidator(client) + + // Attempting to update a different repo to use a subdirectory of the root repo + attempted := makeRepo("other-repo", "ns1", "http://gitea.local/org/repo.git", "subdir", "main") + + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Update, + Object: runtime.RawExtension{Raw: marshalRepo(t, attempted)}, + }, + } + + resp := validator.Handle(context.Background(), req) + assert.False(t, resp.Allowed) + assert.Contains(t, resp.Result.Message, "conflict") +} + +// TestNormalizeURL tests URL normalization. +func TestNormalizeURL(t *testing.T) { + tests := []struct { + input string + expected string + }{ + {"http://172.18.255.200:3000/porch/myrepo.git", "http---172.18.255.200-3000-porch-myrepo.git"}, + {"https://github.com/org/repo.git", "https---github.com-org-repo.git"}, + {"ssh://git@host.com:2222/repo.git", "ssh---git@host.com-2222-repo.git"}, + } + + for _, tc := range tests { + t.Run(tc.input, func(t *testing.T) { + assert.Equal(t, tc.expected, NormalizeURL(tc.input)) + }) + } +} + +// TestIsNestedConflict tests nested directory detection. +func TestIsNestedConflict(t *testing.T) { + tests := []struct { + a, b string + expected bool + }{ + {"base", "base/sub", true}, + {"base/sub", "base", true}, + {"base", "base", false}, + {"base", "other", false}, + {"base/sub", "base/sub/deep", true}, + {"base/sub/deep", "base/sub", true}, + {"dir/sub/sub1", "dir/sub/sub2", false}, + } + + for _, tc := range tests { + t.Run(tc.a+"_vs_"+tc.b, func(t *testing.T) { + assert.Equal(t, tc.expected, IsNestedConflict(tc.a, tc.b)) + }) + } +} + +// TestIsConflict tests the full conflict detection logic. +func TestIsConflict(t *testing.T) { + tests := []struct { + name string + existing *configapi.Repository + attempt *configapi.Repository + expected bool + }{ + { + name: "same url, branch, dir, namespace → conflict", + existing: makeRepo("repo1", "ns1", "http://host/repo.git", "dir", "main"), + attempt: makeRepo("repo2", "ns1", "http://host/repo.git", "dir", "main"), + expected: true, + }, + { + name: "same url, branch, dir, different namespace → no conflict", + existing: makeRepo("repo1", "ns1", "http://host/repo.git", "dir", "main"), + attempt: makeRepo("repo2", "ns2", "http://host/repo.git", "dir", "main"), + expected: false, + }, + { + name: "different branch → no conflict", + existing: makeRepo("repo1", "ns1", "http://host/repo.git", "dir", "main"), + attempt: makeRepo("repo2", "ns1", "http://host/repo.git", "dir", "develop"), + expected: false, + }, + { + name: "different url → no conflict", + existing: makeRepo("repo1", "ns1", "http://host/repo1.git", "dir", "main"), + attempt: makeRepo("repo2", "ns1", "http://host/repo2.git", "dir", "main"), + expected: false, + }, + { + name: "root vs subdirectory → conflict", + existing: makeRepo("repo1", "ns1", "http://host/repo.git", "", "main"), + attempt: makeRepo("repo2", "ns1", "http://host/repo.git", "subdir", "main"), + expected: true, + }, + { + name: "subdirectory vs root → conflict", + existing: makeRepo("repo1", "ns1", "http://host/repo.git", "subdir", "main"), + attempt: makeRepo("repo2", "ns1", "http://host/repo.git", "", "main"), + expected: true, + }, + { + name: "nested directory → conflict", + existing: makeRepo("repo1", "ns1", "http://host/repo.git", "base", "main"), + attempt: makeRepo("repo2", "ns1", "http://host/repo.git", "base/sub", "main"), + expected: true, + }, + { + name: "sibling directories → no conflict", + existing: makeRepo("repo1", "ns1", "http://host/repo.git", "dir/sub/sub1", "main"), + attempt: makeRepo("repo2", "ns1", "http://host/repo.git", "dir/sub/sub2", "main"), + expected: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.expected, IsConflict(tc.existing, tc.attempt)) + }) + } +} + +// TestHandleConflictScenarios tests the full Handle flow for various conflict scenarios. +func TestHandleConflictScenarios(t *testing.T) { + tests := []struct { + name string + attempted *configapi.Repository + existing []configapi.Repository + expectPass bool + }{ + { + name: "no existing repos", + attempted: makeRepo("repo1", "ns1", "http://gitea/repo.git", "dir1", "main"), + existing: nil, + expectPass: true, + }, + { + name: "same git location different namespace → allowed", + attempted: makeRepo("repo2", "ns2", "http://gitea/repo.git", "dir1", "main"), + existing: []configapi.Repository{ + *makeRepo("repo1", "ns1", "http://gitea/repo.git", "dir1", "main"), + }, + expectPass: true, + }, + { + name: "same git location same namespace → conflict", + attempted: makeRepo("repo2", "ns1", "http://gitea/repo.git", "dir1", "main"), + existing: []configapi.Repository{ + *makeRepo("repo1", "ns1", "http://gitea/repo.git", "dir1", "main"), + }, + expectPass: false, + }, + { + name: "root dir conflicts with subdirectory", + attempted: makeRepo("repo2", "ns1", "http://gitea/repo.git", "subdir", "main"), + existing: []configapi.Repository{ + *makeRepo("repo1", "ns1", "http://gitea/repo.git", "", "main"), + }, + expectPass: false, + }, + { + name: "different branch → no conflict", + attempted: makeRepo("repo2", "ns1", "http://gitea/repo.git", "dir1", "develop"), + existing: []configapi.Repository{ + *makeRepo("repo1", "ns1", "http://gitea/repo.git", "dir1", "main"), + }, + expectPass: true, + }, + { + name: "updating self → no conflict", + attempted: makeRepo("repo1", "ns1", "http://gitea/repo.git", "dir1", "main"), + existing: []configapi.Repository{ + *makeRepo("repo1", "ns1", "http://gitea/repo.git", "dir1", "main"), + }, + expectPass: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + scheme := newScheme() + objs := make([]client.Object, 0, len(tc.existing)) + for i := range tc.existing { + objs = append(objs, &tc.existing[i]) + } + cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objs...).Build() + validator := NewRepositoryValidator(cl) + + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Create, + Object: runtime.RawExtension{Raw: marshalRepo(t, tc.attempted)}, + }, + } + + resp := validator.Handle(context.Background(), req) + if tc.expectPass { + assert.True(t, resp.Allowed, "expected allowed but got: %s", resp.Result.Message) + } else { + assert.False(t, resp.Allowed, "expected denied") + assert.Contains(t, resp.Result.Message, "conflict") + } + }) + } +} diff --git a/deployments/porch/3-porch-server.yaml b/deployments/porch/3-porch-server.yaml index aa9b2f797..4c79c8ce6 100644 --- a/deployments/porch/3-porch-server.yaml +++ b/deployments/porch/3-porch-server.yaml @@ -36,8 +36,6 @@ spec: volumes: - name: cache-volume emptyDir: {} - - name: webhook-certs - emptyDir: {} - name: api-server-certs emptyDir: {} containers: @@ -58,8 +56,6 @@ spec: volumeMounts: - mountPath: /cache name: cache-volume - - mountPath: /etc/webhook/certs - name: webhook-certs - name: api-server-certs mountPath: /tmp/certs envFrom: @@ -70,8 +66,6 @@ spec: env: - name: OTEL_SERVICE_NAME value: porch-server - - name: CERT_STORAGE_DIR - value: /etc/webhook/certs - name: GOOGLE_API_GO_EXPERIMENTAL_DISABLE_NEW_AUTH_LIB value: "true" - name: OTEL_METRICS_EXPORTER @@ -135,10 +129,6 @@ spec: protocol: TCP targetPort: 4443 name: api - - port: 8443 - protocol: TCP - targetPort: 8443 - name: webhooks - port: 9464 protocol: TCP targetPort: 9464 diff --git a/go.mod b/go.mod index 8af4247ef..54719ffd1 100644 --- a/go.mod +++ b/go.mod @@ -10,7 +10,6 @@ replace k8s.io/apiserver v0.36.1 => ./third_party/k8s.io/apiserver-v0.36.1 require ( cloud.google.com/go/iam v1.11.0 github.com/fergusstrange/embedded-postgres v1.34.0 - github.com/fsnotify/fsnotify v1.10.1 github.com/go-git/go-billy/v5 v5.9.0 github.com/go-git/go-git/v5 v5.19.1 github.com/golang/protobuf v1.5.4 @@ -110,6 +109,7 @@ require ( github.com/evanphx/json-patch/v5 v5.9.11 // indirect github.com/exponent-io/jsonpath v0.0.0-20210407135951-1de76d718b3f // indirect github.com/felixge/httpsnoop v1.0.4 // indirect + github.com/fsnotify/fsnotify v1.10.1 // indirect github.com/fxamacker/cbor/v2 v2.9.2 // indirect github.com/go-errors/errors v1.5.1 // indirect github.com/go-git/gcfg v1.5.1-0.20230307220236-3a3c6141e376 // indirect diff --git a/pkg/apiserver/webhooks.go b/pkg/apiserver/webhooks.go deleted file mode 100644 index ac98595ea..000000000 --- a/pkg/apiserver/webhooks.go +++ /dev/null @@ -1,679 +0,0 @@ -// Copyright 2022, 2024, 2026 The kpt Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package apiserver - -import ( - "bytes" - "context" - cryptorand "crypto/rand" - "crypto/rsa" - "crypto/tls" - "crypto/x509" - "crypto/x509/pkix" - "encoding/json" - "encoding/pem" - "fmt" - "io" - "math/big" - "net/http" - "os" - "path/filepath" - "strconv" - "strings" - "time" - - "github.com/fsnotify/fsnotify" - "go.opentelemetry.io/otel" - - configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" - "github.com/kptdev/porch/pkg/util" - admissionv1 "k8s.io/api/admission/v1" - admissionregistrationv1 "k8s.io/api/admissionregistration/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/serializer" - "k8s.io/client-go/kubernetes" - "k8s.io/klog/v2" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" -) - -const repositoryValidationEndpoint = "/validate-repository" - -var ( - cert tls.Certificate - certModTime time.Time -) - -var tracer = otel.Tracer("repository-webhook") - -// WebhookConfig defines the configuration for the Repository validation webhook -type WebhookConfig struct { - Port int32 - RepositoryPath string - RepoServiceName string - RepoServiceNamespace string - RepoHost string - CertStorageDir string - CertManWebhook bool - timeout int32 -} - -// newWebhookConfig creates a new WebhookConfig object filled with values read from environment variables -func newWebhookConfig(ctx context.Context) *WebhookConfig { - var cfg WebhookConfig - cfg.RepositoryPath = repositoryValidationEndpoint - cfg.RepoServiceName, cfg.RepoServiceNamespace = webhookServiceName(ctx) - cfg.RepoHost = fmt.Sprintf("%s.%s.svc", cfg.RepoServiceName, cfg.RepoServiceNamespace) - - cfg.Port = getEnvInt32("WEBHOOK_PORT", 8443) - cfg.CertStorageDir = getEnv("CERT_STORAGE_DIR", "/tmp/cert") - cfg.CertManWebhook = getEnvBool("USE_CERT_MAN_FOR_WEBHOOK", false) - return &cfg -} - -// webhookServiceName returns the name and namespace of Kubernetes service belonging to the webhook -func webhookServiceName(ctx context.Context) (serviceName, serviceNamespace string) { - var apiSvcNs string - - // the webhook service namespace gets it value from the following sources in order of precedence: - // - WEBHOOK_SERVICE_NAME environment variable - // - the name of the service referenced in porch's APIService object - serviceName = os.Getenv("WEBHOOK_SERVICE_NAME") - if serviceName == "" { // empty value and unset envvar are the same for our purposes - // if WEBHOOK_SERVICE_NAME is not set, try to use the porch API service name - apiSvc, err := util.GetPorchApiServiceKey(ctx) - if err != nil { - panic(fmt.Sprintf("WEBHOOK_SERVICE_NAME environment variable is not set, and could not find porch's APIservice: %v", err)) - } - serviceName = apiSvc.Name - apiSvcNs = apiSvc.Namespace // cache the namespace value to avoid duplicate calls of GetPorchApiServiceKey() - } - - // the webhook service namespace gets it value from the following sources in order of precedence: - // - WEBHOOK_SERVICE_NAMESPACE environment variable - // - CERT_NAMESPACE environment variable - // - the namespace of the service referenced in porch's APIService object - // - namespace of the current process (if running in a pod) - serviceNamespace = os.Getenv("WEBHOOK_SERVICE_NAMESPACE") - if serviceNamespace == "" { - serviceNamespace = os.Getenv("CERT_NAMESPACE") - } - if serviceNamespace == "" { - serviceNamespace = apiSvcNs - } - if serviceNamespace == "" { - apiSvc, err := util.GetPorchApiServiceKey(ctx) - if err == nil { - serviceNamespace = apiSvc.Namespace - } - } - if serviceNamespace == "" { - var err error - serviceNamespace, err = util.GetInClusterNamespace() - if err != nil { - // this was our last resort, so panic if failed - panic(fmt.Sprintf("WEBHOOK_SERVICE_NAMESPACE environment variable is not set, and couldn't deduce its value either: %v", err)) - } - } - // theoretically this should never happen, but this is a failsafe - if serviceName == "" || serviceNamespace == "" { - panic("Couldn't automatically determine a valid value for WEBHOOK_SERVICE_NAME and WEBHOOK_SERVICE_NAMESPACE environment variables. Please set them explicitly!") - } - return -} - -func setupWebhooks(ctx context.Context, clientReader client.Reader) error { - cfg := newWebhookConfig(ctx) - // TODO: Refactor webhook setup to support optional webhooks and better separation of concerns. - // Currently webhooks are always enabled and required for Repository validation. - // Consider: 1) Making webhooks optional via explicit flag, 2) Separating cert management from webhook lifecycle, - // 3) Supporting webhook-less mode for development/testing. - if !cfg.CertManWebhook { - caBytes, err := createCerts(cfg) - if err != nil { - return err - } - if err := createValidatingWebhook(ctx, cfg, caBytes); err != nil { - return err - } - } - - if err := runWebhookServer(ctx, cfg, clientReader); err != nil { - return err - } - return nil -} - -func createCerts(cfg *WebhookConfig) ([]byte, error) { - klog.Infof("creating self-signing TLS cert and key for %q in directory %s", cfg.RepoHost, cfg.CertStorageDir) - commonName := cfg.RepoHost - dnsNames := []string{commonName} - dnsNames = append(dnsNames, cfg.RepoServiceName) - dnsNames = append(dnsNames, fmt.Sprintf("%s.%s", cfg.RepoServiceName, cfg.RepoServiceNamespace)) - dnsNames = append(dnsNames, fmt.Sprintf("%s.%s.svc", cfg.RepoServiceName, cfg.RepoServiceNamespace)) - dnsNames = append(dnsNames, fmt.Sprintf("%s.%s.svc.cluster.local", cfg.RepoServiceName, cfg.RepoServiceNamespace)) - - var caPEM, serverCertPEM, serverPrivateKeyPEM *bytes.Buffer - // CA config - ca := &x509.Certificate{ - SerialNumber: big.NewInt(2020), - Subject: pkix.Name{ - Organization: []string{"google.com"}, - }, - NotBefore: time.Now(), - NotAfter: time.Now().AddDate(1, 0, 0), - IsCA: true, - ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth}, - KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, - BasicConstraintsValid: true, - } - - privateKey, err := rsa.GenerateKey(cryptorand.Reader, 4096) - if err != nil { - return nil, err - } - caBytes, err := x509.CreateCertificate(cryptorand.Reader, ca, ca, &privateKey.PublicKey, privateKey) - if err != nil { - return nil, err - } - caPEM = new(bytes.Buffer) - _ = pem.Encode(caPEM, &pem.Block{ - Type: "CERTIFICATE", - Bytes: caBytes, - }) - - // server cert config - cert := &x509.Certificate{ - DNSNames: dnsNames, - SerialNumber: big.NewInt(1658), - Subject: pkix.Name{ - CommonName: commonName, - Organization: []string{"google.com"}, - }, - NotBefore: time.Now(), - NotAfter: time.Now().AddDate(1, 0, 0), - SubjectKeyId: []byte{1, 2, 3, 4, 6}, - ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth}, - KeyUsage: x509.KeyUsageDigitalSignature, - } - - serverPrivateKey, err := rsa.GenerateKey(cryptorand.Reader, 4096) - if err != nil { - return nil, err - } - serverCertBytes, err := x509.CreateCertificate(cryptorand.Reader, cert, ca, &serverPrivateKey.PublicKey, privateKey) - if err != nil { - return nil, err - } - serverCertPEM = new(bytes.Buffer) - _ = pem.Encode(serverCertPEM, &pem.Block{ - Type: "CERTIFICATE", - Bytes: serverCertBytes, - }) - serverPrivateKeyPEM = new(bytes.Buffer) - _ = pem.Encode(serverPrivateKeyPEM, &pem.Block{ - Type: "RSA PRIVATE KEY", - Bytes: x509.MarshalPKCS1PrivateKey(serverPrivateKey), - }) - - err = os.MkdirAll(cfg.CertStorageDir, 0750) - if err != nil { - return nil, err - } - err = WriteFile(filepath.Join(cfg.CertStorageDir, "tls.crt"), serverCertPEM.Bytes()) - if err != nil { - return nil, err - } - err = WriteFile(filepath.Join(cfg.CertStorageDir, "tls.key"), serverPrivateKeyPEM.Bytes()) - if err != nil { - return nil, err - } - - return caPEM.Bytes(), nil -} - -// WriteFile writes data in the file at the given path -func WriteFile(filepath string, c []byte) error { - f, err := os.Create(filepath) - if err != nil { - return err - } - defer f.Close() - - _, err = f.Write(c) - if err != nil { - return err - } - return nil -} - -func createValidatingWebhook(ctx context.Context, cfg *WebhookConfig, caCert []byte) error { - - klog.Infof("Creating validating webhook for %s:%d", cfg.RepoHost, cfg.Port) - - kubeConfig := ctrl.GetConfigOrDie() - kubeClient, err := kubernetes.NewForConfig(kubeConfig) - if err != nil { - return fmt.Errorf("failed to setup kubeClient: %w", err) - } - // Set max timeout value for ValidatingWebhooks - cfg.timeout = 30 - var ( - repositoryCfgName = "repository-validating-webhook" - fail = admissionregistrationv1.Fail - none = admissionregistrationv1.SideEffectClassNone - ) - - // Webhook for Repository validation - repositoryWebhook := admissionregistrationv1.ValidatingWebhookConfiguration{ - ObjectMeta: metav1.ObjectMeta{ - Name: repositoryCfgName, - }, - Webhooks: []admissionregistrationv1.ValidatingWebhook{{ - Name: "porchrepositorywebhook.kpt.dev", - ClientConfig: admissionregistrationv1.WebhookClientConfig{ - CABundle: caCert, - }, - Rules: []admissionregistrationv1.RuleWithOperations{{ - Operations: []admissionregistrationv1.OperationType{ - admissionregistrationv1.Create, - admissionregistrationv1.Update, - admissionregistrationv1.Delete, - }, - Rule: admissionregistrationv1.Rule{ - APIGroups: []string{configapi.GroupVersion.Group}, - APIVersions: []string{configapi.GroupVersion.Version}, - Resources: []string{"repositories"}, - }, - }}, - AdmissionReviewVersions: []string{"v1"}, - SideEffects: &none, - FailurePolicy: &fail, - TimeoutSeconds: &cfg.timeout, - }}, - } - - // Set service for repository webhook - repositoryWebhook.Webhooks[0].ClientConfig.Service = &admissionregistrationv1.ServiceReference{ - Name: cfg.RepoServiceName, - Namespace: cfg.RepoServiceNamespace, - Path: &cfg.RepositoryPath, - Port: &cfg.Port, - } - - // Delete and recreate repository webhook to allow updates in webhook configurations - _ = kubeClient.AdmissionregistrationV1().ValidatingWebhookConfigurations().Delete(ctx, repositoryCfgName, metav1.DeleteOptions{}) - - if _, err := kubeClient.AdmissionregistrationV1().ValidatingWebhookConfigurations().Create(ctx, &repositoryWebhook, metav1.CreateOptions{}); err != nil { - return fmt.Errorf("failed to create repository validation webhook: %w", err) - } - - return nil -} - -// load the certificate & keep note of time loaded for reload on new cert details -func loadCertificate(certPath, keyPath string) (tls.Certificate, error) { - certInfo, err := os.Stat(certPath) - if err != nil { - return tls.Certificate{}, err - } - if certInfo.ModTime().After(certModTime) { - newCert, err := tls.LoadX509KeyPair(certPath, keyPath) - if err != nil { - return tls.Certificate{}, err - } - cert = newCert - certModTime = certInfo.ModTime() - } - return cert, nil -} - -// watch for changes on the mount path of the secret as volume -func watchCertificates(ctx context.Context, directory, certFile, keyFile string) { - // Set up a watcher for the certificate directory - watcher, err := fsnotify.NewWatcher() - if err != nil { - klog.Errorf("failed to start certificate watcher: %v", err) - return - } - defer watcher.Close() - // Start watching the directory - err = watcher.Add(directory) - if err != nil { - klog.Errorf("invalid certificate watcher directory : %v", err) - return - } - go func() { - for { - select { - case event, ok := <-watcher.Events: - if !ok { - return // Exit if the watcher.Events channel was closed - } - if event.Op&fsnotify.Create == fsnotify.Create || event.Op&fsnotify.Write == fsnotify.Write { - _, err := loadCertificate(certFile, keyFile) - if err != nil { - klog.Errorf("Failed to load updated certificate: %v", err) - } else { - klog.Info("Certificate reloaded successfully") - } - } - case err, ok := <-watcher.Errors: - if !ok { - return // Exit if the watcher.Errors channel was closed - } - klog.Errorf("Error watching certificates: %v", err) - case <-ctx.Done(): - return - } - } - }() - // Wait for the context to be canceled before returning and cleaning up - <-ctx.Done() - klog.Info("Shutting down certificate watcher") -} - -func getCertificate(*tls.ClientHelloInfo) (*tls.Certificate, error) { - return &cert, nil -} - -func runWebhookServer(ctx context.Context, cfg *WebhookConfig, clientReader client.Reader) error { - certFile := filepath.Join(cfg.CertStorageDir, "tls.crt") - keyFile := filepath.Join(cfg.CertStorageDir, "tls.key") - // load the cert for the first time - - _, err := loadCertificate(certFile, keyFile) - if err != nil { - klog.Errorf("failed to load certificate: %v", err) - return err - } - if cfg.CertManWebhook { - go watchCertificates(ctx, cfg.CertStorageDir, certFile, keyFile) - } - klog.Infoln("Starting webhook server") - http.HandleFunc(cfg.RepositoryPath, func(w http.ResponseWriter, r *http.Request) { - validateRepository(w, r, clientReader) - }) - server := http.Server{ - Addr: fmt.Sprintf(":%d", cfg.Port), - TLSConfig: &tls.Config{ - GetCertificate: getCertificate, - MinVersion: tls.VersionTLS12, - }, - ReadHeaderTimeout: 30 * time.Second, - } - go func() { - err = server.ListenAndServeTLS("", "") - if err != nil { - klog.Errorf("could not start server: %v", err) - } - }() - return err - -} - -func decodeAdmissionReview(r *http.Request) (*admissionv1.AdmissionReview, error) { - if r.Header.Get("Content-Type") != "application/json" { - return nil, fmt.Errorf("expected Content-Type 'application/json'") - } - var requestData []byte - if r.Body != nil { - var err error - requestData, err = io.ReadAll(r.Body) - if err != nil { - return nil, err - } - } - admissionReviewRequest := &admissionv1.AdmissionReview{} - deserializer := serializer.NewCodecFactory(runtime.NewScheme()).UniversalDeserializer() - if _, _, err := deserializer.Decode(requestData, nil, admissionReviewRequest); err != nil { - return nil, err - } - if admissionReviewRequest.Request == nil { - return nil, fmt.Errorf("admission review request is empty") - } - return admissionReviewRequest, nil -} - -func constructResponse(response *admissionv1.AdmissionResponse, - request *admissionv1.AdmissionReview) ([]byte, error) { - var admissionReviewResponse admissionv1.AdmissionReview - admissionReviewResponse.Response = response - admissionReviewResponse.SetGroupVersionKind(request.GroupVersionKind()) - admissionReviewResponse.Response.UID = request.Request.UID - - resp, err := json.Marshal(admissionReviewResponse) - if err != nil { - return nil, fmt.Errorf("error marshalling response json: %w", err) - } - return resp, nil -} - -func writeErr(errMsg string, w *http.ResponseWriter) { - klog.Errorf("%s", errMsg) - (*w).WriteHeader(500) - if _, err := (*w).Write([]byte(errMsg)); err != nil { // #nosec G705 - klog.Errorf("could not write error message: %v", err) - } -} - -func getEnv(key string, defaultValue string) string { - value, found := os.LookupEnv(key) - if !found { - return defaultValue - } - return value -} - -func getEnvBool(key string, defaultValue bool) bool { - value, found := os.LookupEnv(key) - if !found { - return defaultValue - } - boolean, err := strconv.ParseBool(value) - if err != nil { - panic("could not parse boolean from environment variable: " + key) - } - return boolean -} - -func getEnvInt32(key string, defaultValue int32) int32 { - value, found := os.LookupEnv(key) - if !found { - return defaultValue - } - i64, err := strconv.ParseInt(value, 10, 32) - if err != nil { - panic("could not parse int32 from environment variable: " + key) - } - return int32(i64) // this is safe because of the size parameter of the ParseInt call -} - -func validateRepository(w http.ResponseWriter, r *http.Request, clientReader client.Reader) { - ctx, span := tracer.Start(r.Context(), "validateRepository") - defer span.End() - - admissionReviewRequest, err := decodeAdmissionReview(r) - if err != nil { - writeErr(fmt.Sprintf("error decoding admission review: %v", err), &w) - return - } - - operation := strings.ToLower(string(admissionReviewRequest.Request.Operation)) - repoName := fmt.Sprintf("%s/%s", admissionReviewRequest.Request.Namespace, admissionReviewRequest.Request.Name) - klog.Infof("received request to validate repository %s for %s", repoName, operation) - - if admissionReviewRequest.Request.Resource.Resource != "repositories" { - writeErr(fmt.Sprintf("unexpected resource: %s", admissionReviewRequest.Request.Resource.Resource), &w) - return - } - - // For DELETE operations, Object.Raw is empty — the API server sends the object in OldObject. - // No conflict detection needed for deletes, just allow them through. - if admissionReviewRequest.Request.Operation == admissionv1.Delete { - klog.Infof("repository deletion validated for %s", repoName) - resp := &admissionv1.AdmissionResponse{ - Allowed: true, - Result: &metav1.Status{ - Status: "Success", - Message: "Repository deletion validated successfully", - }, - } - responseBytes, err := constructResponse(resp, admissionReviewRequest) - if err != nil { - writeErr(fmt.Sprintf("error constructing response: %v", err), &w) - return - } - w.Header().Set("Content-Type", "application/json") - _, err = w.Write(responseBytes) // #nosec G705 - if err != nil { - klog.Errorf("error writing response: %v", err) - return - } - return - } - - var attempted configapi.Repository - if err := json.Unmarshal(admissionReviewRequest.Request.Object.Raw, &attempted); err != nil { - klog.Errorf("failed to unmarshal repository object: %v", err) - writeErr(fmt.Sprintf("could not unmarshal repository: %v", err), &w) - return - } - - // NOTE: Immutability checks (URL, branch, directory) are handled by CEL validation in the CRD. - // This webhook only performs complex cross-resource conflict detection that CEL cannot do. - - // Check for conflicts with existing repositories - var repoList configapi.RepositoryList - if err := clientReader.List(ctx, &repoList); err != nil { - klog.Errorf("failed to list repositories: %v", err) - writeErr(fmt.Sprintf("could not list repositories: %v", err), &w) - return - } - - for _, existing := range repoList.Items { - if existing.Name == attempted.Name && existing.Namespace == attempted.Namespace { - continue - } - if isConflict(&existing, &attempted) { - klog.Errorf("repository validation failed: conflict detected between attempted %s/%s and existing %s/%s", attempted.Namespace, attempted.Name, existing.Namespace, existing.Name) - writeConflictResponse(fmt.Sprintf("Repository conflict with existing repository: %s/%s", existing.Namespace, existing.Name), "RepositoryConflict", admissionReviewRequest, &w) - return - } - } - - klog.V(1).Infof("repository validation passed for %s", repoName) - resp := &admissionv1.AdmissionResponse{ - Allowed: true, - Result: &metav1.Status{ - Status: "Success", - Message: "Repository validated successfully", - }, - } - responseBytes, err := constructResponse(resp, admissionReviewRequest) - if err != nil { - errMsg := fmt.Sprintf("error constructing response: %v", err) - writeErr(errMsg, &w) - return - } - - w.Header().Set("Content-Type", "application/json") - _, err = w.Write(responseBytes) // #nosec G705 - if err != nil { - klog.Errorf("error writing response: %v", err) - return - } -} - -func normalizeURL(url string) string { - // Convert URL to cache-safe format - // Example: http://172.18.255.200:3000/porch/myrepo.git → http---172.18.255.200-3000-porch-myrepo.git - replace := strings.NewReplacer("://", "---", ":", "-", "/", "-") - return replace.Replace(url) -} - -func isConflict(existing, attempted *configapi.Repository) bool { - existingURL := normalizeURL(existing.Spec.Git.Repo) - attemptedURL := normalizeURL(attempted.Spec.Git.Repo) - - existingDir := strings.Trim(existing.Spec.Git.Directory, "/") - attemptedDir := strings.Trim(attempted.Spec.Git.Directory, "/") - - // Branch defaults to "main" via CRD default, so no need to handle empty values - existingBranch := existing.Spec.Git.Branch - attemptedBranch := attempted.Spec.Git.Branch - - // Rule 1: Same URL, branch and directory in same namespace → conflict - // (Kubernetes only prevents duplicate names, not duplicate Git locations) - if existingURL == attemptedURL && existingBranch == attemptedBranch && existingDir == attemptedDir && - existing.Namespace == attempted.Namespace { - return true - } - - // Rule 2: Root directory conflicts with any other directory under same URL and branch - if existingURL == attemptedURL && existingBranch == attemptedBranch { - if (existingDir == "" && attemptedDir != "") || (existingDir != "" && attemptedDir == "") { - return true - } - } - - // Rule 3: Nested directory conflicts with its base directory under same URL and branch - if existingURL == attemptedURL && existingBranch == attemptedBranch { - if isNestedConflict(existingDir, attemptedDir) { - return true - } - } - - return false -} - -func isNestedConflict(a, b string) bool { - // Check if one path is nested within the other using filepath.Rel - relAtoB, err1 := filepath.Rel(a, b) - relBtoA, err2 := filepath.Rel(b, a) - - // If either relative path doesn't start with "../", it means one is nested in the other - if err1 == nil && !strings.HasPrefix(relAtoB, "../") && relAtoB != "." { - return true // b is nested within a - } - if err2 == nil && !strings.HasPrefix(relBtoA, "../") && relBtoA != "." { - return true // a is nested within b - } - - return false -} - -func writeConflictResponse(message, reason string, admissionReviewRequest *admissionv1.AdmissionReview, w *http.ResponseWriter) { - resp := &admissionv1.AdmissionResponse{ - Allowed: false, - Result: &metav1.Status{ - Status: "Failure", - Message: message, - Reason: metav1.StatusReason(reason), - }, - } - responseBytes, err := constructResponse(resp, admissionReviewRequest) - if err != nil { - klog.Errorf("failed to construct conflict response: %v", err) - writeErr(fmt.Sprintf("error constructing response: %v", err), w) - return - } - (*w).Header().Set("Content-Type", "application/json") - _, err = (*w).Write(responseBytes) // #nosec G705 - if err != nil { - klog.Errorf("error writing response: %v", err) - } -} diff --git a/pkg/apiserver/webhooks_test.go b/pkg/apiserver/webhooks_test.go deleted file mode 100644 index 6927f8768..000000000 --- a/pkg/apiserver/webhooks_test.go +++ /dev/null @@ -1,754 +0,0 @@ -// Copyright 2022,2024 The kpt Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package apiserver - -import ( - "bytes" - "context" - "encoding/json" - "errors" - "io" - "net/http" - "net/http/httptest" - "os" - "path/filepath" - "strings" - "testing" - "time" - - configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" - "github.com/stretchr/testify/require" - admissionv1 "k8s.io/api/admission/v1" - v1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "sigs.k8s.io/controller-runtime/pkg/client/fake" -) - -func TestCreateCerts(t *testing.T) { - webhookCfg := WebhookConfig{ - RepoHost: "localhost", - CertStorageDir: t.TempDir(), - } - defer func() { - require.NoError(t, os.RemoveAll(webhookCfg.CertStorageDir)) - }() - - caCert, err := createCerts(&webhookCfg) - require.NoError(t, err) - - caStr := strings.TrimSpace(string(caCert)) - require.True(t, strings.HasPrefix(caStr, "-----BEGIN CERTIFICATE-----\n")) - require.True(t, strings.HasSuffix(caStr, "\n-----END CERTIFICATE-----")) - - crt, err := os.ReadFile(filepath.Join(webhookCfg.CertStorageDir, "tls.crt")) - require.NoError(t, err) - - key, err := os.ReadFile(filepath.Join(webhookCfg.CertStorageDir, "tls.key")) - require.NoError(t, err) - - crtStr := strings.TrimSpace(string(crt)) - require.True(t, strings.HasPrefix(crtStr, "-----BEGIN CERTIFICATE-----\n")) - require.True(t, strings.HasSuffix(crtStr, "\n-----END CERTIFICATE-----")) - - keyStr := strings.TrimSpace(string(key)) - require.True(t, strings.HasPrefix(keyStr, "-----BEGIN RSA PRIVATE KEY-----\n")) - require.True(t, strings.HasSuffix(keyStr, "\n-----END RSA PRIVATE KEY-----")) -} - -func TestLoadCertificate(t *testing.T) { - - //what do i need to test. - // first create dummy certs for testing - webhookCfg := WebhookConfig{ - RepoHost: "localhost", - CertStorageDir: t.TempDir(), - } - defer func() { - require.NoError(t, os.RemoveAll(webhookCfg.CertStorageDir)) - }() - - _, err := createCerts(&webhookCfg) - require.NoError(t, err) - - //1. test file that cannot be os.stated or causes that function to fail - _, err1 := loadCertificate(filepath.Join(webhookCfg.CertStorageDir, "nonexistingcrtfile.key"), filepath.Join(webhookCfg.CertStorageDir, "nonexistingkeyfile.key")) - require.Error(t, err1) - // reseting back to 0 - certModTime = time.Time{} - //2. test happy path of os.stat and continue to next error - //3. test loading good cert happy path - keypath := filepath.Join(webhookCfg.CertStorageDir, "tls.key") - crtpath := filepath.Join(webhookCfg.CertStorageDir, "tls.crt") - - _, err2 := loadCertificate(crtpath, keypath) - require.NoError(t, err2) - certModTime = time.Time{} - - //4. test loading faulty cert error - data := []byte("Hello, World!") - - writeErr := os.WriteFile(keypath, data, 0644) - require.NoError(t, writeErr) - writeErr2 := os.WriteFile(crtpath, data, 0644) - require.NoError(t, writeErr2) - - _, err3 := loadCertificate(filepath.Join(webhookCfg.CertStorageDir, "tls.crt"), filepath.Join(webhookCfg.CertStorageDir, "tls.key")) - require.Error(t, err3) - certModTime = time.Time{} -} - -// method for capturing klog error's -func captureStderr(f func()) string { - read, write, _ := os.Pipe() - stderr := os.Stderr - os.Stderr = write - outputChannel := make(chan string) - - // Copy the output in a separate goroutine so printing can't block indefinitely. - go func() { - var buf bytes.Buffer - //nolint:errcheck - io.Copy(&buf, read) - outputChannel <- buf.String() - }() - - // Run the provided function and capture the stderr output - f() - - // Restore the original stderr and close the write-end of the pipe so the goroutine will exit - os.Stderr = stderr - write.Close() - out := <-outputChannel - - return out -} - -func TestWatchCertificatesInvalidDirectory(t *testing.T) { - // method for processing klog output - assertLogMessages := func(log string) error { - if len(log) > 0 { - if log[0] == 'E' || log[0] == 'W' || log[0] == 'F' { - return errors.New("Error Occurred in Watcher") - } - } - return nil - } - // Set up the temp directory with dummy certificate files - webhookCfg := WebhookConfig{ - RepoHost: "localhost", - CertStorageDir: t.TempDir(), - } - defer func() { - require.NoError(t, os.RemoveAll(webhookCfg.CertStorageDir)) - }() - - _, err := createCerts(&webhookCfg) - require.NoError(t, err) - - keyFile := filepath.Join(webhookCfg.CertStorageDir, "tls.key") - certFile := filepath.Join(webhookCfg.CertStorageDir, "tls.crt") - - // Create a context with a cancel function - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - invalid_watch_entity_logs := captureStderr(func() { - // firstly test error occurring from invalid entity for watcher to watch. aka invalid dir, err expected - go watchCertificates(ctx, "/random-dir-that-does-not-exist", certFile, keyFile) - time.Sleep(5 * time.Second) // Give some time for the logs to be flushed - }) - t.Log(invalid_watch_entity_logs) - err = assertLogMessages(invalid_watch_entity_logs) - require.Error(t, err) -} - -func TestWatchCertificatesSuccessfulReload(t *testing.T) { - // method for processing klog output - assertLogMessages := func(log string) error { - if len(log) > 0 { - if log[0] == 'E' || log[0] == 'W' || log[0] == 'F' { - return errors.New("Error Occured in Watcher") - } - } - return nil - } - // Set up the temp directory with dummy certificate files - webhookCfg := WebhookConfig{ - RepoHost: "localhost", - CertStorageDir: t.TempDir(), - } - defer func() { - require.NoError(t, os.RemoveAll(webhookCfg.CertStorageDir)) - }() - - _, err := createCerts(&webhookCfg) - require.NoError(t, err) - - keyFile := filepath.Join(webhookCfg.CertStorageDir, "tls.key") - certFile := filepath.Join(webhookCfg.CertStorageDir, "tls.crt") - - // Create a context with a cancel function - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - valid_reload_logs := captureStderr(func() { - go watchCertificates(ctx, webhookCfg.CertStorageDir, certFile, keyFile) - // give some time for watchCertificates method to spin up - time.Sleep(3 * time.Second) - - //create file to trigger change but not alter the certificate contents - //should trigger reload and certificate reloaded successfully - newFilePath := filepath.Join(webhookCfg.CertStorageDir, "new_temp_file.txt") - _, err = os.Create(newFilePath) - require.NoError(t, err) - - time.Sleep(5 * time.Second) // Give some time for the logs to be flushed - }) - t.Log(valid_reload_logs) - err = assertLogMessages(valid_reload_logs) - require.NoError(t, err) -} - -func TestWatchCertificatesInvalidCertReload(t *testing.T) { - - // method for processing klog output - assertLogMessages := func(log string) error { - if len(log) > 0 { - if log[0] == 'E' || log[0] == 'W' || log[0] == 'F' { - return errors.New("Error Occured in Watcher") - } - } - return nil - } - // Set up the temp directory with dummy certificate files - webhookCfg := WebhookConfig{ - RepoHost: "localhost", - CertStorageDir: t.TempDir(), - } - defer func() { - require.NoError(t, os.RemoveAll(webhookCfg.CertStorageDir)) - }() - - _, err := createCerts(&webhookCfg) - require.NoError(t, err) - - keyFile := filepath.Join(webhookCfg.CertStorageDir, "tls.key") - certFile := filepath.Join(webhookCfg.CertStorageDir, "tls.crt") - - // Create a context with a cancel function - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - invalid_reload_logs := captureStderr(func() { - go watchCertificates(ctx, webhookCfg.CertStorageDir, certFile, keyFile) - // give some time for watchCertificates method to spin up - time.Sleep(3 * time.Second) - - // Modify the certificate file to trigger a file system event - // should cause an error log since cert contents are not valid anymore - certModTime = time.Time{} - err = os.WriteFile(certFile, []byte("dummy text"), 0660) - require.NoError(t, err) - - time.Sleep(5 * time.Second) - }) - t.Log(invalid_reload_logs) - err = assertLogMessages(invalid_reload_logs) - require.Error(t, err) -} - -func TestWatchCertificatesGracefulTermination(t *testing.T) { - // method for processing klog output - assertLogMessages := func(log string) error { - if len(log) > 0 { - if log[0] == 'E' || log[0] == 'W' || log[0] == 'F' { - return errors.New("Error Occured in Watcher") - } - } - return nil - } - // Set up the temp directory with dummy certificate files - webhookCfg := WebhookConfig{ - RepoHost: "localhost", - CertStorageDir: t.TempDir(), - } - defer func() { - require.NoError(t, os.RemoveAll(webhookCfg.CertStorageDir)) - }() - - _, err := createCerts(&webhookCfg) - require.NoError(t, err) - - keyFile := filepath.Join(webhookCfg.CertStorageDir, "tls.key") - certFile := filepath.Join(webhookCfg.CertStorageDir, "tls.crt") - - // Create a context with a cancel function - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - graceful_termination_logs := captureStderr(func() { - go watchCertificates(ctx, webhookCfg.CertStorageDir, certFile, keyFile) - // give some time for watchCertificates method to spin up - time.Sleep(3 * time.Second) - - // trigger graceful termination - cancel() - - time.Sleep(5 * time.Second) - }) - t.Log(graceful_termination_logs) - err = assertLogMessages(graceful_termination_logs) - require.NoError(t, err) -} - -func TestValidateRepository(t *testing.T) { - scheme := runtime.NewScheme() - configapi.AddToScheme(scheme) - fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build() - t.Run("invalid content-type", func(t *testing.T) { - request, err := http.NewRequest(http.MethodPost, repositoryValidationEndpoint, nil) - require.NoError(t, err) - request.Header.Set("Content-Type", "foo") - response := httptest.NewRecorder() - - validateRepository(response, request, fakeClient) - require.Equal(t, - "error decoding admission review: expected Content-Type 'application/json'", - response.Body.String()) - }) - - t.Run("valid content-type, but no body", func(t *testing.T) { - request, err := http.NewRequest(http.MethodPost, repositoryValidationEndpoint, nil) - require.NoError(t, err) - request.Header.Set("Content-Type", "application/json") - response := httptest.NewRecorder() - - validateRepository(response, request, fakeClient) - require.Equal(t, - "error decoding admission review: admission review request is empty", - response.Body.String()) - }) - - t.Run("unexpected resource type", func(t *testing.T) { - request, err := http.NewRequest(http.MethodPost, repositoryValidationEndpoint, nil) - require.NoError(t, err) - request.Header.Set("Content-Type", "application/json") - - admissionReviewRequest := admissionv1.AdmissionReview{ - TypeMeta: v1.TypeMeta{ - Kind: "AdmissionReview", - APIVersion: "admission.k8s.io/v1", - }, - Request: &admissionv1.AdmissionRequest{ - Resource: v1.GroupVersionResource{ - Group: "config.porch.kpt.dev", - Version: "v1alpha1", - Resource: "not-repositories", - }, - }, - } - - body, err := json.Marshal(admissionReviewRequest) - require.NoError(t, err) - - request.Body = io.NopCloser(bytes.NewReader(body)) - response := httptest.NewRecorder() - - validateRepository(response, request, fakeClient) - require.Equal(t, - "unexpected resource: not-repositories", - response.Body.String()) - }) - - t.Run("delete operation skips unmarshal and allows", func(t *testing.T) { - admissionReview := admissionv1.AdmissionReview{ - TypeMeta: v1.TypeMeta{ - Kind: "AdmissionReview", - APIVersion: "admission.k8s.io/v1", - }, - Request: &admissionv1.AdmissionRequest{ - UID: "delete-123", - Resource: v1.GroupVersionResource{ - Group: "config.porch.kpt.dev", - Version: "v1alpha1", - Resource: "repositories", - }, - Operation: admissionv1.Delete, - Name: "my-repo", - Namespace: "test-ns", - // Object.Raw is empty on DELETE — this is the scenario that was failing - OldObject: runtime.RawExtension{Raw: []byte(`{"metadata":{"name":"my-repo","namespace":"test-ns"}}`)}, - }, - } - body, err := json.Marshal(admissionReview) - require.NoError(t, err) - - request, err := http.NewRequest(http.MethodPost, repositoryValidationEndpoint, bytes.NewReader(body)) - require.NoError(t, err) - request.Header.Set("Content-Type", "application/json") - - response := httptest.NewRecorder() - validateRepository(response, request, fakeClient) - - require.Equal(t, http.StatusOK, response.Code) - require.Contains(t, response.Body.String(), "Repository deletion validated successfully") - }) - - createAdmissionReview := func(name, namespace, gitURL, directory, branch string) []byte { - repo := configapi.Repository{ - ObjectMeta: v1.ObjectMeta{ - Name: name, - Namespace: namespace, - }, - Spec: configapi.RepositorySpec{ - Git: &configapi.GitRepository{ - Repo: gitURL, - Directory: directory, - Branch: branch, - }, - }, - } - rawRepo, err := json.Marshal(repo) - require.NoError(t, err) - - admissionReview := admissionv1.AdmissionReview{ - TypeMeta: v1.TypeMeta{ - Kind: "AdmissionReview", - APIVersion: "admission.k8s.io/v1", - }, - Request: &admissionv1.AdmissionRequest{ - UID: "12345", - Resource: v1.GroupVersionResource{ - Group: "config.porch.kpt.dev", - Version: "v1alpha1", - Resource: "repositories", - }, - Object: runtime.RawExtension{Raw: rawRepo}, - Name: name, - Namespace: namespace, - }, - } - body, err := json.Marshal(admissionReview) - require.NoError(t, err) - return body - } - tests := []struct { - name string - repoName string - namespace string - gitURL string - directory string - branch string - setupRepos []configapi.Repository - expectOK bool - }{ - { - name: "valid repository creation", - repoName: "repo1", - namespace: "ns1", - gitURL: "http://gitea.local/myrepo.git", - directory: "dir1", - branch: "main", - expectOK: true, - }, - { - name: "same git url and directory in different namespace", - repoName: "repo2", - namespace: "ns2", - gitURL: "http://gitea.local/myrepo.git", - directory: "dir1", - branch: "main", - setupRepos: []configapi.Repository{ - { - ObjectMeta: v1.ObjectMeta{Name: "repo1", Namespace: "ns1"}, - Spec: configapi.RepositorySpec{ - Git: &configapi.GitRepository{Repo: "http://gitea.local/myrepo.git", Directory: "dir1", Branch: "main"}, - }, - }, - }, - expectOK: true, - }, - { - name: "same git url different directory in same namespace", - repoName: "repo3", - namespace: "ns1", - gitURL: "http://gitea.local/myrepo.git", - directory: "dir2", - branch: "main", - setupRepos: []configapi.Repository{ - { - ObjectMeta: v1.ObjectMeta{Name: "repo1", Namespace: "ns1"}, - Spec: configapi.RepositorySpec{ - Git: &configapi.GitRepository{Repo: "http://gitea.local/myrepo.git", Directory: "dir1", Branch: "main"}, - }, - }, - }, - expectOK: true, - }, - { - name: "same url, directory different branch - no conflict", - repoName: "repo4", - namespace: "ns1", - gitURL: "http://gitea.local/myrepo.git", - directory: "dir1", - branch: "develop", - setupRepos: []configapi.Repository{ - { - ObjectMeta: v1.ObjectMeta{Name: "repo1", Namespace: "ns1"}, - Spec: configapi.RepositorySpec{ - Git: &configapi.GitRepository{Repo: "http://gitea.local/myrepo.git", Directory: "dir1", Branch: "main"}, - }, - }, - }, - expectOK: true, - }, - { - name: "conflict: same url, branch, directory, and namespace", - repoName: "repo5", - namespace: "ns1", - gitURL: "http://gitea.local/myrepo.git", - directory: "dir1", - branch: "main", - setupRepos: []configapi.Repository{ - { - ObjectMeta: v1.ObjectMeta{Name: "repo1", Namespace: "ns1"}, - Spec: configapi.RepositorySpec{ - Git: &configapi.GitRepository{Repo: "http://gitea.local/myrepo.git", Directory: "dir1", Branch: "main"}, - }, - }, - }, - expectOK: false, - }, - { - name: "conflict: root directory vs subdirectory", - repoName: "repo6", - namespace: "ns1", - gitURL: "http://gitea.local/myrepo.git", - directory: "subdir", - branch: "main", - setupRepos: []configapi.Repository{ - { - ObjectMeta: v1.ObjectMeta{Name: "repo1", Namespace: "ns1"}, - Spec: configapi.RepositorySpec{ - Git: &configapi.GitRepository{Repo: "http://gitea.local/myrepo.git", Directory: "", Branch: "main"}, - }, - }, - }, - expectOK: false, - }, - { - name: "conflict: nested directory", - repoName: "repo7", - namespace: "ns1", - gitURL: "http://gitea.local/myrepo.git", - directory: "base/sub", - branch: "main", - setupRepos: []configapi.Repository{ - { - ObjectMeta: v1.ObjectMeta{Name: "repo1", Namespace: "ns1"}, - Spec: configapi.RepositorySpec{ - Git: &configapi.GitRepository{Repo: "http://gitea.local/myrepo.git", Directory: "base", Branch: "main"}, - }, - }, - }, - expectOK: false, - }, - { - name: "Valid: same url, directory, namespace different branch", - repoName: "repo8", - namespace: "ns1", - gitURL: "http://gitea.local/myrepo.git", - directory: "dir1", - branch: "myBranch", - setupRepos: []configapi.Repository{ - { - ObjectMeta: v1.ObjectMeta{Name: "repo1", Namespace: "ns1"}, - Spec: configapi.RepositorySpec{ - Git: &configapi.GitRepository{Repo: "http://gitea.local/myrepo.git", Directory: "dir1", Branch: "main"}, - }, - }, - }, - expectOK: true, - }, - { - name: "conflict: branch defaults to main (CRD handles defaulting)", - repoName: "repo9", - namespace: "ns1", - gitURL: "http://gitea.local/myrepo.git", - directory: "dir1", - branch: "main", - setupRepos: []configapi.Repository{ - { - ObjectMeta: v1.ObjectMeta{Name: "repo1", Namespace: "ns1"}, - Spec: configapi.RepositorySpec{ - Git: &configapi.GitRepository{Repo: "http://gitea.local/myrepo.git", Directory: "dir1", Branch: "main"}, - }, - }, - }, - expectOK: false, - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - // Create a fresh client for each test - scheme := runtime.NewScheme() - configapi.AddToScheme(scheme) - testClient := fake.NewClientBuilder().WithScheme(scheme).Build() - - // Setup existing repositories - for _, repo := range tc.setupRepos { - err := testClient.Create(context.Background(), &repo) - require.NoError(t, err) - } - - body := createAdmissionReview(tc.repoName, tc.namespace, tc.gitURL, tc.directory, tc.branch) - request, err := http.NewRequest(http.MethodPost, repositoryValidationEndpoint, bytes.NewReader(body)) - require.NoError(t, err) - request.Header.Set("Content-Type", "application/json") - - response := httptest.NewRecorder() - validateRepository(response, request, testClient) - - if tc.expectOK { - require.Contains(t, response.Body.String(), "Repository validated successfully") - } else { - require.Contains(t, response.Body.String(), "Repository conflict") - } - }) - } -} - -func TestNormalizeURL(t *testing.T) { - tests := []struct { - input string - expected string - }{ - { - input: "http://172.18.255.200:3000/porch/myrepo.git", - expected: "http---172.18.255.200-3000-porch-myrepo.git", - }, - { - input: "https://github.com/org/repo.git", - expected: "https---github.com-org-repo.git", - }, - { - input: "ssh://git@host.com:2222/repo.git", - expected: "ssh---git@host.com-2222-repo.git", - }, - } - - for _, tc := range tests { - t.Run(tc.input, func(t *testing.T) { - actual := normalizeURL(tc.input) - require.Equal(t, tc.expected, actual) - }) - } -} - -func TestIsNestedConflict(t *testing.T) { - tests := []struct { - a, b string - expected bool - }{ - {"base", "base/sub", true}, - {"base/sub", "base", true}, - {"base", "base", false}, - {"base", "other", false}, - {"base/sub", "base/sub/deep", true}, - {"base/sub/deep", "base/sub", true}, - } - - for _, tc := range tests { - t.Run(tc.a+"_"+tc.b, func(t *testing.T) { - actual := isNestedConflict(tc.a, tc.b) - require.Equal(t, tc.expected, actual) - }) - } -} - -func TestIsConflict(t *testing.T) { - makeRepo := func(name, ns, url, dir string) *configapi.Repository { - return &configapi.Repository{ - ObjectMeta: v1.ObjectMeta{ - Name: name, - Namespace: ns, - }, - Spec: configapi.RepositorySpec{ - Git: &configapi.GitRepository{ - Repo: url, - Directory: dir, - }, - }, - } - } - - tests := []struct { - name string - existing *configapi.Repository - attempt *configapi.Repository - expected bool - }{ - { - name: "same url and dir in same namespace", - existing: makeRepo("repo1", "ns1", "http://host/repo.git", "dir"), - attempt: makeRepo("repo2", "ns1", "http://host/repo.git", "dir"), - expected: true, - }, - { - name: "same url and dir in different namespace", - existing: makeRepo("repo1", "ns1", "http://host/repo.git", "dir"), - attempt: makeRepo("repo2", "ns2", "http://host/repo.git", "dir"), - expected: false, - }, - { - name: "root vs non-root conflict", - existing: makeRepo("repo1", "ns1", "http://host/repo.git", ""), - attempt: makeRepo("repo2", "ns1", "http://host/repo.git", "dir/sub"), - expected: true, - }, - { - name: "non-root vs root conflict", - existing: makeRepo("repo1", "ns1", "http://host/repo.git", "dir/sub"), - attempt: makeRepo("repo2", "ns1", "http://host/repo.git", ""), - expected: true, - }, - { - name: "nested conflict", - existing: makeRepo("repo1", "ns1", "http://host/repo.git", "base"), - attempt: makeRepo("repo2", "ns1", "http://host/repo.git", "base/sub"), - expected: true, - }, - { - name: "no conflict different url", - existing: makeRepo("repo1", "ns1", "http://host/repo1.git", "dir"), - attempt: makeRepo("repo2", "ns1", "http://host/repo2.git", "dir"), - expected: false, - }, - { - name: "no conflict subdirectories", - existing: makeRepo("repo1", "ns1", "http://host/repo.git", "dir/sub/sub1"), - attempt: makeRepo("repo2", "ns1", "http://host/repo.git", "dir/sub/sub2"), - expected: false, - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - actual := isConflict(tc.existing, tc.attempt) - require.Equal(t, tc.expected, actual) - }) - } -} From 278d883c308cb66001b4570f8f6a409d54cf4a42 Mon Sep 17 00:00:00 2001 From: Fiachra Corcoran Date: Wed, 22 Jul 2026 16:04:36 +0100 Subject: [PATCH 2/7] Unique name for each webhook Signed-off-by: Fiachra Corcoran --- controllers/packagerevisions/config/webhook/manifests.yaml | 2 +- .../controllers/packagerevision/packagerevision_controller.go | 2 +- controllers/repositories/config/webhook/manifests.yaml | 2 +- .../pkg/controllers/repository/repository_controller.go | 1 + go.mod | 1 - 5 files changed, 4 insertions(+), 4 deletions(-) diff --git a/controllers/packagerevisions/config/webhook/manifests.yaml b/controllers/packagerevisions/config/webhook/manifests.yaml index 03ff184bc..6e4eff10a 100644 --- a/controllers/packagerevisions/config/webhook/manifests.yaml +++ b/controllers/packagerevisions/config/webhook/manifests.yaml @@ -15,7 +15,7 @@ apiVersion: admissionregistration.k8s.io/v1 kind: ValidatingWebhookConfiguration metadata: - name: validating-webhook-configuration + name: packagerevision-validating-webhook-configuration webhooks: - admissionReviewVersions: - v1 diff --git a/controllers/packagerevisions/pkg/controllers/packagerevision/packagerevision_controller.go b/controllers/packagerevisions/pkg/controllers/packagerevision/packagerevision_controller.go index 364a2739d..a9b54c851 100644 --- a/controllers/packagerevisions/pkg/controllers/packagerevision/packagerevision_controller.go +++ b/controllers/packagerevisions/pkg/controllers/packagerevision/packagerevision_controller.go @@ -33,8 +33,8 @@ import ( //go:generate go run sigs.k8s.io/controller-tools/cmd/controller-gen@v0.21.0 rbac:headerFile=../../../../../scripts/boilerplate.yaml.txt,roleName=porch-controllers-packagerevisions,year=$YEAR_GEN webhook:headerFile=../../../../../scripts/boilerplate.yaml.txt,year=$YEAR_GEN paths="." output:rbac:artifacts:config=../../../config/rbac output:webhook:artifacts:config=../../../config/webhook +//+kubebuilder:webhookconfiguration:mutating=false,name=packagerevision-validating-webhook-configuration //+kubebuilder:webhook:path=/validate-porch-kpt-dev-v1alpha2-packagerevision,mutating=false,failurePolicy=fail,groups=porch.kpt.dev,resources=packagerevisions,verbs=create;update;delete,versions=v1alpha2,name=packagerevision-validator.porch.kpt.dev,admissionReviewVersions=v1,sideEffects=None,serviceName=porch-controllers,serviceNamespace=porch-system,servicePort=9443,timeoutSeconds=30 - //+kubebuilder:rbac:groups=porch.kpt.dev,resources=packagerevisions,verbs=get;list;watch;update;patch //+kubebuilder:rbac:groups=porch.kpt.dev,resources=packagerevisions/status,verbs=get;update;patch //+kubebuilder:rbac:groups=porch.kpt.dev,resources=packagerevisions/finalizers,verbs=update diff --git a/controllers/repositories/config/webhook/manifests.yaml b/controllers/repositories/config/webhook/manifests.yaml index 5dd37a456..f2664d75c 100644 --- a/controllers/repositories/config/webhook/manifests.yaml +++ b/controllers/repositories/config/webhook/manifests.yaml @@ -15,7 +15,7 @@ apiVersion: admissionregistration.k8s.io/v1 kind: ValidatingWebhookConfiguration metadata: - name: validating-webhook-configuration + name: repository-validating-webhook-configuration webhooks: - admissionReviewVersions: - v1 diff --git a/controllers/repositories/pkg/controllers/repository/repository_controller.go b/controllers/repositories/pkg/controllers/repository/repository_controller.go index 92a444e13..546d66d6f 100644 --- a/controllers/repositories/pkg/controllers/repository/repository_controller.go +++ b/controllers/repositories/pkg/controllers/repository/repository_controller.go @@ -76,6 +76,7 @@ type RepositoryReconciler struct { //go:generate go run sigs.k8s.io/controller-tools/cmd/controller-gen@v0.21.0 rbac:headerFile=../../../../../scripts/boilerplate.yaml.txt,roleName=porch-controllers-repositories,year=$YEAR_GEN webhook:headerFile=../../../../../scripts/boilerplate.yaml.txt,year=$YEAR_GEN paths="." output:rbac:artifacts:config=../../../config/rbac output:webhook:artifacts:config=../../../config/webhook +//+kubebuilder:webhookconfiguration:mutating=false,name=repository-validating-webhook-configuration //+kubebuilder:webhook:path=/validate-repository,mutating=false,failurePolicy=fail,groups=config.porch.kpt.dev,resources=repositories,verbs=create;update;delete,versions=v1alpha1,name=repository-validator.porch.kpt.dev,admissionReviewVersions=v1,sideEffects=None,serviceName=porch-controllers,serviceNamespace=porch-system,servicePort=9443,timeoutSeconds=30 //+kubebuilder:rbac:groups=config.porch.kpt.dev,resources=repositories,verbs=get;list;watch;create;update;patch;delete diff --git a/go.mod b/go.mod index 54719ffd1..349f2579d 100644 --- a/go.mod +++ b/go.mod @@ -235,7 +235,6 @@ require ( k8s.io/kube-openapi v0.0.0-20260520065146-aa012df4f4af // indirect k8s.io/streaming v0.36.1 // indirect sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.36.0 // indirect - sigs.k8s.io/cli-utils v0.37.2 // indirect sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730 // indirect sigs.k8s.io/kustomize/api v0.21.1 // indirect sigs.k8s.io/randfill v1.0.0 // indirect From 104e3a1e834340f1fa1f9d0e76ede83a4135f226 Mon Sep 17 00:00:00 2001 From: Fiachra Corcoran Date: Fri, 24 Jul 2026 09:16:55 +0100 Subject: [PATCH 3/7] Address Copilot review comments on repository webhook - Use GetAPIReader() instead of GetClient() for strong consistency during admission validation (prevents cache-induced race conditions) - Scope repository list query to namespace level to reduce admission latency at scale - Test UPDATE operation semantics explicitly for the 'updating self' test case - Fix misleading documentation comment to link to actual deployment config All unit tests pass. No lint issues. Signed-off-by: Fiachra Corcoran --- .../repositories/pkg/controllers/repository/config.go | 5 +++-- .../repositories/pkg/webhooks/repository_webhook.go | 3 ++- .../repositories/pkg/webhooks/repository_webhook_test.go | 8 +++++++- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/controllers/repositories/pkg/controllers/repository/config.go b/controllers/repositories/pkg/controllers/repository/config.go index c57e5d9c6..238427b4d 100644 --- a/controllers/repositories/pkg/controllers/repository/config.go +++ b/controllers/repositories/pkg/controllers/repository/config.go @@ -97,8 +97,9 @@ func (r *RepositoryReconciler) Init(mgr ctrl.Manager) error { // Register Repository validating webhook. // The validator implements admission.Handler interface via its Handle method. - // Webhook TLS certificates are mounted from Secret at /etc/webhook/certs (see deployment). - validator := webhooks.NewRepositoryValidator(mgr.GetClient()) + // Use GetAPIReader() for strong consistency during admission validation. + // See: deployments/porch/3-porch-controllers.yaml + validator := webhooks.NewRepositoryValidator(mgr.GetAPIReader()) mgr.GetWebhookServer().Register( "/validate-repository", diff --git a/controllers/repositories/pkg/webhooks/repository_webhook.go b/controllers/repositories/pkg/webhooks/repository_webhook.go index de35042d7..551c925d6 100644 --- a/controllers/repositories/pkg/webhooks/repository_webhook.go +++ b/controllers/repositories/pkg/webhooks/repository_webhook.go @@ -81,7 +81,8 @@ func (v *RepositoryValidator) handleCreateOrUpdate(ctx context.Context, req admi // This webhook only performs complex cross-resource conflict detection that CEL cannot do. var repoList configapi.RepositoryList - if err := v.client.List(ctx, &repoList); err != nil { + opts := []client.ListOption{client.InNamespace(attempted.Namespace)} + if err := v.client.List(ctx, &repoList, opts...); err != nil { logger.Error(err, "failed to list repositories for conflict check") return admission.Errored(http.StatusInternalServerError, fmt.Errorf("could not list repositories: %w", err)) diff --git a/controllers/repositories/pkg/webhooks/repository_webhook_test.go b/controllers/repositories/pkg/webhooks/repository_webhook_test.go index 3d155aa12..15875d382 100644 --- a/controllers/repositories/pkg/webhooks/repository_webhook_test.go +++ b/controllers/repositories/pkg/webhooks/repository_webhook_test.go @@ -353,9 +353,15 @@ func TestHandleConflictScenarios(t *testing.T) { cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objs...).Build() validator := NewRepositoryValidator(cl) + operation := admissionv1.Create + // Use Update operation for the "updating self" test case to verify UPDATE semantics + if tc.name == "updating self → no conflict" { + operation = admissionv1.Update + } + req := admission.Request{ AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Create, + Operation: operation, Object: runtime.RawExtension{Raw: marshalRepo(t, tc.attempted)}, }, } From 4f4b99ea76f0dfd5861642ab3a06b6e83cf1eb20 Mon Sep 17 00:00:00 2001 From: Fiachra Corcoran Date: Fri, 24 Jul 2026 12:57:31 +0100 Subject: [PATCH 4/7] Increase test coverage Signed-off-by: Fiachra Corcoran --- .../pkg/controllers/repository/config_test.go | 250 +++++++++++++++ .../pkg/webhooks/repository_webhook_test.go | 288 ++++++++++++++++++ 2 files changed, 538 insertions(+) diff --git a/controllers/repositories/pkg/controllers/repository/config_test.go b/controllers/repositories/pkg/controllers/repository/config_test.go index c9f83c910..8393fa227 100644 --- a/controllers/repositories/pkg/controllers/repository/config_test.go +++ b/controllers/repositories/pkg/controllers/repository/config_test.go @@ -15,12 +15,19 @@ package repository import ( + "context" "flag" + "net/http" "testing" "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/healthz" + "sigs.k8s.io/controller-runtime/pkg/manager" + "sigs.k8s.io/controller-runtime/pkg/webhook" ) func TestInitDefaults(t *testing.T) { @@ -127,3 +134,246 @@ func TestLogConfig(t *testing.T) { }) } } + +func TestValidateConfig(t *testing.T) { + tests := []struct { + name string + input *RepositoryReconciler + expected *RepositoryReconciler + }{ + { + name: "all valid values - no changes", + input: &RepositoryReconciler{ + HealthCheckFrequency: 5 * time.Minute, + FullSyncFrequency: 1 * time.Hour, + MaxConcurrentReconciles: 100, + MaxConcurrentSyncs: 50, + SyncStaleTimeout: 20 * time.Minute, + RepoOperationRetryAttempts: 3, + }, + expected: &RepositoryReconciler{ + HealthCheckFrequency: 5 * time.Minute, + FullSyncFrequency: 1 * time.Hour, + MaxConcurrentReconciles: 100, + MaxConcurrentSyncs: 50, + SyncStaleTimeout: 20 * time.Minute, + RepoOperationRetryAttempts: 3, + }, + }, + { + name: "zero health check - uses default", + input: &RepositoryReconciler{ + HealthCheckFrequency: 0, + FullSyncFrequency: 1 * time.Hour, + MaxConcurrentReconciles: 100, + MaxConcurrentSyncs: 50, + SyncStaleTimeout: 20 * time.Minute, + RepoOperationRetryAttempts: 3, + }, + expected: &RepositoryReconciler{ + HealthCheckFrequency: 5 * time.Minute, + FullSyncFrequency: 1 * time.Hour, + MaxConcurrentReconciles: 100, + MaxConcurrentSyncs: 50, + SyncStaleTimeout: 20 * time.Minute, + RepoOperationRetryAttempts: 3, + }, + }, + { + name: "negative full sync - uses default", + input: &RepositoryReconciler{ + HealthCheckFrequency: 5 * time.Minute, + FullSyncFrequency: -1 * time.Hour, + MaxConcurrentReconciles: 100, + MaxConcurrentSyncs: 50, + SyncStaleTimeout: 20 * time.Minute, + RepoOperationRetryAttempts: 3, + }, + expected: &RepositoryReconciler{ + HealthCheckFrequency: 5 * time.Minute, + FullSyncFrequency: 1 * time.Hour, + MaxConcurrentReconciles: 100, + MaxConcurrentSyncs: 50, + SyncStaleTimeout: 20 * time.Minute, + RepoOperationRetryAttempts: 3, + }, + }, + { + name: "zero stale timeout - uses default", + input: &RepositoryReconciler{ + HealthCheckFrequency: 5 * time.Minute, + FullSyncFrequency: 1 * time.Hour, + MaxConcurrentReconciles: 100, + MaxConcurrentSyncs: 50, + SyncStaleTimeout: 0, + RepoOperationRetryAttempts: 3, + }, + expected: &RepositoryReconciler{ + HealthCheckFrequency: 5 * time.Minute, + FullSyncFrequency: 1 * time.Hour, + MaxConcurrentReconciles: 100, + MaxConcurrentSyncs: 50, + SyncStaleTimeout: 20 * time.Minute, + RepoOperationRetryAttempts: 3, + }, + }, + { + name: "zero max concurrent reconciles - uses default", + input: &RepositoryReconciler{ + HealthCheckFrequency: 5 * time.Minute, + FullSyncFrequency: 1 * time.Hour, + MaxConcurrentReconciles: 0, + MaxConcurrentSyncs: 50, + SyncStaleTimeout: 20 * time.Minute, + RepoOperationRetryAttempts: 3, + }, + expected: &RepositoryReconciler{ + HealthCheckFrequency: 5 * time.Minute, + FullSyncFrequency: 1 * time.Hour, + MaxConcurrentReconciles: 100, + MaxConcurrentSyncs: 50, + SyncStaleTimeout: 20 * time.Minute, + RepoOperationRetryAttempts: 3, + }, + }, + { + name: "negative max concurrent syncs - uses default", + input: &RepositoryReconciler{ + HealthCheckFrequency: 5 * time.Minute, + FullSyncFrequency: 1 * time.Hour, + MaxConcurrentReconciles: 100, + MaxConcurrentSyncs: -10, + SyncStaleTimeout: 20 * time.Minute, + RepoOperationRetryAttempts: 3, + }, + expected: &RepositoryReconciler{ + HealthCheckFrequency: 5 * time.Minute, + FullSyncFrequency: 1 * time.Hour, + MaxConcurrentReconciles: 100, + MaxConcurrentSyncs: 50, + SyncStaleTimeout: 20 * time.Minute, + RepoOperationRetryAttempts: 3, + }, + }, + { + name: "zero retry attempts - uses default", + input: &RepositoryReconciler{ + HealthCheckFrequency: 5 * time.Minute, + FullSyncFrequency: 1 * time.Hour, + MaxConcurrentReconciles: 100, + MaxConcurrentSyncs: 50, + SyncStaleTimeout: 20 * time.Minute, + RepoOperationRetryAttempts: 0, + }, + expected: &RepositoryReconciler{ + HealthCheckFrequency: 5 * time.Minute, + FullSyncFrequency: 1 * time.Hour, + MaxConcurrentReconciles: 100, + MaxConcurrentSyncs: 50, + SyncStaleTimeout: 20 * time.Minute, + RepoOperationRetryAttempts: 3, + }, + }, + { + name: "all invalid - all use defaults", + input: &RepositoryReconciler{ + HealthCheckFrequency: 0, + FullSyncFrequency: -1, + MaxConcurrentReconciles: -1, + MaxConcurrentSyncs: 0, + SyncStaleTimeout: 0, + RepoOperationRetryAttempts: -10, + }, + expected: &RepositoryReconciler{ + HealthCheckFrequency: 5 * time.Minute, + FullSyncFrequency: 1 * time.Hour, + MaxConcurrentReconciles: 100, + MaxConcurrentSyncs: 50, + SyncStaleTimeout: 20 * time.Minute, + RepoOperationRetryAttempts: 3, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tt.input.validateConfig() + assert.Equal(t, tt.expected.HealthCheckFrequency, tt.input.HealthCheckFrequency) + assert.Equal(t, tt.expected.FullSyncFrequency, tt.input.FullSyncFrequency) + assert.Equal(t, tt.expected.MaxConcurrentReconciles, tt.input.MaxConcurrentReconciles) + assert.Equal(t, tt.expected.MaxConcurrentSyncs, tt.input.MaxConcurrentSyncs) + assert.Equal(t, tt.expected.SyncStaleTimeout, tt.input.SyncStaleTimeout) + assert.Equal(t, tt.expected.RepoOperationRetryAttempts, tt.input.RepoOperationRetryAttempts) + }) + } +} + +// fakeManager is a minimal manager.Manager for unit testing Init(). +// Only GetClient(), GetAPIReader(), and GetWebhookServer() are implemented; all other methods will panic if called. +type fakeManager struct { + manager.Manager + client client.Client +} + +func (f *fakeManager) GetClient() client.Client { + return f.client +} + +func (f *fakeManager) GetAPIReader() client.Reader { + return f.client +} + +func (f *fakeManager) GetWebhookServer() webhook.Server { + return &fakeWebhookServer{} +} + +// fakeWebhookServer is a minimal webhook.Server for testing. +type fakeWebhookServer struct{} + +func (f *fakeWebhookServer) Register(path string, handler http.Handler) { + // No-op for testing +} + +func (f *fakeWebhookServer) Start(ctx context.Context) error { + return nil +} + +func (f *fakeWebhookServer) NeedLeaderElection() bool { + return false +} + +func (f *fakeWebhookServer) StartedChecker() healthz.Checker { + // Return a no-op checker for testing + return healthz.Ping +} + +func (f *fakeWebhookServer) WebhookMux() *http.ServeMux { + return nil +} + +func TestInit(t *testing.T) { + tests := []struct { + name string + wantErr bool + }{ + { + name: "successfully registers repository webhook", + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reconciler := &RepositoryReconciler{} + mgr := &fakeManager{client: fake.NewClientBuilder().Build()} + + err := reconciler.Init(mgr) + + if tt.wantErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} diff --git a/controllers/repositories/pkg/webhooks/repository_webhook_test.go b/controllers/repositories/pkg/webhooks/repository_webhook_test.go index 15875d382..cfbe06cd7 100644 --- a/controllers/repositories/pkg/webhooks/repository_webhook_test.go +++ b/controllers/repositories/pkg/webhooks/repository_webhook_test.go @@ -213,6 +213,14 @@ func TestIsNestedConflict(t *testing.T) { {"base/sub", "base/sub/deep", true}, {"base/sub/deep", "base/sub", true}, {"dir/sub/sub1", "dir/sub/sub2", false}, + // Additional edge cases + {"", "", false}, + {".", ".", false}, + {"/", "/", false}, + {"a/b/c", "a/b", true}, + {"a/b", "a/b/c/d/e", true}, + {"pkg", "package", false}, + {"config", "config-old", false}, } for _, tc := range tests { @@ -376,3 +384,283 @@ func TestHandleConflictScenarios(t *testing.T) { }) } } + +// TestNamespaceScopedConflictDetection verifies that conflict detection +// correctly scopes to namespace level and allows same git location in different namespaces +func TestNamespaceScopedConflictDetection(t *testing.T) { + scheme := newScheme() + repoNs1 := makeRepo("repo1", "namespace-1", "http://gitea.local/org/repo.git", "dir1", "main") + repoNs2 := makeRepo("repo2", "namespace-2", "http://gitea.local/org/repo.git", "dir1", "main") + + // Create client with repo in ns1 + client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(repoNs1).Build() + validator := NewRepositoryValidator(client) + + // Try to create same location in different namespace - should succeed + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Create, + Object: runtime.RawExtension{Raw: marshalRepo(t, repoNs2)}, + Namespace: "namespace-2", + }, + } + + resp := validator.Handle(context.Background(), req) + assert.True(t, resp.Allowed, "same git location should be allowed in different namespace") +} + +// TestRootDirectoryConflictDetection verifies root directory conflicts with subdirectories +func TestRootDirectoryConflictDetection(t *testing.T) { + tests := []struct { + name string + existing *configapi.Repository + attempted *configapi.Repository + expectPass bool + }{ + { + name: "root conflicts with subdir", + existing: makeRepo("root-repo", "ns1", "http://gitea.local/org/repo.git", "", "main"), + attempted: makeRepo("sub-repo", "ns1", "http://gitea.local/org/repo.git", "packages/config", "main"), + expectPass: false, + }, + { + name: "subdir conflicts with root", + existing: makeRepo("sub-repo", "ns1", "http://gitea.local/org/repo.git", "packages/config", "main"), + attempted: makeRepo("root-repo", "ns1", "http://gitea.local/org/repo.git", "", "main"), + expectPass: false, + }, + { + name: "different root dirs allowed", + existing: makeRepo("pkg1", "ns1", "http://gitea.local/org/repo.git", "", "main"), + attempted: makeRepo("pkg2", "ns2", "http://gitea.local/org/repo.git", "", "main"), + expectPass: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + scheme := newScheme() + client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tc.existing).Build() + validator := NewRepositoryValidator(client) + + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Create, + Object: runtime.RawExtension{Raw: marshalRepo(t, tc.attempted)}, + Namespace: tc.attempted.Namespace, + }, + } + + resp := validator.Handle(context.Background(), req) + if tc.expectPass { + assert.True(t, resp.Allowed, "expected allowed: %s", resp.Result.Message) + } else { + assert.False(t, resp.Allowed, "expected denied") + assert.Contains(t, resp.Result.Message, "conflict") + } + }) + } +} + +// TestNestedDirectoryConflictDetection verifies nested directory conflicts +func TestNestedDirectoryConflictDetection(t *testing.T) { + tests := []struct { + name string + existing *configapi.Repository + attempted *configapi.Repository + expectPass bool + }{ + { + name: "deep nested conflicts", + existing: makeRepo("parent", "ns1", "http://gitea.local/org/repo.git", "config", "main"), + attempted: makeRepo("child", "ns1", "http://gitea.local/org/repo.git", "config/overlays/prod", "main"), + expectPass: false, + }, + { + name: "sibling dirs allowed", + existing: makeRepo("base", "ns1", "http://gitea.local/org/repo.git", "config/base", "main"), + attempted: makeRepo("overlay", "ns1", "http://gitea.local/org/repo.git", "config/overlays", "main"), + expectPass: true, + }, + { + name: "same dir same ns conflict", + existing: makeRepo("repo1", "ns1", "http://gitea.local/org/repo.git", "packages", "main"), + attempted: makeRepo("repo2", "ns1", "http://gitea.local/org/repo.git", "packages", "main"), + expectPass: false, + }, + { + name: "same dir different ns allowed", + existing: makeRepo("repo1", "ns1", "http://gitea.local/org/repo.git", "packages", "main"), + attempted: makeRepo("repo2", "ns2", "http://gitea.local/org/repo.git", "packages", "main"), + expectPass: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + scheme := newScheme() + client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tc.existing).Build() + validator := NewRepositoryValidator(client) + + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Create, + Object: runtime.RawExtension{Raw: marshalRepo(t, tc.attempted)}, + Namespace: tc.attempted.Namespace, + }, + } + + resp := validator.Handle(context.Background(), req) + if tc.expectPass { + assert.True(t, resp.Allowed, "expected allowed: %s", resp.Result.Message) + } else { + assert.False(t, resp.Allowed, "expected denied") + assert.Contains(t, resp.Result.Message, "conflict") + } + }) + } +} + +// TestDeleteOperation verifies delete operations are always allowed +func TestDeleteOperation(t *testing.T) { + validator := NewRepositoryValidator(fake.NewClientBuilder().Build()) + + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Delete, + Name: "some-repo", + Namespace: "default", + }, + } + + resp := validator.Handle(context.Background(), req) + assert.True(t, resp.Allowed) + assert.Contains(t, resp.Result.Message, "validated successfully") +} + +// TestNormalizeURLVariants tests URL normalization with various formats +func TestNormalizeURLVariants(t *testing.T) { + tests := []struct { + input string + expected string + }{ + {"http://example.com/repo.git", "http---example.com-repo.git"}, + {"https://github.com:443/org/repo.git", "https---github.com-443-org-repo.git"}, + {"ssh://git@host.com:2222/repo.git", "ssh---git@host.com-2222-repo.git"}, + {"git@github.com:org/repo.git", "git@github.com-org-repo.git"}, + {"http://localhost:8080/path/to/repo.git", "http---localhost-8080-path-to-repo.git"}, + {"file:///local/path/repo.git", "file----local-path-repo.git"}, + } + + for _, tc := range tests { + t.Run(tc.input, func(t *testing.T) { + result := NormalizeURL(tc.input) + assert.Equal(t, tc.expected, result) + }) + } +} + +// TestBranchHandling tests that different branches don't conflict +func TestBranchHandling(t *testing.T) { + tests := []struct { + name string + existing *configapi.Repository + attempted *configapi.Repository + expectPass bool + }{ + { + name: "different branches - no conflict", + existing: makeRepo("main-repo", "ns1", "http://gitea.local/org/repo.git", "dir1", "main"), + attempted: makeRepo("dev-repo", "ns1", "http://gitea.local/org/repo.git", "dir1", "develop"), + expectPass: true, + }, + { + name: "same branch conflict", + existing: makeRepo("repo1", "ns1", "http://gitea.local/org/repo.git", "dir1", "main"), + attempted: makeRepo("repo2", "ns1", "http://gitea.local/org/repo.git", "dir1", "main"), + expectPass: false, + }, + { + name: "release branches - no conflict", + existing: makeRepo("release-v1", "ns1", "http://gitea.local/org/repo.git", "dir1", "release-1.0"), + attempted: makeRepo("release-v2", "ns1", "http://gitea.local/org/repo.git", "dir1", "release-2.0"), + expectPass: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + scheme := newScheme() + client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tc.existing).Build() + validator := NewRepositoryValidator(client) + + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Create, + Object: runtime.RawExtension{Raw: marshalRepo(t, tc.attempted)}, + Namespace: tc.attempted.Namespace, + }, + } + + resp := validator.Handle(context.Background(), req) + if tc.expectPass { + assert.True(t, resp.Allowed, "expected allowed: %s", resp.Result.Message) + } else { + assert.False(t, resp.Allowed, "expected denied") + assert.Contains(t, resp.Result.Message, "conflict") + } + }) + } +} + +// TestURLVariations tests that URL variations (http vs https, different ports) don't conflict +func TestURLVariations(t *testing.T) { + tests := []struct { + name string + existing *configapi.Repository + attempted *configapi.Repository + expectPass bool + }{ + { + name: "http vs https - treated as different", + existing: makeRepo("repo1", "ns1", "http://gitea.local/org/repo.git", "dir1", "main"), + attempted: makeRepo("repo2", "ns1", "https://gitea.local/org/repo.git", "dir1", "main"), + expectPass: true, + }, + { + name: "different ports - treated as different", + existing: makeRepo("repo1", "ns1", "http://gitea.local:3000/org/repo.git", "dir1", "main"), + attempted: makeRepo("repo2", "ns1", "http://gitea.local:8080/org/repo.git", "dir1", "main"), + expectPass: true, + }, + { + name: "exact same URL - conflict", + existing: makeRepo("repo1", "ns1", "http://gitea.local/org/repo.git", "dir1", "main"), + attempted: makeRepo("repo2", "ns1", "http://gitea.local/org/repo.git", "dir1", "main"), + expectPass: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + scheme := newScheme() + client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tc.existing).Build() + validator := NewRepositoryValidator(client) + + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Create, + Object: runtime.RawExtension{Raw: marshalRepo(t, tc.attempted)}, + Namespace: tc.attempted.Namespace, + }, + } + + resp := validator.Handle(context.Background(), req) + if tc.expectPass { + assert.True(t, resp.Allowed, "expected allowed: %s", resp.Result.Message) + } else { + assert.False(t, resp.Allowed, "expected denied") + } + }) + } +} From 0ca628a7c965747591ab14a6cae36ed1eb8a190c Mon Sep 17 00:00:00 2001 From: Fiachra Corcoran Date: Fri, 24 Jul 2026 14:38:41 +0100 Subject: [PATCH 5/7] Block repo spec.git secret updates Signed-off-by: Fiachra Corcoran --- .../pkg/webhooks/repository_webhook.go | 48 +++ .../pkg/webhooks/repository_webhook_test.go | 402 +++++++++++++++++- 2 files changed, 449 insertions(+), 1 deletion(-) diff --git a/controllers/repositories/pkg/webhooks/repository_webhook.go b/controllers/repositories/pkg/webhooks/repository_webhook.go index 551c925d6..6446aad8d 100644 --- a/controllers/repositories/pkg/webhooks/repository_webhook.go +++ b/controllers/repositories/pkg/webhooks/repository_webhook.go @@ -77,6 +77,23 @@ func (v *RepositoryValidator) handleCreateOrUpdate(ctx context.Context, req admi fmt.Errorf("failed to unmarshal repository: %w", err)) } + // Block updates to git secret reference + if req.Operation == admissionv1.Update { + var existing configapi.Repository + if err := json.Unmarshal(req.OldObject.Raw, &existing); err != nil { + return admission.Errored(http.StatusInternalServerError, + fmt.Errorf("failed to unmarshal existing repository: %w", err)) + } + + if isSecretRefChanged(&existing, &attempted) { + logger.V(3).Info("secret reference change denied", + "namespace", attempted.Namespace, "name", attempted.Name) + return admission.Denied( + "spec.git.secretRef and spec.oci.secretRef are immutable and cannot be updated. " + + "Delete and recreate the repository to change the secret reference.") + } + } + // NOTE: Immutability checks (URL, branch, directory) are handled by CEL validation in the CRD. // This webhook only performs complex cross-resource conflict detection that CEL cannot do. @@ -168,3 +185,34 @@ func IsNestedConflict(a, b string) bool { return false } + +// isSecretRefChanged checks if either git or OCI secret reference has changed between existing and attempted repositories. +func isSecretRefChanged(existing, attempted *configapi.Repository) bool { + // Check git secret reference + existingGitSecret := "" + if existing.Spec.Git != nil { + existingGitSecret = existing.Spec.Git.SecretRef.Name + } + attemptedGitSecret := "" + if attempted.Spec.Git != nil { + attemptedGitSecret = attempted.Spec.Git.SecretRef.Name + } + if existingGitSecret != attemptedGitSecret { + return true + } + + // Check OCI secret reference + existingOciSecret := "" + if existing.Spec.Oci != nil { + existingOciSecret = existing.Spec.Oci.SecretRef.Name + } + attemptedOciSecret := "" + if attempted.Spec.Oci != nil { + attemptedOciSecret = attempted.Spec.Oci.SecretRef.Name + } + if existingOciSecret != attemptedOciSecret { + return true + } + + return false +} diff --git a/controllers/repositories/pkg/webhooks/repository_webhook_test.go b/controllers/repositories/pkg/webhooks/repository_webhook_test.go index cfbe06cd7..22dd86353 100644 --- a/controllers/repositories/pkg/webhooks/repository_webhook_test.go +++ b/controllers/repositories/pkg/webhooks/repository_webhook_test.go @@ -17,6 +17,8 @@ package webhooks import ( "context" "encoding/json" + "fmt" + "net/http" "testing" configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" @@ -27,6 +29,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" ) @@ -170,10 +173,18 @@ func TestHandleUpdateWithConflict(t *testing.T) { // Attempting to update a different repo to use a subdirectory of the root repo attempted := makeRepo("other-repo", "ns1", "http://gitea.local/org/repo.git", "subdir", "main") + oldData, err := json.Marshal(attempted) + require.NoError(t, err) + newData, err := json.Marshal(attempted) + require.NoError(t, err) + req := admission.Request{ AdmissionRequest: admissionv1.AdmissionRequest{ Operation: admissionv1.Update, - Object: runtime.RawExtension{Raw: marshalRepo(t, attempted)}, + Object: runtime.RawExtension{Raw: newData}, + OldObject: runtime.RawExtension{Raw: oldData}, + Namespace: attempted.Namespace, + Name: attempted.Name, }, } @@ -374,6 +385,11 @@ func TestHandleConflictScenarios(t *testing.T) { }, } + // For UPDATE operations, also pass OldObject + if operation == admissionv1.Update { + req.OldObject = runtime.RawExtension{Raw: marshalRepo(t, tc.attempted)} + } + resp := validator.Handle(context.Background(), req) if tc.expectPass { assert.True(t, resp.Allowed, "expected allowed but got: %s", resp.Result.Message) @@ -664,3 +680,387 @@ func TestURLVariations(t *testing.T) { }) } } + +// TestIsSecretRefChanged tests secret reference change detection +func TestIsSecretRefChanged(t *testing.T) { + tests := []struct { + name string + existing *configapi.Repository + attempt *configapi.Repository + expected bool + }{ + { + name: "no secret refs - no change", + existing: &configapi.Repository{ + Spec: configapi.RepositorySpec{ + Git: &configapi.GitRepository{}, + }, + }, + attempt: &configapi.Repository{ + Spec: configapi.RepositorySpec{ + Git: &configapi.GitRepository{}, + }, + }, + expected: false, + }, + { + name: "same git secret - no change", + existing: &configapi.Repository{ + Spec: configapi.RepositorySpec{ + Git: &configapi.GitRepository{ + SecretRef: configapi.SecretRef{Name: "my-secret"}, + }, + }, + }, + attempt: &configapi.Repository{ + Spec: configapi.RepositorySpec{ + Git: &configapi.GitRepository{ + SecretRef: configapi.SecretRef{Name: "my-secret"}, + }, + }, + }, + expected: false, + }, + { + name: "git secret changed - change detected", + existing: &configapi.Repository{ + Spec: configapi.RepositorySpec{ + Git: &configapi.GitRepository{ + SecretRef: configapi.SecretRef{Name: "old-secret"}, + }, + }, + }, + attempt: &configapi.Repository{ + Spec: configapi.RepositorySpec{ + Git: &configapi.GitRepository{ + SecretRef: configapi.SecretRef{Name: "new-secret"}, + }, + }, + }, + expected: true, + }, + { + name: "git secret added - change detected", + existing: &configapi.Repository{ + Spec: configapi.RepositorySpec{ + Git: &configapi.GitRepository{}, + }, + }, + attempt: &configapi.Repository{ + Spec: configapi.RepositorySpec{ + Git: &configapi.GitRepository{ + SecretRef: configapi.SecretRef{Name: "new-secret"}, + }, + }, + }, + expected: true, + }, + { + name: "git secret removed - change detected", + existing: &configapi.Repository{ + Spec: configapi.RepositorySpec{ + Git: &configapi.GitRepository{ + SecretRef: configapi.SecretRef{Name: "old-secret"}, + }, + }, + }, + attempt: &configapi.Repository{ + Spec: configapi.RepositorySpec{ + Git: &configapi.GitRepository{}, + }, + }, + expected: true, + }, + { + name: "same OCI secret - no change", + existing: &configapi.Repository{ + Spec: configapi.RepositorySpec{ + Oci: &configapi.OciRepository{ + SecretRef: configapi.SecretRef{Name: "oci-secret"}, + }, + }, + }, + attempt: &configapi.Repository{ + Spec: configapi.RepositorySpec{ + Oci: &configapi.OciRepository{ + SecretRef: configapi.SecretRef{Name: "oci-secret"}, + }, + }, + }, + expected: false, + }, + { + name: "OCI secret changed - change detected", + existing: &configapi.Repository{ + Spec: configapi.RepositorySpec{ + Oci: &configapi.OciRepository{ + SecretRef: configapi.SecretRef{Name: "old-oci-secret"}, + }, + }, + }, + attempt: &configapi.Repository{ + Spec: configapi.RepositorySpec{ + Oci: &configapi.OciRepository{ + SecretRef: configapi.SecretRef{Name: "new-oci-secret"}, + }, + }, + }, + expected: true, + }, + { + name: "git present in existing, OCI in attempt - change detected", + existing: &configapi.Repository{ + Spec: configapi.RepositorySpec{ + Git: &configapi.GitRepository{ + SecretRef: configapi.SecretRef{Name: "git-secret"}, + }, + }, + }, + attempt: &configapi.Repository{ + Spec: configapi.RepositorySpec{ + Oci: &configapi.OciRepository{ + SecretRef: configapi.SecretRef{Name: "oci-secret"}, + }, + }, + }, + expected: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + result := isSecretRefChanged(tc.existing, tc.attempt) + assert.Equal(t, tc.expected, result) + }) + } +} + +// TestUpdateBlocksSecretRefChange verifies UPDATE blocks secret reference changes +func TestUpdateBlocksSecretRefChange(t *testing.T) { + scheme := newScheme() + + tests := []struct { + name string + existing *configapi.Repository + attempted *configapi.Repository + expectPass bool + message string + }{ + { + name: "git secret change blocked", + existing: makeRepo("repo1", "ns1", "http://gitea.local/org/repo.git", "dir1", "main"), + attempted: func() *configapi.Repository { + r := makeRepo("repo1", "ns1", "http://gitea.local/org/repo.git", "dir1", "main") + r.Spec.Git.SecretRef = configapi.SecretRef{Name: "new-secret"} + return r + }(), + expectPass: false, + message: "immutable", + }, + { + name: "git secret addition blocked", + existing: makeRepo("repo1", "ns1", "http://gitea.local/org/repo.git", "dir1", "main"), + attempted: func() *configapi.Repository { + r := makeRepo("repo1", "ns1", "http://gitea.local/org/repo.git", "dir1", "main") + r.Spec.Git.SecretRef = configapi.SecretRef{Name: "new-secret"} + return r + }(), + expectPass: false, + message: "immutable", + }, + { + name: "git secret removal blocked", + existing: func() *configapi.Repository { + r := makeRepo("repo1", "ns1", "http://gitea.local/org/repo.git", "dir1", "main") + r.Spec.Git.SecretRef = configapi.SecretRef{Name: "old-secret"} + return r + }(), + attempted: makeRepo("repo1", "ns1", "http://gitea.local/org/repo.git", "dir1", "main"), + expectPass: false, + message: "immutable", + }, + { + name: "same git secret allowed", + existing: func() *configapi.Repository { + r := makeRepo("repo1", "ns1", "http://gitea.local/org/repo.git", "dir1", "main") + r.Spec.Git.SecretRef = configapi.SecretRef{Name: "my-secret"} + return r + }(), + attempted: func() *configapi.Repository { + r := makeRepo("repo1", "ns1", "http://gitea.local/org/repo.git", "dir1", "main") + r.Spec.Git.SecretRef = configapi.SecretRef{Name: "my-secret"} + return r + }(), + expectPass: true, + message: "", + }, + { + name: "other field changed but secret same - allowed", + existing: func() *configapi.Repository { + r := makeRepo("repo1", "ns1", "http://gitea.local/org/repo.git", "dir1", "main") + r.Spec.Git.SecretRef = configapi.SecretRef{Name: "my-secret"} + r.Spec.Description = "old description" + return r + }(), + attempted: func() *configapi.Repository { + r := makeRepo("repo1", "ns1", "http://gitea.local/org/repo.git", "dir1", "main") + r.Spec.Git.SecretRef = configapi.SecretRef{Name: "my-secret"} + r.Spec.Description = "new description" + return r + }(), + expectPass: true, + message: "", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tc.existing).Build() + validator := NewRepositoryValidator(client) + + oldData, err := json.Marshal(tc.existing) + require.NoError(t, err) + newData, err := json.Marshal(tc.attempted) + require.NoError(t, err) + + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Update, + Object: runtime.RawExtension{Raw: newData}, + OldObject: runtime.RawExtension{Raw: oldData}, + Namespace: tc.existing.Namespace, + Name: tc.existing.Name, + }, + } + + resp := validator.Handle(context.Background(), req) + if tc.expectPass { + assert.True(t, resp.Allowed, "expected allowed but got: %s", resp.Result.Message) + } else { + assert.False(t, resp.Allowed, "expected denied") + assert.Contains(t, resp.Result.Message, tc.message, "expected message contains: %s", tc.message) + } + }) + } +} + +// TestCreateAllowsSecretRef verifies CREATE allows secret references +func TestCreateAllowsSecretRef(t *testing.T) { + scheme := newScheme() + client := fake.NewClientBuilder().WithScheme(scheme).Build() + validator := NewRepositoryValidator(client) + + repo := makeRepo("new-repo", "ns1", "http://gitea.local/org/repo.git", "dir1", "main") + repo.Spec.Git.SecretRef = configapi.SecretRef{Name: "my-secret"} + + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Create, + Object: runtime.RawExtension{Raw: marshalRepo(t, repo)}, + Namespace: repo.Namespace, + }, + } + + resp := validator.Handle(context.Background(), req) + assert.True(t, resp.Allowed, "CREATE should allow secret references: %s", resp.Result.Message) +} + +// TestOCISecretRefBlocking verifies OCI secret refs are also protected +func TestOCISecretRefBlocking(t *testing.T) { + scheme := newScheme() + + existing := &configapi.Repository{ + ObjectMeta: metav1.ObjectMeta{Name: "oci-repo", Namespace: "ns1"}, + Spec: configapi.RepositorySpec{ + Type: configapi.RepositoryTypeOCI, + Oci: &configapi.OciRepository{ + Registry: "gcr.io/my-project", + SecretRef: configapi.SecretRef{Name: "old-secret"}, + }, + }, + } + + attempted := &configapi.Repository{ + ObjectMeta: metav1.ObjectMeta{Name: "oci-repo", Namespace: "ns1"}, + Spec: configapi.RepositorySpec{ + Type: configapi.RepositoryTypeOCI, + Oci: &configapi.OciRepository{ + Registry: "gcr.io/my-project", + SecretRef: configapi.SecretRef{Name: "new-secret"}, + }, + }, + } + + client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(existing).Build() + validator := NewRepositoryValidator(client) + + oldData, err := json.Marshal(existing) + require.NoError(t, err) + newData, err := json.Marshal(attempted) + require.NoError(t, err) + + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Update, + Object: runtime.RawExtension{Raw: newData}, + OldObject: runtime.RawExtension{Raw: oldData}, + Namespace: "ns1", + Name: "oci-repo", + }, + } + + resp := validator.Handle(context.Background(), req) + assert.False(t, resp.Allowed, "UPDATE should block OCI secret ref changes") + assert.Contains(t, resp.Result.Message, "immutable") +} + +// TestHandleCreateOrUpdateListError verifies that list errors are handled gracefully +func TestHandleCreateOrUpdateListError(t *testing.T) { + scheme := newScheme() + failingClient := fake.NewClientBuilder().WithScheme(scheme). + WithInterceptorFuncs(interceptor.Funcs{ + List: func(ctx context.Context, client client.WithWatch, list client.ObjectList, opts ...client.ListOption) error { + return fmt.Errorf("mock list error") + }, + }).Build() + validator := NewRepositoryValidator(failingClient) + + repo := makeRepo("test-repo", "ns1", "http://gitea.local/org/repo.git", "dir1", "main") + + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Create, + Object: runtime.RawExtension{Raw: marshalRepo(t, repo)}, + Namespace: repo.Namespace, + }, + } + + resp := validator.Handle(context.Background(), req) + assert.False(t, resp.Allowed) + assert.Equal(t, int32(http.StatusInternalServerError), resp.Result.Code) + assert.Contains(t, resp.Result.Message, "could not list repositories") +} + +// TestHandleUpdateMalformedOldObject verifies malformed OldObject on UPDATE is rejected +func TestHandleUpdateMalformedOldObject(t *testing.T) { + scheme := newScheme() + client := fake.NewClientBuilder().WithScheme(scheme).Build() + validator := NewRepositoryValidator(client) + + repo := makeRepo("test-repo", "ns1", "http://gitea.local/org/repo.git", "dir1", "main") + + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Update, + Object: runtime.RawExtension{Raw: marshalRepo(t, repo)}, + OldObject: runtime.RawExtension{Raw: []byte(`{invalid}`)}, + Namespace: repo.Namespace, + Name: repo.Name, + }, + } + + resp := validator.Handle(context.Background(), req) + assert.False(t, resp.Allowed) + assert.Equal(t, int32(http.StatusInternalServerError), resp.Result.Code) + assert.Contains(t, resp.Result.Message, "failed to unmarshal existing repository") +} From 0062504cdb50b4c58bc9e4fe6910cd201a6f5e1a Mon Sep 17 00:00:00 2001 From: Fiachra Corcoran Date: Fri, 24 Jul 2026 15:08:40 +0100 Subject: [PATCH 6/7] Move repo secret validation to CEL Signed-off-by: Fiachra Corcoran --- .../config.porch.kpt.dev_repositories.yaml | 6 + api/porchconfig/v1alpha1/repository_types.go | 2 + .../pkg/webhooks/repository_webhook.go | 50 +-- .../pkg/webhooks/repository_webhook_test.go | 386 +----------------- 4 files changed, 10 insertions(+), 434 deletions(-) diff --git a/api/generated/crds/config.porch.kpt.dev_repositories.yaml b/api/generated/crds/config.porch.kpt.dev_repositories.yaml index d058256e2..063fe7050 100644 --- a/api/generated/crds/config.porch.kpt.dev_repositories.yaml +++ b/api/generated/crds/config.porch.kpt.dev_repositories.yaml @@ -296,6 +296,12 @@ spec: - message: spec.oci.registry is immutable rule: '!has(oldSelf.spec.oci) || !has(oldSelf.spec.oci.registry) || self.spec.oci.registry == oldSelf.spec.oci.registry' + - message: spec.git.secretRef is immutable + rule: '!has(oldSelf.spec.git) || !has(oldSelf.spec.git.secretRef) || self.spec.git.secretRef.name + == oldSelf.spec.git.secretRef.name' + - message: spec.oci.secretRef is immutable + rule: '!has(oldSelf.spec.oci) || !has(oldSelf.spec.oci.secretRef) || self.spec.oci.secretRef.name + == oldSelf.spec.oci.secretRef.name' served: true storage: true subresources: diff --git a/api/porchconfig/v1alpha1/repository_types.go b/api/porchconfig/v1alpha1/repository_types.go index 6b309a1f2..f00cd9fad 100644 --- a/api/porchconfig/v1alpha1/repository_types.go +++ b/api/porchconfig/v1alpha1/repository_types.go @@ -36,6 +36,8 @@ import ( // +kubebuilder:validation:XValidation:rule="!has(oldSelf.spec.git) || !has(oldSelf.spec.git.branch) || self.spec.git.branch == oldSelf.spec.git.branch",message="spec.git.branch is immutable" // +kubebuilder:validation:XValidation:rule="!has(oldSelf.spec.git) || !has(oldSelf.spec.git.directory) || self.spec.git.directory == oldSelf.spec.git.directory",message="spec.git.directory is immutable" // +kubebuilder:validation:XValidation:rule="!has(oldSelf.spec.oci) || !has(oldSelf.spec.oci.registry) || self.spec.oci.registry == oldSelf.spec.oci.registry",message="spec.oci.registry is immutable" +// +kubebuilder:validation:XValidation:rule="!has(oldSelf.spec.git) || !has(oldSelf.spec.git.secretRef) || self.spec.git.secretRef.name == oldSelf.spec.git.secretRef.name",message="spec.git.secretRef is immutable" +// +kubebuilder:validation:XValidation:rule="!has(oldSelf.spec.oci) || !has(oldSelf.spec.oci.secretRef) || self.spec.oci.secretRef.name == oldSelf.spec.oci.secretRef.name",message="spec.oci.secretRef is immutable" // Repository type Repository struct { diff --git a/controllers/repositories/pkg/webhooks/repository_webhook.go b/controllers/repositories/pkg/webhooks/repository_webhook.go index 6446aad8d..70f923886 100644 --- a/controllers/repositories/pkg/webhooks/repository_webhook.go +++ b/controllers/repositories/pkg/webhooks/repository_webhook.go @@ -77,24 +77,7 @@ func (v *RepositoryValidator) handleCreateOrUpdate(ctx context.Context, req admi fmt.Errorf("failed to unmarshal repository: %w", err)) } - // Block updates to git secret reference - if req.Operation == admissionv1.Update { - var existing configapi.Repository - if err := json.Unmarshal(req.OldObject.Raw, &existing); err != nil { - return admission.Errored(http.StatusInternalServerError, - fmt.Errorf("failed to unmarshal existing repository: %w", err)) - } - - if isSecretRefChanged(&existing, &attempted) { - logger.V(3).Info("secret reference change denied", - "namespace", attempted.Namespace, "name", attempted.Name) - return admission.Denied( - "spec.git.secretRef and spec.oci.secretRef are immutable and cannot be updated. " + - "Delete and recreate the repository to change the secret reference.") - } - } - - // NOTE: Immutability checks (URL, branch, directory) are handled by CEL validation in the CRD. + // NOTE: Immutability checks (URL, branch, directory, secretRef) are handled by CEL validation in the CRD. // This webhook only performs complex cross-resource conflict detection that CEL cannot do. var repoList configapi.RepositoryList @@ -185,34 +168,3 @@ func IsNestedConflict(a, b string) bool { return false } - -// isSecretRefChanged checks if either git or OCI secret reference has changed between existing and attempted repositories. -func isSecretRefChanged(existing, attempted *configapi.Repository) bool { - // Check git secret reference - existingGitSecret := "" - if existing.Spec.Git != nil { - existingGitSecret = existing.Spec.Git.SecretRef.Name - } - attemptedGitSecret := "" - if attempted.Spec.Git != nil { - attemptedGitSecret = attempted.Spec.Git.SecretRef.Name - } - if existingGitSecret != attemptedGitSecret { - return true - } - - // Check OCI secret reference - existingOciSecret := "" - if existing.Spec.Oci != nil { - existingOciSecret = existing.Spec.Oci.SecretRef.Name - } - attemptedOciSecret := "" - if attempted.Spec.Oci != nil { - attemptedOciSecret = attempted.Spec.Oci.SecretRef.Name - } - if existingOciSecret != attemptedOciSecret { - return true - } - - return false -} diff --git a/controllers/repositories/pkg/webhooks/repository_webhook_test.go b/controllers/repositories/pkg/webhooks/repository_webhook_test.go index 22dd86353..85e4415de 100644 --- a/controllers/repositories/pkg/webhooks/repository_webhook_test.go +++ b/controllers/repositories/pkg/webhooks/repository_webhook_test.go @@ -17,8 +17,7 @@ package webhooks import ( "context" "encoding/json" - "fmt" - "net/http" + "testing" configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" @@ -29,7 +28,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" - "sigs.k8s.io/controller-runtime/pkg/client/interceptor" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" ) @@ -682,385 +680,3 @@ func TestURLVariations(t *testing.T) { } // TestIsSecretRefChanged tests secret reference change detection -func TestIsSecretRefChanged(t *testing.T) { - tests := []struct { - name string - existing *configapi.Repository - attempt *configapi.Repository - expected bool - }{ - { - name: "no secret refs - no change", - existing: &configapi.Repository{ - Spec: configapi.RepositorySpec{ - Git: &configapi.GitRepository{}, - }, - }, - attempt: &configapi.Repository{ - Spec: configapi.RepositorySpec{ - Git: &configapi.GitRepository{}, - }, - }, - expected: false, - }, - { - name: "same git secret - no change", - existing: &configapi.Repository{ - Spec: configapi.RepositorySpec{ - Git: &configapi.GitRepository{ - SecretRef: configapi.SecretRef{Name: "my-secret"}, - }, - }, - }, - attempt: &configapi.Repository{ - Spec: configapi.RepositorySpec{ - Git: &configapi.GitRepository{ - SecretRef: configapi.SecretRef{Name: "my-secret"}, - }, - }, - }, - expected: false, - }, - { - name: "git secret changed - change detected", - existing: &configapi.Repository{ - Spec: configapi.RepositorySpec{ - Git: &configapi.GitRepository{ - SecretRef: configapi.SecretRef{Name: "old-secret"}, - }, - }, - }, - attempt: &configapi.Repository{ - Spec: configapi.RepositorySpec{ - Git: &configapi.GitRepository{ - SecretRef: configapi.SecretRef{Name: "new-secret"}, - }, - }, - }, - expected: true, - }, - { - name: "git secret added - change detected", - existing: &configapi.Repository{ - Spec: configapi.RepositorySpec{ - Git: &configapi.GitRepository{}, - }, - }, - attempt: &configapi.Repository{ - Spec: configapi.RepositorySpec{ - Git: &configapi.GitRepository{ - SecretRef: configapi.SecretRef{Name: "new-secret"}, - }, - }, - }, - expected: true, - }, - { - name: "git secret removed - change detected", - existing: &configapi.Repository{ - Spec: configapi.RepositorySpec{ - Git: &configapi.GitRepository{ - SecretRef: configapi.SecretRef{Name: "old-secret"}, - }, - }, - }, - attempt: &configapi.Repository{ - Spec: configapi.RepositorySpec{ - Git: &configapi.GitRepository{}, - }, - }, - expected: true, - }, - { - name: "same OCI secret - no change", - existing: &configapi.Repository{ - Spec: configapi.RepositorySpec{ - Oci: &configapi.OciRepository{ - SecretRef: configapi.SecretRef{Name: "oci-secret"}, - }, - }, - }, - attempt: &configapi.Repository{ - Spec: configapi.RepositorySpec{ - Oci: &configapi.OciRepository{ - SecretRef: configapi.SecretRef{Name: "oci-secret"}, - }, - }, - }, - expected: false, - }, - { - name: "OCI secret changed - change detected", - existing: &configapi.Repository{ - Spec: configapi.RepositorySpec{ - Oci: &configapi.OciRepository{ - SecretRef: configapi.SecretRef{Name: "old-oci-secret"}, - }, - }, - }, - attempt: &configapi.Repository{ - Spec: configapi.RepositorySpec{ - Oci: &configapi.OciRepository{ - SecretRef: configapi.SecretRef{Name: "new-oci-secret"}, - }, - }, - }, - expected: true, - }, - { - name: "git present in existing, OCI in attempt - change detected", - existing: &configapi.Repository{ - Spec: configapi.RepositorySpec{ - Git: &configapi.GitRepository{ - SecretRef: configapi.SecretRef{Name: "git-secret"}, - }, - }, - }, - attempt: &configapi.Repository{ - Spec: configapi.RepositorySpec{ - Oci: &configapi.OciRepository{ - SecretRef: configapi.SecretRef{Name: "oci-secret"}, - }, - }, - }, - expected: true, - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - result := isSecretRefChanged(tc.existing, tc.attempt) - assert.Equal(t, tc.expected, result) - }) - } -} - -// TestUpdateBlocksSecretRefChange verifies UPDATE blocks secret reference changes -func TestUpdateBlocksSecretRefChange(t *testing.T) { - scheme := newScheme() - - tests := []struct { - name string - existing *configapi.Repository - attempted *configapi.Repository - expectPass bool - message string - }{ - { - name: "git secret change blocked", - existing: makeRepo("repo1", "ns1", "http://gitea.local/org/repo.git", "dir1", "main"), - attempted: func() *configapi.Repository { - r := makeRepo("repo1", "ns1", "http://gitea.local/org/repo.git", "dir1", "main") - r.Spec.Git.SecretRef = configapi.SecretRef{Name: "new-secret"} - return r - }(), - expectPass: false, - message: "immutable", - }, - { - name: "git secret addition blocked", - existing: makeRepo("repo1", "ns1", "http://gitea.local/org/repo.git", "dir1", "main"), - attempted: func() *configapi.Repository { - r := makeRepo("repo1", "ns1", "http://gitea.local/org/repo.git", "dir1", "main") - r.Spec.Git.SecretRef = configapi.SecretRef{Name: "new-secret"} - return r - }(), - expectPass: false, - message: "immutable", - }, - { - name: "git secret removal blocked", - existing: func() *configapi.Repository { - r := makeRepo("repo1", "ns1", "http://gitea.local/org/repo.git", "dir1", "main") - r.Spec.Git.SecretRef = configapi.SecretRef{Name: "old-secret"} - return r - }(), - attempted: makeRepo("repo1", "ns1", "http://gitea.local/org/repo.git", "dir1", "main"), - expectPass: false, - message: "immutable", - }, - { - name: "same git secret allowed", - existing: func() *configapi.Repository { - r := makeRepo("repo1", "ns1", "http://gitea.local/org/repo.git", "dir1", "main") - r.Spec.Git.SecretRef = configapi.SecretRef{Name: "my-secret"} - return r - }(), - attempted: func() *configapi.Repository { - r := makeRepo("repo1", "ns1", "http://gitea.local/org/repo.git", "dir1", "main") - r.Spec.Git.SecretRef = configapi.SecretRef{Name: "my-secret"} - return r - }(), - expectPass: true, - message: "", - }, - { - name: "other field changed but secret same - allowed", - existing: func() *configapi.Repository { - r := makeRepo("repo1", "ns1", "http://gitea.local/org/repo.git", "dir1", "main") - r.Spec.Git.SecretRef = configapi.SecretRef{Name: "my-secret"} - r.Spec.Description = "old description" - return r - }(), - attempted: func() *configapi.Repository { - r := makeRepo("repo1", "ns1", "http://gitea.local/org/repo.git", "dir1", "main") - r.Spec.Git.SecretRef = configapi.SecretRef{Name: "my-secret"} - r.Spec.Description = "new description" - return r - }(), - expectPass: true, - message: "", - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tc.existing).Build() - validator := NewRepositoryValidator(client) - - oldData, err := json.Marshal(tc.existing) - require.NoError(t, err) - newData, err := json.Marshal(tc.attempted) - require.NoError(t, err) - - req := admission.Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Update, - Object: runtime.RawExtension{Raw: newData}, - OldObject: runtime.RawExtension{Raw: oldData}, - Namespace: tc.existing.Namespace, - Name: tc.existing.Name, - }, - } - - resp := validator.Handle(context.Background(), req) - if tc.expectPass { - assert.True(t, resp.Allowed, "expected allowed but got: %s", resp.Result.Message) - } else { - assert.False(t, resp.Allowed, "expected denied") - assert.Contains(t, resp.Result.Message, tc.message, "expected message contains: %s", tc.message) - } - }) - } -} - -// TestCreateAllowsSecretRef verifies CREATE allows secret references -func TestCreateAllowsSecretRef(t *testing.T) { - scheme := newScheme() - client := fake.NewClientBuilder().WithScheme(scheme).Build() - validator := NewRepositoryValidator(client) - - repo := makeRepo("new-repo", "ns1", "http://gitea.local/org/repo.git", "dir1", "main") - repo.Spec.Git.SecretRef = configapi.SecretRef{Name: "my-secret"} - - req := admission.Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Create, - Object: runtime.RawExtension{Raw: marshalRepo(t, repo)}, - Namespace: repo.Namespace, - }, - } - - resp := validator.Handle(context.Background(), req) - assert.True(t, resp.Allowed, "CREATE should allow secret references: %s", resp.Result.Message) -} - -// TestOCISecretRefBlocking verifies OCI secret refs are also protected -func TestOCISecretRefBlocking(t *testing.T) { - scheme := newScheme() - - existing := &configapi.Repository{ - ObjectMeta: metav1.ObjectMeta{Name: "oci-repo", Namespace: "ns1"}, - Spec: configapi.RepositorySpec{ - Type: configapi.RepositoryTypeOCI, - Oci: &configapi.OciRepository{ - Registry: "gcr.io/my-project", - SecretRef: configapi.SecretRef{Name: "old-secret"}, - }, - }, - } - - attempted := &configapi.Repository{ - ObjectMeta: metav1.ObjectMeta{Name: "oci-repo", Namespace: "ns1"}, - Spec: configapi.RepositorySpec{ - Type: configapi.RepositoryTypeOCI, - Oci: &configapi.OciRepository{ - Registry: "gcr.io/my-project", - SecretRef: configapi.SecretRef{Name: "new-secret"}, - }, - }, - } - - client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(existing).Build() - validator := NewRepositoryValidator(client) - - oldData, err := json.Marshal(existing) - require.NoError(t, err) - newData, err := json.Marshal(attempted) - require.NoError(t, err) - - req := admission.Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Update, - Object: runtime.RawExtension{Raw: newData}, - OldObject: runtime.RawExtension{Raw: oldData}, - Namespace: "ns1", - Name: "oci-repo", - }, - } - - resp := validator.Handle(context.Background(), req) - assert.False(t, resp.Allowed, "UPDATE should block OCI secret ref changes") - assert.Contains(t, resp.Result.Message, "immutable") -} - -// TestHandleCreateOrUpdateListError verifies that list errors are handled gracefully -func TestHandleCreateOrUpdateListError(t *testing.T) { - scheme := newScheme() - failingClient := fake.NewClientBuilder().WithScheme(scheme). - WithInterceptorFuncs(interceptor.Funcs{ - List: func(ctx context.Context, client client.WithWatch, list client.ObjectList, opts ...client.ListOption) error { - return fmt.Errorf("mock list error") - }, - }).Build() - validator := NewRepositoryValidator(failingClient) - - repo := makeRepo("test-repo", "ns1", "http://gitea.local/org/repo.git", "dir1", "main") - - req := admission.Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Create, - Object: runtime.RawExtension{Raw: marshalRepo(t, repo)}, - Namespace: repo.Namespace, - }, - } - - resp := validator.Handle(context.Background(), req) - assert.False(t, resp.Allowed) - assert.Equal(t, int32(http.StatusInternalServerError), resp.Result.Code) - assert.Contains(t, resp.Result.Message, "could not list repositories") -} - -// TestHandleUpdateMalformedOldObject verifies malformed OldObject on UPDATE is rejected -func TestHandleUpdateMalformedOldObject(t *testing.T) { - scheme := newScheme() - client := fake.NewClientBuilder().WithScheme(scheme).Build() - validator := NewRepositoryValidator(client) - - repo := makeRepo("test-repo", "ns1", "http://gitea.local/org/repo.git", "dir1", "main") - - req := admission.Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Update, - Object: runtime.RawExtension{Raw: marshalRepo(t, repo)}, - OldObject: runtime.RawExtension{Raw: []byte(`{invalid}`)}, - Namespace: repo.Namespace, - Name: repo.Name, - }, - } - - resp := validator.Handle(context.Background(), req) - assert.False(t, resp.Allowed) - assert.Equal(t, int32(http.StatusInternalServerError), resp.Result.Code) - assert.Contains(t, resp.Result.Message, "failed to unmarshal existing repository") -} From 5ab51315e698245f01c01bc7e2afbb9b03a7ba7a Mon Sep 17 00:00:00 2001 From: Fiachra Corcoran Date: Thu, 30 Jul 2026 09:30:43 +0100 Subject: [PATCH 7/7] Revert git secret immutability Signed-off-by: Fiachra Corcoran --- api/generated/crds/config.porch.kpt.dev_repositories.yaml | 6 ------ api/porchconfig/v1alpha1/repository_types.go | 2 -- controllers/repositories/pkg/webhooks/repository_webhook.go | 2 +- .../repositories/pkg/webhooks/repository_webhook_test.go | 2 -- 4 files changed, 1 insertion(+), 11 deletions(-) diff --git a/api/generated/crds/config.porch.kpt.dev_repositories.yaml b/api/generated/crds/config.porch.kpt.dev_repositories.yaml index 063fe7050..d058256e2 100644 --- a/api/generated/crds/config.porch.kpt.dev_repositories.yaml +++ b/api/generated/crds/config.porch.kpt.dev_repositories.yaml @@ -296,12 +296,6 @@ spec: - message: spec.oci.registry is immutable rule: '!has(oldSelf.spec.oci) || !has(oldSelf.spec.oci.registry) || self.spec.oci.registry == oldSelf.spec.oci.registry' - - message: spec.git.secretRef is immutable - rule: '!has(oldSelf.spec.git) || !has(oldSelf.spec.git.secretRef) || self.spec.git.secretRef.name - == oldSelf.spec.git.secretRef.name' - - message: spec.oci.secretRef is immutable - rule: '!has(oldSelf.spec.oci) || !has(oldSelf.spec.oci.secretRef) || self.spec.oci.secretRef.name - == oldSelf.spec.oci.secretRef.name' served: true storage: true subresources: diff --git a/api/porchconfig/v1alpha1/repository_types.go b/api/porchconfig/v1alpha1/repository_types.go index f00cd9fad..6b309a1f2 100644 --- a/api/porchconfig/v1alpha1/repository_types.go +++ b/api/porchconfig/v1alpha1/repository_types.go @@ -36,8 +36,6 @@ import ( // +kubebuilder:validation:XValidation:rule="!has(oldSelf.spec.git) || !has(oldSelf.spec.git.branch) || self.spec.git.branch == oldSelf.spec.git.branch",message="spec.git.branch is immutable" // +kubebuilder:validation:XValidation:rule="!has(oldSelf.spec.git) || !has(oldSelf.spec.git.directory) || self.spec.git.directory == oldSelf.spec.git.directory",message="spec.git.directory is immutable" // +kubebuilder:validation:XValidation:rule="!has(oldSelf.spec.oci) || !has(oldSelf.spec.oci.registry) || self.spec.oci.registry == oldSelf.spec.oci.registry",message="spec.oci.registry is immutable" -// +kubebuilder:validation:XValidation:rule="!has(oldSelf.spec.git) || !has(oldSelf.spec.git.secretRef) || self.spec.git.secretRef.name == oldSelf.spec.git.secretRef.name",message="spec.git.secretRef is immutable" -// +kubebuilder:validation:XValidation:rule="!has(oldSelf.spec.oci) || !has(oldSelf.spec.oci.secretRef) || self.spec.oci.secretRef.name == oldSelf.spec.oci.secretRef.name",message="spec.oci.secretRef is immutable" // Repository type Repository struct { diff --git a/controllers/repositories/pkg/webhooks/repository_webhook.go b/controllers/repositories/pkg/webhooks/repository_webhook.go index 70f923886..551c925d6 100644 --- a/controllers/repositories/pkg/webhooks/repository_webhook.go +++ b/controllers/repositories/pkg/webhooks/repository_webhook.go @@ -77,7 +77,7 @@ func (v *RepositoryValidator) handleCreateOrUpdate(ctx context.Context, req admi fmt.Errorf("failed to unmarshal repository: %w", err)) } - // NOTE: Immutability checks (URL, branch, directory, secretRef) are handled by CEL validation in the CRD. + // NOTE: Immutability checks (URL, branch, directory) are handled by CEL validation in the CRD. // This webhook only performs complex cross-resource conflict detection that CEL cannot do. var repoList configapi.RepositoryList diff --git a/controllers/repositories/pkg/webhooks/repository_webhook_test.go b/controllers/repositories/pkg/webhooks/repository_webhook_test.go index 85e4415de..53a5d67ee 100644 --- a/controllers/repositories/pkg/webhooks/repository_webhook_test.go +++ b/controllers/repositories/pkg/webhooks/repository_webhook_test.go @@ -678,5 +678,3 @@ func TestURLVariations(t *testing.T) { }) } } - -// TestIsSecretRefChanged tests secret reference change detection