refactor(kube): replace manual GVR pluralization with REST mapper

[pbrissaud]

Summary

• GetResourceGVR used naive manual pluralization (switch + pluralize() helper), requiring a hardcoded list of special cases and producing wrong results for edge cases like Kind: Endpoints → endpointss
• Replaced by meta.RESTMapper.RESTMapping(): GVR and scope are now resolved directly from the cluster's API discovery, with zero special cases in code
• IsNamespaced hardcoded list replaced by mapping.Scope.Name() == meta.RESTScopeNameNamespace
• Third-party CRDs (Kyverno ClusterPolicy, etc.) now resolve automatically without any code changes

Changes

• internal/kube/manifest.go: ApplyManifest now accepts meta.RESTMapper instead of *kubernetes.Clientset; uses mapper.RESTMapping() for GVR + scope; IsNamespaced removed
• internal/kube/client.go: Removed pluralize, vowels, GetResourceGVR (~50 lines)
• internal/deployer/infrastructure.go: Builds mapper via restmapper.GetAPIGroupResources(clientset.Discovery()) once before applying manifests
• internal/deployer/challenge.go: Same
• internal/deployer/local.go: Same
• internal/kube/manifest_test.go: Replaced TestIsNamespaced and TestApplyManifest_NilClientset with testrestmapper-based tests; added TestApplyManifest_RESTMapperGVR and TestApplyManifest_RESTMapperScope
• internal/kube/client_test.go: Removed TestPluralize and TestGetResourceGVR

Test plan

• TestApplyManifest_RESTMapperGVR — verifies Deployment, ClusterRole, ServiceAccount resolve to correct GVRs via mapper
• TestApplyManifest_RESTMapperScope — verifies ClusterRole (cluster-scoped) gets no namespace injected; ServiceAccount (namespaced) does
• TestApplyManifest_ErrorHandling/skips_unknown_kinds_gracefully — unknown kinds (e.g. Kyverno CRDs not in scheme) are logged and skipped without error
• All existing TestApplyManifest_* tests pass
• task lint — 0 issues
• task build — clean build

🤖 Generated with Claude Code

[claude]

Claude finished @pbrissaud's task in 1m 51s —— View job

---

PR Review: fix(kube): correct pluralization for kinds ending in -y

• Gather context and read changed files
• Analyze the implementation
• Check test coverage
• Post review feedback

---

Summary

The fix correctly addresses the root cause: GetResourceGVR was using naive kind + "s" pluralization, producing policys for Policy/ClusterPolicy, which caused the dynamic client to 404 and silently skip Kyverno policies during challenge deployment. The pluralize() helper and updated default are the right approach.

Overall verdict: approve with minor notes.

---

Findings

1. Potential over-broad y → ies rule (low risk, inline comment left)

pluralize() applies y → ies to all kinds ending in y, including vowel+y endings (e.g., a CRD named Relay would become relaies instead of relays). No standard Kubernetes resource is affected, but 3rd-party CRDs with vowel+y names could break.

The proper fix is to only apply the rule when the preceding character is a consonant — see the inline suggestion on client.go:259–264.

2. Redundant switch cases (cosmetic)

networkpolicy and podsecuritypolicy both end in -y, so pluralize() now handles them correctly by default. Their entries in the switch statement (lines 288–307 pre-PR) are now dead code — both paths produce the same result. Removing them would keep GetResourceGVR easier to reason about, but it's not a correctness issue.

The switch should ideally only contain truly irregular plurals: ingresses, endpoints, and the identity cases that already pass through + "s" (apps resources, RBAC resources).

3. Test coverage — good

The two new test cases (Kyverno Policy and Kyverno ClusterPolicy) directly cover the bug scenario. Existing 12 cases continue to pass. A direct unit test for pluralize() itself (covering consonant-y, vowel-y, and non-y cases) would further future-proof the helper, but is not strictly necessary given the integration-style coverage in TestGetResourceGVR.

4. No silent failure mitigation

The original bug caused ApplyManifest to silently skip resources on 404. This PR corrects the pluralization, but the silent-skip behaviour in ApplyManifest (or wherever the 404 is swallowed) is still present. Consider whether a future PR should surface 404s during manifest apply as warnings/errors so that mis-pluralized CRDs from other operators don't go unnoticed.

---

Positive notes

• Minimal, focused change with no unrelated modifications
• Commit message follows Conventional Commits ✓
• pluralize is unexported (correct scope)
• PR title and description are clear and accurate

[claude]

Potential over-broad pluralization rule

