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.

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

feat: shortName for the CRDs. #896

Merged
merged 1 commit into from
Oct 2, 2024
Merged

feat: shortName for the CRDs. #896

merged 1 commit into from
Oct 2, 2024

Conversation

jvanz
Copy link
Member

@jvanz jvanz commented Sep 26, 2024

Description

Adds shortName for the Kubewarden CRDs. This allows users to get Kubewarden types using acronymous and making the commands smaller to type.

@kubewarden/kubewarden-developers I'm creating this PR as draft because I would like to investigate why controller-gen is not updating the CRDs with the shortNames for some CRDs. However, I would like to know if you are fine with the proposed shortNames. I've prefixed all of them with k to flag this is a kubewarden resource.

Copy link

codecov bot commented Sep 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.65%. Comparing base (5a4df6b) to head (815b486).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #896      +/-   ##
==========================================
+ Coverage   70.53%   70.65%   +0.11%     
==========================================
  Files          30       30              
  Lines        2559     2559              
==========================================
+ Hits         1805     1808       +3     
+ Misses        584      582       -2     
+ Partials      170      169       -1     
Flag Coverage Δ
integration-tests 60.43% <ø> (+0.11%) ⬆️
unit-tests 39.21% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@viccuad viccuad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, I'm happy with the shorNames selected.

@viccuad
Copy link
Member

viccuad commented Sep 27, 2024

On second thought, I wonder if there's a possibility to have them pronounceable.
Maybe taking out the "k" makes them more pronounceable: ap, cap, apg (apgroup), capg (capgroup), ps. It also makes it easier to "white label" them.

Copy link
Contributor

@fabriziosestito fabriziosestito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love this! looking forward

@kkaempf kkaempf added this to the 1.18 milestone Sep 27, 2024
@jvanz jvanz force-pushed the shortnames branch 3 times, most recently from 51f93fb to 2b5f5fe Compare September 27, 2024 18:32
@@ -77,12 +77,13 @@ type ClusterAdmissionPolicySpec struct {
// ClusterAdmissionPolicy is the Schema for the clusteradmissionpolicies API
// +kubebuilder:object:root=true
// +kubebuilder:subresource:status
// +kubebuilder:resource:scope=Cluster
// +kubebuilder:resource:scope=Cluster,shortName=cap
Copy link
Member Author

@jvanz jvanz Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kubewarden/kubewarden-developers I've needed to update v1alpha2 CRDs because controller-gen change the order of it process the files to extract the kubebuilder markers each time it runs. Therefore, each time it runs the CRDs generation is different. When the v1alpha2 CRDs types are the last one to be processed, the shortNames are not define.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened an issue asking if this is expected or it is a bug.

Copy link
Member

@flavio flavio Sep 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we drop these alpha resources? They are not used since a long time...

CC @fabriziosestito, @viccuad

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on dropping the alpha resources.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thoug about that as well. I've even use the //+kubebuilder:skip to ignore the v1alpha2. But than I've wondered, what happens if someone is still using the alpha version? The conversion will happen out of the box for resources already installed? Because of that, I've decided to mark the alpha version as deprecated first. But if we can drop it with no issue, I prefer removing it as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need more care on how we will remove the CRDs. I've did the following test:

  1. Start clean cluster
  2. Install kubewarden-crds version 0.1.4 and kubewarden-controller version 0.4.6. These are versions before the CRDs v1 and Kubewarden v1.0.0 release
  3. Install a v1alpha2 policy. See below the policy used
  4. Upgrade to v1.0.0 of the crds and controller helm charts
  5. Upgrade to latest crds and controller helm charts
  6. Change my local kubewarden-crds helm chart removing the v1alpha2 version
  7. Try to upgrade the crds using my local helm chart

I can see this error:

Error: UPGRADE FAILED: cannot patch "clusteradmissionpolicies.policies.kubewarden.io" with kind CustomResourceDefinition: CustomResourceDefinition.apiextensions.k8s.io "clusteradmissionpolicies.policies.kubewarden.io" is invalid: status.storedVersions[0]: Invalid value: "v1alpha2": must appear in spec.versions

Therefore, even if the policies have been migrated to v1 during the upgrade path. The storedVersion fields is still telling that we have v1alpha2 installed. This is the field description:

storedVersions lists all versions of CustomResources that were ever persisted. Tracking these versions allows a migration path for stored versions in etcd. The field is mutable so a migration controller can finish a migration to another version (ensuring no old objects are left in storage), and then remove the rest of the versions from this list. Versions may not be removed from spec.versions while they exist in this list.

Considering this documentation. I guess our controller needs to updates this field to allow the removal of the old CRD.

In case you want to setup a similar testing environment. This is the commands used to create a cluster with old kubewarden stack version:

minikube delete --all && \
minikube start && \
helm install --wait --namespace cert-manager --create-namespace	--set crds.enabled=true cert-manager jetstack/cert-manager && \
helm install --wait -n kubewarden --create-namespace kubewarden-crds kubewarden/kubewarden-crds --version 0.1.4 && \
helm install --wait -n kubewarden kubewarden-controller kubewarden/kubewarden-controller --version 0.4.6 && \
k8s apply -f policy.yaml && \
k8s get clusteradmissionpolicy pod-privileged -o yaml 

The policy definition:

apiVersion: policies.kubewarden.io/v1alpha2
kind: ClusterAdmissionPolicy
metadata:
  name: privileged-pods
spec:
  module: registry://ghcr.io/kubewarden/policies/pod-privileged:v0.3.2
  rules:
  - apiGroups: [""]
    apiVersions: ["v1"]
    resources: ["pods"]
    operations:
    - CREATE
    - UPDATE
  mutating: false

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering the previous comment. I suggest to keep the v1alpha2 for now, mark it as deprecated and create an issue to attack the old CRD removal.

@jvanz jvanz marked this pull request as ready for review September 27, 2024 18:39
@jvanz jvanz requested a review from a team as a code owner September 27, 2024 18:39
@jvanz jvanz mentioned this pull request Sep 30, 2024
@jvanz jvanz requested a review from flavio September 30, 2024 17:46
Adds shortName for the Kubewarden CRDs. This allows users to get
Kubewarden types using acronymous and making the commands smaller to
type.

Signed-off-by: José Guilherme Vanz <[email protected]>
@jvanz jvanz merged commit 436dbb4 into kubewarden:main Oct 2, 2024
9 checks passed
@jvanz jvanz deleted the shortnames branch October 2, 2024 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants