Provisioning: Add validation for Job specifications (#113590)
* Validate Job Specs * Add comprehensive unit test coverage for job validator - Added 8 new test cases to improve coverage from 88.9% to ~100% - Tests for migrate action without options - Tests for delete/move actions with resources (missing kind) - Tests for move action with valid resources - Tests for move/delete with both paths and resources - Tests for move action with invalid source paths - Tests for push action with valid paths Now covers all validation paths including resource validation and edge cases for all job action types. * Add integration tests for job validation Added comprehensive integration tests that verify the job validator properly rejects invalid job specifications via the API: - Test job without action (required field) - Test job with invalid action - Test pull job without pull options - Test push job without push options - Test push job with invalid branch name (consecutive dots) - Test push job with path traversal attempt - Test delete job without paths or resources - Test delete job with invalid path (path traversal) - Test move job without target path - Test move job without paths or resources - Test move job with invalid target path (path traversal) - Test migrate job without migrate options - Test valid pull job to ensure validation doesn't block legitimate requests These tests verify that the admission controller properly validates job specs before they are persisted, ensuring security (path traversal prevention) and data integrity (required fields/options). * Remove valid job test case from integration tests Removed the positive test case as it's not necessary for validation testing. The integration tests now focus solely on verifying that invalid job specs are properly rejected by the admission controller. * Fix movejob_test to expect validation error at creation time Updated the 'move without target path' test to expect the job creation to fail with a validation error, rather than expecting the job to be created and then fail during execution. This aligns with the new job validation logic which rejects invalid job specs at the API admission control level (422 Unprocessable Entity) before they can be persisted. This is better behavior as it prevents invalid jobs from being created in the first place, rather than allowing them to be created and then failing during execution. * Simplify action validation using slices.Contains Replaced manual loop with slices.Contains for cleaner, more idiomatic Go code. This reduces code complexity while maintaining the same validation logic. - Added import for 'slices' package - Replaced 8-line loop with 1-line slices.Contains call - All unit tests pass * Refactor job action validation in ValidateJob function Removed the hardcoded valid actions array and simplified the validation logic. The function now directly appends an error for invalid actions, improving code clarity and maintainability. This change aligns with the recent updates to job validation, ensuring that invalid job specifications are properly handled.
This commit is contained in:
Roberto Jiménez Sánchez
@@ -0,0 +1,172 @@
|
||||
package jobs
|
||||
|
||||
import (
|
||||
apierrors "k8s.io/apimachinery/pkg/api/errors"
|
||||
"k8s.io/apimachinery/pkg/util/validation/field"
|
||||
|
||||
provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1"
|
||||
"github.com/grafana/grafana/apps/provisioning/pkg/repository/git"
|
||||
"github.com/grafana/grafana/apps/provisioning/pkg/safepath"
|
||||
)
|
||||
|
||||
// ValidateJob performs validation on the Job specification and returns an error if validation fails
|
||||
func ValidateJob(job *provisioning.Job) error {
|
||||
list := field.ErrorList{}
|
||||
|
||||
// Validate action is specified
|
||||
if job.Spec.Action == "" {
|
||||
list = append(list, field.Required(field.NewPath("spec", "action"), "action must be specified"))
|
||||
return toError(job.Name, list) // Early return since we can't validate further without knowing the action
|
||||
}
|
||||
|
||||
// Validate repository is specified
|
||||
if job.Spec.Repository == "" {
|
||||
list = append(list, field.Required(field.NewPath("spec", "repository"), "repository must be specified"))
|
||||
}
|
||||
|
||||
// Validate action-specific options
|
||||
switch job.Spec.Action {
|
||||
case provisioning.JobActionPull:
|
||||
if job.Spec.Pull == nil {
|
||||
list = append(list, field.Required(field.NewPath("spec", "pull"), "pull options required for pull action"))
|
||||
}
|
||||
// Pull options are simple, just incremental bool - no further validation needed
|
||||
|
||||
case provisioning.JobActionPush:
|
||||
if job.Spec.Push == nil {
|
||||
list = append(list, field.Required(field.NewPath("spec", "push"), "push options required for push action"))
|
||||
} else {
|
||||
list = append(list, validateExportJobOptions(job.Spec.Push)...)
|
||||
}
|
||||
|
||||
case provisioning.JobActionPullRequest:
|
||||
if job.Spec.PullRequest == nil {
|
||||
list = append(list, field.Required(field.NewPath("spec", "pr"), "pull request options required for pr action"))
|
||||
}
|
||||
// PullRequest options are mostly informational - no strict validation needed
|
||||
|
||||
case provisioning.JobActionMigrate:
|
||||
if job.Spec.Migrate == nil {
|
||||
list = append(list, field.Required(field.NewPath("spec", "migrate"), "migrate options required for migrate action"))
|
||||
}
|
||||
// Migrate options are simple - no further validation needed
|
||||
|
||||
case provisioning.JobActionDelete:
|
||||
if job.Spec.Delete == nil {
|
||||
list = append(list, field.Required(field.NewPath("spec", "delete"), "delete options required for delete action"))
|
||||
} else {
|
||||
list = append(list, validateDeleteJobOptions(job.Spec.Delete)...)
|
||||
}
|
||||
|
||||
case provisioning.JobActionMove:
|
||||
if job.Spec.Move == nil {
|
||||
list = append(list, field.Required(field.NewPath("spec", "move"), "move options required for move action"))
|
||||
} else {
|
||||
list = append(list, validateMoveJobOptions(job.Spec.Move)...)
|
||||
}
|
||||
default:
|
||||
list = append(list, field.Invalid(field.NewPath("spec", "action"), job.Spec.Action, "invalid action"))
|
||||
}
|
||||
|
||||
return toError(job.Name, list)
|
||||
}
|
||||
|
||||
// toError converts a field.ErrorList to an error, returning nil if the list is empty
|
||||
func toError(name string, list field.ErrorList) error {
|
||||
if len(list) == 0 {
|
||||
return nil
|
||||
}
|
||||
return apierrors.NewInvalid(
|
||||
provisioning.JobResourceInfo.GroupVersionKind().GroupKind(),
|
||||
name, list)
|
||||
}
|
||||
|
||||
// validateExportJobOptions validates export (push) job options
|
||||
func validateExportJobOptions(opts *provisioning.ExportJobOptions) field.ErrorList {
|
||||
list := field.ErrorList{}
|
||||
|
||||
// Validate branch name if specified
|
||||
if opts.Branch != "" {
|
||||
if !git.IsValidGitBranchName(opts.Branch) {
|
||||
list = append(list, field.Invalid(field.NewPath("spec", "push", "branch"), opts.Branch, "invalid git branch name"))
|
||||
}
|
||||
}
|
||||
|
||||
// Validate path if specified
|
||||
if opts.Path != "" {
|
||||
if err := safepath.IsSafe(opts.Path); err != nil {
|
||||
list = append(list, field.Invalid(field.NewPath("spec", "push", "path"), opts.Path, err.Error()))
|
||||
}
|
||||
}
|
||||
|
||||
return list
|
||||
}
|
||||
|
||||
// validateDeleteJobOptions validates delete job options
|
||||
func validateDeleteJobOptions(opts *provisioning.DeleteJobOptions) field.ErrorList {
|
||||
list := field.ErrorList{}
|
||||
|
||||
// At least one of paths or resources must be specified
|
||||
if len(opts.Paths) == 0 && len(opts.Resources) == 0 {
|
||||
list = append(list, field.Required(field.NewPath("spec", "delete"), "at least one path or resource must be specified"))
|
||||
return list
|
||||
}
|
||||
|
||||
// Validate paths
|
||||
for i, p := range opts.Paths {
|
||||
if err := safepath.IsSafe(p); err != nil {
|
||||
list = append(list, field.Invalid(field.NewPath("spec", "delete", "paths").Index(i), p, err.Error()))
|
||||
}
|
||||
}
|
||||
|
||||
// Validate resources
|
||||
for i, r := range opts.Resources {
|
||||
if r.Name == "" {
|
||||
list = append(list, field.Required(field.NewPath("spec", "delete", "resources").Index(i).Child("name"), "resource name is required"))
|
||||
}
|
||||
if r.Kind == "" {
|
||||
list = append(list, field.Required(field.NewPath("spec", "delete", "resources").Index(i).Child("kind"), "resource kind is required"))
|
||||
}
|
||||
}
|
||||
|
||||
return list
|
||||
}
|
||||
|
||||
// validateMoveJobOptions validates move job options
|
||||
func validateMoveJobOptions(opts *provisioning.MoveJobOptions) field.ErrorList {
|
||||
list := field.ErrorList{}
|
||||
|
||||
// At least one of paths or resources must be specified
|
||||
if len(opts.Paths) == 0 && len(opts.Resources) == 0 {
|
||||
list = append(list, field.Required(field.NewPath("spec", "move"), "at least one path or resource must be specified"))
|
||||
return list
|
||||
}
|
||||
|
||||
// Target path is required
|
||||
if opts.TargetPath == "" {
|
||||
list = append(list, field.Required(field.NewPath("spec", "move", "targetPath"), "target path is required"))
|
||||
} else {
|
||||
if err := safepath.IsSafe(opts.TargetPath); err != nil {
|
||||
list = append(list, field.Invalid(field.NewPath("spec", "move", "targetPath"), opts.TargetPath, err.Error()))
|
||||
}
|
||||
}
|
||||
|
||||
// Validate source paths
|
||||
for i, p := range opts.Paths {
|
||||
if err := safepath.IsSafe(p); err != nil {
|
||||
list = append(list, field.Invalid(field.NewPath("spec", "move", "paths").Index(i), p, err.Error()))
|
||||
}
|
||||
}
|
||||
|
||||
// Validate resources
|
||||
for i, r := range opts.Resources {
|
||||
if r.Name == "" {
|
||||
list = append(list, field.Required(field.NewPath("spec", "move", "resources").Index(i).Child("name"), "resource name is required"))
|
||||
}
|
||||
if r.Kind == "" {
|
||||
list = append(list, field.Required(field.NewPath("spec", "move", "resources").Index(i).Child("kind"), "resource kind is required"))
|
||||
}
|
||||
}
|
||||
|
||||
return list
|
||||
}
|
||||
@@ -0,0 +1,593 @@
|
||||
package jobs
|
||||
|
||||
import (
|
||||
"testing"
|
||||
|
||||
"github.com/stretchr/testify/require"
|
||||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
|
||||
|
||||
provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1"
|
||||
)
|
||||
|
||||
func TestValidateJob(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
job *provisioning.Job
|
||||
wantErr bool
|
||||
validateError func(t *testing.T, err error)
|
||||
}{
|
||||
{
|
||||
name: "valid pull job",
|
||||
job: &provisioning.Job{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-job",
|
||||
},
|
||||
Spec: provisioning.JobSpec{
|
||||
Action: provisioning.JobActionPull,
|
||||
Repository: "test-repo",
|
||||
Pull: &provisioning.SyncJobOptions{
|
||||
Incremental: true,
|
||||
},
|
||||
},
|
||||
},
|
||||
wantErr: false,
|
||||
},
|
||||
{
|
||||
name: "missing action",
|
||||
job: &provisioning.Job{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-job",
|
||||
},
|
||||
Spec: provisioning.JobSpec{
|
||||
Repository: "test-repo",
|
||||
},
|
||||
},
|
||||
wantErr: true,
|
||||
validateError: func(t *testing.T, err error) {
|
||||
require.Contains(t, err.Error(), "spec.action: Required value")
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "invalid action",
|
||||
job: &provisioning.Job{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-job",
|
||||
},
|
||||
Spec: provisioning.JobSpec{
|
||||
Action: provisioning.JobAction("invalid"),
|
||||
Repository: "test-repo",
|
||||
},
|
||||
},
|
||||
wantErr: true,
|
||||
validateError: func(t *testing.T, err error) {
|
||||
require.Contains(t, err.Error(), "spec.action: Invalid value")
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "missing repository",
|
||||
job: &provisioning.Job{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-job",
|
||||
},
|
||||
Spec: provisioning.JobSpec{
|
||||
Action: provisioning.JobActionPull,
|
||||
Pull: &provisioning.SyncJobOptions{
|
||||
Incremental: true,
|
||||
},
|
||||
},
|
||||
},
|
||||
wantErr: true,
|
||||
validateError: func(t *testing.T, err error) {
|
||||
require.Contains(t, err.Error(), "spec.repository: Required value")
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "pull action without pull options",
|
||||
job: &provisioning.Job{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-job",
|
||||
},
|
||||
Spec: provisioning.JobSpec{
|
||||
Action: provisioning.JobActionPull,
|
||||
Repository: "test-repo",
|
||||
},
|
||||
},
|
||||
wantErr: true,
|
||||
validateError: func(t *testing.T, err error) {
|
||||
require.Contains(t, err.Error(), "spec.pull: Required value")
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "push action without push options",
|
||||
job: &provisioning.Job{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-job",
|
||||
},
|
||||
Spec: provisioning.JobSpec{
|
||||
Action: provisioning.JobActionPush,
|
||||
Repository: "test-repo",
|
||||
},
|
||||
},
|
||||
wantErr: true,
|
||||
validateError: func(t *testing.T, err error) {
|
||||
require.Contains(t, err.Error(), "spec.push: Required value")
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "valid push job with valid branch",
|
||||
job: &provisioning.Job{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-job",
|
||||
},
|
||||
Spec: provisioning.JobSpec{
|
||||
Action: provisioning.JobActionPush,
|
||||
Repository: "test-repo",
|
||||
Push: &provisioning.ExportJobOptions{
|
||||
Branch: "main",
|
||||
Message: "Test commit",
|
||||
},
|
||||
},
|
||||
},
|
||||
wantErr: false,
|
||||
},
|
||||
{
|
||||
name: "push job with invalid branch name",
|
||||
job: &provisioning.Job{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-job",
|
||||
},
|
||||
Spec: provisioning.JobSpec{
|
||||
Action: provisioning.JobActionPush,
|
||||
Repository: "test-repo",
|
||||
Push: &provisioning.ExportJobOptions{
|
||||
Branch: "feature..branch", // Invalid: contains consecutive dots
|
||||
Message: "Test commit",
|
||||
},
|
||||
},
|
||||
},
|
||||
wantErr: true,
|
||||
validateError: func(t *testing.T, err error) {
|
||||
require.Contains(t, err.Error(), "spec.push.branch")
|
||||
require.Contains(t, err.Error(), "invalid git branch name")
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "push job with invalid path",
|
||||
job: &provisioning.Job{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-job",
|
||||
},
|
||||
Spec: provisioning.JobSpec{
|
||||
Action: provisioning.JobActionPush,
|
||||
Repository: "test-repo",
|
||||
Push: &provisioning.ExportJobOptions{
|
||||
Path: "../../../etc/passwd", // Invalid: path traversal
|
||||
Message: "Test commit",
|
||||
},
|
||||
},
|
||||
},
|
||||
wantErr: true,
|
||||
validateError: func(t *testing.T, err error) {
|
||||
require.Contains(t, err.Error(), "spec.push.path")
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "delete action without options",
|
||||
job: &provisioning.Job{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-job",
|
||||
},
|
||||
Spec: provisioning.JobSpec{
|
||||
Action: provisioning.JobActionDelete,
|
||||
Repository: "test-repo",
|
||||
},
|
||||
},
|
||||
wantErr: true,
|
||||
validateError: func(t *testing.T, err error) {
|
||||
require.Contains(t, err.Error(), "spec.delete: Required value")
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "delete action without paths or resources",
|
||||
job: &provisioning.Job{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-job",
|
||||
},
|
||||
Spec: provisioning.JobSpec{
|
||||
Action: provisioning.JobActionDelete,
|
||||
Repository: "test-repo",
|
||||
Delete: &provisioning.DeleteJobOptions{},
|
||||
},
|
||||
},
|
||||
wantErr: true,
|
||||
validateError: func(t *testing.T, err error) {
|
||||
require.Contains(t, err.Error(), "at least one path or resource must be specified")
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "valid delete action with paths",
|
||||
job: &provisioning.Job{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-job",
|
||||
},
|
||||
Spec: provisioning.JobSpec{
|
||||
Action: provisioning.JobActionDelete,
|
||||
Repository: "test-repo",
|
||||
Delete: &provisioning.DeleteJobOptions{
|
||||
Paths: []string{"dashboard.json", "folder/other.json"},
|
||||
},
|
||||
},
|
||||
},
|
||||
wantErr: false,
|
||||
},
|
||||
{
|
||||
name: "valid delete action with resources",
|
||||
job: &provisioning.Job{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-job",
|
||||
},
|
||||
Spec: provisioning.JobSpec{
|
||||
Action: provisioning.JobActionDelete,
|
||||
Repository: "test-repo",
|
||||
Delete: &provisioning.DeleteJobOptions{
|
||||
Resources: []provisioning.ResourceRef{
|
||||
{
|
||||
Name: "my-dashboard",
|
||||
Kind: "Dashboard",
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
wantErr: false,
|
||||
},
|
||||
{
|
||||
name: "delete action with invalid path",
|
||||
job: &provisioning.Job{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-job",
|
||||
},
|
||||
Spec: provisioning.JobSpec{
|
||||
Action: provisioning.JobActionDelete,
|
||||
Repository: "test-repo",
|
||||
Delete: &provisioning.DeleteJobOptions{
|
||||
Paths: []string{"../../etc/passwd"}, // Invalid: path traversal
|
||||
},
|
||||
},
|
||||
},
|
||||
wantErr: true,
|
||||
validateError: func(t *testing.T, err error) {
|
||||
require.Contains(t, err.Error(), "spec.delete.paths[0]")
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "delete action with resource missing name",
|
||||
job: &provisioning.Job{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-job",
|
||||
},
|
||||
Spec: provisioning.JobSpec{
|
||||
Action: provisioning.JobActionDelete,
|
||||
Repository: "test-repo",
|
||||
Delete: &provisioning.DeleteJobOptions{
|
||||
Resources: []provisioning.ResourceRef{
|
||||
{
|
||||
Kind: "Dashboard",
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
wantErr: true,
|
||||
validateError: func(t *testing.T, err error) {
|
||||
require.Contains(t, err.Error(), "spec.delete.resources[0].name")
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "move action without options",
|
||||
job: &provisioning.Job{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-job",
|
||||
},
|
||||
Spec: provisioning.JobSpec{
|
||||
Action: provisioning.JobActionMove,
|
||||
Repository: "test-repo",
|
||||
},
|
||||
},
|
||||
wantErr: true,
|
||||
validateError: func(t *testing.T, err error) {
|
||||
require.Contains(t, err.Error(), "spec.move: Required value")
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "move action without paths or resources",
|
||||
job: &provisioning.Job{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-job",
|
||||
},
|
||||
Spec: provisioning.JobSpec{
|
||||
Action: provisioning.JobActionMove,
|
||||
Repository: "test-repo",
|
||||
Move: &provisioning.MoveJobOptions{
|
||||
TargetPath: "new-location/",
|
||||
},
|
||||
},
|
||||
},
|
||||
wantErr: true,
|
||||
validateError: func(t *testing.T, err error) {
|
||||
require.Contains(t, err.Error(), "at least one path or resource must be specified")
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "move action without target path",
|
||||
job: &provisioning.Job{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-job",
|
||||
},
|
||||
Spec: provisioning.JobSpec{
|
||||
Action: provisioning.JobActionMove,
|
||||
Repository: "test-repo",
|
||||
Move: &provisioning.MoveJobOptions{
|
||||
Paths: []string{"dashboard.json"},
|
||||
},
|
||||
},
|
||||
},
|
||||
wantErr: true,
|
||||
validateError: func(t *testing.T, err error) {
|
||||
require.Contains(t, err.Error(), "spec.move.targetPath: Required value")
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "valid move action",
|
||||
job: &provisioning.Job{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-job",
|
||||
},
|
||||
Spec: provisioning.JobSpec{
|
||||
Action: provisioning.JobActionMove,
|
||||
Repository: "test-repo",
|
||||
Move: &provisioning.MoveJobOptions{
|
||||
Paths: []string{"old-location/dashboard.json"},
|
||||
TargetPath: "new-location/",
|
||||
},
|
||||
},
|
||||
},
|
||||
wantErr: false,
|
||||
},
|
||||
{
|
||||
name: "move action with invalid target path",
|
||||
job: &provisioning.Job{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-job",
|
||||
},
|
||||
Spec: provisioning.JobSpec{
|
||||
Action: provisioning.JobActionMove,
|
||||
Repository: "test-repo",
|
||||
Move: &provisioning.MoveJobOptions{
|
||||
Paths: []string{"dashboard.json"},
|
||||
TargetPath: "../../../etc/", // Invalid: path traversal
|
||||
},
|
||||
},
|
||||
},
|
||||
wantErr: true,
|
||||
validateError: func(t *testing.T, err error) {
|
||||
require.Contains(t, err.Error(), "spec.move.targetPath")
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "valid migrate job",
|
||||
job: &provisioning.Job{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-job",
|
||||
},
|
||||
Spec: provisioning.JobSpec{
|
||||
Action: provisioning.JobActionMigrate,
|
||||
Repository: "test-repo",
|
||||
Migrate: &provisioning.MigrateJobOptions{
|
||||
History: true,
|
||||
Message: "Migrate from legacy",
|
||||
},
|
||||
},
|
||||
},
|
||||
wantErr: false,
|
||||
},
|
||||
{
|
||||
name: "migrate action without migrate options",
|
||||
job: &provisioning.Job{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-job",
|
||||
},
|
||||
Spec: provisioning.JobSpec{
|
||||
Action: provisioning.JobActionMigrate,
|
||||
Repository: "test-repo",
|
||||
},
|
||||
},
|
||||
wantErr: true,
|
||||
validateError: func(t *testing.T, err error) {
|
||||
require.Contains(t, err.Error(), "spec.migrate: Required value")
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "valid pr job",
|
||||
job: &provisioning.Job{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-job",
|
||||
},
|
||||
Spec: provisioning.JobSpec{
|
||||
Action: provisioning.JobActionPullRequest,
|
||||
Repository: "test-repo",
|
||||
PullRequest: &provisioning.PullRequestJobOptions{
|
||||
PR: 123,
|
||||
Ref: "refs/pull/123/head",
|
||||
},
|
||||
},
|
||||
},
|
||||
wantErr: false,
|
||||
},
|
||||
{
|
||||
name: "delete action with resource missing kind",
|
||||
job: &provisioning.Job{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-job",
|
||||
},
|
||||
Spec: provisioning.JobSpec{
|
||||
Action: provisioning.JobActionDelete,
|
||||
Repository: "test-repo",
|
||||
Delete: &provisioning.DeleteJobOptions{
|
||||
Resources: []provisioning.ResourceRef{
|
||||
{
|
||||
Name: "my-dashboard",
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
wantErr: true,
|
||||
validateError: func(t *testing.T, err error) {
|
||||
require.Contains(t, err.Error(), "spec.delete.resources[0].kind")
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "move action with valid resources",
|
||||
job: &provisioning.Job{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-job",
|
||||
},
|
||||
Spec: provisioning.JobSpec{
|
||||
Action: provisioning.JobActionMove,
|
||||
Repository: "test-repo",
|
||||
Move: &provisioning.MoveJobOptions{
|
||||
Resources: []provisioning.ResourceRef{
|
||||
{
|
||||
Name: "my-dashboard",
|
||||
Kind: "Dashboard",
|
||||
},
|
||||
},
|
||||
TargetPath: "new-location/",
|
||||
},
|
||||
},
|
||||
},
|
||||
wantErr: false,
|
||||
},
|
||||
{
|
||||
name: "move action with resource missing kind",
|
||||
job: &provisioning.Job{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-job",
|
||||
},
|
||||
Spec: provisioning.JobSpec{
|
||||
Action: provisioning.JobActionMove,
|
||||
Repository: "test-repo",
|
||||
Move: &provisioning.MoveJobOptions{
|
||||
Resources: []provisioning.ResourceRef{
|
||||
{
|
||||
Name: "my-dashboard",
|
||||
},
|
||||
},
|
||||
TargetPath: "new-location/",
|
||||
},
|
||||
},
|
||||
},
|
||||
wantErr: true,
|
||||
validateError: func(t *testing.T, err error) {
|
||||
require.Contains(t, err.Error(), "spec.move.resources[0].kind")
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "move action with both paths and resources",
|
||||
job: &provisioning.Job{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-job",
|
||||
},
|
||||
Spec: provisioning.JobSpec{
|
||||
Action: provisioning.JobActionMove,
|
||||
Repository: "test-repo",
|
||||
Move: &provisioning.MoveJobOptions{
|
||||
Paths: []string{"dashboard.json"},
|
||||
Resources: []provisioning.ResourceRef{
|
||||
{
|
||||
Name: "my-dashboard",
|
||||
Kind: "Dashboard",
|
||||
},
|
||||
},
|
||||
TargetPath: "new-location/",
|
||||
},
|
||||
},
|
||||
},
|
||||
wantErr: false,
|
||||
},
|
||||
{
|
||||
name: "move action with invalid source path",
|
||||
job: &provisioning.Job{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-job",
|
||||
},
|
||||
Spec: provisioning.JobSpec{
|
||||
Action: provisioning.JobActionMove,
|
||||
Repository: "test-repo",
|
||||
Move: &provisioning.MoveJobOptions{
|
||||
Paths: []string{"../invalid/path"},
|
||||
TargetPath: "valid/target/",
|
||||
},
|
||||
},
|
||||
},
|
||||
wantErr: true,
|
||||
validateError: func(t *testing.T, err error) {
|
||||
require.Contains(t, err.Error(), "spec.move.paths[0]")
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "delete action with both paths and resources",
|
||||
job: &provisioning.Job{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-job",
|
||||
},
|
||||
Spec: provisioning.JobSpec{
|
||||
Action: provisioning.JobActionDelete,
|
||||
Repository: "test-repo",
|
||||
Delete: &provisioning.DeleteJobOptions{
|
||||
Paths: []string{"dashboard.json"},
|
||||
Resources: []provisioning.ResourceRef{
|
||||
{
|
||||
Name: "my-dashboard",
|
||||
Kind: "Dashboard",
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
wantErr: false,
|
||||
},
|
||||
{
|
||||
name: "push action with valid path",
|
||||
job: &provisioning.Job{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-job",
|
||||
},
|
||||
Spec: provisioning.JobSpec{
|
||||
Action: provisioning.JobActionPush,
|
||||
Repository: "test-repo",
|
||||
Push: &provisioning.ExportJobOptions{
|
||||
Path: "some/valid/path",
|
||||
Message: "Test commit",
|
||||
},
|
||||
},
|
||||
},
|
||||
wantErr: false,
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
err := ValidateJob(tt.job)
|
||||
if tt.wantErr {
|
||||
require.Error(t, err)
|
||||
if tt.validateError != nil {
|
||||
tt.validateError(t, err)
|
||||
}
|
||||
} else {
|
||||
require.NoError(t, err)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -35,6 +35,7 @@ import (
|
||||
clientset "github.com/grafana/grafana/apps/provisioning/pkg/generated/clientset/versioned"
|
||||
client "github.com/grafana/grafana/apps/provisioning/pkg/generated/clientset/versioned/typed/provisioning/v0alpha1"
|
||||
informers "github.com/grafana/grafana/apps/provisioning/pkg/generated/informers/externalversions"
|
||||
jobsvalidation "github.com/grafana/grafana/apps/provisioning/pkg/jobs"
|
||||
"github.com/grafana/grafana/apps/provisioning/pkg/loki"
|
||||
"github.com/grafana/grafana/apps/provisioning/pkg/repository"
|
||||
"github.com/grafana/grafana/pkg/apimachinery/identity"
|
||||
@@ -576,10 +577,10 @@ func (b *APIBuilder) Validate(ctx context.Context, a admission.Attributes, o adm
|
||||
return nil
|
||||
}
|
||||
|
||||
// FIXME: Do nothing for Jobs for now
|
||||
_, ok = obj.(*provisioning.Job)
|
||||
// Validate Jobs
|
||||
job, ok := obj.(*provisioning.Job)
|
||||
if ok {
|
||||
return nil
|
||||
return jobsvalidation.ValidateJob(job)
|
||||
}
|
||||
|
||||
repo, err := b.asRepository(ctx, obj, a.GetOldObject())
|
||||
|
||||
@@ -0,0 +1,182 @@
|
||||
package provisioning
|
||||
|
||||
import (
|
||||
"context"
|
||||
"fmt"
|
||||
"net/http"
|
||||
"testing"
|
||||
|
||||
"github.com/stretchr/testify/require"
|
||||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
|
||||
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
|
||||
|
||||
provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1"
|
||||
"github.com/grafana/grafana/pkg/util/testutil"
|
||||
)
|
||||
|
||||
func TestIntegrationProvisioning_JobValidation(t *testing.T) {
|
||||
testutil.SkipIntegrationTestInShortMode(t)
|
||||
|
||||
helper := runGrafana(t)
|
||||
ctx := context.Background()
|
||||
|
||||
// Create a test repository first
|
||||
const repo = "job-validation-test-repo"
|
||||
testRepo := TestRepo{
|
||||
Name: repo,
|
||||
Target: "instance",
|
||||
Copies: map[string]string{},
|
||||
ExpectedDashboards: 0,
|
||||
ExpectedFolders: 0,
|
||||
}
|
||||
helper.CreateRepo(t, testRepo)
|
||||
|
||||
tests := []struct {
|
||||
name string
|
||||
jobSpec map[string]interface{}
|
||||
expectedErr string
|
||||
}{
|
||||
{
|
||||
name: "job without action",
|
||||
jobSpec: map[string]interface{}{
|
||||
"repository": repo,
|
||||
},
|
||||
expectedErr: "spec.action: Required value: action must be specified",
|
||||
},
|
||||
{
|
||||
name: "job with invalid action",
|
||||
jobSpec: map[string]interface{}{
|
||||
"action": "invalid-action",
|
||||
"repository": repo,
|
||||
},
|
||||
expectedErr: "spec.action: Invalid value: \"invalid-action\": invalid action",
|
||||
},
|
||||
{
|
||||
name: "pull job without pull options",
|
||||
jobSpec: map[string]interface{}{
|
||||
"action": string(provisioning.JobActionPull),
|
||||
"repository": repo,
|
||||
},
|
||||
expectedErr: "spec.pull: Required value: pull options required for pull action",
|
||||
},
|
||||
{
|
||||
name: "push job without push options",
|
||||
jobSpec: map[string]interface{}{
|
||||
"action": string(provisioning.JobActionPush),
|
||||
"repository": repo,
|
||||
},
|
||||
expectedErr: "spec.push: Required value: push options required for push action",
|
||||
},
|
||||
{
|
||||
name: "push job with invalid branch name",
|
||||
jobSpec: map[string]interface{}{
|
||||
"action": string(provisioning.JobActionPush),
|
||||
"repository": repo,
|
||||
"push": map[string]interface{}{
|
||||
"branch": "feature..branch", // Invalid: consecutive dots
|
||||
"message": "Test commit",
|
||||
},
|
||||
},
|
||||
expectedErr: "spec.push.branch: Invalid value: \"feature..branch\": invalid git branch name",
|
||||
},
|
||||
{
|
||||
name: "push job with path traversal",
|
||||
jobSpec: map[string]interface{}{
|
||||
"action": string(provisioning.JobActionPush),
|
||||
"repository": repo,
|
||||
"push": map[string]interface{}{
|
||||
"path": "../../etc/passwd", // Invalid: path traversal
|
||||
"message": "Test commit",
|
||||
},
|
||||
},
|
||||
expectedErr: "spec.push.path: Invalid value: \"../../etc/passwd\"",
|
||||
},
|
||||
{
|
||||
name: "delete job without paths or resources",
|
||||
jobSpec: map[string]interface{}{
|
||||
"action": string(provisioning.JobActionDelete),
|
||||
"repository": repo,
|
||||
"delete": map[string]interface{}{},
|
||||
},
|
||||
expectedErr: "spec.delete: Required value: at least one path or resource must be specified",
|
||||
},
|
||||
{
|
||||
name: "delete job with invalid path",
|
||||
jobSpec: map[string]interface{}{
|
||||
"action": string(provisioning.JobActionDelete),
|
||||
"repository": repo,
|
||||
"delete": map[string]interface{}{
|
||||
"paths": []string{"../invalid/path"},
|
||||
},
|
||||
},
|
||||
expectedErr: "spec.delete.paths[0]: Invalid value: \"../invalid/path\"",
|
||||
},
|
||||
{
|
||||
name: "move job without target path",
|
||||
jobSpec: map[string]interface{}{
|
||||
"action": string(provisioning.JobActionMove),
|
||||
"repository": repo,
|
||||
"move": map[string]interface{}{
|
||||
"paths": []string{"dashboard.json"},
|
||||
},
|
||||
},
|
||||
expectedErr: "spec.move.targetPath: Required value: target path is required",
|
||||
},
|
||||
{
|
||||
name: "move job without paths or resources",
|
||||
jobSpec: map[string]interface{}{
|
||||
"action": string(provisioning.JobActionMove),
|
||||
"repository": repo,
|
||||
"move": map[string]interface{}{
|
||||
"targetPath": "new-location/",
|
||||
},
|
||||
},
|
||||
expectedErr: "spec.move: Required value: at least one path or resource must be specified",
|
||||
},
|
||||
{
|
||||
name: "move job with invalid target path",
|
||||
jobSpec: map[string]interface{}{
|
||||
"action": string(provisioning.JobActionMove),
|
||||
"repository": repo,
|
||||
"move": map[string]interface{}{
|
||||
"paths": []string{"dashboard.json"},
|
||||
"targetPath": "../../../etc/", // Invalid: path traversal
|
||||
},
|
||||
},
|
||||
expectedErr: "spec.move.targetPath: Invalid value: \"../../../etc/\"",
|
||||
},
|
||||
{
|
||||
name: "migrate job without migrate options",
|
||||
jobSpec: map[string]interface{}{
|
||||
"action": string(provisioning.JobActionMigrate),
|
||||
"repository": repo,
|
||||
},
|
||||
expectedErr: "spec.migrate: Required value: migrate options required for migrate action",
|
||||
},
|
||||
}
|
||||
|
||||
for i, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
// Create the job object directly
|
||||
jobObj := &unstructured.Unstructured{
|
||||
Object: map[string]interface{}{
|
||||
"apiVersion": "provisioning.grafana.app/v0alpha1",
|
||||
"kind": "Job",
|
||||
"metadata": map[string]interface{}{
|
||||
"name": fmt.Sprintf("test-job-validation-%d", i),
|
||||
"namespace": "default",
|
||||
},
|
||||
"spec": tt.jobSpec,
|
||||
},
|
||||
}
|
||||
|
||||
// Try to create the job - should fail with validation error
|
||||
_, err := helper.Jobs.Resource.Create(ctx, jobObj, metav1.CreateOptions{})
|
||||
require.Error(t, err, "expected validation error for invalid job spec")
|
||||
|
||||
// Verify it's a validation error with correct status code
|
||||
statusError := helper.RequireApiErrorStatus(err, metav1.StatusReasonInvalid, http.StatusUnprocessableEntity)
|
||||
require.Contains(t, statusError.Message, tt.expectedErr, "error message should contain expected validation message")
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -171,7 +171,7 @@ func TestIntegrationProvisioning_MoveJob(t *testing.T) {
|
||||
})
|
||||
|
||||
t.Run("move without target path", func(t *testing.T) {
|
||||
// Create move job without target path (should fail)
|
||||
// Create move job without target path (should fail validation at creation time)
|
||||
spec := provisioning.JobSpec{
|
||||
Action: provisioning.JobActionMove,
|
||||
Move: &provisioning.MoveJobOptions{
|
||||
@@ -180,9 +180,20 @@ func TestIntegrationProvisioning_MoveJob(t *testing.T) {
|
||||
},
|
||||
}
|
||||
|
||||
job := helper.TriggerJobAndWaitForComplete(t, repo, spec)
|
||||
state := mustNestedString(job.Object, "status", "state")
|
||||
assert.Equal(t, "error", state, "move job should have failed due to missing target path")
|
||||
// The job should be rejected by the admission controller with validation error
|
||||
body := asJSON(&spec)
|
||||
result := helper.AdminREST.Post().
|
||||
Namespace("default").
|
||||
Resource("repositories").
|
||||
Name(repo).
|
||||
SubResource("jobs").
|
||||
Body(body).
|
||||
SetHeader("Content-Type", "application/json").
|
||||
Do(ctx)
|
||||
|
||||
require.Error(t, result.Error(), "move job without target path should fail validation")
|
||||
statusError := helper.RequireApiErrorStatus(result.Error(), metav1.StatusReasonInvalid, 422)
|
||||
require.Contains(t, statusError.Message, "spec.move.targetPath", "error should mention missing target path")
|
||||
})
|
||||
|
||||
t.Run("move by resource reference", func(t *testing.T) {
|
||||
|
||||
Reference in New Issue
Block a user