Skip to content

Commit

Permalink
Fix resource kind lookup using URN.Type instead of QualifiedType (#2719)
Browse files Browse the repository at this point in the history
This pull request uses the URN`Type` method, rather than `QualifiedType`
to search if resources are a patch or list resource. Qualified types
include the parent resource if there is one, so it would have failed the
lookup.

Additionally, comprehensive unit tests have been included to validate
this change.

Fixes: #2718 (manual validation was done to ensure this fix solves this
CUJ)
  • Loading branch information
rquitales committed Dec 15, 2023
1 parent 6c8f031 commit 27c5dd4
Show file tree
Hide file tree
Showing 6 changed files with 102 additions and 20 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
## Unreleased

## 4.5.6 (December 15, 2023)
- Fix: Refine URN lookup by using its core type for more accurate resource identification (https://github.com/pulumi/pulumi-kubernetes/issues/2719)

## 4.5.5 (November 28, 2023)
- Fix: Make the invoke calls for Helm charts and YAML config resilient to the value being None or an empty dict (https://github.com/pulumi/pulumi-kubernetes/pull/2665)

Expand Down
4 changes: 2 additions & 2 deletions provider/pkg/await/await.go
Original file line number Diff line number Diff line change
Expand Up @@ -618,7 +618,7 @@ func makeInterfaceSlice[T any](inputs []T) []interface{} {
// fixCSAFieldManagers patches the field managers for an existing resource that was managed using client-side apply.
// The new server-side apply field manager takes ownership of all these fields to avoid conflicts.
func fixCSAFieldManagers(c *UpdateConfig, liveOldObj *unstructured.Unstructured, client dynamic.ResourceInterface) (*unstructured.Unstructured, error) {
if kinds.PatchQualifiedTypes.Has(c.URN.QualifiedType().String()) {
if kinds.IsPatchURN(c.URN) {
// When dealing with a patch resource, there's no need to patch the field managers.
// Doing so would inadvertently make us responsible for managing fields that are not relevant to us during updates,
// which occurs when reusing a patch resource. Patch resources do not need to worry about other fields
Expand Down Expand Up @@ -711,7 +711,7 @@ func Deletion(c DeleteConfig) error {
return nilIfGVKDeleted(err)
}

patchResource := kinds.PatchQualifiedTypes.Has(c.URN.QualifiedType().String())
patchResource := kinds.IsPatchURN(c.URN)
if c.ServerSideApply && patchResource {
err = ssa.Relinquish(c.Context, client, c.Inputs, c.FieldManager)
return err
Expand Down
19 changes: 19 additions & 0 deletions provider/pkg/kinds/util.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package kinds

import (
"github.com/pulumi/pulumi/sdk/v3/go/common/resource"
)

// IsPatchURN returns true if the URN is for a Patch resource.
func IsPatchURN(urn resource.URN) bool {
urnS := urn.Type().String()

return PatchQualifiedTypes.Has(urnS)
}

// IsListURN returns true if the URN is for a List resource.
func IsListURN(urn resource.URN) bool {
urnS := urn.Type().String()

return ListQualifiedTypes.Has(urnS)
}
69 changes: 69 additions & 0 deletions provider/pkg/kinds/util_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
package kinds

import (
"testing"

"github.com/pulumi/pulumi/sdk/v3/go/common/resource"
)

func TestIsPatchURN(t *testing.T) {
tests := []struct {
name string
urn resource.URN
want bool
}{
{
"Simple Patch resource - Happy Path",
resource.URN("urn:pulumi:dev::kubernetes-ts::kubernetes:apps/v1:DaemonSetPatch::k8s-daemonset-patch"),
true,
},
{
"Patch resource within a Component Resource",
resource.URN("urn:pulumi:dev::kubernetes-ts::my:component:Resource$kubernetes:apps/v1:DaemonSetPatch::k8s-daemonset-patch-child"),
true,
},
{
"Non-Patch DaemonSet resource",
resource.URN("urn:pulumi:dev::kubernetes-ts::kubernetes:apps/v1:DaemonSet::k8s-daemonset"),
false,
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
if got := IsPatchURN(tc.urn); got != tc.want {
t.Errorf("IsPatchURN() = %v, want %v", got, tc.want)
}
})
}
}

func TestIsListURN(t *testing.T) {
tests := []struct {
name string
urn resource.URN
want bool
}{
{
"Simple List resource - Happy Path",
resource.URN("urn:pulumi:dev::kubernetes-ts::kubernetes:apps/v1:DaemonSetList::k8s-daemonset-list"),
true,
},
{
"List resource within a Component Resource",
resource.URN("urn:pulumi:dev::kubernetes-ts::my:component:Resource$kubernetes:apps/v1:DaemonSetList::k8s-daemonset-list-child"),
true,
},
{
"Non-List DaemonSet resource",
resource.URN("urn:pulumi:dev::kubernetes-ts::kubernetes:apps/v1:DaemonSet::k8s-daemonset"),
false,
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
if got := IsListURN(tc.urn); got != tc.want {
t.Errorf("IsListURN() = %v, want %v", got, tc.want)
}
})
}
}
22 changes: 6 additions & 16 deletions provider/pkg/provider/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -1295,14 +1295,14 @@ func (k *kubeProvider) Check(ctx context.Context, req *pulumirpc.CheckRequest) (
return k.helmReleaseProvider.Check(ctx, req)
}

if isListURN(urn) {
if kinds.IsListURN(urn) {
// TODO: It might be possible to automatically expand List resources into a list of the underlying resources.
// Until then, return a descriptive error message. https://github.com/pulumi/pulumi-kubernetes/issues/2494
return nil, fmt.Errorf("list resources exist for compatibility with YAML manifests and Helm charts, " +
"and cannot be created directly. Use the underlying resource type instead")
}

if !k.serverSideApplyMode && isPatchURN(urn) {
if !k.serverSideApplyMode && kinds.IsPatchURN(urn) {
return nil, fmt.Errorf("patch resources require Server-side Apply mode, which is enabled using the " +
"`enableServerSideApply` Provider config")
}
Expand Down Expand Up @@ -1341,7 +1341,7 @@ func (k *kubeProvider) Check(ctx context.Context, req *pulumirpc.CheckRequest) (
return nil, err
}

if k.serverSideApplyMode && isPatchURN(urn) {
if k.serverSideApplyMode && kinds.IsPatchURN(urn) {
if len(newInputs.GetName()) == 0 {
return nil, fmt.Errorf("patch resources require the resource `.metadata.name` to be set")
}
Expand Down Expand Up @@ -1635,7 +1635,7 @@ func (k *kubeProvider) Diff(ctx context.Context, req *pulumirpc.DiffRequest) (*p
if len(patchObj) != 0 {
// Changing the identity of the resource always causes a replacement.
forceNewFields := []string{".metadata.name", ".metadata.namespace"}
if !isPatchURN(urn) { // Patch resources can be updated in place for all other properties.
if !kinds.IsPatchURN(urn) { // Patch resources can be updated in place for all other properties.
forceNewFields = k.forceNewProperties(newInputs)
}
if detailedDiff, err = convertPatchToDiff(patchObj, patchBase, newInputs.Object, oldLivePruned.Object, forceNewFields...); err != nil {
Expand Down Expand Up @@ -1781,7 +1781,7 @@ func (k *kubeProvider) Create(
// 2: The resource GVK does not exist
// 3: The resource is a Patch resource
// 4: We are in client-side-apply mode
skipPreview := hasComputedValue(newInputs) || !k.gvkExists(newInputs) || isPatchURN(urn) || !k.serverSideApplyMode
skipPreview := hasComputedValue(newInputs) || !k.gvkExists(newInputs) || kinds.IsPatchURN(urn) || !k.serverSideApplyMode
// If this is a preview and the input meets one of the skip criteria, then return them as-is. This is compatible
// with prior behavior implemented by the Pulumi engine.
if req.GetPreview() && skipPreview {
Expand Down Expand Up @@ -2495,7 +2495,7 @@ func (k *kubeProvider) Delete(ctx context.Context, req *pulumirpc.DeleteRequest)
// to consider the CR to be deleted as well in this case.
return &pbempty.Empty{}, nil
}
if isPatchURN(urn) && await.IsDeleteRequiredFieldErr(awaitErr) {
if kinds.IsPatchURN(urn) && await.IsDeleteRequiredFieldErr(awaitErr) {
if cause, ok := apierrors.StatusCause(awaitErr, metav1.CauseTypeFieldValueRequired); ok {
awaitErr = fmt.Errorf(
"this Patch resource is currently managing a required field, so it can't be deleted "+
Expand Down Expand Up @@ -2530,16 +2530,6 @@ func (k *kubeProvider) Delete(ctx context.Context, req *pulumirpc.DeleteRequest)
return &pbempty.Empty{}, nil
}

// isPatchURN returns true if the URN is for a Patch resource.
func isPatchURN(urn resource.URN) bool {
return kinds.PatchQualifiedTypes.Has(urn.QualifiedType().String())
}

// isListURN returns true if the URN is for a List resource.
func isListURN(urn resource.URN) bool {
return kinds.ListQualifiedTypes.Has(urn.QualifiedType().String())
}

// GetPluginInfo returns generic information about this plugin, like its version.
func (k *kubeProvider) GetPluginInfo(context.Context, *pbempty.Empty) (*pulumirpc.PluginInfo, error) {
return &pulumirpc.PluginInfo{
Expand Down
5 changes: 3 additions & 2 deletions provider/pkg/provider/provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package provider
import (
"testing"

"github.com/pulumi/pulumi-kubernetes/provider/v4/pkg/kinds"
"github.com/pulumi/pulumi/sdk/v3/go/common/resource"
"github.com/stretchr/testify/assert"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
Expand Down Expand Up @@ -187,7 +188,7 @@ func Test_isPatchURN(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assert.Equalf(t, tt.want, isPatchURN(tt.args.urn), "isPatchURN(%v)", tt.args.urn)
assert.Equalf(t, tt.want, kinds.IsPatchURN(tt.args.urn), "isPatchURN(%v)", tt.args.urn)
})
}
}
Expand Down Expand Up @@ -225,7 +226,7 @@ func Test_isListURN(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assert.Equalf(t, tt.want, isListURN(tt.args.urn), "isListURN(%v)", tt.args.urn)
assert.Equalf(t, tt.want, kinds.IsListURN(tt.args.urn), "isListURN(%v)", tt.args.urn)
})
}
}

0 comments on commit 27c5dd4

Please sign in to comment.