Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Full CRDs are incomplete and causes json2jsii to fail #8532

Open
adrianord opened this issue Apr 28, 2022 · 7 comments · May be fixed by #14044
Open

Full CRDs are incomplete and causes json2jsii to fail #8532

adrianord opened this issue Apr 28, 2022 · 7 comments · May be fixed by #14044
Assignees
@MasonM
Labels
area/manifests P1 High priority. All bugs with >=5 thumbs up that aren’t P0, plus: Any other bugs deemed high priority type/bug

Comments

@adrianord
Copy link

Summary

The generated CustomResourceDefinitions seem to be incomplete because of the steps property.

argo-workflows/manifests/base/crds/full/argoproj.io_workflowtemplates.yaml

Lines 7673 to 7676 in ecd91b1

steps:
items:
type: array
type: array

The items property has a type property but does not have an accompanying items property.

This causes json2jsii to fail.
https://github.com/cdklabs/json2jsii/blob/60e891447e35a8bb1a9dac3496d6adce87d73292/src/type-generator.ts#L534-L540
This is an issue for tools that depend on json2jsii. cdk8s-team/cdk8s#2157

However, the json schema has these missing properties.

argo-workflows/api/jsonschema/schema.json

Lines 7315 to 7321 in ecd91b1

"steps": {
"description": "Steps define a series of sequential/parallel workflow steps",
"items": {
"$ref": "#/definitions/io.argoproj.workflow.v1alpha1.ParallelSteps"
},
"type": "array"
},

argo-workflows/api/jsonschema/schema.json

Lines 6449 to 6454 in ecd91b1

"io.argoproj.workflow.v1alpha1.ParallelSteps": {
"items": {
"$ref": "#/definitions/io.argoproj.workflow.v1alpha1.WorkflowStep"
},
"type": "array"
},

It looks like this problem was fixed at one point with #4828, but was later reverted by @JPZ13 here #4810 because of the CRDs becoming too long.

Is there way we can get the full CRDs for use with json2jsii as I do not believe it has any limit on CRD size like Kubernetes does. I'd like to have all the types for steps so that I can get strong typing with cdk8s.

Message from the maintainers:

Impacted by this bug? Give it a 👍. We prioritise the issues with the most 👍.
@alexec
Copy link
Contributor

alexec commented Apr 29, 2022

Would you like to submit a PR to fix this?

@adrianord
Copy link
Author

I would need some guidance, I spent some time looking into the issue yesterday on a fork https://github.com/adrianord/argo-workflows, but I believe the fix I've done there causes the CRDs to be too large for Kubernetes, which is why it was reverted before.

@alexec
Copy link
Contributor

alexec commented Apr 29, 2022

Share you PR and maybe we can take a look at it.

@stale
Copy link

stale bot commented Jun 18, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is a mentoring request, please provide an update here. Thank you for your contributions.

@stale stale bot added the problem/stale This has not had a response in some time label Jun 18, 2022
@stale
Copy link

stale bot commented Jul 10, 2022

This issue has been closed due to inactivity. Feel free to re-open if you still encounter this issue.

@stale stale bot closed this as completed Jul 10, 2022
@sstaley-hioscar
Copy link

Also getting an issue here with crds and json2jsii.

Error: unsupported array type undefined
                         at TypeGenerator.typeForArray (/snapshot/node_modules/cdk8s-cli/node_modules/json2jsii/lib/type-generator.js:404:19)
                         at TypeGenerator.emitArray (/snapshot/node_modules/cdk8s-cli/node_modules/json2jsii/lib/type-generator.js:260:25)
                         at TypeGenerator.emitTypeInternal (/snapshot/node_modules/cdk8s-cli/node_modules/json2jsii/lib/type-generator.js:186:29)
                         at TypeGenerator.typeForProperty (/snapshot/node_modules/cdk8s-cli/node_modules/json2jsii/lib/type-generator.js:383:21)
                         at TypeGenerator.typeForArray (/snapshot/node_modules/cdk8s-cli/node_modules/json2jsii/lib/type-generator.js:406:21)
                         at TypeGenerator.emitArray (/snapshot/node_modules/cdk8s-cli/node_modules/json2jsii/lib/type-generator.js:260:25)
                         at TypeGenerator.emitTypeInternal (/snapshot/node_modules/cdk8s-cli/node_modules/json2jsii/lib/type-generator.js:186:29)
                         at TypeGenerator.typeForProperty (/snapshot/node_modules/cdk8s-cli/node_modules/json2jsii/lib/type-generator.js:383:21)
                         at TypeGenerator.emitProperty (/snapshot/node_modules/cdk8s-cli/node_modules/json2jsii/lib/type-generator.js:322:35)
                         at /snapshot/node_modules/cdk8s-cli/node_modules/json2jsii/lib/type-generator.js:302:22

