diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 864fc603..403af343 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -48,6 +48,9 @@ jobs: - name: Lint run: make lint + - name: Vulnerability check + run: make vulncheck + build-bink: uses: $/.github/workflows/build-bink.yaml with: diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 00000000..0286eabe --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,19 @@ +version: "2" + +linters: + default: standard + enable: + - gosec + settings: + staticcheck: + # golangci-lint disables some stylistic checks by default; enable + # the full staticcheck suite. + checks: + - all + exclusions: + rules: + # The e2e suite drives external tools (kubectl, helm, bink) with + # computed arguments by design. + - path: ^test/e2e/ + linters: + - gosec diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6e4ee6a8..779e1365 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -34,9 +34,9 @@ For end-to-end testing you will also need: backed by bootc nodes - [skopeo](https://github.com/containers/skopeo) — container image inspection -Tool dependencies like `controller-gen`, `kustomize`, `golangci-lint`, and -`setup-envtest` are downloaded automatically to `./bin/` by the Makefile when -needed. +Tool dependencies like `controller-gen`, `kustomize`, `golangci-lint`, +`govulncheck`, and `setup-envtest` are downloaded automatically to `./bin/` by +the Makefile when needed. ### Development Environment @@ -106,6 +106,26 @@ make unit V=1 # Verbose output make unit RUN=Foo # Run tests matching "Foo" ``` +### Checking for Vulnerabilities + +```shell +make vulncheck +``` + +This runs [govulncheck](https://go.dev/blog/vuln) against all Go packages in +the repository, looking for known vulnerabilities from the +[Go vulnerability database](https://vuln.go.dev/). Both the `govulncheck` +binary and the database are downloaded on demand, so the target requires +network access. + +Only reachable findings — vulnerabilities in functions that the code actually +calls — fail the check. Fix them by upgrading the affected dependency or +changing the affected code. The scan may also report informational findings +in packages you import or modules you require whose vulnerable functions are +not called; these do not fail the check. + +CI runs this check in the `unit` job. + ### Running End-to-End Tests E2E tests live in `test/e2e/` and run against a real @@ -196,7 +216,10 @@ matching the structure described in [Finding Test Logs](#finding-test-logs). - Run `make fmt` and `make vet` before submitting. - Run `make lint` to check with [golangci-lint](https://golangci-lint.run/). Use `make lint-fix` to apply - automatic fixes. + automatic fixes. The configuration in `.golangci.yml` enables the full + staticcheck suite and gosec (except for the e2e tests). If a gosec finding + is a false positive, suppress it with `//nolint:gosec // ` and a + comment explaining why. - All Go files must include the SPDX license header: ```go @@ -247,6 +270,7 @@ Follow a commit style similar to the Linux kernel: make vet make lint make unit + make vulncheck ``` 4. Push your branch and open a pull request against `main`. diff --git a/Makefile b/Makefile index 88fe837a..046edd27 100644 --- a/Makefile +++ b/Makefile @@ -98,6 +98,10 @@ go-lines: get-golines go-lines-fix: get-golines "$(GOLINES_BIN)" -w . +.PHONY: vulncheck +vulncheck: govulncheck ## Check Go code for known vulnerabilities (requires network access to the Go vulnerability database). + "$(GOVULNCHECK)" ./... + .PHONY: e2e e2e: ## Run e2e tests (requires: make deploy-bink). V=1 for verbose. RUN= to filter. # NB: we `cd` here instead of passing a package path to `go test` so that `-v` @@ -288,11 +292,13 @@ CONTROLLER_GEN ?= $(LOCALBIN)/controller-gen ENVTEST ?= $(LOCALBIN)/setup-envtest GOLANGCI_LINT = $(LOCALBIN)/golangci-lint GOLINES_BIN = $(LOCALBIN)/golines +GOVULNCHECK = $(LOCALBIN)/govulncheck YQ ?= $(LOCALBIN)/yq KUSTOMIZE_VERSION ?= v5.8.1 CONTROLLER_TOOLS_VERSION ?= v0.20.1 GOLANGCI_LINT_VERSION ?= v2.11.4 +GOVULNCHECK_VERSION ?= v1.9.0 YQ_VERSION ?= v4.53.2 # ENVTEST_VERSION is the version of controller-runtime release branch to fetch the envtest setup script (i.e. release-0.20) @@ -332,6 +338,11 @@ golangci-lint: $(GOLANGCI_LINT) ## Download golangci-lint locally if necessary. $(GOLANGCI_LINT): $(LOCALBIN) $(call go-install-tool,$(GOLANGCI_LINT),github.com/golangci/golangci-lint/v2/cmd/golangci-lint,$(GOLANGCI_LINT_VERSION)) +.PHONY: govulncheck +govulncheck: $(GOVULNCHECK) ## Download govulncheck locally if necessary. +$(GOVULNCHECK): $(LOCALBIN) + $(call go-install-tool,$(GOVULNCHECK),golang.org/x/vuln/cmd/govulncheck,$(GOVULNCHECK_VERSION)) + .PHONY: get-golines get-golines: $(GOLINES_BIN) ## Download golines locally if necessary. $(GOLINES_BIN): $(LOCALBIN) diff --git a/api/v1alpha1/bootcnodepool_types.go b/api/v1alpha1/bootcnodepool_types.go index 2277cbbc..559c691a 100644 --- a/api/v1alpha1/bootcnodepool_types.go +++ b/api/v1alpha1/bootcnodepool_types.go @@ -200,19 +200,23 @@ type BootcNodePoolStatus struct { // nodeCount is the total number of nodes in this pool. // +optional + // +kubebuilder:validation:Minimum=0 NodeCount int32 `json:"nodeCount,omitempty"` // updatedCount is the number of nodes running the target digest. // +optional + // +kubebuilder:validation:Minimum=0 UpdatedCount int32 `json:"updatedCount,omitempty"` // updatingCount is the number of nodes actively being updated // (staging, staged, or rebooting). // +optional + // +kubebuilder:validation:Minimum=0 UpdatingCount int32 `json:"updatingCount,omitempty"` // degradedCount is the number of nodes in a degraded state. // +optional + // +kubebuilder:validation:Minimum=0 DegradedCount int32 `json:"degradedCount,omitempty"` // conditions represent the current state of the pool. diff --git a/config/crd/bases/node.bootc.dev_bootcnodepools.yaml b/config/crd/bases/node.bootc.dev_bootcnodepools.yaml index db056742..2332791a 100644 --- a/config/crd/bases/node.bootc.dev_bootcnodepools.yaml +++ b/config/crd/bases/node.bootc.dev_bootcnodepools.yaml @@ -263,6 +263,7 @@ spec: degradedCount: description: degradedCount is the number of nodes in a degraded state. format: int32 + minimum: 0 type: integer deployedDigest: description: |- @@ -278,6 +279,7 @@ spec: nodeCount: description: nodeCount is the total number of nodes in this pool. format: int32 + minimum: 0 type: integer observedGeneration: description: |- @@ -300,12 +302,14 @@ spec: description: updatedCount is the number of nodes running the target digest. format: int32 + minimum: 0 type: integer updatingCount: description: |- updatingCount is the number of nodes actively being updated (staging, staged, or rebooting). format: int32 + minimum: 0 type: integer type: object required: diff --git a/go.mod b/go.mod index 7b8502a0..6f5f7fcd 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/bootc-dev/bootc-operator -go 1.26.0 +go 1.26.10 require ( github.com/aws/aws-sdk-go-v2 v1.47.3 @@ -95,7 +95,7 @@ require ( go.uber.org/zap v1.27.1 // indirect go.yaml.in/yaml/v2 v2.4.4 // indirect go.yaml.in/yaml/v3 v3.0.5 // indirect - golang.org/x/net v0.59.0 // indirect + golang.org/x/net v0.60.0 // indirect golang.org/x/oauth2 v0.37.0 // indirect golang.org/x/sync v0.23.0 // indirect golang.org/x/sys v0.48.0 // indirect diff --git a/go.sum b/go.sum index 82079f68..e52bc2c9 100644 --- a/go.sum +++ b/go.sum @@ -211,6 +211,8 @@ golang.org/x/mod v0.41.0 h1:qJmnOUb4YB+FsEuM3HcWucdZASCPGhsX6uljO6pog0c= golang.org/x/mod v0.41.0/go.mod h1:Ek9pY8RKWXwsWvd3rQiHYtMqkjSUV+s1Rj7j4H5Ur6o= golang.org/x/net v0.59.0 h1:5zfYln+w5XCxwrnMMJPufRgNoXEaGxl0wo5GqPXyues= golang.org/x/net v0.59.0/go.mod h1:2DA/G1UfVbCpQPeWTmMPGY7Cs2PkBkwu743bVX5PIVg= +golang.org/x/net v0.60.0 h1:79p50tfZlm0J9YfoDsSi639qSXNGVwEzOPLCxM2FsYU= +golang.org/x/net v0.60.0/go.mod h1:2DA/G1UfVbCpQPeWTmMPGY7Cs2PkBkwu743bVX5PIVg= golang.org/x/oauth2 v0.37.0 h1:JUlcxA8oAtauLfiH8FX2/FkAWHAdi0QtGCGc+hofE98= golang.org/x/oauth2 v0.37.0/go.mod h1:IxwZNxUULJmpBFf9K/9NTMSIfZZuvuTy1gGxhigP/58= golang.org/x/sync v0.23.0 h1:KameEIfc1IkluZyXWLn39Wd4tURc6GbCiISGiZm2bQk= diff --git a/internal/bootc/executor.go b/internal/bootc/executor.go index 6d7a0a7c..eaaa9bf7 100644 --- a/internal/bootc/executor.go +++ b/internal/bootc/executor.go @@ -1,5 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 +// Package bootc runs bootc commands on the host and parses their output. package bootc import ( @@ -60,7 +61,10 @@ func (e *HostExecutor) nsenterCmd(ctx context.Context, args ...string) *exec.Cmd "--setuid", "0", "--setgid", "0", "--env", "--", } - return exec.CommandContext(ctx, "nsenter", append(base, args...)...) + // Callers in this package pass fixed program names; the only external + // input is a validated, digested image reference. Arguments are not + // passed through a shell. + return exec.CommandContext(ctx, "nsenter", append(base, args...)...) //nolint:gosec // G204 } func (e *HostExecutor) Status(ctx context.Context) ([]byte, error) { diff --git a/internal/controller/bootcnodepool_controller.go b/internal/controller/bootcnodepool_controller.go index 7d334476..6f9dee32 100644 --- a/internal/controller/bootcnodepool_controller.go +++ b/internal/controller/bootcnodepool_controller.go @@ -1,5 +1,7 @@ // SPDX-License-Identifier: Apache-2.0 +// Package controller implements the BootcNodePool controller, which +// resolves a pool's target image and rolls it out across member nodes. package controller import ( diff --git a/internal/controller/rollout.go b/internal/controller/rollout.go index f1c3d069..212cb1cc 100644 --- a/internal/controller/rollout.go +++ b/internal/controller/rollout.go @@ -307,7 +307,8 @@ func (r *BootcNodePoolReconciler) ensureDrain( log.Info("Starting drain", "node", bn.Name) - drainCtx, cancel := context.WithCancel(ctx) + // cancel is stored in r.drains and called when the drain is collected. + drainCtx, cancel := context.WithCancel(ctx) //nolint:gosec // G118 status := &drainStatus{ result: make(chan error, 1), ctx: drainCtx, @@ -371,11 +372,14 @@ func (r *BootcNodePoolReconciler) collectDrainResults( continue } - // Drain finished; remove from the map. + // Drain finished; remove from the map and release its context, + // which would otherwise stay registered with the parent. delete(r.drains, nodeName) + cancelled := ds.ctx.Err() != nil + ds.cancel() if drainErr != nil { - if ds.ctx.Err() != nil { + if cancelled { // Context was cancelled (node left pool or controller // shutdown). The canceller is responsible for cleanup. log.V(1).Info("Drain cancelled", "node", nodeName) diff --git a/internal/controller/rollout_test.go b/internal/controller/rollout_test.go index 18ffd244..b35b039f 100644 --- a/internal/controller/rollout_test.go +++ b/internal/controller/rollout_test.go @@ -3,6 +3,8 @@ package controller import ( + "context" + "errors" "testing" "time" @@ -522,3 +524,23 @@ func TestFindUnhealthySlots(t *testing.T) { g.Expect(findUnhealthySlots(rs, targetDigest)).To(BeEmpty()) }) } + +// Verifies that collecting a finished drain cancels its context, so it +// does not stay registered with the long-lived parent context. +func TestCollectDrainResultsReleasesContext(t *testing.T) { + g := NewWithT(t) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + drainCtx, drainCancel := context.WithCancel(ctx) + defer drainCancel() + ds := &drainStatus{result: make(chan error, 1), ctx: drainCtx, cancel: drainCancel} + ds.result <- errors.New("drain failed") + + r := &BootcNodePoolReconciler{drains: map[string]*drainStatus{"n1": ds}} + pool := testutil.NewPool("drain-release", testImageDigestRefA, testutil.WithWorkerSelector()) + + g.Expect(r.collectDrainResults(ctx, pool, nil)).To(Succeed()) + g.Expect(r.drains).To(BeEmpty()) + g.Expect(drainCtx.Err()).To(MatchError(context.Canceled)) +} diff --git a/internal/controller/status.go b/internal/controller/status.go index 83fc2fee..266d0ac4 100644 --- a/internal/controller/status.go +++ b/internal/controller/status.go @@ -4,6 +4,7 @@ package controller import ( "fmt" + "math" "strings" apimeta "k8s.io/apimachinery/pkg/api/meta" @@ -19,12 +20,12 @@ func syncPoolStatus(pool *bootcv1alpha1.BootcNodePool, rs *rolloutState) { return } pool.Status.ObservedGeneration = pool.Generation - pool.Status.NodeCount = int32(rs.nodeCount()) - pool.Status.UpdatedCount = int32(len(rs.upToDate)) - pool.Status.UpdatingCount = int32( + pool.Status.NodeCount = clampCount(rs.nodeCount()) + pool.Status.UpdatedCount = clampCount(len(rs.upToDate)) + pool.Status.UpdatingCount = clampCount( len(rs.pending) + len(rs.staging) + len(rs.staged) + len(rs.rebooting), ) - pool.Status.DegradedCount = int32(len(rs.degraded)) + pool.Status.DegradedCount = clampCount(len(rs.degraded)) if pool.Status.NodeCount == pool.Status.UpdatedCount { pool.Status.DeployedDigest = pool.Status.TargetDigest @@ -34,6 +35,18 @@ func syncPoolStatus(pool *bootcv1alpha1.BootcNodePool, rs *rolloutState) { syncUpToDateCondition(pool, rs) } +// clampCount converts a count to int32 for the status API, saturating to +// [0, math.MaxInt32] to match the schema instead of wrapping. +func clampCount(n int) int32 { + if n < 0 { + return 0 + } + if n > math.MaxInt32 { + return math.MaxInt32 + } + return int32(n) +} + func syncUpToDateCondition(pool *bootcv1alpha1.BootcNodePool, rs *rolloutState) { if pool == nil || rs == nil { return diff --git a/internal/controller/status_test.go b/internal/controller/status_test.go index 7568b51b..20dcc0df 100644 --- a/internal/controller/status_test.go +++ b/internal/controller/status_test.go @@ -3,6 +3,7 @@ package controller import ( + "math" "testing" . "github.com/onsi/gomega" @@ -170,3 +171,15 @@ func TestSyncPoolStatusDeployedDigestPreserved(t *testing.T) { g.Expect(pool.Status.DeployedDigest).To(Equal(testDigestA)) g.Expect(pool.Status.UpdateAvailable).To(BeTrue()) } + +// Verifies that clampCount passes in-range counts through and saturates +// instead of wrapping when a count does not fit in int32. +func TestClampCount(t *testing.T) { + g := NewWithT(t) + + g.Expect(clampCount(0)).To(Equal(int32(0))) + g.Expect(clampCount(42)).To(Equal(int32(42))) + g.Expect(clampCount(math.MaxInt32)).To(Equal(int32(math.MaxInt32))) + g.Expect(clampCount(math.MaxInt32 + 1)).To(Equal(int32(math.MaxInt32))) + g.Expect(clampCount(-1)).To(Equal(int32(0))) +} diff --git a/internal/daemon/reconciler.go b/internal/daemon/reconciler.go index 6bf422b3..7a873336 100644 --- a/internal/daemon/reconciler.go +++ b/internal/daemon/reconciler.go @@ -1,5 +1,7 @@ // SPDX-License-Identifier: Apache-2.0 +// Package daemon implements the per-node daemon that watches bootc +// status and reconciles the node's BootcNode. package daemon import ( @@ -234,7 +236,17 @@ func (r *BootcNodeReconciler) reconcileBootcNode( switchCtx, cancel := context.WithCancel(context.Background()) r.inflight.acquire(log, desiredImage, cancel) log.Info("Starting staging", "image", desiredImage) - go r.inflight.run(switchCtx, r.NodeName, desiredImage, r.Executor, r.stageDone) + // Staging must outlive this reconcile, so it is not derived from + // the request context. The context has no parent, so it cannot + // leak; acquire cancels it when a newer stage replaces it. + //nolint:gosec // G118 + go r.inflight.run( + switchCtx, + r.NodeName, + desiredImage, + r.Executor, + r.stageDone, + ) case actionAwaitStage: reason = bootcv1alpha1.NodeReasonStaging @@ -438,7 +450,9 @@ func (r *BootcNodeReconciler) writeAuthFile(data []byte) error { } dir := filepath.Join(hostRoot, "run", "ostree") - if err := os.MkdirAll(dir, 0755); err != nil { + // Standard mode for a host system directory under /run; it only + // applies if the host has not created the directory already. + if err := os.MkdirAll(dir, 0755); err != nil { //nolint:gosec // G301 return fmt.Errorf("creating auth dir: %w", err) } diff --git a/internal/daemon/watcher_test.go b/internal/daemon/watcher_test.go index ccf59a6c..9fe13f57 100644 --- a/internal/daemon/watcher_test.go +++ b/internal/daemon/watcher_test.go @@ -78,12 +78,12 @@ func TestWatcherEvents(t *testing.T) { composefsPath := filepath.Join(dir, "deploy") if tt.mkOstree { - if err := os.Mkdir(ostreePath, 0o755); err != nil { + if err := os.Mkdir(ostreePath, 0o750); err != nil { t.Fatal(err) } } if tt.mkComposefs { - if err := os.Mkdir(composefsPath, 0o755); err != nil { + if err := os.Mkdir(composefsPath, 0o750); err != nil { t.Fatal(err) } } @@ -100,7 +100,7 @@ func TestWatcherEvents(t *testing.T) { t.Fatal(err) } case triggerCreate: - if err := os.Mkdir(filepath.Join(composefsPath, "new-deploy"), 0o755); err != nil { + if err := os.Mkdir(filepath.Join(composefsPath, "new-deploy"), 0o750); err != nil { t.Fatal(err) } } @@ -204,7 +204,7 @@ func TestWatcherGetStatusColdCache(t *testing.T) { func TestWatcherShutdown(t *testing.T) { dir := t.TempDir() watchDir := filepath.Join(dir, "bootc") - if err := os.Mkdir(watchDir, 0o755); err != nil { + if err := os.Mkdir(watchDir, 0o750); err != nil { t.Fatal(err) } @@ -226,7 +226,7 @@ func TestWatcherShutdown(t *testing.T) { func TestWatcherNoPollWhenFsnotifyHealthy(t *testing.T) { dir := t.TempDir() ostreePath := filepath.Join(dir, "bootc") - if err := os.Mkdir(ostreePath, 0o755); err != nil { + if err := os.Mkdir(ostreePath, 0o750); err != nil { t.Fatal(err) } diff --git a/internal/image/digest.go b/internal/image/digest.go index 82a9c545..f46b5fd4 100644 --- a/internal/image/digest.go +++ b/internal/image/digest.go @@ -1,5 +1,7 @@ // SPDX-License-Identifier: Apache-2.0 +// Package image provides helpers for matching container image +// references against digests. package image import ( diff --git a/internal/registry/resolver.go b/internal/registry/resolver.go index b9ee71e6..0c8631ee 100644 --- a/internal/registry/resolver.go +++ b/internal/registry/resolver.go @@ -1,3 +1,4 @@ +// Package registry resolves container image tags to digests. package registry import ( diff --git a/internal/version/version.go b/internal/version/version.go index 07251d1a..de51aa6f 100644 --- a/internal/version/version.go +++ b/internal/version/version.go @@ -1,5 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 +// Package version holds build metadata set via linker flags. package version var (