The current rule applies y → ies to all kinds ending in y, including those ending in a vowel+y (e.g., Relay → relaies, Key → kies, Monkey → monkies). For standard Kubernetes resources this is fine — none end in vowel+y — but 3rd-party CRDs could be affected.

The proper English rule is: only replace y with ies when the preceding letter is a consonant.

Suggested change

	}
	func pluralize(kind string) string {
	if strings.HasSuffix(kind, "y") && len(kind) >= 2 {
	prev := kind[len(kind)-2]
	if prev != 'a' && prev != 'e' && prev != 'i' && prev != 'o' && prev != 'u' {
	return kind[:len(kind)-1] + "ies"
	}
	}
	return kind + "s"
	}

This is a low-risk edge case for this project's current scope, but worth considering for robustness.

[sentry]

Codecov Report

❌ Patch coverage is 26.08696% with 17 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/deployer/challenge.go	0.00%	6 Missing ⚠️
internal/deployer/infrastructure.go	0.00%	6 Missing ⚠️
internal/deployer/local.go	0.00%	5 Missing ⚠️

📢 Thoughts on this report? Let us know!

[claude]

Claude finished @pbrissaud's task in 1m 22s —— View job

---

PR Review (updated): fix(kube): correct pluralization for kinds ending in -y

• Gather context
• Read changed files and diff
• Analyze implementation
• Post final review

---

All three concerns from the previous review have been addressed in the latest commit (29aa43b). The PR is in great shape.

What changed in the latest commit

1. ✅ Consonant-y restriction added (previously flagged as over-broad)

The pluralize() helper now correctly restricts the y → ies transformation to consonant-preceding letters using a vowels map:

var vowels = map[byte]bool{'a': true, 'e': true, 'i': true, 'o': true, 'u': true}

func pluralize(kind string) string {
    if strings.HasSuffix(kind, "y") && len(kind) >= 2 && !vowels[kind[len(kind)-2]] {
        return kind[:len(kind)-1] + "ies"
    }
    return kind + "s"
}

relay → relays ✓, policy → policies ✓, key → keys ✓. The implementation correctly follows the English rule.

2. ✅ Redundant switch cases removed

networkpolicy and podsecuritypolicy entries have been removed from the switch — pluralize() now handles both correctly by default since the preceding character (c in both cases) is a consonant. The switch now only contains genuinely irregular plurals (ingresses, customresourcedefinitions) and identity cases.

3. ✅ Unit tests for pluralize() added

TestPluralize directly covers all four cases: consonant-y, vowel-y, and non-y inputs. This was listed as a nice-to-have in the first review and has been included.

---

Minor observations (non-blocking)

• vowels is a package-level var (unexported, correct scope). Using a map is idiomatic Go since map[byte]bool cannot be a const. No issue.
• The customresourcedefinition switch case (client.go:290–293) is now technically redundant — pluralize("customresourcedefinition") already returns "customresourcedefinitions". The group guard preserves it as explicit documentation of expected usage. Harmless, and pre-existing.
• The "endpoints" entry in the case "endpoints", "configmap", ..." line would produce "endpointss" for a GVK with Kind: "Endpoints", but this is pre-existing behaviour and out of scope for this fix.

---

Summary

The fix is correct, minimal, and well-tested. All previous feedback incorporated. Ready to merge.

[claude]

Claude finished @pbrissaud's task in 1m 54s —— View job

---

PR Review (updated): fix(kube): correct pluralization for kinds ending in -y

• Gather context
• Read changed files and diff
• Analyze implementation
• Post final review

---

Summary

All feedback from the initial review has been addressed in the two follow-up commits. The PR is in excellent shape. Here's the full analysis of the current state.

---

What changed since the last review

Commit 29aa43b — fix(kube): restrict y→ies rule to consonant-y, remove redundant switch cases

• Addresses both findings from the initial review in one commit
• pluralize() now correctly guards against vowel+y endings using a package-level vowels map
• Removed the networkpolicy and podsecuritypolicy switch cases that became dead code

Commit 8c49a42 — style(kube): fix gofmt alignment in TestPluralize

• Style-only fix for test alignment

---

Current implementation (client.go:259–266)

var vowels = map[byte]bool{'a': true, 'e': true, 'i': true, 'o': true, 'u': true}

func pluralize(kind string) string {
    if strings.HasSuffix(kind, "y") && len(kind) >= 2 && !vowels[kind[len(kind)-2]] {
        return kind[:len(kind)-1] + "ies"
    }
    return kind + "s"
}

Correctness: The consonant-y guard is correct. policy → policies, relay → relays, deployment → deployments all work as expected. The len(kind) >= 2 guard prevents an out-of-bounds access for single-character inputs.

