Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance detection logic for patch variants #3102

Merged
merged 4 commits into from
Jul 12, 2024

Conversation

rquitales
Copy link
Member

@rquitales rquitales commented Jul 12, 2024

Proposed Changes

This PR improves our Patch resource detection logic by removing the reliance on a hardcoded list. Hardcoded lists are fragile because new Patch resources introduced in future SDK versions, or those generated dynamically by tools like crd2pulumi, would not be recognized as Patch variants by our provider. This issue is already present with CustomResourcePatch resources, as CustomResource kinds will never be included in a hardcoded list.

The updated logic now compares the resource URN and the object's Kind. Since the token within the URN of a patch variant always follows the pattern kind + "Patch", we can use this for comparison. Previously, the logic simply checked if the URN ended in Patch, which was inaccurate due to the possibility of a Custom Resource (CR) Kind ending in Patch (e.g., "MyResourcePatch"). This would lead to the Pulumi patch variant URN being MyResourcePatchPatch.

Testing

  • Added integration tests for the YAML, Node.js, and Go languages since CustomResources are overlays and not supported in YAML. Existing tests did not ensure that deleted Patch resources only removed the fields applied, and not the whole object. This behavior is now exercised as well.
  • Expanded existing unit tests.

Note: I did not expand the test coverage for Python, Java, and .NET at this time, as porting the test programs for these languages requires additional effort. We may revisit and add tests for these languages in the future.

Related Issues (Optional)

Fixes: #3023

@rquitales rquitales requested review from blampe, EronWright and a team July 12, 2024 19:23
@rquitales rquitales force-pushed the rquitales/fix-patch-distinguish-logic branch from a398a9a to d795fad Compare July 12, 2024 19:26
@@ -155,44 +155,6 @@ func Test_equalNumbers(t *testing.T) {
}
}

func Test_isPatchURN(t *testing.T) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is duplicated in provider/pkg/kinds/util_test.go.


return PatchQualifiedTypes.Has(urnS)
return kind+"Patch" == resourceName
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The crux of the change.

Copy link

Does the PR have any schema changes?

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

Copy link

codecov bot commented Jul 12, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 8 lines in your changes missing coverage. Please review.

Project coverage is 36.60%. Comparing base (1dca605) to head (d795fad).

Files Patch % Lines
provider/pkg/provider/provider.go 0.00% 7 Missing ⚠️
provider/pkg/await/await.go 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3102      +/-   ##
==========================================
+ Coverage   36.57%   36.60%   +0.03%     
==========================================
  Files          71       71              
  Lines        9263     9263              
==========================================
+ Hits         3388     3391       +3     
+ Misses       5536     5534       -2     
+ Partials      339      338       -1     

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

Copy link
Contributor

@blampe blampe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brilliant!

Copy link
Contributor

@EronWright EronWright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand how a non-patch resource of kind MyResourcePatch would work.

@rquitales
Copy link
Member Author

I don't understand how a non-patch resource of kind MyResourcePatch would work.

For a non-patch resource with Kind MyResourcePatch, the urn for the non-patch variant would be something like kubernetes:gv:MyResourcePatch. The urn for the patch variant will be kubernetes:gv:MyResourcePatchPatch. The kind for both patch, and non-patch variant is always MyResourcePatch.

So, we can determine if a resource is the patch variant or not simply by checking that: kind+"Patch == pulumiResourceName.

This PR includes both unit and integration tests to showcase this behavior:
Unit test:

name: "CustomResource with Patch suffix that is not a patch resource",
urn: resource.NewURN("test", "test", "", "kubernetes:kuma.io/v1alpha1:MeshProxyPatch", "test"),
kind: "MeshProxyPatch",
want: false,
},
{
name: "CustomResource with Patch suffix and is a patch resource as well",
urn: resource.NewURN("test", "test", "", "kubernetes:kuma.io/v1alpha1:MeshProxyPatchPatch", "test"),
kind: "MeshProxyPatch",
want: true,
},

Integration test (nodejs example):
const plainCRPatching = new kubernetes.apiextensions.CustomResourcePatch(
"plain-cr-patching",
{
apiVersion: "patchtest.pulumi.com/v1",
kind: "TestPatchResource",
metadata: {
name: plainCR.metadata.name,
namespace: patchRscNamespace.metadata.name,
annotations: {
"pulumi.com/testPatchAnnotation": "patched",
},
},
}
);
const patchCRPatching = new kubernetes.apiextensions.CustomResourcePatch(
"patch-cr-patching",
{
apiVersion: "patchtest.pulumi.com/v1",
kind: "TestPatchResourcePatch",
metadata: {
name: patchCR.metadata.name,
namespace: patchRscNamespace.metadata.name,
annotations: {
"pulumi.com/testPatchAnnotation": "patched",
},
},
}
);

