✨ create apis/v1alpha2, add new ResourceSchemas #3318
Conversation
kcp-ci-bot
added
release-note-none
|
Skipping CI for Draft Pull Request. |
kcp-ci-bot
added
the
kind/api-change
|
[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 |
kcp-ci-bot
added
the
size/XXL
|
/test all |
There was a problem hiding this comment.
I think few things are missing here:
- Conversion webhook for v1alpha1 to v1alpha2
- I would really like it to be split in commits:
All API changes and webhook into 1 commit
Sed replace changes to another.
But Im in the same boat as you for the approach. @sttts - we need your guidance here :)
Will do, for rebasing it was just simpler to keep it locally as 1 commit for now. |
xrstf
force-pushed
the
resourceschemas
branch
3 times, most recently
from
05c99d7 to
ba86ec7
Compare
|
/test all |
kcp-ci-bot
added
the
needs-rebase
kcp-ci-bot
removed
the
needs-rebase
|
/test all |
xrstf
force-pushed
the
resourceschemas
branch
2 times, most recently
from
bbc80c8 to
b6aabe3
Compare
|
/test all |
kcp-ci-bot
added
release-note
|
/test all |
| Schema: "foo", | ||
| Storage: ResourceSchemaStorage{ | ||
| CRD: &ResourceSchemaStorageCRD{}, | ||
| }, |
There was a problem hiding this comment.
I don't see valid ResourceSchemas. This would be tested with fuzzing.
| }, | ||
| }, | ||
| }}, | ||
| }, |
There was a problem hiding this comment.
how do we know that everything in the spec is copied over to v1alpha1 and back? This would be tested with fuzzing. We must have these tests. Somebody adds a field and forgets the conversion. Then we have dataloss 😞
| } | ||
|
|
||
| func Convert_v1alpha1_APIExportSpec_To_v1alpha2_APIExportSpec(in *apisv1alpha1.APIExportSpec, out *APIExportSpec, s kubeconversion.Scope) error { | ||
| if in.Identity != nil { |
| } | ||
| } | ||
|
|
||
| if in.MaximalPermissionPolicy != nil { |
| } | ||
| } | ||
|
|
||
| if claims := in.PermissionClaims; claims != nil { |
There was a problem hiding this comment.
By untested, you mean if it's being converted roundtrip?
| if overhanging, ok := in.Annotations[resourceSchemasAnnotation]; ok { | ||
| resourceSchemas := []ResourceSchema{} | ||
| if err := json.Unmarshal([]byte(overhanging), &resourceSchemas); err != nil { | ||
| return fmt.Errorf("failed to decode schemas from JSON: %w", err) |
There was a problem hiding this comment.
do we have admission that checks this not to happen?
| out.Spec.ResourceSchemas = []ResourceSchema{} | ||
| } | ||
|
|
||
| out.Spec.ResourceSchemas = append(out.Spec.ResourceSchemas, resourceSchemas...) |
There was a problem hiding this comment.
this can lead to duplicates without admission checking that there is no duplicate.
| out.Spec.ResourceSchemas = append(out.Spec.ResourceSchemas, resourceSchemas...) | ||
| } | ||
|
|
||
| delete(out.Annotations, resourceSchemasAnnotation) |
There was a problem hiding this comment.
All these mutations look suspicious. Wondering whether we might cache mutations here.
Look at the invariant defined here:
// Convert attempts to convert one object into another, or returns an error. This
// method does not mutate the in object, but the in and out object might share data structures,
// i.e. the out object cannot be mutated without mutating the in object as well.
// The context argument will be passed to all nested conversions.
Convert(in, out, context interface{}) error
In other words, in and out share data structure. But when you mutate out, you must do a (hopefully shallow, avoid deep copies) copy.
There was a problem hiding this comment.
This is fuzzing test where we check that we didn't modified the input itself, right? Or you had something else inmind with cache ?
kcp-ci-bot
added
the
needs-rebase
On-behalf-of: @SAP [email protected]
On-behalf-of: @SAP [email protected]
The conversion ensures that virtual ResourceSchemas are retained in v1alpha1 by using an annotion. I made the assumption that the order of resource schemas is not important, as during a roundtrip conversion, non-cRD resource schemas would always be added *after* CRD resource schemas. On-behalf-of: @SAP [email protected]
The original plan was to use a regular, actual conversion webhook server to handle converting between v1alpha1 and v1alpha2. This was to be hosted in the same kcp shard process and server handler (i.e. as part of the preHandlerChainMux). However this presented a long list of problems: * If the webhook was part of the main kcp process, it would share the serving certificate. Since the CABundle has to be included in the CRDs, this means that kcp would need to know the CA that signed its serving cert, which can be troublesome to automate (cert-manager ACME issuers do not provide the CA) and to configure (requiring an additional CLI flag). * If the webhook would use its own certificate (by making use of SNI), it would need its own dedicated hostname that resolves to localhost. But since the admin is technically free to run kcp with `--external-hostname=localhost`, choosing a good name is difficult (localhost or 127.0.0.1). One could try to inject a custom HTTP client that has a custom RoundTripper that understands a special "kcp.local" domain, but this requires copying hundreds of lines of upstream k/k code because ultimately the client is constructed in deeply nested structs. * Additionally, if the webhook were to create its own certificate and CA, it would be different on each shard. And since the CABundle is par of the system CRDs on each shard's etcd, it would not be possible to actually re-generatet the CA on each startup, it would need to be persisted. But persistence is annoying, especially in Kubernetes. Having a volume for just a single certificate is overkill. Another solution for this could have been to persist the certificate in etcd. But at this point we abandoned this idea entirely. Instead, this commit adds a simple wrapper around k/k's CRConverter. The wrapper detects the apis.kcp.io API group and simply calls the conversion functions directly, skipping the entire webhook logic. This is faster, more efficient, requires no extra configuration and is simple to understand. On-behalf-of: @SAP [email protected]
This is mostly adding a bunch of imports and here and there converting between PermissionClaims, as we still have those for APIBindings in v1alpha1. On-behalf-of: @SAP [email protected]
same, but less interesting than the previous commit On-behalf-of: @SAP [email protected]
On-behalf-of: @SAP [email protected]
On-behalf-of: @SAP [email protected]
On-behalf-of: @SAP [email protected]
On-behalf-of: @SAP [email protected]
On-behalf-of: @SAP [email protected]
This was not properly done in kcp-dev#3336. On-behalf-of: @SAP [email protected]
On-behalf-of: @SAP [email protected] Signed-off-by: Mangirdas Judeikis <[email protected]>
mjudeikis
force-pushed
the
resourceschemas
branch
from
791ea95 to
573d6a3
Compare
kcp-ci-bot
removed
the
needs-rebase
|
/retest |
Summary
This PR adds a new version to the
apis.kcp.ioAPI,v1alpha2. This new version only contains the slightly updated APIExports, which now support a more elaborate configuration structure to enable virtual resources in the future.All the code was updated to refer to
v1alpha2.APIExports.Conversion between the two versions is handled by a wrapper around the stock CR Converter, see individual commits for more information.
Related issue(s)
Fixes #3301
TODO:
Confirmed that if you add a new type into a new API and don't sort converters, all stuff just explodes in tests and generation :) which is good!
Release Notes