Minor stylistic note (non-blocking): The package-level vowels map is a reasonable approach. An inline check (prev != 'a' && prev != 'e' && prev != 'i' && prev != 'o' && prev != 'u') as originally suggested would also work and avoids an extra package-level symbol. Either style is acceptable.

---

Switch statement (client.go:278–302)

The networkpolicy and podsecuritypolicy dead cases were correctly removed. The remaining cases serve a purpose:

• ingress → ingresses (genuine irregular)
• customresourcedefinition → customresourcedefinitions (identity/explicit)
• endpoint → endpoints (irregular, singular→irregular plural)

Observation (non-blocking, pre-existing): The deployment/daemonset/statefulset/replicaset and configmap/secret/service/namespace cases with group guards still produce the same output that pluralize() would generate anyway. They're effectively dead code too, but they pre-date this PR and are out of scope here.

---

Test coverage (client_test.go)

TestPluralize (new, lines 366–383): Direct unit test covering all three branches:

• policy → policies (consonant-y)
• clusterpolicy → clusterpolicies (consonant-y)
• relay → relays (vowel-y — the key edge case)
• deployment → deployments (non-y)
• secret → secrets (non-y)

This directly addresses the initial review's note about testing pluralize() in isolation.

TestGetResourceGVR: 14 cases total, including NetworkPolicy → networkpolicies, PodSecurityPolicy → podsecuritypolicies, and the two new bug-scenario cases (Kyverno Policy, Kyverno ClusterPolicy).

---

Remaining open item (out of scope for this PR)

The silent-fail behaviour in ApplyManifest (or wherever 404s from the dynamic client are swallowed) is still present. A mis-pluralised CRD from a third-party operator would go unnoticed. This was flagged in the initial review and remains a candidate for a follow-up.

---

Verdict

All initial review feedback has been addressed. The fix is correct, well-tested, and minimal. Ready to merge.

[claude]

Claude finished @pbrissaud's task in 2m 30s —— View job

---

PR Review: refactor(kube): replace manual GVR pluralization with REST mapper

• Gather context
• Read changed files
• Analyze implementation
• Post review feedback

---

This is a solid architectural improvement. Replacing the manual pluralize()/GetResourceGVR()/IsNamespaced stack with the cluster's own REST mapper is the correct long-term approach — zero special cases, automatic CRD support, and correctness guaranteed by the API server itself. Previous reviews covered the y→ies fix; this final commit takes the right next step.

What the refactor does

• ApplyManifest now accepts meta.RESTMapper (interface) instead of *kubernetes.Clientset — cleaner dependency, easier to test
• GVR and scope resolved via mapper.RESTMapping(gvk.GroupKind(), gvk.Version) — no more switch tables or pluralization heuristics
• Each deployer (challenge.go, infrastructure.go, local.go) builds a fresh mapper via restmapper.GetAPIGroupResources before applying manifests
• Tests use testrestmapper.TestOnlyStaticRESTMapper — correct use of the testing-only helper

---

Findings

1. Stale mapper within SetupInfrastructure (low risk, worth documenting)

The mapper is built once at line 49-53 of infrastructure.go and reused for both the Kyverno and local-path-provisioner manifests. This is fine for those specific manifests (all standard types). However, a future reader might not realize that the mapper is a point-in-time snapshot: any CRD types registered by the Kyverno install manifest itself won't appear in this mapper.

In practice this is harmless (Kyverno's install.yaml only applies CRDs as CustomResourceDefinition objects — standard type — and doesn't embed ClusterPolicy instances), but the constraint isn't obvious from the code. A one-line comment would prevent confusion:

// Mapper is a one-time snapshot; CRDs registered by these manifests won't be resolvable
// in the same call. That's acceptable here since no manifest applies its own CRD types.
groups, err := restmapper.GetAPIGroupResources(clientset.Discovery())

Fix this →

2. .yaml vs .yml inconsistency between deployers (pre-existing, non-blocking)

challenge.go:62 only walks .yaml files, while local.go:43 walks both .yaml and .yml. This is pre-existing behaviour, not introduced here, but since both files were touched in this PR it's a good opportunity to align them.

3. Silent-fail behaviour still present (pre-existing, carry-forward from previous review)

ApplyManifest always returns nil (manifest.go:145). Resources that fail due to API errors or unknown types are logged and skipped without surfacing to the caller. This was flagged in an earlier review cycle as a candidate for a follow-up. This PR doesn't make it worse, but the mapper skip path (manifest.go:72-74) now adds another category of silent skip. The comment on line 145 acknowledges this is intentional; it's worth tracking as a follow-up.

4. createdOrUpdated nil check is always true on success (cosmetic)

