-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
build: don't minimize CRD for WorkflowEventBinding
and WorkflowArtifactGCTask
. Fixes #12166
#13754
Conversation
…tifactGCTask. Fixes argoproj#12166 The CRDs under `manifests/base/crds/minimal/` are minimized as a workaround for kubernetes/kubernetes#82292. However, `WorkflowEventBinding` and `WorkflowArtifactGCTask` are small enough that minimization isn't necessary, and allows using `kubectl explain` to view the spec. <details> <summary>Click for <code>kubectl explain --recursive WorkflowEventBinding</code></summary> ``` $ kubectl explain --recursive WorkflowEventBinding GROUP: argoproj.io KIND: WorkflowEventBinding VERSION: v1alpha1 DESCRIPTION: <empty> FIELDS: apiVersion <string> kind <string> metadata <ObjectMeta> -required- annotations <map[string]string> creationTimestamp <string> deletionGracePeriodSeconds <integer> deletionTimestamp <string> finalizers <[]string> generateName <string> generation <integer> labels <map[string]string> managedFields <[]ManagedFieldsEntry> apiVersion <string> fieldsType <string> fieldsV1 <FieldsV1> manager <string> operation <string> subresource <string> time <string> name <string> namespace <string> ownerReferences <[]OwnerReference> apiVersion <string> -required- blockOwnerDeletion <boolean> controller <boolean> kind <string> -required- name <string> -required- uid <string> -required- resourceVersion <string> selfLink <string> uid <string> spec <Object> -required- event <Object> -required- selector <string> -required- submit <Object> arguments <Object> artifacts <[]Object> archive <Object> none <Object> tar <Object> compressionLevel <integer> zip <Object> archiveLogs <boolean> artifactGC <Object> podMetadata <Object> annotations <map[string]string> labels <map[string]string> serviceAccountName <string> strategy <string> artifactory <Object> passwordSecret <Object> key <string> -required- name <string> optional <boolean> url <string> -required- usernameSecret <Object> key <string> -required- name <string> optional <boolean> azure <Object> accountKeySecret <Object> key <string> -required- name <string> optional <boolean> blob <string> -required- container <string> -required- endpoint <string> -required- useSDKCreds <boolean> deleted <boolean> from <string> fromExpression <string> gcs <Object> bucket <string> key <string> -required- serviceAccountKeySecret <Object> key <string> -required- name <string> optional <boolean> git <Object> branch <string> depth <integer> disableSubmodules <boolean> fetch <[]string> insecureIgnoreHostKey <boolean> passwordSecret <Object> key <string> -required- name <string> optional <boolean> repo <string> -required- revision <string> singleBranch <boolean> sshPrivateKeySecret <Object> key <string> -required- name <string> optional <boolean> usernameSecret <Object> key <string> -required- name <string> optional <boolean> globalName <string> hdfs <Object> addresses <[]string> dataTransferProtection <string> force <boolean> hdfsUser <string> krbCCacheSecret <Object> key <string> -required- name <string> optional <boolean> krbConfigConfigMap <Object> key <string> -required- name <string> optional <boolean> krbKeytabSecret <Object> key <string> -required- name <string> optional <boolean> krbRealm <string> krbServicePrincipalName <string> krbUsername <string> path <string> -required- http <Object> auth <Object> basicAuth <Object> passwordSecret <Object> key <string> -required- name <string> optional <boolean> usernameSecret <Object> key <string> -required- name <string> optional <boolean> clientCert <Object> clientCertSecret <Object> key <string> -required- name <string> optional <boolean> clientKeySecret <Object> key <string> -required- name <string> optional <boolean> oauth2 <Object> clientIDSecret <Object> key <string> -required- name <string> optional <boolean> clientSecretSecret <Object> key <string> -required- name <string> optional <boolean> endpointParams <[]Object> key <string> -required- value <string> scopes <[]string> tokenURLSecret <Object> key <string> -required- name <string> optional <boolean> headers <[]Object> name <string> -required- value <string> -required- url <string> -required- mode <integer> name <string> -required- optional <boolean> oss <Object> accessKeySecret <Object> key <string> -required- name <string> optional <boolean> bucket <string> createBucketIfNotPresent <boolean> endpoint <string> key <string> -required- lifecycleRule <Object> markDeletionAfterDays <integer> markInfrequentAccessAfterDays <integer> secretKeySecret <Object> key <string> -required- name <string> optional <boolean> securityToken <string> useSDKCreds <boolean> path <string> raw <Object> data <string> -required- recurseMode <boolean> s3 <Object> accessKeySecret <Object> key <string> -required- name <string> optional <boolean> bucket <string> caSecret <Object> key <string> -required- name <string> optional <boolean> createBucketIfNotPresent <Object> objectLocking <boolean> encryptionOptions <Object> enableEncryption <boolean> kmsEncryptionContext <string> kmsKeyId <string> serverSideCustomerKeySecret <Object> key <string> -required- name <string> optional <boolean> endpoint <string> insecure <boolean> key <string> region <string> roleARN <string> secretKeySecret <Object> key <string> -required- name <string> optional <boolean> sessionTokenSecret <Object> key <string> -required- name <string> optional <boolean> useSDKCreds <boolean> subPath <string> parameters <[]Object> default <string> description <string> enum <[]string> globalName <string> name <string> -required- value <string> valueFrom <Object> configMapKeyRef <Object> key <string> -required- name <string> optional <boolean> default <string> event <string> expression <string> jqFilter <string> jsonPath <string> parameter <string> path <string> supplied <Object> metadata <Object> workflowTemplateRef <Object> -required- clusterScope <boolean> name <string> ``` </details> Signed-off-by: Mason Malone <mmalone@adobe.com>
WorkflowEventBinding
and WorkflowArtifactGCTask
. Fixes #12166WorkflowEventBinding
and WorkflowArtifactGCTask
. Fixes #12166
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.
However,
WorkflowEventBinding
andWorkflowArtifactGCTask
are small enough that minimization isn't necessary.
For now. I think it would be good to add a size check in the script so there are no surprises in the future
Signed-off-by: Mason Malone <mmalone@adobe.com>
@agilgur5 Thanks for the review! I added additional safeguards in a2ca080. I also discovered there's a secondary issue preventing us from generating the full CRDs for
I think this is related to the kubebuilder issues mentioned at #3809 (comment) |
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.
LGTM. I like the two safeguards too
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 <651224+MasonM@users.noreply.github.com>
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 <651224+MasonM@users.noreply.github.com>
Fixes #12166. Partial fix for #8190
Motivation
The CRDs under
manifests/base/crds/minimal/
are minimized as a workaround for kubernetes/kubernetes#82292. However,WorkflowEventBinding
andWorkflowArtifactGCTask
are small enough that minimization isn't necessary. Including the full CRD enables users to usekubectl explain
to explore the CRD definitions.Modifications
removeCRDValidation()
tominimizeCRD()
so it more accurately describes what it's doing and add commentsminimizeCRD()
to only minimizecronworkflows.argoproj.io
,clusterworkflowtemplates.argoproj.io
,workflows.argoproj.io
,workflowtemplates.argoproj.io
,workflowtasksets.argoproj.io
, since the others are small enough they don't need to be minimizedmanifests/base/crds/full/README.md
andmanifests/base/crds/minimal/README.md
Verification
Ran
make start
and verifiedkubectl explain
returns spec details for the CRDs:Click here for
kubectl explain --recursive WorkflowEventBinding
outputClick here for
kubectl explain --recursive WorkflowArtifactGCTask
output