@MasonM MasonM removed the problem/stale This has not had a response in some time label Jan 1, 2025
@MasonM MasonM reopened this Jan 1, 2025
@MasonM MasonM self-assigned this Jan 1, 2025
@MasonM
Copy link
Contributor

MasonM commented Jan 1, 2025

Re-opening because having full CRDs would enable a lot of requested features, e.g. kubectl explain output (#8190) and validating admission policies (#13503)

@MasonM MasonM added area/manifests P1 High priority. All bugs with >=5 thumbs up that aren’t P0, plus: Any other bugs deemed high priority labels Jan 1, 2025
MasonM added a commit to MasonM/argo-workflows that referenced this issue Jan 2, 2025
@MasonM
fix(manifests): fix full CRDs and update tests to use them
0d64c1d 
The full CRDs at `manifests/base/crds/full` are only intended for usage
in editors
(argoproj#11266 (comment))
and are unusable otherwise. Trying to install them will cause spurious
validation errors with nearly every workflow.

Having the full CRDs would enable many useful features, such as `kubectl
explain` output (argoproj#8190),
[validating admission
policies](https://kubernetes.io/docs/reference/access-authn-authz/validating-admission-policy/)
(argoproj#13503), and
integration with tools like cdk8s
(argoproj#8532).

As explained at
argoproj#13754 (comment),
there's two issues that prevent us from using the full CRDs everywhere:
1. [Kubernetes size
   limits](kubernetes/kubernetes#82292) when
   using client-side apply. This is not a problem when using
   .
2. Wrong validation information due to kubebuilder limitations.

For the first issue, I verified that this is no longer an issue when
using [server-side
apply](https://kubernetes.io/docs/reference/using-api/server-side-apply/),
so I updated the `make install` target to use it. Since `kubectl apply --server-side`
isn't compatible with `kubectl apply --prune`, I had to switch to use
[apply
sets](https://kubernetes.io/docs/tasks/manage-kubernetes-objects/declarative-config/#alternative-kubectl-apply-f-directory-prune),
which is intended to replace `--prune`, and seems to work well.

For the second issue, I went through and added workarounds to
`hack/manifests/crds.go` for all the kubebuilder issues I could find.
Since the E2E test suite now uses the full CRDs, it should tell us if
there's anything I missed.

I didn't update the release manifests to use the full CRDs, since that's
a big change and we probably should wait awhile to make sure there
aren't any unexpected issues. Users can easily opt into the full
CRDs if they want.

Signed-off-by: Mason Malone <[email protected]>
@MasonM MasonM linked a pull request Jan 2, 2025 that will close this issue
Open
MasonM added a commit to MasonM/argo-workflows that referenced this issue Jan 2, 2025
@MasonM
fix(manifests): fix full CRDs and update tests to use them
57ba14a 
The full CRDs at `manifests/base/crds/full` are only intended for usage
in editors
(argoproj#11266 (comment))
and are unusable otherwise. Trying to install them will cause spurious
validation errors with nearly every workflow.

Having the full CRDs would enable many useful features, such as `kubectl
explain` output (argoproj#8190),
[validating admission
policies](https://kubernetes.io/docs/reference/access-authn-authz/validating-admission-policy/)
(argoproj#13503), and
integration with tools like cdk8s
(argoproj#8532).

As explained at
argoproj#13754 (comment),
there's two issues that prevent us from using the full CRDs everywhere:
1. [Kubernetes size
   limits](kubernetes/kubernetes#82292) when
   using client-side apply. This is not a problem when using
   .
2. Wrong validation information due to kubebuilder limitations.

For the first issue, I verified that this is no longer an issue when
using [server-side
apply](https://kubernetes.io/docs/reference/using-api/server-side-apply/),
so I updated the `make install` target to use it. Since `kubectl apply --server-side`
isn't compatible with `kubectl apply --prune`, I had to switch to use
[apply
sets](https://kubernetes.io/docs/tasks/manage-kubernetes-objects/declarative-config/#alternative-kubectl-apply-f-directory-prune),
which is intended to replace `--prune`, and seems to work well.

For the second issue, I went through and added workarounds to
`hack/manifests/crds.go` for all the kubebuilder issues I could find.
Since the E2E test suite now uses the full CRDs, it should tell us if
there's anything I missed.

I didn't update the release manifests to use the full CRDs, since that's
a big change and we probably should wait awhile to make sure there
aren't any unexpected issues. Users can easily opt into the full
CRDs if they want.

Signed-off-by: Mason Malone <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@MasonM MasonM

Labels
area/manifests P1 High priority. All bugs with >=5 thumbs up that aren’t P0, plus: Any other bugs deemed high priority type/bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(manifests): fix full CRDs and update tests to use them. Fixes #8532 MasonM/argo-workflows
4 participants
@MasonM @alexec @adrianord @sstaley-hioscar