Bugfix for ambiguous kinds #2889

EronWright · 2024-03-16T00:27:50Z

Proposed changes

This PR fixes a couple of related problems with "ambiguous kinds":

For kinds with clashing names (e.g. Role), be sure to check the apiversion before using built-in information or dynamic discovery.
For kinds with casing problems, don't mask the problem; show the API server error.

Note that kubectl has the following behavior w.r.t (2):

apiVersion: awx.ansible.com/v1beta1
kind: awx
metadata:
  name: my-awx
  namespace: awx

❯ kubectl apply -f manifest.yaml --server-side=false
The awx "my-awx" is invalid: kind: Invalid value: "awx": must be AWX
❯ kubectl apply -f manifest.yaml --server-side=true
Error from server (BadRequest): invalid object type: awx.ansible.com/v1beta1, Kind=awx

An explanation of the technical approach: the kinds.Kind type is used in the codebase to represent a well-known kind, i.e. known at code generation time. To prepare this PR, I audited the locations where Kind is used, and ensured that it wasn't being used for arbitrary kinds. Where necessary, the use of Kind was conditioned on the apiVersion being one of the well-known values.

Related issues (optional)

Closes #2865
Closes #2143

github-actions · 2024-03-16T00:33:04Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

codecov · 2024-03-16T00:36:04Z

Codecov Report

Attention: Patch coverage is 41.37931% with 17 lines in your changes are missing coverage. Please review.

Project coverage is 26.91%. Comparing base (76bf618) to head (ac13430).
Report is 1 commits behind head on master.

Files	Patch %	Lines
provider/pkg/provider/provider.go	0.00%	9 Missing ⚠️
provider/pkg/provider/invoke_decode_yaml.go	0.00%	4 Missing ⚠️
provider/pkg/clients/clients.go	70.00%	2 Missing and 1 partial ⚠️
provider/pkg/kinds/deprecated.go	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2889      +/-   ##
==========================================
+ Coverage   26.86%   26.91%   +0.04%     
==========================================
  Files          53       53              
  Lines        7724     7736      +12     
==========================================
+ Hits         2075     2082       +7     
- Misses       5476     5481       +5     
  Partials      173      173

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

EronWright · 2024-03-16T00:55:04Z

provider/pkg/clients/clients.go

