Skip to content

implement resources API part 2#17749

Open
aalves08 wants to merge 4 commits into
rancher:masterfrom
aalves08:resources-api-part-2.1
Open

implement resources API part 2#17749
aalves08 wants to merge 4 commits into
rancher:masterfrom
aalves08:resources-api-part-2.1

Conversation

@aalves08
Copy link
Copy Markdown
Member

Summary

Fixes #16898

Implements Resources API Part 2: create, update, patch, and delete operations at both the ResourcesApi level (resources.cluster.* / resources.mgmt.*) and the ResourceInstanceApi level (resourceInstance.*).

NOTE: demo interaction with persistentvolume list view to test (to be deleted in the end)

Occurred changes and/or fixed issues

New write operations added to the Resources API:

Method Level RBAC HTTP Cache
resources.cluster.create(data) ResourcesApi canCreate POST Updates store
resources.cluster.patch(type, id, data) ResourcesApi None PATCH (merge-patch) No cache update
resources.cluster.update(type, id, data) ResourcesApi None PUT (cleanForSave) No cache update
resources.cluster.delete(type, id) ResourcesApi None DELETE No cache update
resourceInstance.patch(data) ResourceInstanceApi canEdit PATCH (merge-patch) Merges response into store
resourceInstance.update() ResourceInstanceApi canEdit PUT (via model.save) Updates store
resourceInstance.delete() ResourceInstanceApi canDelete DELETE (via model.remove) Updates store

All methods are available on both cluster and mgmt scopes (ClusterApi and MgmtApi are type aliases to ResourcesApi).

Existing read operations (find, findAll, findFiltered) now wrap results in ResourceInstanceImpl, so every resource returned by the API has patch(), update(), and delete() available out of the box.

Technical notes summary

  • ResourceInstanceImpl wrapping: All resources returned from the API (find, findAll, findFiltered, create) are now wrapped in ResourceInstanceImpl. This provides the instance-level methods (patch, update, delete) on every returned resource.
  • _model is non-enumerable: The underlying store model reference is stored via Object.defineProperty with enumerable: false. This prevents structuredClone and JSON.stringify from attempting to serialize store internals (functions, dispatch refs, circular references), which is critical for Vue 3 reactive compatibility.
  • declare keyword for _model: Uses declare _model: any instead of a class field initializer to avoid TypeScript emitting a runtime assignment that would override the non-enumerable Object.defineProperty set in the constructor.
  • invalidatePageCache: false: The resourceInstance.patch() method explicitly passes invalidatePageCache: false when dispatching load after a successful patch. Without this, the store mutation defaults invalidatePageCache to true, which clears paginated list data and causes resources to disappear from list views.
  • toJSON fallback: ResourceInstanceImpl.toJSON() defensively checks for _model.toJSON before calling it, falling back to a shallow spread. This prevents errors when the model lacks a toJSON method (e.g. in test mocks).
  • resourceUrl helper: A new private method on ResourcesApiClassImpl builds resource URLs from schema.linkFor('collection') + '/' + resourceId. It reuses the existing isNamespaced() validation to enforce namespace/name format for namespaced resources.
  • CreateResourceData type: New type exported from the barrel — { type: ResourceType } & Record<string, any> — enforces the mandatory type property on create data.
  • delete() added alongside remove(): remove() is kept for backward compatibility. delete() is the new public method that adds RBAC checking before delegating to model.remove().

Areas or cases that should be tested

  • Create: Verify resources.cluster.create() with valid data, verify permission denial when canCreate is false
  • Patch (ResourcesApi): Verify resources.cluster.patch() sends only partial data with application/merge-patch+json content type, verify namespace validation
  • Patch (ResourceInstanceApi): Verify resourceInstance.patch() checks canEdit, sends partial data, merges response back into the instance, and does not invalidate paginated list views
  • Update (ResourcesApi): Verify resources.cluster.update() runs cleanForSave and sends full data with PUT
  • Update (ResourceInstanceApi): Verify resourceInstance.update() checks canEdit and delegates to model.save()
  • Delete (ResourcesApi): Verify resources.cluster.delete() sends DELETE request with correct URL
  • Delete (ResourceInstanceApi): Verify resourceInstance.delete() checks canDelete and delegates to model.remove()
  • All methods on mgmt scope: Verify the same operations work on resources.mgmt.*
  • Namespaced vs cluster-scoped resources: Verify correct URL construction and namespace validation for both types
  • Tested locally with Chrome

Areas which could experience regressions

  • List views using pagination (findFiltered / findPage): The ResourceInstanceImpl wrapping in findFiltered now maps over both array results and transient .data arrays. Verify paginated lists still render correctly.
  • find / findAll consumers: Any code that uses toStrictEqual or instanceof checks against raw store models will see ResourceInstanceImpl instances instead. Property access is unchanged since getters proxy through to the model.
  • Extensions using ResourceInstanceApi: The interface now exposes patch(), update(), and delete() — no breaking changes, but extensions get new methods in IntelliSense.
  • Vue reactivity with resource instances stored in component data: The _model non-enumerable change should be transparent, but verify that resource data remains reactive when stored in Vue component state.

Screenshot/Video

N/A — API-only changes with no UI modifications.

