🐛 fix: Don't flatten schemas for type idents we don't know about #627
Conversation
k8s-ci-robot
added
do-not-merge/invalid-commit-message
|
Welcome @abayer! |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @abayer. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: abayer 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 |
I hit this problem with cases where there are multiple packages with the same group and version. The schema patcher would hit a nil pointer trying to flatten a schema for a kind that exists in one package, but not another, because it seems to assume that everything's in one package. By skipping cases where the parser hasn't already found the given kind in the given package, we got past that panic and everything works correctly. I believe this should address kubernetes-sigs#624, though I can't be sure if this is the only path that can lead to that panic. Signed-off-by: Andrew Bayer <[email protected]>
abayer
force-pushed
the
ignore-groupkind-from-other-package
branch
from
9e0d3f0
to
8eda5d7
Compare
k8s-ci-robot
removed
the
do-not-merge/invalid-commit-message
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
So you have multiple go packages that all have types for the same api group and version? Why? |
|
@alvaroaleman Honestly? The best explanation is probably that we screwed up. =) |
So what keeps you from reorganizing them? I am fine better handling this case and emit a proper error rather than panicking. But I am not sure we want to actually support it. |
|
That's a good question - Tekton's pretty established at this point, which obviously would make any significant reorganization more difficult. I'll raise the topic, but given that this is the first time I'm aware of us hitting problems due to our quirky structure, and we're only hitting the problem now that we're trying to add auto-generated OpenAPI schemas to our previously-handcrafted CRDs, I'm not sure if there'll be much, if any, willingness to shift things around. |
|
Ok, so https://github.com/tektoncd/pipeline/tree/main/pkg/apis has three relevant subdirectories - So yeah, we definitely screwed up - since Tekton came out of Knative, I assume our original I'll continue to see if we can reorganize, and definitely understand not wanting to support a weird edge case of a layout like Tekton's, but it'd be really handy for us (and possibly for others) if we could at least have an option to enable the behavior I add in this PR (as well as default behavior that errors out nicely rather than panicking). |
…llow instead Signed-off-by: Andrew Bayer <[email protected]>
|
I've updated the PR to add an |
|
We did not screwed up per se, it was done on purpose with only go type (and aliasing stuff in mind), to ease the migration of the whole code base from v1alpha1 to v1beta1. The assumption we made (or at least I made) was that nothing would/should prevent us from having multiple packages presenting one api version. I would still make that assumption but I also understand that it can be trickier to support. The real trick in our case is the deprecated, in alpha, resources that we didn’t want to move to the v1beta1 package (and api version) - but still allowing our user to refer to these resources from the v1beta1 for the time being. The plan is to get rid of v1alpha1 at some point. I also see no real problems to remove type aliasing, etc.. and having some duplication to allow us to fix this (sometime duplication is better than the wrong abstraction). |
| @@ -75,6 +75,10 @@ type Generator struct { | |||
|
|
|||
| // GenerateEmbeddedObjectMeta specifies if any embedded ObjectMeta in the CRD should be generated | |||
| GenerateEmbeddedObjectMeta *bool `marker:",optional"` | |||
|
|
|||
| // AllowMultiPackageGroups specifies whether cases where a group is used for multiple packages should be allowed, | |||
| // rather than the default behavior of failing. | |||
There was a problem hiding this comment.
After adding this PR thedefault behaviour would not be a failing right?
Could you please fix the comment to allow it describes what the variable represents and when it will be used such as we have for the other ones?
There was a problem hiding this comment.
The default behavior would still be failing in the multiple-packages-for-a-single-group scenario. The new option allows you to change that behavior, but the default stays the same.
| @@ -41,7 +41,7 @@ var _ = Describe("CRD Patching From Parsing to Editing", func() { | |||
| var crdSchemaGen genall.Generator = &Generator{ | |||
| ManifestsPath: "./invalid", | |||
| } | |||
| rt, err := genall.Generators{&crdSchemaGen}.ForRoots("./...") | |||
| rt, err := genall.Generators{&crdSchemaGen}.ForRoots("./apis/kubebuilder/...", "./apis/legacy/...") | |||
There was a problem hiding this comment.
Why did you need to change it for the specific pkgs?
Where/what is getting started to fail after these changes?
There was a problem hiding this comment.
This is because I made the (probably wrong!) decision to reuse the contents of pkg/schemapatcher/testdata for both the existing tests and the new testing of multiple-packages-per-group. I'll separate the test data for those situations.
| @@ -67,7 +67,7 @@ var _ = Describe("CRD Patching From Parsing to Editing", func() { | |||
| var crdSchemaGen genall.Generator = &Generator{ | |||
| ManifestsPath: "./valid", | |||
| } | |||
| rt, err := genall.Generators{&crdSchemaGen}.ForRoots("./...") | |||
| rt, err := genall.Generators{&crdSchemaGen}.ForRoots("./apis/kubebuilder/...", "./apis/legacy/...") | |||
There was a problem hiding this comment.
same. I think it needs to be reverted to ensure that the changes made here do not break any current test. Could you please clarify why this change was required?
There was a problem hiding this comment.
#627 (comment)
+1
Hi @abayer, @vdemeester,
IHMO it is missing ( probably in the issue )
- An example of the scenario
- A description of when it is required and why
Also, we would need to check if it is a valid scenario or if this PR is trying to add support to a scenario that ought not to be done.
Are you trying here to allow in the same project you specific the same GVK?
If you deprecated the version, should you not have a different GVK since the version would not be the same?
If I understood your scenario adequately, I think the controller-gen out to raise an error saying that the user is trying to specify the same GVK more than once instead to add support and begin to allow it.
|
@camilamacedo86 Sorry for the delay! I missed the notifications originally and just remembered to check this PR. 🤦 Thanks for your comments, and I'll address them today. |
|
@camilamacedo86 So the situation in Tekton Pipeline is that we have |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
k8s-ci-robot
added
lifecycle/rotten
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
|
@k8s-triage-robot: Closed this PR. In response to this:
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 hit this problem with cases where there are multiple packages with the same group and version. The schema patcher would hit a nil pointer trying to flatten a schema for a kind that exists in one package, but not another, because it seems to assume that everything's in one package. By skipping cases where the parser hasn't already found the given kind in the given package, we got past that panic and everything works correctly.
This adds an option for
schemapatch,allowMultiPackageGroup- when that is true, we won't error out in scenarios where a group is used in multiple packages, while in the default case, we'll error out with the package path and kind that couldn't be found.I believe this fixes #624, though I can't be sure if this is the only path that can lead to that panic.
Signed-off-by: Andrew Bayer [email protected]