feat: adding k8s version metadata to catalog api - #4662
feat: adding k8s version metadata to catalog api#4662IndulekhaPrathapan wants to merge 16 commits into
Conversation
|
|
There was a problem hiding this comment.
Pull request overview
This PR adds Kubernetes version compatibility metadata (min/max) to the gator policy catalog and enforces it during gator policy install / upgrade. Version bounds are sourced from metadata.gatekeeper.sh/{min,max}KubernetesVersion annotations on ConstraintTemplates, with a fallback that derives bounds from the API lifecycle metadata of the built-in resources a policy's sample constraints target. A --force flag bypasses the gate, and incompatible policies are surfaced as skips (not failures) in output and exit codes.
Changes:
- Add
MinKubernetesVersion/MaxKubernetesVersionto the catalogPolicy,SearchResult, and JSON/table output; validate them and render a human-readable range. - Add a cluster version compatibility gate to install/upgrade (resolved via a new
discovery-backedServerVersionclient method, resolved once per batch), with--forceoverride and partial-success signaling. - Add
catalog/lifecycle.goto derive version bounds fromk8s.io/apilifecycle metadata as a fallback, plus refactors (normalizeVersion, lenientparseVersion,findConstraintFile).
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
pkg/gator/policy/catalog/types.go |
Adds min/max k8s version fields to Policy with json/yaml tags |
pkg/gator/policy/catalog/generator.go |
Parses/normalizes/validates version bounds; adds K8sVersionInRange/FormatK8sVersionRange; lenient semver parsing |
pkg/gator/policy/catalog/lifecycle.go |
New: derives version range from built-in API lifecycle metadata |
pkg/gator/policy/catalog/lifecycle_test.go |
New tests for derivation, match-kind parsing, end-to-end fallback |
pkg/gator/policy/catalog/generator_test.go |
Tests for annotations, schema validation, range formatting/checking |
pkg/gator/policy/client/client.go |
Adds ServerVersion + discovery client; dedups constructor logic |
pkg/gator/policy/client/install.go |
Adds compatibility gate, IncompatibleEntry, and anyPolicyHasVersionBounds |
pkg/gator/policy/client/upgrade.go |
Batch version resolution; surfaces incompatible as skip vs failure |
pkg/gator/policy/client/client_test.go |
Fake client ServerVersion + gate/upgrade tests |
pkg/gator/policy/output/output.go |
Adds SkippedEntry and Incompatible result fields |
pkg/gator/policy/output/table.go |
Adds K8S VERSION column and incompatible rendering |
pkg/gator/policy/output/output_test.go |
Tests for new column and incompatible output |
cmd/gator/policy/install.go |
--force flag, wiring, dryRun ServerVersion, partial-success message |
cmd/gator/policy/upgrade.go |
--force flag, wiring, dry-run cluster-access messaging |
cmd/gator/policy/search.go |
Populates new version fields in search results |
go.mod |
Promotes blang/semver/v4 from indirect to direct dependency |
|
@IndulekhaPrathapan can you fix easycla? |
d2fb126 to
741f434
Compare
Fixed now, conflict between two linked emails. |
a4c0fa0 to
94213ec
Compare
1b6150d to
4301036
Compare
699a766 to
c966917
Compare
4c9a813 to
0140044
Compare
Signed-off-by: Indulekha Prathapan <indulekhamp@gmail.com>
0a3f86b to
96b1465
Compare
|
@JaydipGabani Can you PTAL - fixed issues from copilot review |
|
@IndulekhaPrathapan thanks for working on this, this pr is in my review list I will try to get to this pr by EOW. |
Just want to make sure this is in your radar. |
JaydipGabani
left a comment
JaydipGabani
left a comment
There was a problem hiding this comment.
Thanks for working on this pr!
| if upgradeDryRun { | ||
| return fmt.Errorf("creating Kubernetes client: %w (upgrade --dry-run still requires cluster access to read installed policy versions)", err) | ||
| } |
There was a problem hiding this comment.
why is this being added? the flag description already clarifies that cluster access is needed. I dont think we need this.
| var constraintFilenames = []string{"constraint.yaml", "constraint.yml"} | ||
|
|
||
| // findConstraintFile returns the path to the constraint file in dir, or "" if | ||
| // none exists. | ||
| func findConstraintFile(dir string) string { | ||
| for _, name := range constraintFilenames { |
There was a problem hiding this comment.
Since these filenames are fixed and only used by findConstraintFile, could we keep them local instead of introducing a mutable package-level variable?
| var constraintFilenames = []string{"constraint.yaml", "constraint.yml"} | |
| // findConstraintFile returns the path to the constraint file in dir, or "" if | |
| // none exists. | |
| func findConstraintFile(dir string) string { | |
| for _, name := range constraintFilenames { | |
| func findConstraintFile(dir string) string { | |
| for _, name := range [...]string{"constraint.yaml", "constraint.yml"} { |
This keeps the candidate list close to its only use and communicates that it is a small, fixed set.
| func VersionRangeContradicts(minVersion, maxVersion string) bool { | ||
| if minVersion == "" || maxVersion == "" { | ||
| return false | ||
| } | ||
| inRange, err := K8sVersionInRange(minVersion, "", maxVersion) | ||
| if err != nil { | ||
| // An unparseable bound is treated as non-contradictory; catalog schema | ||
| // validation flags bad version strings separately, up front. | ||
| return false | ||
| } | ||
| return !inRange | ||
| } |
There was a problem hiding this comment.
Passing minVersion as the serverVersion argument works, but it makes the contradiction check difficult to understand. Could we extract the maximum-bound comparison into a helper and use it from both VersionRangeContradicts and K8sVersionInRange?
For example:
func exceedsMaxVersion(candidate, max *version.Version) bool {
if len(max.Components()) >= 3 {
return candidate.GreaterThan(max)
}
return candidate.Major() > max.Major() ||
(candidate.Major() == max.Major() && candidate.Minor() > max.Minor())
}Then VersionRangeContradicts can parse both bounds and call exceedsMaxVersion(min, max) directly. This avoids treating the minimum as a synthetic server version while keeping the whole-minor versus exact-patch semantics shared with the runtime compatibility check.
| {"invalid server version", "notaversion", "v1.21.0", "", false, true}, | ||
| {"invalid min", "v1.25.0", "bogus", "", false, true}, | ||
| {"invalid max", "v1.25.0", "", "bogus", false, true}, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| got, err := K8sVersionInRange(tt.serverVersion, tt.minVer, tt.maxVer) | ||
| if tt.wantErr { | ||
| if err == nil { | ||
| t.Fatalf("expected error, got nil") | ||
| } | ||
| return |
There was a problem hiding this comment.
Could these error cases also assert which input failed to parse? Checking only err != nil could let a regression pass if the function returns an error from the wrong validation branch. For example, assert that the errors contain server version, minKubernetesVersion, and maxKubernetesVersion for the corresponding cases.
| func TestK8sClient_ServerVersion_Discovery(t *testing.T) { | ||
| newDiscoveryK8sClient := func(t *testing.T, server *httptest.Server) *K8sClient { | ||
| t.Helper() | ||
| discoveryClient, err := discovery.NewDiscoveryClientForConfig(&rest.Config{Host: server.URL}) | ||
| require.NoError(t, err) | ||
| return &K8sClient{discoveryClient: discoveryClient} | ||
| } | ||
|
|
||
| t.Run("successful gitVersion response", func(t *testing.T) { | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| assert.Equal(t, "/version", r.URL.Path) | ||
| w.Header().Set("Content-Type", "application/json") | ||
| _, _ = w.Write([]byte(`{"gitVersion":"v1.30.2"}`)) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| client := newDiscoveryK8sClient(t, server) | ||
| version, err := client.ServerVersion(context.Background()) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, "v1.30.2", version) | ||
| }) | ||
|
|
||
| t.Run("server error response is a genuine error", func(t *testing.T) { | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.WriteHeader(http.StatusInternalServerError) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| client := newDiscoveryK8sClient(t, server) | ||
| _, err := client.ServerVersion(context.Background()) | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "getting server version") | ||
| }) | ||
|
|
||
| t.Run("malformed response body is a genuine error", func(t *testing.T) { | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| _, _ = w.Write([]byte("not json")) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| client := newDiscoveryK8sClient(t, server) | ||
| _, err := client.ServerVersion(context.Background()) | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "parsing server version") | ||
| }) | ||
|
|
||
| t.Run("request error (connection refused) is a genuine error", func(t *testing.T) { | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) | ||
| addr := server.URL | ||
| server.Close() // nothing is listening at addr anymore | ||
|
|
||
| discoveryClient, err := discovery.NewDiscoveryClientForConfig(&rest.Config{Host: addr}) | ||
| require.NoError(t, err) | ||
| client := &K8sClient{discoveryClient: discoveryClient} | ||
|
|
||
| _, err = client.ServerVersion(context.Background()) | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "getting server version") | ||
| }) | ||
|
|
||
| t.Run("context cancellation aborts a slow request instead of blocking forever", func(t *testing.T) { | ||
| release := make(chan struct{}) | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| <-release | ||
| })) | ||
| defer server.Close() | ||
| defer close(release) | ||
|
|
||
| client := newDiscoveryK8sClient(t, server) | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) | ||
| defer cancel() | ||
|
|
||
| start := time.Now() | ||
| _, err := client.ServerVersion(ctx) | ||
| elapsed := time.Since(start) | ||
|
|
||
| require.Error(t, err) | ||
| assert.Less(t, elapsed, 5*time.Second, "ServerVersion must honor context cancellation via Do(ctx) instead of blocking on the handler") | ||
| }) | ||
| } |
There was a problem hiding this comment.
Could we add a case where /version returns valid JSON without gitVersion, such as {}? The current implementation would decode this successfully and return ("", nil). Since callers rely on a non-empty version for compatibility checks, either ServerVersion should reject a missing/empty gitVersion, or the test should explicitly document that returning an empty version is intentional.
| existing, err := k8sClient.GetTemplate(ctx, policy.Name) | ||
| if err == nil { | ||
| // Template exists - check if managed by gator | ||
| if !labels.IsManagedByGator(existing) { |
There was a problem hiding this comment.
installPolicy checks ownership before compatibility. If an out-of-range policy has an unmanaged same-name ConstraintTemplate, it is recorded as Failed with ConflictError, and fail-fast can prevent later compatible policies from installing. This contradicts the intended behavior that incompatible policies are skipped before write-related failures.
Please defer the unmanaged-template conflict until after the compatibility gate.
dd7bf5e to
c464c17
Compare
Signed-off-by: Indulekha Prathapan <indulekhamp@gmail.com>
054acd4 to
906e526
Compare
|
@JaydipGabani Addressed the comments , do you mind taking another look pls? |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Suppressed comments (1)
docs/design/gator-policy.md:200
- This blanket statement is inaccurate for
gator policy install --dry-run: the command intentionally uses an offline client and skips the compatibility gate, so an out-of-range policy is previewed as installable. Clarify this exception so users do not rely on dry-run as a compatibility check.
`gator policy install` and `gator policy upgrade` skip policies outside this range; pass `--force` to bypass the compatibility check.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Indulekha Prathapan <indulekhamp@gmail.com>
Signed-off-by: Indulekha Prathapan <indulekhamp@gmail.com>
What this PR does / why we need it:
This PR add a minimum and maximum k8s version to policy. It also updates gator install/upgrade to match k8s version before installing policies.
Which issue(s) this PR fixes (optional, using fixes #(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #4383
Special notes for your reviewer:
Add a min and max k8s version to the policy - the versions are derived from annotations in the template
Added a check for k8s version match in gator install and upgrade with a force flag to enable install even if k8s version is outside the range
Added tests