Checklist

  • The PR is linked to an issue and the linked issue has a Milestone, or no issue is needed
  • The PR has a Milestone
  • The PR template has been filled out
  • The PR has been self reviewed
  • The PR has a reviewer assigned
  • The PR has automated tests or clear instructions for manual tests and the linked issue has appropriate QA labels, or tests are not needed
  • The PR has reviewed with UX and tested in light and dark mode, or there are no UX changes
  • The PR has been reviewed in terms of Accessibility
  • The PR has considered, and if applicable tested with, the three Global Roles Admin, Standard User and User Base

@aalves08 aalves08 added this to the v2.15.0 milestone May 20, 2026
@aalves08 aalves08 requested review from nwmac and richard-cox May 20, 2026 13:59
@aalves08 aalves08 self-assigned this May 20, 2026
@aalves08 aalves08 removed the request for review from nwmac May 20, 2026 14:02
@aalves08 aalves08 requested a review from nwmac May 27, 2026 14:44
Copy link
Copy Markdown
Member

@richard-cox richard-cox left a comment

Choose a reason for hiding this comment

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

There's lots of compile / transpile time TS warnings

WARNING  Compiled with 13 warnings                                                                                                                                                                         

warning  in ./shell/apis/intf/resources.ts

export 'ResourceType' (reexported as 'ResourceType') was not found in '@shell/apis/intf/resources-api/resource-base' (module has no exports)

Comment thread docusaurus/docs/extensions/resources-api.md
sort: ['provisioner'],
};

export const PERSISTENT_VOLUME_CAPACITY = {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

just in case, remember to remove this with the other persistentvolume changes

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks for the reminder!

Comment thread shell/apis/intf/resources-api/resource-base.ts
Comment thread shell/apis/intf/resources-api/resource-base.ts
Comment thread shell/apis/intf/resources-api/resource-instance.ts
await model.save();

return new ResourceInstanceImpl(model) as T;
} catch (e: unknown) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i was playing around with the API and got this consoe.error

Resource API error - cluster - Failed to create resource of type "pod": undefined

no stack trace or anything to go on

this took me down a long path of trying to work out how to resolve

  • the Error thrown wasn't an error, but an array of strings (this shouldn't happen, but is)
  • the console.error didn't print this because an array has not e.message
  • i'd try/catch'd the line that threw the error so i didn't get anything on console regarding the error that was actually an array

i think to help with this we just need to update surfaceError

  private surfaceError(message: string, e?: any): never {
    console.error(`Resource API error - ${ this.storeType } - ${ message }`, e); // eslint-disable-line no-console
    throw new Error(`Resource API error - ${ this.storeType } - ${ message }`, { cause: e });
  }

await model.save();

return new ResourceInstanceImpl(model) as T;
} catch (e: unknown) {
Copy link
Copy Markdown
Member

@richard-cox richard-cox May 27, 2026

Choose a reason for hiding this comment

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

Following from https://github.com/rancher/dashboard/pull/17749/changes#r3311237610

the object used in create was

{
        type: 'pod',
        spec: { template: {} }
      }

which is missing required stuff that our validation picks up. however the error message is orientated to form world "Name" is required.

for api use though we want the raw path in the error message not the human form name... alternatively we don't run validation and let the http error handle it

throw new Error(`ResourceInstance API error - ${ this._model?.type }/${ this._model?.id } - ${ message }`, { cause: e });
}

constructor(model: any) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This process is a bit scary and generally with this stuff there's hidden dragons that only surface later in production.

I asked our dear friend gemini and they came up with some good catches

  • Object.getOwnPropertyDescriptors(model) only grabs the keys that exist at the exact moment of instantiation. If the underlying _model later receives a new property dynamically (e.g., a websocket update injects a new field that wasn't there initially), the ResourceInstanceImpl instance will not have a getter/setter for it and the property will be invisible to the consumer.
  • getOwnPropertyDescriptors only looks at the object's own properties. If the underlying model relies on getters or properties located on its prototype chain (which is common in class-based models like SteveModel), this constructor completely misses them.
  • Heavy Instantiation: Looping through every single key and calling Object.defineProperty is computationally expensive.
  • Memory Bloat: Inside the loop, get: () => this._model[key] creates a new anonymous closure function for every single property on every single instance.

i think we need to go down the worse but safest path of

  • aways return objects returned from dashboard actions without passing them through another class
  • when returning the object instance as it as the required interface
  • update SteveModel to implement the required interface (note though it's not TS)

though am open to other ideas, but basically we need to ensure we return the instance from the store

this.surfaceError('Cannot patch: permission denied');
}

const url = this._model.linkFor('update') || this._model.linkFor('self');
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we should just be able to call save with additional args

});

if (Array.isArray(response)) {
return response.map((r: any) => new ResourceInstanceImpl(r)) as FindFilteredPageResponse<T>;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we're de-references the collection. so any new / removed entries won't be applied to the response

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same applies for the other map's

aalves08 and others added 3 commits May 27, 2026 18:33
Co-authored-by: Richard Cox <18697775+richard-cox@users.noreply.github.com>
Co-authored-by: Richard Cox <18697775+richard-cox@users.noreply.github.com>
Co-authored-by: Richard Cox <18697775+richard-cox@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.

Feature: Rancher API for K8s resources - part 2

2 participants