-
Notifications
You must be signed in to change notification settings - Fork 378
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
Make the CRD Puller more robust #66
Conversation
- correctly get the OpenAPI v2 model based on the GVK (insteaof of guessing it erroneously) - avoid pulling resource types that are part of the kcp control plane - avoid stack overflow for recursive OpenAPI V2 schemas - support resources with non structural OpenAPI schemas, but disable validation in imported resource type - support protected community groups by adding the required annotation in imported CRD. Signed-off-by: David Festal <dfestal@redhat.com>
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.
avoid stack overflow for recursive OpenAPI V2 schemas
Do we have examples of recursive schemas like this? This seems like a useful test case.
pkg/crdpuller/discovery.go
Outdated
@@ -92,133 +107,161 @@ func (sp *schemaPuller) PullCRDs(context context.Context, resourceNames ...strin | |||
for _, apiResourcesList := range apiResourcesLists { | |||
gv, err := schema.ParseGroupVersion(apiResourcesList.GroupVersion) | |||
if err != nil { | |||
klog.Errorf("skipping discovery due to error parsing GroupVersion %s: %v", apiResourcesList.GroupVersion, err) | |||
klog.Errorf("CRDPuller: Skipping discovery due to error parsing GroupVersion %s: %v", apiResourcesList.GroupVersion, err) |
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.
If klog is used here mainly to get line numbers, I think we could remove the CRDPuller prefix, wdyt?
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.
Let me remove them, right.
|
||
break | ||
apiextensionsv1.SetDefaults_CustomResourceDefinition(crd) | ||
if apihelpers.IsProtectedCommunityGroup(gv.Group) { |
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.
Add a comment describing why we're doing this here, and how it's related to that PR.
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.
Done
The CustomResourceDefinition resource was a typical example (JsonSchemaProps schema is recursive), which I used to test this use-case. However with the other changes in this PR, it will not even be imported since we now skip all the resources that are already known by KCP internally. |
Signed-off-by: David Festal <dfestal@redhat.com>
@imjasonh Can you confirm whether it's OK for you now ? |
Make the CRD Puller more robust: