cache-server: expose apiresourceschemas and apiexports #1815

p0lyn0mial · 2022-08-23T15:59:09Z

Summary

exposes schema-less apiresourceschemas and apiexports resources. It allows us to store any fields we need. Since this server will be used by the shards, schemas will be implicit.

I did the following manual test which indicates storing "foo": "bar" with an apiexport obj works.

POST:

curl -k -X POST 'https://localhost:6443/apis/apis.kcp.dev/v1alpha1/apiexports' -H 'Content-Type: application/json' -d '{"apiVersion":"apis.kcp.dev/v1alpha1","kind":"APIExport","metadata":{"name":"ab"},"foo":"bar"}'

GET:

curl -k 'https://localhost:6443/apis/apis.kcp.dev/v1alpha1/apiexports/ab'
{
  "apiVersion": "apis.kcp.dev/v1alpha1",
  "foo": "bar",
  "kind": "APIExport",
  "metadata": {
    "annotations": {
      "kcp.dev/cluster": "system:admin"
    },
    "creationTimestamp": "2022-08-24T08:50:09Z",
    "generation": 1,
    "managedFields": [
      {
        "apiVersion": "apis.kcp.dev/v1alpha1",
        "fieldsType": "FieldsV1",
        "fieldsV1": {
          "f:foo": {}
        },
        "manager": "curl",
        "operation": "Update",
        "time": "2022-08-24T08:50:09Z"
      }
    ],
    "name": "ab",
    "resourceVersion": "11",
    "uid": "b4b9447e-5ea8-4b73-a996-eb4a98eae3e0"
  }
}

note for now passing a logical cluster name is not required in the future it might be required along with a shard name.

Related issue(s)

requires kcp-dev/kubernetes#93

p0lyn0mial · 2022-08-23T15:59:29Z

/hold

I still need to test it manually.

p0lyn0mial · 2022-08-23T16:01:04Z

pkg/cache/server/bootstrap/bootstrap.go

+
+	return wait.PollInfiniteWithContext(ctx, time.Second, func(ctx context.Context) (bool, error) {
+		for _, crd := range crds {
+			_, err := apiExtensionsClusterClient.Cluster(SystemCRDLogicalCluster).ApiextensionsV1().CustomResourceDefinitions().Create(ctx, crd, metav1.CreateOptions{})


creating crds with empty schemas is cheap and doesn't require creating an in-memory storage implementation.

it can be reworked in the future if we don't like it.

the storage is required for listing CRDs and for discovery (not implemented yet).

p0lyn0mial · 2022-08-24T09:28:30Z

/hold cancel

stevekuznetsov

Mostly seems reasonable, some small comments

config/crds/bootstrap.go

stevekuznetsov · 2022-08-24T12:31:28Z

pkg/cache/server/bootstrap/bootstrap.go

+	configcrds "github.com/kcp-dev/kcp/config/crds"
+)
+
+var SystemCRDLogicalCluster = logicalcluster.New("system:system-crds")


This is a constant somewhere already, could we import it?

I think we should treat it as a distinct value, I picked up the same logical cluster for symmetry.

A comment to explain that would be great! Makes sense.

pkg/cache/server/bootstrap/bootstrap.go

stevekuznetsov · 2022-08-24T12:33:03Z

pkg/cache/server/bootstrap/bootstrap.go

+					Type:                   "object",
+					XPreserveUnknownFields: pointer.BoolPtr(true),
+				},
+			} // wipe the schema, we don't need validation


Is this the only diff from the other bootstrap functions? could we DRY this out?

we could call CreateSingle from this function and retry on any error.

stevekuznetsov · 2022-08-24T12:33:39Z

pkg/cache/server/config.go

