diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 379c729..b27ad7a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -14,8 +14,8 @@ on: env: # Common versions - GO_VERSION: '1.24' - GOLANGCI_VERSION: 'v1.64.8' + GO_VERSION: '1.25' + GOLANGCI_VERSION: 'v2.12.2' DOCKER_BUILDX_VERSION: 'v0.8.2' # Common users. We can't run a step 'if secrets.XXX != ""' but we can run a @@ -70,7 +70,7 @@ jobs: # We could run 'make lint' but we prefer this action because it leaves # 'annotations' (i.e. it comments on PRs to point out linter violations). - name: Lint - uses: golangci/golangci-lint-action@3a919529898de77ec3da873e3063ca4b10e7f5cc # v3 + uses: golangci/golangci-lint-action@9fae48acfc02a90574d7c304a1758ef9895495fa # v7.0.1 with: version: ${{ env.GOLANGCI_VERSION }} diff --git a/.golangci.yml b/.golangci.yml index eb0e236..a8f3ecc 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -2,188 +2,136 @@ # # SPDX-License-Identifier: CC0-1.0 +version: "2" run: - timeout: 90m concurrency: 1 - output: - # colored-line-number|line-number|json|tab|checkstyle|code-climate, default is "colored-line-number" formats: - - format: colored-line-number - print-linter-name: true - show-stats: true - -linters-settings: - errcheck: - # report about not checking of errors in type assetions: `a := b.(MyStruct)`; - # default is false: such cases aren't reported by default. - check-type-assertions: false - - # report about assignment of errors to blank identifier: `num, _ := strconv.Atoi(numStr)`; - # default is false: such cases aren't reported by default. - check-blank: false - - exclude-functions: - - io/ioutil.ReadFile - - io/ioutil.ReadDir - - io/ioutil.ReadAll - - govet: - # report about shadowed variables - check-shadowing: false - - revive: - # confidence for issues, default is 0.8 - confidence: 0.8 - - gofmt: - # simplify code: gofmt with `-s` option, true by default - simplify: true - - goimports: - # put imports beginning with prefix after 3rd-party packages; - # it's a comma-separated list of prefixes - local-prefixes: github.com/crossplane/uptest - - gocyclo: - # minimal code complexity to report, 30 by default (but we recommend 10-20) - min-complexity: 10 - - dupl: - # tokens count to trigger issue, 150 by default - threshold: 100 - - goconst: - # minimal length of string constant, 3 by default - min-len: 3 - # minimal occurrences count to trigger, 3 by default - min-occurrences: 5 - - lll: - # tab width in spaces. Default to 1. - tab-width: 1 - - unparam: - # Inspect exported functions, default is false. Set to true if no external program/library imports your code. - # XXX: if you enable this setting, unparam will report a lot of false-positives in text editors: - # if it's called for subdir of a project it can't find external interfaces. All text editor integrations - # with golangci-lint call it on a directory with the changed file. - check-exported: false - - nakedret: - # make an issue if func has more lines of code than this setting and it has naked returns; default is 30 - max-func-lines: 30 - - prealloc: - # XXX: we don't recommend using this linter before doing performance profiling. - # For most programs usage of prealloc will be a premature optimization. - - # Report preallocation suggestions only on simple loops that have no returns/breaks/continues/gotos in them. - # True by default. - simple: true - range-loops: true # Report preallocation suggestions on range loops, true by default - for-loops: false # Report preallocation suggestions on for loops, false by default - - gocritic: - # Enable multiple checks by tags, run `GL_DEBUG=gocritic golangci-lint` run to see all tags and checks. - # Empty list by default. See https://github.com/go-critic/go-critic#usage -> section "Tags". - enabled-tags: - - performance - - settings: # settings passed to gocritic - captLocal: # must be valid enabled check name - paramsOnly: true - rangeValCopy: - sizeThreshold: 32 - + text: + path: stdout + print-linter-name: true linters: enable: - - govet - - gocyclo - - gocritic + - asasalint + - asciicheck + - bidichk + - bodyclose + - contextcheck + - durationcheck + - errchkjson + - errorlint + - exhaustive + - gocheckcompilerdirectives + - gochecksumtype - goconst - - goimports - - gofmt # We enable this as well as goimports for its simplify mode. - - gosimple + - gocritic + - gocyclo + - gosec + - gosmopolitan + - loggercheck + - makezero + - misspell + - musttag + - nakedret + - nilerr + - nilnesserr + - noctx - prealloc + - protogetter + - reassign + - recvcheck - revive - - staticcheck + - rowserrcheck + - spancheck + - sqlclosecheck + - testifylint - unconvert - - unused - - misspell - - nakedret - - presets: - - bugs - - unused - fast: false - + - unparam + - zerologlint + settings: + dupl: + threshold: 100 + errcheck: + check-type-assertions: false + check-blank: false + exclude-functions: + - io/ioutil.ReadFile + - io/ioutil.ReadDir + - io/ioutil.ReadAll + goconst: + min-len: 3 + min-occurrences: 5 + gocritic: + enabled-tags: + - performance + settings: + captLocal: + paramsOnly: true + rangeValCopy: + sizeThreshold: 32 + gocyclo: + min-complexity: 10 + lll: + tab-width: 1 + nakedret: + max-func-lines: 30 + prealloc: + simple: true + range-loops: true + for-loops: false + revive: + confidence: 0.8 + unparam: + check-exported: false + exclusions: + generated: lax + rules: + - linters: + - dupl + - errcheck + - goconst + - gocyclo + - gosec + - scopelint + - unparam + path: _test(ing)?\.go + - linters: + - gocritic + path: _test\.go + text: (unnamedResult|exitAfterDefer) + - linters: + - gocritic + text: '(hugeParam|rangeValCopy):' + - linters: + - staticcheck + text: 'SA3000:' + - linters: + - gosec + text: 'G101:' + - linters: + - gosec + text: 'G104:' + paths: + - third_party$ + - builtin$ + - examples$ issues: - # Excluding configuration per-path and per-linter - exclude-rules: - # Exclude some linters from running on tests files. - - path: _test(ing)?\.go - linters: - - gocyclo - - errcheck - - dupl - - gosec - - scopelint - - unparam - - # Ease some gocritic warnings on test files. - - path: _test\.go - text: "(unnamedResult|exitAfterDefer)" - linters: - - gocritic - - # These are performance optimisations rather than style issues per se. - # They warn when function arguments or range values copy a lot of memory - # rather than using a pointer. - - text: "(hugeParam|rangeValCopy):" - linters: - - gocritic - - # This "TestMain should call os.Exit to set exit code" warning is not clever - # enough to notice that we call a helper method that calls os.Exit. - - text: "SA3000:" - linters: - - staticcheck - - - text: "k8s.io/api/core/v1" - linters: - - goimports - - # This is a "potential hardcoded credentials" warning. It's triggered by - # any variable with 'secret' in the same, and thus hits a lot of false - # positives in Kubernetes land where a Secret is an object type. - - text: "G101:" - linters: - - gosec - - gas - - # This is an 'errors unhandled' warning that duplicates errcheck. - - text: "G104:" - linters: - - gosec - - gas - - # Independently from option `exclude` we use default exclude patterns, - # it can be disabled by this option. To list all - # excluded by default patterns execute `golangci-lint run --help`. - # Default value for this option is true. - exclude-use-default: false - - # Show only new issues: if there are unstaged changes or untracked files, - # only those changes are analyzed, else only changes in HEAD~ are analyzed. - # It's a super-useful option for integration of golangci-lint into existing - # large codebase. It's not practical to fix all existing issues at the moment - # of integration: much better don't allow issues in new code. - # Default is false. - new: false - - # Maximum issues count per one linter. Set to 0 to disable. Default is 50. max-issues-per-linter: 0 - - # Maximum count of issues with the same text. Set to 0 to disable. Default is 3. max-same-issues: 0 + new: false +formatters: + enable: + - gofmt + - goimports + settings: + gofmt: + simplify: true + goimports: + local-prefixes: + - github.com/crossplane/uptest + exclusions: + generated: lax + paths: + - third_party$ + - builtin$ + - examples$ diff --git a/Makefile b/Makefile index ccfd23e..5cdf24d 100644 --- a/Makefile +++ b/Makefile @@ -21,10 +21,10 @@ S3_BUCKET ?= crossplane.uptest.releases # ==================================================================================== # Setup Go -GO_REQUIRED_VERSION = 1.24 +GO_REQUIRED_VERSION = 1.25 # GOLANGCILINT_VERSION is inherited from build submodule by default. # Uncomment below if you need to override the version. -GOLANGCILINT_VERSION ?= 1.64.8 +GOLANGCILINT_VERSION ?= 2.12.2 GO_STATIC_PACKAGES = $(GO_PROJECT)/cmd/uptest GO_LDFLAGS += -X $(GO_PROJECT)/internal/version.Version=$(VERSION) diff --git a/cmd/uptest/main.go b/cmd/uptest/main.go index 0c7ec4f..a6a0bed 100644 --- a/cmd/uptest/main.go +++ b/cmd/uptest/main.go @@ -33,6 +33,12 @@ var ( setupScript = e2e.Flag("setup-script", "Script that will be executed before running tests.").Default("").String() teardownScript = e2e.Flag("teardown-script", "Script that will be executed after running tests.").Default("").String() + postAssertScript = e2e.Flag("post-assert-script", "Script that will be executed once after ALL resources have been asserted, "+ + "before the update, import and delete steps.\n"+ + "The per-resource \"uptest.upbound.io/post-assert-hook\" annotation runs once per resource, interleaved with the "+ + "assertions. This runs a single time for the whole test case, so a check that must observe every resource in the "+ + "same steady state has somewhere to live.").Default("").String() + defaultTimeout = e2e.Flag("default-timeout", "Default timeout in seconds for the test.\n"+ "Timeout could be overridden per resource using \"uptest.upbound.io/timeout\" annotation.").Default("1200s").Duration() defaultConditions = e2e.Flag("default-conditions", "Comma separated list of default conditions to wait for a successful test.\n"+ @@ -75,21 +81,9 @@ func e2eTests() { kingpin.Fatalf("No manifest to test provided.") } - setupPath := "" - if *setupScript != "" { - setupPath, err = filepath.Abs(*setupScript) - if err != nil { - kingpin.FatalIfError(err, "cannot get absolute path of setup script") - } - } - - teardownPath := "" - if *teardownScript != "" { - teardownPath, err = filepath.Abs(*teardownScript) - if err != nil { - kingpin.FatalIfError(err, "cannot get absolute path of teardown script") - } - } + setupPath := absScriptPath(*setupScript, "setup script") + teardownPath := absScriptPath(*teardownScript, "teardown script") + postAssertPath := absScriptPath(*postAssertScript, "post-assert script") builder := pkg.NewAutomatedTestBuilder() automatedTest := builder. @@ -97,6 +91,7 @@ func e2eTests() { SetDataSourcePath(*dataSourcePath). SetSetupScriptPath(setupPath). SetTeardownScriptPath(teardownPath). + SetPostAssertScriptPath(postAssertPath). SetDefaultConditions(strings.Split(*defaultConditions, ",")). SetDefaultTimeout(*defaultTimeout). SetDirectory(*testDir). @@ -113,3 +108,18 @@ func e2eTests() { ctx := context.Background() kingpin.FatalIfError(pkg.RunTestContext(ctx, automatedTest), "cannot run e2e tests successfully") } + +// absScriptPath resolves a script flag to an absolute path, mapping an unset +// flag to the empty string, which every template reads as "no script". +// +// The three script flags all need this and none of them may proceed on a +// resolution failure, so doing it inline once per flag put e2eTests over the +// cyclomatic-complexity limit for no gain in clarity. +func absScriptPath(script, description string) string { + if script == "" { + return "" + } + path, err := filepath.Abs(script) + kingpin.FatalIfError(err, "cannot get absolute path of %s", description) + return path +} diff --git a/go.mod b/go.mod index 2885fbd..b46a854 100644 --- a/go.mod +++ b/go.mod @@ -4,13 +4,14 @@ module github.com/crossplane/uptest/v2 -go 1.24.6 +go 1.25.0 require ( github.com/alecthomas/kong v1.4.0 github.com/crossplane/crossplane-runtime/v2 v2.0.0 github.com/crossplane/crossplane/v2 v2.0.2 github.com/google/go-cmp v0.7.0 + github.com/kaessert/crossplane-update-tester/sidecar v0.1.0 github.com/kyverno/chainsaw v0.2.13-0.20250116043056-57a42010852a github.com/kyverno/pkg/ext v0.0.0-20240418121121-df8add26c55c gopkg.in/alecthomas/kingpin.v2 v2.2.6 diff --git a/go.sum b/go.sum index 1a758c7..da04a21 100644 --- a/go.sum +++ b/go.sum @@ -1060,6 +1060,8 @@ github.com/jtolds/gls v4.20.0+incompatible h1:xdiiI2gbIgH/gLH7ADydsJ1uDOEzR8yvV7 github.com/jtolds/gls v4.20.0+incompatible/go.mod h1:QJZ7F/aHp+rZTRtaJ1ow/lLfFfVYBRgL+9YlvaHOwJU= github.com/jung-kurt/gofpdf v1.0.0/go.mod h1:7Id9E/uU8ce6rXgefFLlgrJj/GYY22cpxn+r32jIOes= github.com/jung-kurt/gofpdf v1.0.3-0.20190309125859-24315acbbda5/go.mod h1:7Id9E/uU8ce6rXgefFLlgrJj/GYY22cpxn+r32jIOes= +github.com/kaessert/crossplane-update-tester/sidecar v0.1.0 h1:8JGr1gG3OwZe6vbKfd0egpKCq6QQMhrbdRqbYlCeghc= +github.com/kaessert/crossplane-update-tester/sidecar v0.1.0/go.mod h1:CA5PJj/rBCfdEEHDi8XrxEVrkbc2Wtwq5nQ/KpLayDY= github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51/go.mod h1:CzGEWj7cYgsdH8dAjBGEr58BoE7ScuLd+fwFZ44+/x8= github.com/kisielk/errcheck v1.5.0/go.mod h1:pFxgyoBC7bSaBwPgfKdkLd5X25qrDl4LWUI2bnpBCr8= github.com/kisielk/gotool v1.0.0/go.mod h1:XhKaO+MFFWcvkIS/tQcRk01m1F5IRFswLeQ+oQHNcck= diff --git a/internal/config/builder.go b/internal/config/builder.go index eb837d1..8535659 100644 --- a/internal/config/builder.go +++ b/internal/config/builder.go @@ -51,6 +51,13 @@ func (b *Builder) SetTeardownScriptPath(teardownScriptPath string) *Builder { return b } +// SetPostAssertScriptPath sets the path of the script run once after every +// resource has been asserted, and returns the Builder. +func (b *Builder) SetPostAssertScriptPath(postAssertScriptPath string) *Builder { + b.test.PostAssertScriptPath = postAssertScriptPath + return b +} + // SetDefaultTimeout sets the default timeout duration for the AutomatedTest and returns the Builder. func (b *Builder) SetDefaultTimeout(defaultTimeout time.Duration) *Builder { b.test.DefaultTimeout = defaultTimeout diff --git a/internal/config/config.go b/internal/config/config.go index 728dd78..c054583 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -50,6 +50,12 @@ type AutomatedTest struct { SetupScriptPath string TeardownScriptPath string + // PostAssertScriptPath is run once, after every resource in the test + // case has been asserted, and before the update, import and delete + // steps. It complements the per-resource post-assert hook, which is + // interleaved with the assertions and therefore cannot observe the + // test case as a whole. + PostAssertScriptPath string DefaultTimeout time.Duration DefaultConditions []string @@ -75,12 +81,13 @@ type Manifest struct { // TestCase represents a test-case to be run by chainsaw. type TestCase struct { - Timeout time.Duration - SetupScriptPath string - TeardownScriptPath string - SkipUpdate bool - SkipImport bool - SkipWebhookCheck bool + Timeout time.Duration + SetupScriptPath string + TeardownScriptPath string + PostAssertScriptPath string + SkipUpdate bool + SkipImport bool + SkipWebhookCheck bool OnlyCleanUptestResources bool diff --git a/internal/prepare.go b/internal/prepare.go index a95e200..1f7885a 100644 --- a/internal/prepare.go +++ b/internal/prepare.go @@ -19,6 +19,7 @@ import ( "strings" "github.com/crossplane/crossplane-runtime/v2/pkg/errors" + "github.com/kaessert/crossplane-update-tester/sidecar" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" kyaml "k8s.io/apimachinery/pkg/util/yaml" "sigs.k8s.io/yaml" @@ -38,6 +39,13 @@ var ( type injectedManifest struct { Path string Manifest string + // Sidecar holds the injected text of the manifest's .uptest + // sidecar when one exists, and HasSidecar distinguishes "no sidecar" + // from "an empty one" — the former is the ordinary un-migrated state + // every example starts in, and it must behave exactly as it did before + // sidecars existed. + Sidecar string + HasSidecar bool } // PreparerOption is a functional option type for configuring a Preparer. @@ -101,36 +109,137 @@ func (p *Preparer) PrepareManifests() ([]config.Manifest, error) { } manifests := make([]config.Manifest, 0, len(injectedFiles)) + sidecarsLoaded := 0 for _, data := range injectedFiles { - decoder := kyaml.NewYAMLOrJSONDecoder(bytes.NewBufferString(data.Manifest), 1024) - for { - u := &unstructured.Unstructured{} - if err := decoder.Decode(&u); err != nil { - if errors.Is(err, io.EOF) { - break - } - return nil, errors.Wrap(err, "cannot decode manifest") + docs, err := decodeDocuments(data.Manifest) + if err != nil { + return nil, errors.Wrapf(err, "cannot decode manifest %s", data.Path) + } + if data.HasSidecar { + if err := applySidecar(data.Path, data.Sidecar, docs); err != nil { + return nil, err } - if u != nil { - if v, ok := u.GetAnnotations()["upjet.upbound.io/manual-intervention"]; ok { - log.Printf("Skipping %s with name %s since it requires the following manual intervention: %s\n", u.GroupVersionKind().String(), u.GetName(), v) - continue - } - y, err := yaml.Marshal(u) - if err != nil { - return nil, errors.Wrapf(err, "cannot marshal manifest for \"%s/%s\"", u.GetObjectKind(), u.GetName()) - } - manifests = append(manifests, config.Manifest{ - FilePath: data.Path, - Object: u, - YAML: string(y), - }) + sidecarsLoaded++ + } + for _, u := range docs { + if v, ok := u.GetAnnotations()["upjet.upbound.io/manual-intervention"]; ok { + log.Printf("Skipping %s with name %s since it requires the following manual intervention: %s\n", u.GroupVersionKind().String(), u.GetName(), v) + continue } + y, err := yaml.Marshal(u) + if err != nil { + return nil, errors.Wrapf(err, "cannot marshal manifest for \"%s/%s\"", u.GetObjectKind(), u.GetName()) + } + manifests = append(manifests, config.Manifest{ + FilePath: data.Path, + Object: u, + YAML: string(y), + }) } } + // Visible in ordinary E2E output so an operator can tell a sidecar-aware + // run from a blind one — an old binary on a migrated tree exits 0 having + // read zero annotations, and this line is what makes that silent failure + // mode detectable from the log alone. + log.Printf("Loaded %d manifest sidecar(s)\n", sidecarsLoaded) return manifests, nil } +// decodeDocuments decodes every YAML document of one manifest file's +// (already variable-injected) text into unstructured objects. +// +// Decoding the whole file before anything is applied to it is what lets a +// sidecar be resolved against the file as a WHOLE: an ambiguous selector, an +// unmatched selector and two sidecar documents claiming one object are +// properties of the pairing between a sidecar and its manifest's full +// document set, and a loop that handles one document at a time cannot see +// any of them. +func decodeDocuments(data string) ([]*unstructured.Unstructured, error) { + decoder := kyaml.NewYAMLOrJSONDecoder(bytes.NewBufferString(data), 1024) + var docs []*unstructured.Unstructured + for { + u := &unstructured.Unstructured{} + if err := decoder.Decode(&u); err != nil { + if errors.Is(err, io.EOF) { + return docs, nil + } + return nil, errors.Wrap(err, "cannot decode manifest") + } + if u == nil || len(u.Object) == 0 { + continue + } + docs = append(docs, u) + } +} + +// ownsAnnotation reports whether key is one that uptest itself reads. +// uptest polices only the keys it owns — a key live in both a manifest and +// its sidecar has no defensible precedence — and every OTHER consumer of +// example manifests (e.g. the update-tester tool) owns its own keys the +// same way, so there is no shared closed set to drift between the two. +func ownsAnnotation(key string) bool { + return strings.HasPrefix(key, "uptest.upbound.io/") || key == config.AnnotationKeyExampleID +} + +// applySidecar merges a manifest's sidecar onto its already-decoded +// documents, in place, before anything downstream consumes them. +// +// The annotations are set on the objects themselves rather than held in a +// side channel, so the rendered chainsaw case, the applied object and every +// assertion template see exactly what they would have seen had the +// annotations been written inline. The sidecar changes only where a file's +// author writes its harness configuration, never what uptest does with it. +func applySidecar(path, sidecarText string, docs []*unstructured.Unstructured) error { + sc, err := sidecar.Parse([]byte(sidecarText)) + if err != nil { + return errors.Wrapf(err, "cannot parse %s", sidecar.PathFor(path)) + } + + // Switch, not overlay: once a sidecar exists for this file, any document + // in it still carrying one of uptest's own annotation keys inline is a + // hard error rather than a silently-ignored duplicate. Checked across + // EVERY document, not only the ones the sidecar targets — a stray + // timeout left behind on a prerequisite Secret is exactly the residue a + // migration leaves, and it is otherwise silent. + for _, u := range docs { + for key := range u.GetAnnotations() { + if ownsAnnotation(key) { + return errors.Errorf( + "%s: %s %q carries annotation %q inline, but a sidecar exists at %s — "+ + "a sidecar REPLACES a manifest's harness annotations, it does not overlay them", + path, u.GroupVersionKind().Kind, u.GetName(), key, sidecar.PathFor(path)) + } + } + } + + targets := make([]sidecar.ObjectID, len(docs)) + for i, u := range docs { + apiVersion, kind := u.GroupVersionKind().ToAPIVersionAndKind() + targets[i] = sidecar.ObjectID{ + APIVersion: apiVersion, + Kind: kind, + Name: u.GetName(), + Namespace: u.GetNamespace(), + } + } + + resolved, err := sidecar.Resolve(sc, targets) + if err != nil { + return errors.Wrapf(err, "cannot resolve %s", sidecar.PathFor(path)) + } + for idx, anns := range resolved { + merged := docs[idx].GetAnnotations() + if merged == nil { + merged = map[string]string{} + } + for k, v := range anns { + merged[k] = v + } + docs[idx].SetAnnotations(merged) + } + return nil +} + func (p *Preparer) injectVariables() ([]injectedManifest, error) { dataSourceMap := make(map[string]string) if p.dataSourcePath != "" { @@ -153,19 +262,52 @@ func (p *Preparer) injectVariables() ([]injectedManifest, error) { Path: f, Manifest: p.injectValues(string(manifestData), dataSourceMap), } + + // A sidecar carries the same ${data.*} placeholders a manifest may, + // so it goes through the same substitution step. It deliberately does + // NOT go through the ${Rand.*} step: injectValues generates a fresh + // random value at every occurrence, so a sidecar's copy of a random + // placeholder could never equal the manifest's, and a name:/ + // namespace: selector built from one would silently never match. + // Leaving ${Rand.*} untouched means the sidecar parser's own + // templating-placeholder check rejects it outright instead. + sidecarData, err := os.ReadFile(filepath.Clean(sidecar.PathFor(f))) + switch { + case err == nil: + inputs[i].HasSidecar = true + inputs[i].Sidecar = p.injectDataSource(string(sidecarData), dataSourceMap) + case os.IsNotExist(err): + // No sidecar: the un-migrated state every provider is in until it + // migrates. Nothing changes for it. + default: + return nil, errors.Wrapf(err, "cannot read %s", sidecar.PathFor(f)) + } } return inputs, nil } func (p *Preparer) injectValues(manifestData string, dataSourceMap map[string]string) string { - // Inject data source values such as tenantID, objectID, accountID + manifestData = p.injectDataSource(manifestData, dataSourceMap) + return p.injectRandom(manifestData) +} + +// injectDataSource substitutes ${data.*} placeholders such as tenantID, +// objectID or accountID. Split out from injectValues so a sidecar's text can +// go through the identical substitution without also going through +// injectRandom — see the comment at its one non-manifest call site. +func (p *Preparer) injectDataSource(manifestData string, dataSourceMap map[string]string) string { dataSourceKeys := dataSourceRegex.FindAllStringSubmatch(manifestData, -1) for _, dataSourceKey := range dataSourceKeys { if v, ok := dataSourceMap[dataSourceKey[1]]; ok { manifestData = strings.ReplaceAll(manifestData, dataSourceKey[0], v) } } - // Inject random strings + return manifestData +} + +// injectRandom substitutes ${Rand.*} placeholders with a freshly generated +// value at every occurrence. +func (p *Preparer) injectRandom(manifestData string) string { randomKeys := randomStrRegex.FindAllStringSubmatch(manifestData, -1) for _, randomKey := range randomKeys { switch randomKey[1] { diff --git a/internal/prepare_test.go b/internal/prepare_test.go new file mode 100644 index 0000000..553b6ae --- /dev/null +++ b/internal/prepare_test.go @@ -0,0 +1,387 @@ +// SPDX-FileCopyrightText: 2024 The Crossplane Authors +// +// SPDX-License-Identifier: CC0-1.0 + +package internal + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/google/go-cmp/cmp" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + "github.com/crossplane/uptest/v2/internal/config" +) + +const ( + widgetManifest = `apiVersion: widget.example.org/v1alpha1 +kind: Widget +metadata: + name: test-widget +spec: + forProvider: {} +` + + secretManifest = `apiVersion: v1 +kind: Secret +metadata: + name: prerequisite-secret + namespace: crossplane-system +type: Opaque +stringData: + key: value +` +) + +func TestDecodeDocuments(t *testing.T) { + cases := map[string]struct { + reason string + data string + wantLen int + wantErr bool + }{ + "SingleDocument": { + reason: "A single-document manifest decodes to exactly one object.", + data: widgetManifest, + wantLen: 1, + }, + "MultiDocument": { + reason: "A multi-document manifest decodes every document, in file order.", + data: secretManifest + "---\n" + widgetManifest, + wantLen: 2, + }, + "BlankDocument": { + reason: "A trailing '---' with nothing after it produces no extra document.", + data: widgetManifest + "---\n", + wantLen: 1, + }, + "Empty": { + reason: "An empty file decodes to zero documents and no error.", + data: "", + wantLen: 0, + }, + "Malformed": { + reason: "Malformed YAML is a decode error, not a silently skipped document.", + data: "not: valid: yaml: at: all: [", + wantErr: true, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + docs, err := decodeDocuments(tc.data) + if tc.wantErr { + if err == nil { + t.Fatalf("\n%s\ndecodeDocuments(...): expected an error, got none", tc.reason) + } + return + } + if err != nil { + t.Fatalf("\n%s\ndecodeDocuments(...): unexpected error: %v", tc.reason, err) + } + if len(docs) != tc.wantLen { + t.Errorf("\n%s\ndecodeDocuments(...): -want %d documents, +got %d", tc.reason, tc.wantLen, len(docs)) + } + }) + } +} + +func TestOwnsAnnotation(t *testing.T) { + cases := map[string]struct { + reason string + key string + want bool + }{ + "Timeout": { + reason: "uptest.upbound.io/timeout is an uptest key.", + key: config.AnnotationKeyTimeout, + want: true, + }, + "PostAssertHook": { + reason: "uptest.upbound.io/post-assert-hook is an uptest key.", + key: config.AnnotationKeyPostAssertHook, + want: true, + }, + "ExampleID": { + reason: "meta.upbound.io/example-id is an uptest key even though it does not share the uptest.upbound.io prefix.", + key: config.AnnotationKeyExampleID, + want: true, + }, + "UpdateTest": { + reason: "crossplane.io/update-test belongs to update-tester, not uptest. uptest polices only the keys it reads.", + key: "crossplane.io/update-test", + want: false, + }, + "ExternalName": { + reason: "crossplane.io/external-name is a real Crossplane annotation, never uptest's to police.", + key: "crossplane.io/external-name", + want: false, + }, + "UnrelatedPrefix": { + reason: "An unrelated annotation is never owned.", + key: "example.org/some-key", + want: false, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + got := ownsAnnotation(tc.key) + if got != tc.want { + t.Errorf("\n%s\nownsAnnotation(%q): -want %v, +got %v", tc.reason, tc.key, tc.want, got) + } + }) + } +} + +func TestApplySidecar(t *testing.T) { + cases := map[string]struct { + reason string + path string + sidecarText string + docs []*unstructured.Unstructured + wantErr string + wantAnns map[string]map[string]string // object name -> expected annotations + }{ + "MergesOntoSelectedTarget": { + reason: "A sidecar targeting the sole matching object merges its annotations onto it.", + path: "widget.yaml", + sidecarText: `for: widget.example.org/v1alpha1/Widget +uptest.upbound.io/timeout: "1200" +`, + docs: mustDecode(t, widgetManifest), + wantAnns: map[string]map[string]string{ + "test-widget": {"uptest.upbound.io/timeout": "1200"}, + }, + }, + "SelectsNonFirstDocument": { + reason: "Selection targets the object the sidecar names, not document order — the Secret is first in the file but the sidecar targets the Widget.", + path: "bundle.yaml", + sidecarText: `for: widget.example.org/v1alpha1/Widget +meta.upbound.io/example-id: widget/v1alpha1/widget +`, + docs: mustDecode(t, secretManifest+"---\n"+widgetManifest), + wantAnns: map[string]map[string]string{ + "prerequisite-secret": nil, + "test-widget": {"meta.upbound.io/example-id": "widget/v1alpha1/widget"}, + }, + }, + "ConflictInlineWithSidecar": { + reason: "A sidecar REPLACES a manifest's harness annotations. One still present inline is a hard error, not a silent overlay.", + path: "widget.yaml", + sidecarText: `for: widget.example.org/v1alpha1/Widget +uptest.upbound.io/timeout: "1200" +`, + docs: mustDecode(t, `apiVersion: widget.example.org/v1alpha1 +kind: Widget +metadata: + name: test-widget + annotations: + uptest.upbound.io/timeout: "600" +`), + wantErr: "carries annotation", + }, + "ConflictOnUnselectedDocument": { + reason: "The switch check runs across every document in the file, not only the ones the sidecar targets — a stray annotation left on a prerequisite Secret must be caught too.", + path: "bundle.yaml", + sidecarText: `for: widget.example.org/v1alpha1/Widget +uptest.upbound.io/timeout: "1200" +`, + docs: mustDecode(t, `apiVersion: v1 +kind: Secret +metadata: + name: prerequisite-secret + namespace: crossplane-system + annotations: + meta.upbound.io/example-id: leftover +--- +`+widgetManifest), + wantErr: "carries annotation", + }, + "AmbiguousSelector": { + reason: "A selector matching more than one object without a narrowing name:/namespace: is an error from the sidecar package itself, propagated unchanged.", + path: "bundle.yaml", + sidecarText: `for: widget.example.org/v1alpha1/Widget +uptest.upbound.io/timeout: "1200" +`, + docs: mustDecode(t, widgetManifest+`--- +apiVersion: widget.example.org/v1alpha1 +kind: Widget +metadata: + name: other-widget +`), + wantErr: "ambiguous", + }, + "MissingForDirective": { + reason: "A sidecar document with no for: is a parse error.", + path: "widget.yaml", + sidecarText: `uptest.upbound.io/timeout: "1200" +`, + docs: mustDecode(t, widgetManifest), + wantErr: "for:", + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + err := applySidecar(tc.path, tc.sidecarText, tc.docs) + if tc.wantErr != "" { + if err == nil { + t.Fatalf("\n%s\napplySidecar(...): expected an error containing %q, got none", tc.reason, tc.wantErr) + } + if !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("\n%s\napplySidecar(...): error %q does not contain %q", tc.reason, err.Error(), tc.wantErr) + } + return + } + if err != nil { + t.Fatalf("\n%s\napplySidecar(...): unexpected error: %v", tc.reason, err) + } + for _, d := range tc.docs { + want, ok := tc.wantAnns[d.GetName()] + if !ok { + continue + } + got := d.GetAnnotations() + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("\n%s\nannotations for %s: -want, +got:\n%s", tc.reason, d.GetName(), diff) + } + } + }) + } +} + +func mustDecode(t *testing.T, data string) []*unstructured.Unstructured { + t.Helper() + docs, err := decodeDocuments(data) + if err != nil { + t.Fatalf("mustDecode: %v", err) + } + return docs +} + +// TestPrepareManifestsSidecar exercises the full PrepareManifests path +// end-to-end against real files on disk, covering both the un-migrated +// (no sidecar) case — which must behave exactly as it did before sidecars +// existed — and the migrated case. +func TestPrepareManifestsSidecar(t *testing.T) { + cases := map[string]struct { + reason string + manifest string + sidecar string // "" means no sidecar file is written at all + writeNoFile bool + wantAnns map[string]string + wantErr string + }{ + "NoSidecarUnchanged": { + reason: "A manifest with no sidecar file behaves exactly as before sidecars existed: whatever annotations are inline are the ones that land, nothing more.", + manifest: widgetManifest, + wantAnns: nil, + }, + "SidecarMerged": { + reason: "A manifest with a sidecar gets the sidecar's annotations merged onto the decoded object.", + manifest: widgetManifest, + sidecar: `for: widget.example.org/v1alpha1/Widget +uptest.upbound.io/timeout: "900" +meta.upbound.io/example-id: widget/v1alpha1/widget +`, + wantAnns: map[string]string{ + "uptest.upbound.io/timeout": "900", + "meta.upbound.io/example-id": "widget/v1alpha1/widget", + }, + }, + "SidecarDataSubstitution": { + reason: "${data.*} placeholders inside a sidecar are substituted through the same injectValues path as the manifest.", + manifest: widgetManifest, + sidecar: `for: widget.example.org/v1alpha1/Widget +uptest.upbound.io/timeout: "${data.timeoutSeconds}" +`, + wantAnns: map[string]string{ + "uptest.upbound.io/timeout": "1800", + }, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + dir := t.TempDir() + manifestPath := filepath.Join(dir, "widget.yaml") + if err := os.WriteFile(manifestPath, []byte(tc.manifest), 0o600); err != nil { + t.Fatalf("WriteFile(manifest): %v", err) + } + if tc.sidecar != "" { + if err := os.WriteFile(manifestPath+".uptest", []byte(tc.sidecar), 0o600); err != nil { + t.Fatalf("WriteFile(sidecar): %v", err) + } + } + + opts := []PreparerOption{WithTestDirectory(t.TempDir())} + if strings.Contains(tc.sidecar, "${data.") { + dsPath := filepath.Join(dir, "datasource.yaml") + if err := os.WriteFile(dsPath, []byte("timeoutSeconds: \"1800\"\n"), 0o600); err != nil { + t.Fatalf("WriteFile(datasource): %v", err) + } + opts = append(opts, WithDataSource(dsPath)) + } + + p := NewPreparer([]string{manifestPath}, opts...) + manifests, err := p.PrepareManifests() + if tc.wantErr != "" { + if err == nil { + t.Fatalf("\n%s\nPrepareManifests(): expected an error containing %q, got none", tc.reason, tc.wantErr) + } + if !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("\n%s\nPrepareManifests(): error %q does not contain %q", tc.reason, err.Error(), tc.wantErr) + } + return + } + if err != nil { + t.Fatalf("\n%s\nPrepareManifests(): unexpected error: %v", tc.reason, err) + } + if len(manifests) != 1 { + t.Fatalf("\n%s\nPrepareManifests(): -want 1 manifest, +got %d", tc.reason, len(manifests)) + } + got := manifests[0].Object.GetAnnotations() + if diff := cmp.Diff(tc.wantAnns, got); diff != "" { + t.Errorf("\n%s\nannotations: -want, +got:\n%s", tc.reason, diff) + } + }) + } +} + +func TestInjectDataSourceVsInjectRandom(t *testing.T) { + p := &Preparer{} + dataSourceMap := map[string]string{"tenantID": "t-1234"} + + t.Run("InjectDataSourceSubstitutesDataPlaceholders", func(t *testing.T) { + got := p.injectDataSource("tenant: ${data.tenantID}", dataSourceMap) + want := "tenant: t-1234" + if got != want { + t.Errorf("injectDataSource(...): -want %q, +got %q", want, got) + } + }) + + t.Run("InjectDataSourceLeavesRandPlaceholdersUntouched", func(t *testing.T) { + // This is the property AC 2 depends on: a sidecar's text must go + // through ${data.*} substitution without also going through + // ${Rand.*} substitution, so a ${Rand.*} placeholder used in a + // name:/namespace: selector is rejected by the sidecar parser + // instead of being silently replaced with a value that could never + // equal the manifest's own random suffix. + in := "name: widget-${Rand.RFC1123Subdomain}" + got := p.injectDataSource(in, dataSourceMap) + if got != in { + t.Errorf("injectDataSource(...): expected ${Rand.*} left untouched, -want %q +got %q", in, got) + } + }) + + t.Run("InjectValuesSubstitutesBoth", func(t *testing.T) { + got := p.injectValues("tenant: ${data.tenantID}", dataSourceMap) + if strings.Contains(got, "${data.") { + t.Errorf("injectValues(...): ${data.*} placeholder was not substituted: %q", got) + } + }) +} diff --git a/internal/templates/00-apply.yaml.tmpl b/internal/templates/00-apply.yaml.tmpl index bf07646..def509a 100644 --- a/internal/templates/00-apply.yaml.tmpl +++ b/internal/templates/00-apply.yaml.tmpl @@ -96,3 +96,15 @@ spec: entrypoint: {{ $resource.PostAssertScriptPath }} {{- end }} {{- end }} +{{- if .TestCase.PostAssertScriptPath }} + - name: Post Assert + description: | + Run the test case's post-assert script, once, after every resource has + been asserted. The per-resource post-assert hook is interleaved with the + assertions and therefore only ever sees one resource at a time; this step + is the only place a check that must observe every resource in the same + steady state can run. + try: + - command: + entrypoint: {{ .TestCase.PostAssertScriptPath }} +{{- end }} diff --git a/internal/templates/renderer.go b/internal/templates/renderer.go index a3fb3fd..671dbe1 100644 --- a/internal/templates/renderer.go +++ b/internal/templates/renderer.go @@ -15,11 +15,21 @@ import ( "github.com/crossplane/uptest/v2/internal/config" ) +// File names of the rendered chainsaw test case steps, shared with callers +// that need to know a step's file name without rendering it (e.g. to build +// the ordered list of files a test case is expected to produce). +const ( + ApplyFilename = "00-apply.yaml" + UpdateFilename = "01-update.yaml" + ImportFilename = "02-import.yaml" + DeleteFilename = "03-delete.yaml" +) + var fileTemplates = map[string]string{ - "00-apply.yaml": inputFileTemplate, - "01-update.yaml": updateFileTemplate, - "02-import.yaml": importFileTemplate, - "03-delete.yaml": deleteFileTemplate, + ApplyFilename: inputFileTemplate, + UpdateFilename: updateFileTemplate, + ImportFilename: importFileTemplate, + DeleteFilename: deleteFileTemplate, } // Render renders the specified list of resources as a test case diff --git a/internal/templates/renderer_test.go b/internal/templates/renderer_test.go index c901546..58b8ce8 100644 --- a/internal/templates/renderer_test.go +++ b/internal/templates/renderer_test.go @@ -5,6 +5,7 @@ package templates import ( + "strings" "testing" "time" @@ -2178,3 +2179,170 @@ spec: }) } } + +func TestRenderWithPostAssertScript(t *testing.T) { + type args struct { + tc *config.TestCase + resources []config.Resource + } + type want struct { + out map[string]string + err error + } + tests := map[string]struct { + args args + want want + }{ + "PostAssertScriptRendersOneStepAfterAllAssertions": { + args: args{ + tc: &config.TestCase{ + SetupScriptPath: "/tmp/setup.sh", + PostAssertScriptPath: "/tmp/post-assert.sh", + Timeout: 10 * time.Minute, + TestDirectory: "/tmp/test-input.yaml", + SkipUpdate: true, + SkipImport: true, + }, + resources: []config.Resource{ + { + Name: "example-bucket", + APIVersion: "bucket.s3.aws.upbound.io/v1alpha1", + Kind: "Bucket", + KindGroup: "s3.aws.upbound.io", + YAML: bucketManifest, + Conditions: []string{"Test"}, + }, + }, + }, + want: want{ + out: map[string]string{ + "00-apply.yaml": `# This file belongs to the resource apply step. +apiVersion: chainsaw.kyverno.io/v1alpha1 +kind: Test +metadata: + name: apply +spec: + timeouts: + apply: 10m0s + assert: 10m0s + exec: 10m0s + steps: + - name: Run Setup Script + description: Setup the test environment by running the setup script. + try: + - command: + entrypoint: /tmp/setup.sh + - name: Apply Resources + description: Apply resources to the cluster. + try: + - script: + content: | + echo "Checking webhook health before proceeding..." + curl -sL https://raw.githubusercontent.com/crossplane/uptest/main/hack/check_endpoints.sh -o /tmp/check_endpoints.sh && chmod +x /tmp/check_endpoints.sh + /tmp/check_endpoints.sh + - sleep: + # Wait for conversion webhook endpoints to become fully operational after health check + duration: 10s + - apply: + file: /tmp/test-input.yaml + - script: + content: | + echo "Running annotation script with retry logic" + retry_annotate() { + local max_attempts=10 + local delay=5 + local attempt=1 + local cmd="$1" + + while [ $attempt -le $max_attempts ]; do + echo "Annotation attempt $attempt/$max_attempts for: $cmd" + if eval "$cmd"; then + echo "Annotation successful on attempt $attempt" + return 0 + else + echo "Annotation failed on attempt $attempt" + if [ $attempt -lt $max_attempts ]; then + echo "Retrying in ${delay}s..." + sleep $delay + fi + ((attempt++)) + fi + done + echo "Annotation failed after $max_attempts attempts" + return 1 + } + retry_annotate "${KUBECTL} annotate s3.aws.upbound.io/example-bucket upjet.upbound.io/test=true --overwrite" + - name: Assert Status Conditions + description: | + Assert applied resources. First, run the pre-assert script if exists. + Then, check the status conditions. Finally run the post-assert script if it + exists. + try: + - assert: + resource: + apiVersion: bucket.s3.aws.upbound.io/v1alpha1 + kind: Bucket + metadata: + name: example-bucket + status: + ((conditions[?type == 'Test'])[0]): + status: "True" + - name: Post Assert + description: | + Run the test case's post-assert script, once, after every resource has + been asserted. The per-resource post-assert hook is interleaved with the + assertions and therefore only ever sees one resource at a time; this step + is the only place a check that must observe every resource in the same + steady state can run. + try: + - command: + entrypoint: /tmp/post-assert.sh +`, + }, + }, + }, + } + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + got, err := Render(tc.args.tc, tc.args.resources, true) + if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + t.Errorf("Render(...): -want error, +got error:\n%s", diff) + } + if diff := cmp.Diff(tc.want.out, got); diff != "" { + t.Errorf("Render(...): -want, +got:\n%s", diff) + } + }) + } +} + +// TestRenderWithoutPostAssertScriptOmitsTheStep is the negative control for +// TestRenderWithPostAssertScript: an unset PostAssertScriptPath must leave the +// rendered test case byte-for-byte as it was before the option existed, so +// every consumer that does not opt in is unaffected. +func TestRenderWithoutPostAssertScriptOmitsTheStep(t *testing.T) { + tc := &config.TestCase{ + SetupScriptPath: "/tmp/setup.sh", + Timeout: 10 * time.Minute, + TestDirectory: "/tmp/test-input.yaml", + SkipUpdate: true, + SkipImport: true, + } + resources := []config.Resource{ + { + Name: "example-bucket", + APIVersion: "bucket.s3.aws.upbound.io/v1alpha1", + Kind: "Bucket", + KindGroup: "s3.aws.upbound.io", + YAML: bucketManifest, + Conditions: []string{"Test"}, + }, + } + + got, err := Render(tc, resources, true) + if err != nil { + t.Fatalf("Render(...): unexpected error: %v", err) + } + if strings.Contains(got["00-apply.yaml"], "Post Assert") { + t.Errorf("rendered a Post Assert step with no PostAssertScriptPath set:\n%s", got["00-apply.yaml"]) + } +} diff --git a/internal/tester.go b/internal/tester.go index 9492da4..906c894 100644 --- a/internal/tester.go +++ b/internal/tester.go @@ -42,10 +42,10 @@ import ( ) var testFiles = []string{ - "00-apply.yaml", - "01-update.yaml", - "02-import.yaml", - "03-delete.yaml", + templates.ApplyFilename, + templates.UpdateFilename, + templates.ImportFilename, + templates.DeleteFilename, } // NewTester returns a Tester object. @@ -216,7 +216,7 @@ func executeSingleTestFileCLIMode(ctx context.Context, t *Tester, tf string, tim }() var mutex sync.Mutex - go logCollectorCLIMode(done, ticker, &mutex, resources) + go logCollectorCLIMode(ctx, done, ticker, &mutex, resources) sc := bufio.NewScanner(stdout) for sc.Scan() { @@ -268,7 +268,7 @@ func logCollectorLibraryMode(done chan bool, ticker *time.Ticker, mutex sync.Loc } } -func logCollectorCLIMode(done chan bool, ticker *time.Ticker, mutex sync.Locker, resources []config.Resource) { +func logCollectorCLIMode(ctx context.Context, done chan bool, ticker *time.Ticker, mutex sync.Locker, resources []config.Resource) { for { select { case <-done: @@ -285,7 +285,7 @@ func logCollectorCLIMode(done chan bool, ticker *time.Ticker, mutex sync.Locker, if r.Namespace != "" { traceCmdArgs = fmt.Sprintf(`"${CROSSPLANE_CLI}" beta trace %s %s -n %s -o wide 2>>/tmp/uptest_crossplane_temp_errors.log`, r.KindGroup, r.Name, r.Namespace) } - traceCmd := exec.Command("bash", "-c", traceCmdArgs) //nolint:gosec // Disabling gosec to allow dynamic shell command execution + traceCmd := exec.CommandContext(ctx, "bash", "-c", traceCmdArgs) //nolint:gosec // Disabling gosec to allow dynamic shell command execution output, err := traceCmd.CombinedOutput() if err == nil { log.Printf("crossplane trace logs %s\n%s\n", time.Now(), string(output)) @@ -301,6 +301,7 @@ func (t *Tester) prepareConfig() (*config.TestCase, []config.Resource, error) { Timeout: t.options.DefaultTimeout, SetupScriptPath: t.options.SetupScriptPath, TeardownScriptPath: t.options.TeardownScriptPath, + PostAssertScriptPath: t.options.PostAssertScriptPath, OnlyCleanUptestResources: t.options.OnlyCleanUptestResources, TestDirectory: "test-input.yaml", }