-
Notifications
You must be signed in to change notification settings - Fork 421
Add virtual resources #3620
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
base: main
Are you sure you want to change the base?
Add virtual resources #3620
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
0c6691f
to
992fc97
Compare
TestingPrerequsites:
Producer:
Consumer:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces Virtual Resources (VRs), a new feature that allows resources to be projected from virtual workspaces without consuming additional etcd storage. VRs are exposed through APIExport-APIBinding relationships with virtual storage configuration instead of CRD storage.
Key changes include:
- Addition of virtual storage configuration in APIExport API
- New virtual resources server for handling VR requests
- Enhanced version discovery aggregation for mixed CRD/virtual storage resources
- Updated replication virtual workspace infrastructure
Reviewed Changes
Copilot reviewed 68 out of 68 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
test/e2e/virtualresources/cachedresources/vr_cachedresources_test.go |
Comprehensive E2E test for virtual resources using cached resources |
test/e2e/virtual/replication/virtualworkspace_test.go |
Removed existing replication virtual workspace test |
test/e2e/fixtures/wildwest/bootstrap.go |
Added CRD helper function for test fixtures |
sdk/apis/apis/v1alpha2/types_apiexport.go |
Extended APIExport API with virtual storage configuration |
pkg/server/virtualresources/server.go |
New server for handling virtual resource requests via proxy |
pkg/server/aggregatingcrdversiondiscovery/server.go |
New version discovery server that handles mixed storage types |
pkg/virtual/replication/builder/unwrap.go |
Enhanced replication builder with cluster-aware unwrapping |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
"github.com/kcp-dev/kcp/test/e2e/framework" | ||
) | ||
|
||
const XXX_timeout = math.MaxInt64 |
Copilot
AI
Oct 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to be a placeholder constant with an inappropriate name and value. Replace 'XXX_timeout' with a meaningful name and use an appropriate timeout value instead of math.MaxInt64.
const XXX_timeout = math.MaxInt64 | |
const defaultTestTimeout = 30 * time.Second |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a leftover from when I was testing the test. I'll get rid of it.
obj := innerObj.DeepCopy() | ||
setCluster(obj, cluster) | ||
if !w.safeWrite(watch.Event{ | ||
Type: watch.Added, |
Copilot
AI
Oct 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the UpdateFunc handler, the event type should be watch.Modified, not watch.Added. This will cause incorrect watch event types to be sent to clients.
Type: watch.Added, | |
Type: watch.Modified, |
Copilot uses AI. Check for mistakes.
obj := innerObj.DeepCopy() | ||
setCluster(obj, cluster) | ||
if !w.safeWrite(watch.Event{ | ||
Type: watch.Added, |
Copilot
AI
Oct 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the DeleteFunc handler, the event type should be watch.Deleted, not watch.Added. This will cause incorrect watch event types to be sent to clients.
Type: watch.Added, | |
Type: watch.Deleted, |
Copilot uses AI. Check for mistakes.
// of an exported virtual resource. | ||
const virtualResourceAPIExportIdentityKey virtualResourceAPIExportIdentityKeyType = "VirtualResourceAPIExportIdentity" | ||
|
||
// WithVirtualWorkspaceName adds the VirtualWorkspace name to the context. |
Copilot
AI
Oct 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment incorrectly describes the function as adding VirtualWorkspace name, but the function actually adds APIExport identity. Update the comment to match the function's actual purpose.
// WithVirtualWorkspaceName adds the VirtualWorkspace name to the context. | |
// WithVirtualResourceAPIExportIdentity adds the APIExport identity to the context. |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did a first round. Next - manual testing will follow :)
config/crds/bootstrap.go
Outdated
return crd, nil | ||
} | ||
|
||
func AllCRDs() ([]*apiextensionsv1.CustomResourceDefinition, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go docs, public function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And its not quite clear what function does? GetAllCRDs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a hacky way to make DynamicRESTMapper resolve resources coming from system CRDs (e.g. the CachedResourceEndpointSlice). It's superseded by #3630 and once it gets in, this change (+ all other rest mapper changes in this PR) won't be necessary anymore.
return urls, nil | ||
} | ||
|
||
func FindOneURL(prefix string, urls []string) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
APIBindingByIdentityAndGroupResource = "apibinding-byIdentityGroupResource" | ||
) | ||
|
||
func IndexAPIBindingByIdentityGroupResource(obj interface{}) ([]string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go doc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return result, nil | ||
} | ||
|
||
func IndexAPIExportByVirtualResourceIdentities(obj interface{}) ([]string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
) | ||
}, | ||
|
||
knownVirtualResourceVerbs: map[string][]string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel like this need code comment why we doing this verbProvider thingy. I know we talked about this but other readers might not be aware of reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
knownVirtualResourceStatusVerbs: map[string][]string{ | ||
"CachedResourceEndpointSlice.cache.kcp.io": {"get"}, | ||
}, | ||
knownVirtualResourceScaleVerbs: map[string][]string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and wondering, if we should ahve "catch all", where if somebody adds customer sub-resource we give get
automatically? if this even possilbe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do real discovery, and see what verbs are supported by the VW.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we leave it for later? We either hard-code get
as you suggested, or do discovery, but in either case it's a functionality that has no use-case for now. It's fairly easy to implement it, but it would be pretty complex to write a test for it. The whole virtual resources codebase is ready for custom *EndpointSlice
, but I think we need to iron out specifics of how we want users to add their own virtual resources / backing virtual workspaces etc.
sliceMapping, err := s.drm.ForCluster(apiExportCluster).RESTMapping(schema.GroupKind{ | ||
Group: ptr.Deref(virtual.Reference.APIGroup, ""), | ||
Kind: virtual.Reference.Kind, | ||
}, "v1alpha1") // HACK: we need to be able to discover the latest version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the comment in #3620 (comment) -- this is to be dropped, because #3630 adds support for version defaulting.
|
||
func virtualResourceURLWithCluster(vwURL, apiExportIdentity string, clusterNameOrWildcard string) string { | ||
// Formats the URL like so: | ||
// <Virtual resource VW endpoint>:<APIExport identity>/clusters/<Target cluster> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add full example how this would look like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="CachedResource reference must not be changed" | ||
CachedResource CachedResourceReference `json:"cachedResource"` | ||
|
||
// partition (optional) points to a partition that is used for filtering the endpoints |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// partition (optional) points to a partition that is used for filtering the endpoints | |
// partition points to a partition that is used for filtering the endpoints |
I pushed 1 commit to you branch (my miskate but lets keep it), where I started wiring in the example for VirtualMachines. But having hard time to make it work :/
|
/retest |
On-behalf-of: @SAP [email protected] Signed-off-by: Robert Vasek <[email protected]>
…3553' CachedResourceSchemaSource is not needed anymore. APIExport holds the reference to the schema for virtual resources. On-behalf-of: @SAP [email protected] Signed-off-by: Robert Vasek <[email protected]>
…ClusterAndNamespace On-behalf-of: @SAP [email protected] Signed-off-by: Robert Vasek <[email protected]>
16c0acc
to
154927d
Compare
Virtual resources add a way to project resources from a provider to consumer clusters using APIExports and APIBindings. On-behalf-of: @SAP [email protected] Signed-off-by: Robert Vasek <[email protected]>
On-behalf-of: @SAP [email protected] Signed-off-by: Robert Vasek <[email protected]>
…dresources On-behalf-of: @SAP [email protected] Signed-off-by: Robert Vasek <[email protected]>
154927d
to
2536889
Compare
@gman0: The following tests failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Summary
This PR adds Virtual resources (VRs) feature, and modifies the Replication virtual workspace to be in line with VRs.
Unlike regular, CRD-backed resources, VRs don't reside in a workspace, and they don't consume additional etcd storage. They exist at a virtual workspace endpoint, and that endpoint is then aggregated with the rest of the GVRs in a workspace, and exposed like that to the client. This way, producers can project resources to consumers, and their changes to the objects are visible immediately (=as soon as the object is propagated to cache, and from there to consumer's shard and informers).
A virtual resource is consumable through the usual APIExport - APIBinding relationship:
The export defines a resource with
storage.virtual
, and refers to an endpoint slice object inreference
by API group, Kind, and name of that object. Binding that export then exposes it in the consumer's workspace.Binding
Binding a VR uses the same code as with CRD-storage resources: a bound CRD is created from the APIResourceSchema, and that CRD must become Established before the resource shows up in discovery and can be used. The only difference is that VRs don't set any
APIBinding.Status.BoundResources[].StorageVersions[]
-- this is the only user-visible way to distinguish VRs from other resources. It was necessary to leave StorageVersions asnil
so that binding deletion works correctly: thecrdcleanup
controller shouldn't touch the virtual resources.Discovery and OpenAPI
Since VRs still use bound CRDs, group discovery and OpenAPI works out-of-the-box, without changes. Version discovery however required some work: see
pkg/server/aggregatingcrdversiondiscovery
. A new apiserver was added to handle aggregation of CRDs (system/apibinding/normal) with VRs.This was necessary because: the apiextensions apiserver would gladly advertise all bound CRDs, even those that belong to VRs, but all APIResources would get the same verbs; this is a problem because a virtual workspaces that backs up a VR can offer a different set of verbs. AggregatingCRDVersionDiscovery lists CRDs using the
apiBindingAwareCRDLister
, and then modifies the resulting APIResourceList so that all entries have correct verbs.How does it know which bound CRDs are exported as VRs?
apiBindingAwareCRDLister
was modified to decorate them withapis.kcp.io/schema-storage
annotation. This is never persisted in etcd.Another approach would be to modify the apiextensions apiserver to handle the discovery, but I wasn't sure how to do that cleanly without making a lot of changes. So for now, order of resolving a discovery request is like so:
/apis/<Group>/<Resource>
:*.k8s.io
or*.kubernetes.io
. This was done to also prevent hijacking discovery for/apis/apiextensions.k8s.io/v1
which is handled by the apiextensions apiserver. I'm not completely sure this is the best way to do it.verbsProvider
to get the correct verbs for the crd.Resource handling
The VR apiserver checks whether the resource is defined as VR in the export: either through digging the associated APIBinding, or through the export identity. If it is, it retrieves the endpoint URL from the endpoint slice, and proxies the request to there.
The handling VW must be able to decode this URL:
<VW endpoint>:<APIExport identity>/<Path...>
, e.g.https://shard-1.kcp.io:7443/services/replication/<CachedResource cluster>/<CachedResource>:<APIExport identity>/apis/wildwest.dev/v1alpha1/cowboys/namespaces/default/cowboy-1
, where<APIExport identity>
is the identity of the export owning the schema of the VR.I'm not completely sure this is ok, but I didn't find a better way to handle local and wildcard requests. The export is the source of the schema (#3553 didn't work well enough), and since a VR (a CachedResource for example) can be exported by multiple exports, there must be a way to refer to a specific one in the request.
storage.virtual
. Then the code considers it as a good match.What's missing in this PR
The code is behind
CacheAPIs
feature flag, disabled by default. Also, at the moment the virtual workspaces server must be running as a separate process -- the tls struct expects the certs to be inopts.Extra.ShardVirtualWorkspaceCAFile, ...
: probably easy to fix, but I haven't tested this properly yet.storage.virtual
storage.virtual
if the feature flag is disabled?CachedResourceEndpointSlice.Spec.CachedResource
is missing an optionalPath
in the referencekcp.io/path
annotationWhat Type of PR Is This?
/kind api-change
/kind feature
Related Issue(s)
Fixes #3487
Release Notes