@rquitales rquitales merged commit 616e127 into master Jul 12, 2024
19 checks passed
@rquitales rquitales deleted the rquitales/fix-patch-distinguish-logic branch July 12, 2024 23:18
@rquitales rquitales self-assigned this Jul 12, 2024
@mjeffryes mjeffryes added this to the 0.107 milestone Jul 24, 2024
blampe added a commit that referenced this pull request Aug 7, 2024
### Added

- `clusterIdentifier` configuration can now be used to manually control
the replacement behavior of a provider resource.
(#3068)

- Pod errors now include the pod's last termination state, as well as
the pod's termination message if available.
(#3091)

The pod's termination message can be helpful in `CrashLoopBackOff`
situations but will only be reported if it was correctly configured.

By default, the pod's termination message is read from
`/dev/termination-log`. This location can be configured with
`terminationMessagePath`.

Use `terminationMessagePolicy: FallbackToLogsOnError` to use the pod's
logs as its termination message.

- Documentation is now generated for all languages supported by overlay
types. (#3107)

### Fixed

- Updated logic to accurately detect if a resource is a Patch variant.
(#3102)
- Added Java as a supported language for `CustomResource` overlays.
(#3120)
- Status messages reported during updates are now more accurately scoped
to the
affected resource.
(#3128)
- `PersistentVolumeClaims` with a bind mode of `WaitForFirstConsumer`
will no
longer hang indefinitely.
(#3130)
- [java] Fixed an issue where child resources could not be registered by
Chart v4. (#3119)
lumiere-bot bot referenced this pull request in coolguy1771/home-ops Aug 8, 2024
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@pulumi/kubernetes](https://pulumi.com)
([source](https://togithub.com/pulumi/pulumi-kubernetes)) | dependencies
| minor | [`4.15.0` ->
`4.16.0`](https://renovatebot.com/diffs/npm/@pulumi%2fkubernetes/4.15.0/4.16.0)
|

---

### Release Notes

<details>
<summary>pulumi/pulumi-kubernetes (@&#8203;pulumi/kubernetes)</summary>

###
[`v4.16.0`](https://togithub.com/pulumi/pulumi-kubernetes/blob/HEAD/CHANGELOG.md#4160-August-7-2024)

[Compare
Source](https://togithub.com/pulumi/pulumi-kubernetes/compare/v4.15.0...v4.16.0)

##### Added

- `clusterIdentifier` configuration can now be used to manually control
the
replacement behavior of a provider
resourc[https://github.com/pulumi/pulumi-kubernetes/pull/3068](https://togithub.com/pulumi/pulumi-kubernetes/pull/3068)ull/3068)

- Pod errors now include the pod's last termination state, as well as
the pod's
termination message if
availabl[https://github.com/pulumi/pulumi-kubernetes/pull/3091](https://togithub.com/pulumi/pulumi-kubernetes/pull/3091)ull/3091)

The pod's termination message can be helpful in `CrashLoopBackOff`
situations but
    will only be reported if it was correctly configured.

    By default, the pod's termination message is read from
    `/dev/termination-log`. This location can be configured with
    `terminationMessagePath`.

Use `terminationMessagePolicy: FallbackToLogsOnError` to use the pod's
logs
    as its termination message.

- Documentation is now generated for all languages supported by overlay
types.

[https://github.com/pulumi/pulumi-kubernetes/pull/3107](https://togithub.com/pulumi/pulumi-kubernetes/pull/3107)3107)

##### Fixed

- Updated logic to accurately detect if a resource is a Patch variant.
([https://github.com/pulumi/pulumi-kubernetes/pull/3102](https://togithub.com/pulumi/pulumi-kubernetes/pull/3102))
- Added Java as a supported language for `CustomResource` overlays.
([https://github.com/pulumi/pulumi-kubernetes/pull/3120](https://togithub.com/pulumi/pulumi-kubernetes/pull/3120))
- Status messages reported during updates are now more accurately scoped
to the
affected
resourc[https://github.com/pulumi/pulumi-kubernetes/pull/3128](https://togithub.com/pulumi/pulumi-kubernetes/pull/3128)3128)
- `PersistentVolumeClaims` with a bind mode of `WaitForFirstConsumer`
will no
longer hang
indefinitel[https://github.com/pulumi/pulumi-kubernetes/pull/3130](https://togithub.com/pulumi/pulumi-kubernetes/pull/3130)3130)
- \[java] Fixed an issue where child resources could not be registered
by Chart v4.
[https://github.com/pulumi/pulumi-kubernetes/pull/3119](https://togithub.com/pulumi/pulumi-kubernetes/pull/3119)9)

</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.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://togithub.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC4yMS4zIiwidXBkYXRlZEluVmVyIjoiMzguMjEuMyIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsidHlwZS9taW5vciJdfQ==-->

Co-authored-by: lumiere-bot[bot] <98047013+lumiere-bot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CustomResourcePatch resource doesn't behave like a normal Patch resource
4 participants