At manifest.go:139:

if createdOrUpdated != nil {
    logger.Info(...)
}

resourceClient.Create only returns a non-nil object on success, so this check is always true at that point. The guard is harmless but redundant — pre-existing, not introduced here.

---

Positive notes

• Removing GetResourceGVR, pluralize, vowels, and IsNamespaced (~50 lines + 188 lines of tests) is a net reduction in surface area and a correctness win
• ApplyManifest's signature now takes an interface (meta.RESTMapper) rather than a concrete *kubernetes.Clientset — better for testability and dependency direction
• testrestmapper.TestOnlyStaticRESTMapper is the idiomatic way to test mapper-dependent code; using it correctly here
• The skips_unknown_kinds_gracefully test directly covers the Kyverno CRD scenario that motivated this refactor
• All three deployers build fresh mappers per-invocation — correct for a CLI tool that shouldn't cache stale discovery data
• Commit messages follow Conventional Commits ✓

---

Verdict

Ready to merge. The implementation is architecturally sound and the two minor pre-existing issues are out of scope for this PR. Finding #1 is a low-priority documentation suggestion. No blocking issues.

[claude]

Claude finished @pbrissaud's task in 1m 33s —— View job

---

PR Review: refactor(kube): replace manual GVR pluralization with REST mapper

• Gather context
• Read changed files
• Analyze implementation
• Post final review

---

This PR is in excellent shape. All findings from previous review cycles have been incorporated, including the latest commit (e3dfc07) which addressed the two remaining documentation and consistency issues.

What the latest commit (e3dfc07) fixed

1. ✅ Stale mapper constraint documented (infrastructure.go:48–53)

The comment now clearly explains why building the mapper once before applying Kyverno and local-path-provisioner manifests is safe:

// Build REST mapper from API discovery (used for all ApplyManifest calls).
// This is a point-in-time snapshot: CRD types registered by the manifests applied
// below won't be resolvable within this call. That is acceptable here because neither
// the Kyverno nor the local-path-provisioner install manifest applies instances of
// their own CRD types — they only create the CRDs themselves (CustomResourceDefinition,
// a standard type always present in discovery).

2. ✅ .yaml/.yml inconsistency resolved (challenge.go:62)

challenge.go now walks both .yaml and .yml, matching local.go. The previous version only walked .yaml.

---

Full implementation review

manifest.go

The refactored ApplyManifest signature (meta.RESTMapper instead of *kubernetes.Clientset) is architecturally correct — the function only needs discovery/mapping, not a full client. GVR and namespace-scoping are now derived from mapper.RESTMapping(), which delegates to the API server's own discovery data. Zero special cases in code.

Minor pre-existing observations (not introduced by this PR, not blocking):

• manifest.go:101: The IsNotFound + strings.Contains(err.Error(), "the server could not find the requested resource") double-check is redundant. apierrors.IsNotFound already handles this case. Harmless.
• manifest.go:139: if createdOrUpdated != nil is always true on the success path. Harmless guard.
• manifest.go:145: return nil regardless of per-document errors is intentional (comment says so), but means callers can't detect partial failures. This is the pre-existing silent-fail behaviour, tracked as a future follow-up.

deployer/{challenge,infrastructure,local}.go

Each deployer builds a fresh mapper per invocation via restmapper.GetAPIGroupResources — correct for a CLI that should not cache stale discovery data. Error from GetAPIGroupResources is properly propagated.

manifest_test.go

Test coverage is comprehensive:

• TestApplyManifest_RESTMapperGVR: Verifies Deployment, ClusterRole, ServiceAccount resolve to correct GVRs via mapper — directly covers the original bug scenario
• TestApplyManifest_RESTMapperScope: Verifies cluster-scoped resources (ClusterRole) receive no namespace injection; namespaced resources (ServiceAccount) do
• TestApplyManifest_ErrorHandling/skips_unknown_kinds_gracefully: Verifies Kyverno CRDs not in scheme are logged and skipped without error
• All existing TestApplyManifest_* tests intact

One minor test nit (non-blocking): In TestApplyManifest_RESTMapperGVR:385, the cluster-scope check branches on tt.gvr.Group == "rbac.authorization.k8s.io" rather than a generic scope check. Fine for current cases, but would silently misclassify a future test entry with a cluster-scoped type in another group. A minor improvement would be to check whether namespace is empty on the retrieved object. Not worth blocking the PR.

---

Summary

The refactor correctly eliminates all manual GVR pluralization heuristics in favour of the cluster's REST mapper. The implementation is minimal, correct, and well-tested. All previous review feedback has been incorporated across the 5 commits.

Ready to merge.