-	namespaced, err := IsNamespacedKind(gvk, dcs)
-	if err != nil {
-		return nil, err
-	}
-
-	if namespaced {


Rationale: the mapper returns the requisite scope information, the call to IsNamespaceKind is redundant. It is also problematic because the mapper is case-insensitive whereas kinds.go is not.

EronWright · 2024-03-16T00:59:08Z

provider/pkg/kinds/deprecated.go

-	switch k {
-	case PodSecurityPolicy, PodSecurityPolicyList:
-		return &v125
-	}
-


rationale: this code wasn't considering the GroupVersion, and was actually returning the wrong result for extensions/v1beta1:PodSecurityPolicy (which was removed in v1.16).

EronWright · 2024-03-16T01:00:18Z

provider/pkg/clients/clients.go

-func IsNamespacedKind(gvk schema.GroupVersionKind, clientSet *DynamicClientSet) (bool, error) {
+func IsNamespacedKind(gvk schema.GroupVersionKind, disco discovery.DiscoveryInterface) (bool, error) {


Rationale: changing the function signature to make it more easily testable (using the fake disco client).

EronWright · 2024-03-16T18:32:50Z

provider/pkg/clients/clients.go

-	if known, namespaced := kinds.Kind(kind).Namespaced(); known {
-		return namespaced, nil
+	if kinds.KnownGroupVersions.Has(gvk.GroupVersion().String()) {
+		kind := strings.TrimSuffix(gvk.Kind, "Patch") // Check using the underlying kind for Patch resources


I believe it unnecessary to trim the Patch suffix (because the kind of the patch resource is Pod not PodPatch), but is also harmless because it is applied only to the well-known kinds. cc @lblackstone

Makes sense. Looking at the context of the change again, I think that may have been a holdover from a previous implementation. It's probably not needed, but also doesn't hurt.

lblackstone

Looks good to me. I'll defer to @rquitales for approval since it's been a long time since I've thought about this code.

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@pulumi/kubernetes](https://pulumi.com) ([source](https://github.com/pulumi/pulumi-kubernetes)) | dependencies | minor | [`4.9.1` -> `4.10.0`](https://renovatebot.com/diffs/npm/@pulumi%2fkubernetes/4.9.1/4.10.0) | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>pulumi/pulumi-kubernetes (@pulumi/kubernetes)</summary> ### [`v4.10.0`](https://github.com/pulumi/pulumi-kubernetes/blob/HEAD/CHANGELOG.md#4100-April-11-2024) [Compare Source](https://github.com/pulumi/pulumi-kubernetes/compare/v4.9.1...v4.10.0) - ConfigGroup V2 ([https://github.com/pulumi/pulumi-kubernetes/pull/2844](https://github.com/pulumi/pulumi-kubernetes/pull/2844)) - ConfigFile V2 ([https://github.com/pulumi/pulumi-kubernetes/pull/2862](https://github.com/pulumi/pulumi-kubernetes/pull/2862)) - Bugfix for ambiguous kinds ([https://github.com/pulumi/pulumi-kubernetes/pull/2889](https://github.com/pulumi/pulumi-kubernetes/pull/2889)) - \[yaml/v2] Support for resource ordering [https://github.com/pulumi/pulumi-kubernetes/pull/2894](https://github.com/pulumi/pulumi-kubernetes/pull/2894)4) - Bugfix for deployment await logic not referencing the correct deployment status ([https://github.com/pulumi/pulumi-kubernetes/pull/2943](https://github.com/pulumi/pulumi-kubernetes/pull/2943)) ##### New Features A new MLC-based implementation of `ConfigGroup` and of `ConfigFile` is now available in the "yaml/v2" package. These resources are usable in all Pulumi languages, including Pulumi YAML and in the Java Pulumi SDK. Note that transformations aren't supported in this release (see [https://github.com/pulumi/pulumi/issues/12996](https://github.com/pulumi/pulumi/issues/12996)). </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Co-authored-by: lumiere-bot[bot] <98047013+lumiere-bot[bot]@users.noreply.github.com>

EronWright added 2 commits March 15, 2024 16:26

Use Kind for built-ins only

66c9ada

simplify dynamic client (IsNamespacedKind)

ec081a9

EronWright requested a review from rquitales March 16, 2024 00:27

changelog

c648515

EronWright commented Mar 16, 2024

View reviewed changes

lblackstone reviewed Mar 18, 2024

View reviewed changes

mjeffryes assigned EronWright Mar 18, 2024

rquitales approved these changes Mar 19, 2024

View reviewed changes

Merge branch 'master' into eronwright/issue-2865

ac13430

EronWright merged commit 1d7e3b6 into master Mar 19, 2024
17 of 18 checks passed

EronWright deleted the eronwright/issue-2865 branch March 19, 2024 21:26

EronWright mentioned this pull request May 23, 2024

Unable to create or patch a longhorn Node CustomResource. #2730

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix for ambiguous kinds #2889

Bugfix for ambiguous kinds #2889

EronWright commented Mar 16, 2024 •

edited

Loading

github-actions bot commented Mar 16, 2024

codecov bot commented Mar 16, 2024 •

edited

Loading

EronWright Mar 16, 2024 •

edited

Loading

EronWright Mar 16, 2024 •

edited

Loading

EronWright Mar 16, 2024

EronWright Mar 16, 2024 •

edited

Loading

lblackstone Mar 18, 2024

lblackstone left a comment

		func IsNamespacedKind(gvk schema.GroupVersionKind, clientSet *DynamicClientSet) (bool, error) {
		func IsNamespacedKind(gvk schema.GroupVersionKind, disco discovery.DiscoveryInterface) (bool, error) {

Bugfix for ambiguous kinds #2889

Bugfix for ambiguous kinds #2889

Conversation

EronWright commented Mar 16, 2024 • edited Loading

Proposed changes

Related issues (optional)

github-actions bot commented Mar 16, 2024

Does the PR have any schema changes?

codecov bot commented Mar 16, 2024 • edited Loading

Codecov Report

EronWright Mar 16, 2024 • edited Loading

Choose a reason for hiding this comment

EronWright Mar 16, 2024 • edited Loading

Choose a reason for hiding this comment

EronWright Mar 16, 2024

Choose a reason for hiding this comment

EronWright Mar 16, 2024 • edited Loading

Choose a reason for hiding this comment

lblackstone Mar 18, 2024

Choose a reason for hiding this comment

lblackstone left a comment

Choose a reason for hiding this comment

EronWright commented Mar 16, 2024 •

edited

Loading

codecov bot commented Mar 16, 2024 •

edited

Loading

EronWright Mar 16, 2024 •

edited

Loading

EronWright Mar 16, 2024 •

edited

Loading

EronWright Mar 16, 2024 •

edited

Loading