diff --git a/Makefile b/Makefile index 90c15eea02b2..8b7101b0f835 100644 --- a/Makefile +++ b/Makefile @@ -19,6 +19,7 @@ STATICCHECK := $(abspath $(TOOLS_BIN_DIR)/staticcheck) GENAPIDOCS := $(abspath $(TOOLS_BIN_DIR)/gen-crd-api-reference-docs) MOCKGEN := $(abspath $(TOOLS_BIN_DIR)/mockgen) YQ := $(abspath $(TOOLS_BIN_DIR)/yq) +VERIFY_API_DEPS := $(abspath $(TOOLS_BIN_DIR)/verify-api-deps) CODESPELL_VER := 2.4.1 CODESPELL_BIN := codespell @@ -143,7 +144,7 @@ verify-codecov: ## Validate codecov.yml against Codecov's API. | tee /dev/stderr | grep -q "^Valid!" .PHONY: verify-parallel -verify-parallel: verify-codespell verify-codecov lint cpo-container-sync run-gitlint verify-docs-nav +verify-parallel: verify-codespell verify-codecov verify-api-deps lint cpo-container-sync run-gitlint verify-docs-nav .PHONY: verify verify: generate update staticcheck fmt vet @@ -168,6 +169,9 @@ $(GENAPIDOCS): $(TOOLS_DIR)/go.mod $(MOCKGEN): ${TOOLS_DIR}/go.mod cd $(TOOLS_DIR); $(GO) build -tags=tools -o $(BIN_DIR)/mockgen go.uber.org/mock/mockgen +$(VERIFY_API_DEPS): $(TOOLS_DIR)/go.mod # Build verify-api-deps tool + cd $(TOOLS_DIR); $(GO) build -o $(BIN_DIR)/verify-api-deps ./verify-api-deps + .PHONY: generate generate: $(MOCKGEN) @@ -582,6 +586,10 @@ verify-docs-nav: $(PYYAML_STAMP) ## Verify docs nav entries are sorted alphabeti verify-codespell: codespell ## Verify codespell. @$(CODESPELL) --count --ignore-words=./.codespellignore --skip="./hack/tools/bin/codespell_dist,./docs/site/*,./vendor/*,./api/vendor/*,./hack/tools/vendor/*,./api/hypershift/v1alpha1/*,./support/thirdparty/*,./docs/content/reference/*,./hack/tools/bin/*,./cmd/install/assets/*,./go.sum,./hack/workspace/go.work.sum,./api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests,./hack/tools/go.mod,./hack/tools/go.sum,./karpenter-operator/controllers/karpenter/assets/*.yaml,./dev/*" +.PHONY: verify-api-deps +verify-api-deps: $(VERIFY_API_DEPS) ## Verify API dependencies against allowlist. + @$(VERIFY_API_DEPS) + .PHONY: run-gitlint run-gitlint: $(GITLINT) ifdef PULL_BASE_SHA diff --git a/api/.imports_allowed b/api/.imports_allowed new file mode 100644 index 000000000000..711bbcccc5b1 --- /dev/null +++ b/api/.imports_allowed @@ -0,0 +1,15 @@ +# Allowed direct dependencies for the HyperShift API module +# +# This file defines the restricted list of dependencies that the API module +# is permitted to directly import. Any changes to this file MUST be reviewed +# and approved by API reviewers. +# +# Format: one module path per line, comments start with # + +# Core Kubernetes API dependencies +k8s.io/api +k8s.io/apimachinery +k8s.io/utils + +# OpenShift API dependencies +github.com/openshift/api diff --git a/api/AGENTS.md b/api/AGENTS.md index 555e0cc08dff..9dc4ee7f33e5 100644 --- a/api/AGENTS.md +++ b/api/AGENTS.md @@ -20,11 +20,19 @@ Key make targets for API work: ```bash make api # Regenerate all CRDs, deepcopy, clients make api-lint-fix # Run API linter and auto-fix violations +make verify-api-deps # Verify API dependencies make verify # Full verification (includes api, fmt, vet, lint) make update # Full update (api-deps, workspace-sync, deps, api, api-docs, clients) ENVTEST_OCP_K8S_VERSIONS=1.35.0 make test-envtest-ocp # Run envtest for CEL validations ``` +### API Dependencies + +It is imperative that the imported dependencies are kept minimal. Use `make verify-api-deps` to verify that the dependencies are allowed. +New dependencies must be approved by API reviewers and added to `api/.imports_allowed`. + +To avoid introducing new dependencies, do not add utils or methods to the API types. + ### Serialization - **Always set `omitempty` or `omitzero` on every field, regardless of whether it is `+required` or `+optional`.** `omitempty`/`omitzero` tags control serialization, not validation. `+required` is a schema constraint enforced at admission time; the serialization tag controls what goes on the wire. Without a tag, a zero-value field serializes as an explicit value (e.g., `"pullSecret": {"name": ""}`), which makes the API server unable to distinguish "not set" from "explicitly set to empty." This breaks defaulting, server-side apply field ownership, and strategic merge patch — all of which rely on field absence to mean "don't touch this." Additionally, without omission a structured client serializes the empty object, which passes the `+required` check (based on key presence) without validating the value — so a user can forget to set a required field, it passes admission, and the reader sees a required field with an unexpected empty value. diff --git a/hack/tools/go.mod b/hack/tools/go.mod index 84649f539800..5f648382b5fb 100644 --- a/hack/tools/go.mod +++ b/hack/tools/go.mod @@ -10,8 +10,10 @@ require ( github.com/mikefarah/yq/v4 v4.44.5 github.com/openshift/api/tools v0.0.0-20250915151906-94481d71bb6f go.uber.org/mock v0.6.0 + golang.org/x/mod v0.34.0 gotest.tools/gotestsum v1.13.0 honnef.co/go/tools v0.7.0 + k8s.io/apimachinery v0.34.3 k8s.io/code-generator v0.34.2 k8s.io/utils v0.0.0-20260108192941-914a6e750570 sigs.k8s.io/controller-runtime/tools/setup-envtest v0.0.0-20260125163108-a19ec76a3c5d @@ -287,7 +289,6 @@ require ( golang.org/x/crypto v0.49.0 // indirect golang.org/x/exp v0.0.0-20250620022241-b7579e27df2b // indirect golang.org/x/exp/typeparams v0.0.0-20260209203927-2842357ff358 // indirect - golang.org/x/mod v0.34.0 // indirect golang.org/x/net v0.52.0 // indirect golang.org/x/oauth2 v0.35.0 // indirect golang.org/x/sync v0.20.0 // indirect @@ -307,7 +308,6 @@ require ( gopkg.in/yaml.v3 v3.0.1 // indirect k8s.io/api v0.34.3 // indirect k8s.io/apiextensions-apiserver v0.34.3 // indirect - k8s.io/apimachinery v0.34.3 // indirect k8s.io/apiserver v0.34.3 // indirect k8s.io/client-go v0.34.3 // indirect k8s.io/component-base v0.34.3 // indirect diff --git a/hack/tools/verify-api-deps/README.md b/hack/tools/verify-api-deps/README.md new file mode 100644 index 000000000000..4718b3e54c5f --- /dev/null +++ b/hack/tools/verify-api-deps/README.md @@ -0,0 +1,83 @@ +# API Dependencies Verification Tool + +This tool enforces strict dependency restrictions on the HyperShift API module (`api/`) to maintain API stability, compatibility, and a minimal dependency footprint. + +## Purpose + +The HyperShift API module is a separate Go module with its own `go.mod` file. It should only have these **direct** dependencies: + +- Core Kubernetes APIs (`k8s.io/api`, `k8s.io/apimachinery`, `k8s.io/utils`) +- OpenShift API definitions (`github.com/openshift/api`) + +## Allowlist Configuration + +The allowed dependencies are defined in `api/.imports_allowed` - a simple text file with one module path per line. This file: + +- **Lives in the API module** alongside the `go.mod` file +- **Falls under API reviewer control** via the OWNERS file +- **Requires API reviewer approval** for any changes +- **Supports comments** (lines starting with `#`) +- **One dependency per line** format + +## How It Works + +1. **Finds** the repository root by walking up directories to locate the `.git` directory +2. **Locates** the API module at `/api` +3. **Loads** the allowed dependencies from `api/.imports_allowed` file +4. **Reads** the `api/go.mod` file +5. **Parses** the required dependencies (ignoring indirect dependencies) +6. **Validates** each dependency against the allowlist +7. **Fails** with a detailed error message if unauthorized dependencies are found + +## Usage + +```bash +# Run as part of verification +make verify + +# Run standalone +make verify-api-deps + +# Build and run directly (works from any directory within the repo) +cd hack/tools/verify-api-deps +go run main.go +``` + +## Adding New Dependencies + +If you need to add a new dependency to the API module: + +1. **Consult API reviewers first** - discuss alternatives and necessity +2. **Ensure the dependency is essential** for API type definitions +3. **Verify compatibility** and that it doesn't introduce breaking changes +4. **After approval**, add the module path to `api/.imports_allowed` +5. **Update this documentation** if the reasoning changes + +## Error Messages + +When the tool detects unauthorized dependencies, it provides: + +- ❌ Clear list of violating dependencies +- 📋 Instructions for the review process +- 📁 Location to update the allowlist after approval +- 👥 Guidance to contact API reviewers + +## Integration + +This tool runs automatically as part of: + +- `make verify` (full verification suite) +- `make verify-parallel` (parallel verification tasks) +- Pre-commit hooks +- CI/CD pipelines + +## Rationale + +The strict direct dependency restrictions for the API module ensure: + +- **Stability**: Minimal direct dependencies mean fewer potential breaking changes +- **Compatibility**: Reduced version conflict risks with consumer projects +- **Performance**: Faster builds and smaller dependency trees +- **Security**: Smaller attack surface with fewer third-party dependencies +- **Maintainability**: Clear separation between API definitions and implementations +- **Simplicity**: Only essential APIs are directly imported, everything else is transitive diff --git a/hack/tools/verify-api-deps/main.go b/hack/tools/verify-api-deps/main.go new file mode 100644 index 000000000000..8ae84024981f --- /dev/null +++ b/hack/tools/verify-api-deps/main.go @@ -0,0 +1,158 @@ +package main + +import ( + "bufio" + "fmt" + "os" + "path/filepath" + "strings" + + "golang.org/x/mod/modfile" + "k8s.io/apimachinery/pkg/util/sets" +) + + +func main() { + if err := verifyAPIDependencies(); err != nil { + fmt.Fprintf(os.Stderr, "Error: %v\n", err) + os.Exit(1) + } + fmt.Println("✅ API dependencies verification passed") +} + +func verifyAPIDependencies() error { + // Find the repository root and locate the API module + repoRoot, err := findRepoRoot() + if err != nil { + return fmt.Errorf("failed to find repository root: %w", err) + } + + apiModPath := filepath.Join(repoRoot, "api") + + // Load allowed dependencies from the .imports_allowed file + allowedAPIModules, err := loadAllowedImports(apiModPath) + if err != nil { + return fmt.Errorf("failed to load allowed imports: %w", err) + } + + // Read the go.mod file + goModPath := filepath.Join(apiModPath, "go.mod") + data, err := os.ReadFile(goModPath) + if err != nil { + return fmt.Errorf("failed to read %s: %w", goModPath, err) + } + + // Parse the go.mod file + modFile, err := modfile.Parse(goModPath, data, nil) + if err != nil { + return fmt.Errorf("failed to parse %s: %w", goModPath, err) + } + + // Check required dependencies + var violations []string + for _, req := range modFile.Require { + if req.Indirect { + // Skip indirect dependencies as they're managed transitively + continue + } + + modulePath := req.Mod.Path + if !allowedAPIModules.Has(modulePath) { + violations = append(violations, modulePath) + } + } + + if len(violations) > 0 { + return fmt.Errorf(`❌ Unauthorized API dependencies detected: + +%s + +The HyperShift API module has strict dependency restrictions to maintain: +- API stability and compatibility +- Minimal dependency footprint +- Clear separation between API and implementation + +Before adding any new dependencies to the API module, you must: + +1. Consult with API reviewers to discuss alternatives +2. Ensure the dependency is absolutely necessary for the API layer +3. Verify it doesn't introduce breaking changes or version conflicts +4. Update the allowlist in api/.imports_allowed after approval + +If this dependency is approved by API reviewers, add it to the allowlist in: +api/.imports_allowed + +For questions, reach out to the HyperShift API review team.`, + formatViolations(violations)) + } + + return nil +} + +func formatViolations(violations []string) string { + var formatted []string + for _, v := range violations { + formatted = append(formatted, fmt.Sprintf(" • %s", v)) + } + return strings.Join(formatted, "\n") +} + +func loadAllowedImports(apiModPath string) (sets.Set[string], error) { + allowedImportsPath := filepath.Join(apiModPath, ".imports_allowed") + + file, err := os.Open(allowedImportsPath) + if err != nil { + return nil, fmt.Errorf("failed to open %s: %w", allowedImportsPath, err) + } + defer file.Close() + + allowedModules := sets.New[string]() + scanner := bufio.NewScanner(file) + + for scanner.Scan() { + line := strings.TrimSpace(scanner.Text()) + + // Skip empty lines and comments + if line == "" || strings.HasPrefix(line, "#") { + continue + } + + allowedModules.Insert(line) + } + + if err := scanner.Err(); err != nil { + return nil, fmt.Errorf("failed to read %s: %w", allowedImportsPath, err) + } + + return allowedModules, nil +} + +func findRepoRoot() (string, error) { + // Start from current working directory and walk up to find .git directory + cwd, err := os.Getwd() + if err != nil { + return "", fmt.Errorf("failed to get working directory: %w", err) + } + + dir := cwd + for { + // Check if .git directory exists + if fileExists(filepath.Join(dir, ".git")) { + return dir, nil + } + + // Check if we've reached the root + parent := filepath.Dir(dir) + if parent == dir { + break + } + dir = parent + } + + return "", fmt.Errorf("could not find repository root (no .git directory found)") +} + +func fileExists(path string) bool { + _, err := os.Stat(path) + return err == nil +}