@@ -125,17 +143,31 @@ func NewConfig(opts *cacheserveroptions.CompletedOptions) (*Config, error) {
 	serverConfig.LoopbackClientConfig.DisableCompression = true
 	clientutils.EnableMultiCluster(serverConfig.LoopbackClientConfig, &serverConfig.Config, "namespaces", "apiservices", "customresourcedefinitions", "clusterroles", "clusterrolebindings", "roles", "rolebindings", "serviceaccounts", "secrets")

+	var err error
+	c.ApiExtensionsClusterClient, err = apiextensionsclient.NewClusterForConfig(serverConfig.LoopbackClientConfig)


nit: we're moving to the context-based clients instead of NewCluster - can we do that here as well to save Varsha some work in the future? :)

np, mind sharing an example?

Look for any use of logicalcluster.WithCluster() e.g.:

return c.kubeClusterClient.CoreV1().Namespaces().Patch(logicalcluster.WithCluster(ctx, clusterName), name, pt, data, opts, subresources...)

okay, but I need a Wildcard logical cluster for informers, it seems like informers don't support the new way of passing a cluster name.

Yes, that is the one case where this approach does not work

#1813 fixes that :)

do we need 2 clients until it merges?

Not sure I follow actually - you need a cluster-scoped client for the factory:

# github.com/kcp-dev/kcp/pkg/client/informers func NewSharedInformerFactoryWithOptions( ... versioned.Interface, ... ) {}

Where you can get such a thing from .Cluster(logicalcluster.Wildcard) which should be identical to SetCluster() on the *rest.Config, no?

# github.com/kcp-dev/kcp/pkg/client/clientset/versioned func (c *Cluster) Cluster(name v2.Name) Interface {}

So NewClusterForConfig(cfg).Cluster(logicalcluster.Wildcard) would become NewClientForConfig(kcpclienthelper.SetCluster(rest.CopyConfig(cfg), logicalcluster.Wildcard))?

something is broken, I think I have to use it along with NewClusterRoundTripper but then the path seems to be incorrect /clusters/*/clusters/system:system-crds/apis/apiextensions.k8s.io/v1/customresourcedefinitions/apiresourceschemas.apis.kcp.dev

I'd like to proceed with this PR if that's okay. I can revisit using the context-based clients in the future

pkg/cache/server/server.go

p0lyn0mial · 2022-08-24T13:27:41Z

pkg/server/server.go

 // run inside of a post-start-hook. The k8s APIServer wrote the post-start-hook context code before contexts
 // were part of the Go stdlib.
-func goContext(parent genericapiserver.PostStartHookContext) context.Context {
+func GoContext(parent genericapiserver.PostStartHookContext) context.Context {


I don't like creating common/utils pkgs :P

You and @sttts both ... but I like copy-pasting something 100x even less than I like creating a utility package ;)

moved to a util pkg

util packages 😱

ncdc · 2022-08-25T11:09:46Z

@p0lyn0mial if you wouldn't mind, please prefix the PR titles with the area they're for (it makes it easier to know at a glance) - thanks!

/retitle cache-server: expose apiresourceschemas and apiexports

stevekuznetsov

Other comments can/will be follow-ups.

stevekuznetsov · 2022-08-25T11:54:15Z

pkg/util/util.go

+import (
+	"context"
+
+	genericapiserver "k8s.io/apiserver/pkg/server"


nit: If I had a crystal ball, I think I'd make the prophecy that Stefan would want this helper to end up in this upstream package ... :^)

How about moving that into the upstream package?

openshift-ci · 2022-08-25T11:57:21Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: stevekuznetsov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [stevekuznetsov]
config/crds/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

stevekuznetsov · 2022-09-16T12:51:59Z

#342

p0lyn0mial requested a review from stevekuznetsov August 23, 2022 15:59

openshift-ci bot requested review from ncdc and sttts August 23, 2022 15:59

p0lyn0mial removed the request for review from ncdc August 23, 2022 15:59

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 23, 2022

p0lyn0mial commented Aug 23, 2022

View reviewed changes

p0lyn0mial force-pushed the cache-exports-schemas branch 2 times, most recently from 675859b to f162ca2 Compare August 24, 2022 09:19

p0lyn0mial mentioned this pull request Aug 24, 2022

Multi-Release-Epic: Sharding #1225

Closed

37 tasks

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2022

stevekuznetsov reviewed Aug 24, 2022

View reviewed changes

p0lyn0mial force-pushed the cache-exports-schemas branch from f162ca2 to 09c3398 Compare August 24, 2022 13:27

p0lyn0mial commented Aug 24, 2022

View reviewed changes

p0lyn0mial added 2 commits August 24, 2022 16:35

expose apiresourceschemas and apiexports

99b7781

move goContext to a util pkg

fe9bf89

p0lyn0mial force-pushed the cache-exports-schemas branch from 09c3398 to fe9bf89 Compare August 24, 2022 14:35

openshift-ci bot changed the title ~~expose apiresourceschemas and apiexports~~ cache-server: expose apiresourceschemas and apiexports Aug 25, 2022

stevekuznetsov approved these changes Aug 25, 2022

View reviewed changes

openshift-ci bot assigned stevekuznetsov Aug 25, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 25, 2022

p0lyn0mial added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 25, 2022

p0lyn0mial mentioned this pull request Aug 25, 2022

bug: cache-server misses the scope client #1829

Closed

openshift-merge-robot merged commit b1fcc08 into kcp-dev:main Aug 25, 2022

p0lyn0mial mentioned this pull request Sep 22, 2022

Build A MVP Persistent Caching Proxy #342

Closed

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cache-server: expose apiresourceschemas and apiexports #1815

cache-server: expose apiresourceschemas and apiexports #1815

p0lyn0mial commented Aug 23, 2022 •

edited

Loading

p0lyn0mial commented Aug 23, 2022

p0lyn0mial Aug 23, 2022

p0lyn0mial Aug 23, 2022

p0lyn0mial commented Aug 24, 2022

stevekuznetsov left a comment

stevekuznetsov Aug 24, 2022

p0lyn0mial Aug 24, 2022

stevekuznetsov Aug 24, 2022

stevekuznetsov Aug 24, 2022

p0lyn0mial Aug 24, 2022

stevekuznetsov Aug 24, 2022

p0lyn0mial Aug 24, 2022

stevekuznetsov Aug 24, 2022

p0lyn0mial Aug 24, 2022

stevekuznetsov Aug 24, 2022

stevekuznetsov Aug 24, 2022

p0lyn0mial Aug 24, 2022

stevekuznetsov Aug 24, 2022

p0lyn0mial Aug 25, 2022

p0lyn0mial Aug 24, 2022

stevekuznetsov Aug 24, 2022

p0lyn0mial Aug 24, 2022

sttts Aug 29, 2022

ncdc commented Aug 25, 2022

stevekuznetsov left a comment

stevekuznetsov Aug 25, 2022

sttts Aug 29, 2022

openshift-ci bot commented Aug 25, 2022

stevekuznetsov commented Sep 16, 2022

cache-server: expose apiresourceschemas and apiexports #1815

cache-server: expose apiresourceschemas and apiexports #1815

Conversation

p0lyn0mial commented Aug 23, 2022 • edited Loading

Summary

Related issue(s)

p0lyn0mial commented Aug 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0lyn0mial commented Aug 24, 2022

stevekuznetsov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncdc commented Aug 25, 2022

stevekuznetsov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Aug 25, 2022

stevekuznetsov commented Sep 16, 2022

p0lyn0mial commented Aug 23, 2022 •

edited

Loading