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

The current CRD from mix bonny.gen.manifest doesn't work on later version of k8s #117

Closed
FreedomBen opened this issue Feb 8, 2022 · 19 comments · Fixed by #143
Closed

The current CRD from mix bonny.gen.manifest doesn't work on later version of k8s #117

FreedomBen opened this issue Feb 8, 2022 · 19 comments · Fixed by #143

Comments

@FreedomBen
Copy link
Contributor

FreedomBen commented Feb 8, 2022

The CRD v1beta1 has been removed from later versions of k8s (v1.20+). Format has changed from v1beta1 to v1 and it is incompatible, so some non-trivial changes are required.

For reference/convenience, here is the v1 spec.

I can work on this later this week. Once I get it working for my operator I'll port the changes back upstream to Bonny. Don't take that as a staking of territory! If someone else wants to take it I won't feel hurt/sad/whatever, just let me know so that we don't duplicate effort. (as always time is limited and needs to be invested wisely)

@mruoss
Copy link
Collaborator

mruoss commented Mar 16, 2022

Hey @FreedomBen, did you get amywhere with this?

@FreedomBen
Copy link
Contributor Author

FreedomBen commented Mar 16, 2022

@mruoss yes! My operator got re-prioritized for a couple of sprints due to unexpected important stuff, but I'm back on it as of yesterday. I've got most of the work done now, I'm just testing. Unless something unexpected happens, I think I'll have it ready for PR by the end of the week. If things went really well (which rarely happens to me in software lol) it might be ready for PR this evening (US timezone).

Do you need it now?

@mruoss
Copy link
Collaborator

mruoss commented Mar 16, 2022

oh, that's great! I'm keen to see your approach. Also feel free to open a "WIP" PR to start a discussion even if it's not ready to be merged...

@FreedomBen
Copy link
Contributor Author

Getting very close 🤞

@danhawkins
Copy link

I'm currently stuck with this issue using later version of k8s, the manifest generated will not work, we need to provide the open api spec right? What about using something like https://github.com/open-api-spex/open_api_spex

@mruoss
Copy link
Collaborator

mruoss commented Aug 20, 2022

@danhawkins We need to provide the CRD in its new format, including the spec yes. open_api_spex is used for creating APIs, no? Can you elaborate in how this lib could help us? Also have a look at the discussions in #9.

@FreedomBen your last post sounded so promising! What happened?

@sleipnir
Copy link

Any updates here? I'm also stuck in my Operator :(

@mruoss
Copy link
Collaborator

mruoss commented Aug 31, 2022

Hmm... it looks like Cory had already implemented something. Can anyone test this? What happens if you add this to your config.exs:

config :bonny,
  api_version: "apiextensions.k8s.io/v1"

Edit: See #101

@mruoss
Copy link
Collaborator

mruoss commented Aug 31, 2022

It would be nice ot there was a way to actually define the openapi schema...

@sleipnir
Copy link

sleipnir commented Aug 31, 2022

When use this option generated file use api_version specified in configuration.
But without the proper schema we still can't apply the resources in kubernetes:

minikube start
😄  minikube v1.26.1 on Debian 11.0
✨  Using the docker driver based on existing profile
👍  Starting control plane node minikube in cluster minikube
🚜  Pulling base image ...
🔄  Restarting existing docker container for "minikube" ...
🐳  Preparing Kubernetes v1.24.3 on Docker 20.10.17 ...
🔎  Verifying Kubernetes components...
    ▪ Using image gcr.io/k8s-minikube/storage-provisioner:v5
🌟  Enabled addons: storage-provisioner, default-storageclass

❗  /usr/local/bin/kubectl is version 1.21.3, which may have incompatibilites with Kubernetes 1.24.3.
    ▪ Want kubectl v1.24.3? Try 'minikube kubectl -- get pods -A'
🏄  Done! kubectl is now configured to use "minikube" cluster and "default" namespace by default
sleipnir @ pop-os spawn main 
└─ $ (k8s: minikube) 🚀 ▶ make apply-k8s-manifests 
kubectl -n eigr-functions apply -f apps/operator/manifest.yaml
deployment.apps/eigr-functions configured
error: error validating "apps/operator/manifest.yaml": error validating data: [ValidationError(CustomResourceDefinition.spec.versions[0]): unknown field "scheme" in io.k8s.apiextensions-apiserver.pkg.apis.apiextensions.v1.CustomResourceDefinitionVersion, ValidationError(CustomResourceDefinition.spec.versions[0]): missing required field "served" in io.k8s.apiextensions-apiserver.pkg.apis.apiextensions.v1.CustomResourceDefinitionVersion, ValidationError(CustomResourceDefinition.spec.versions[0]): missing required field "storage" in io.k8s.apiextensions-apiserver.pkg.apis.apiextensions.v1.CustomResourceDefinitionVersion]; if you choose to ignore these errors, turn validation off with --validate=false
make: *** [Makefile:81: apply-k8s-manifests] Error 1

Yaml generated:

apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  labels:
    eigr_functions_protocol_major_version: '0'
    eigr_functions_protocol_minor_version: '1'
    proxy_name: spawn
    k8s-app: eigr-functions-controller
  name: actorsystems.spawn.eigr.io
spec:
  group: spawn.eigr.io
  names:
    kind: ActorSystem
    plural: actorsystems
    shortNames:
      - as
      - actorsys
      - actorsystem
      - actorsystems
      - system
    singular: actorsystem
  scope: Cluster
  versions:
    - name: v1
      scheme:
        openAPIV3Scheme:
          additionalPrinterColumns:
            - description: Storage type of the Actor System
              jsonPath: .spec.storage.type
              name: storage
              type: string
            - description: |-
                CreationTimestamp is a timestamp representing the server time when this object was created. It is not guaranteed to be set in happens-before order across separate operations. Clients may not set this value. It is represented in RFC3339 form and is in UTC.
                
                      Populated by the system. Read-only. Null for lists. More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#metadata
              jsonPath: .metadata.creationTimestamp
              name: Age
              type: date

@mruoss
Copy link
Collaborator

mruoss commented Aug 31, 2022

Oh boy... seems like typos in #101. What if you manually change

  • scheme to schema
  • openAPIV3Scheme to openAPIV3Schema

No... still not... additionalPrinterColumns should not be part of the schema... this looks all wrong (still)

@sleipnir
Copy link

Oh boy... seems like typos in #101. What if you manually change

* `scheme` to `schema`

* `openAPIV3Scheme` to `openAPIV3Schema`

No... still not... additionalPrinterColumns should not be part of the schema... this looks all wrong (still)

Yes I know https://github.com/sleipnir/bonny/blob/811f50c60a8e41533ca562f4a8d3643b35dc1af3/lib/bonny/crd.ex#L172

@sleipnir
Copy link

These are the mistakes now:

Error from server (Invalid): error when creating "apps/operator/manifest.yaml": CustomResourceDefinition.apiextensions.k8s.io "activators.spawn.eigr.io" is invalid: [spec.versions: Invalid value: []apiextensions.CustomResourceDefinitionVersion{apiextensions.CustomResourceDefinitionVersion{Name:"v1", Served:false, Storage:false, Deprecated:false, DeprecationWarning:(*string)(nil), Schema:(*apiextensions.CustomResourceValidation)(nil), Subresources:(*apiextensions.CustomResourceSubresources)(nil), AdditionalPrinterColumns:[]apiextensions.CustomResourceColumnDefinition(nil)}}: must have exactly one version marked as storage version, spec.validation.openAPIV3Schema.type: Required value: must not be empty at the root, status.storedVersions: Invalid value: []string(nil): must have at least one stored version]
Error from server (Invalid): error when creating "apps/operator/manifest.yaml": CustomResourceDefinition.apiextensions.k8s.io "actornodes.spawn.eigr.io" is invalid: [spec.versions: Invalid value: []apiextensions.CustomResourceDefinitionVersion{apiextensions.CustomResourceDefinitionVersion{Name:"v1", Served:false, Storage:false, Deprecated:false, DeprecationWarning:(*string)(nil), Schema:(*apiextensions.CustomResourceValidation)(nil), Subresources:(*apiextensions.CustomResourceSubresources)(nil), AdditionalPrinterColumns:[]apiextensions.CustomResourceColumnDefinition(nil)}}: must have exactly one version marked as storage version, spec.validation.openAPIV3Schema.type: Required value: must not be empty at the root, status.storedVersions: Invalid value: []string(nil): must have at least one stored version]
Error from server (Invalid): error when creating "apps/operator/manifest.yaml": CustomResourceDefinition.apiextensions.k8s.io "actorsystems.spawn.eigr.io" is invalid: [spec.versions: Invalid value: []apiextensions.CustomResourceDefinitionVersion{apiextensions.CustomResourceDefinitionVersion{Name:"v1", Served:false, Storage:false, Deprecated:false, DeprecationWarning:(*string)(nil), Schema:(*apiextensions.CustomResourceValidation)(nil), Subresources:(*apiextensions.CustomResourceSubresources)(nil), AdditionalPrinterColumns:[]apiextensions.CustomResourceColumnDefinition(nil)}}: must have exactly one version marked as storage version, spec.validation.openAPIV3Schema.type: Required value: must not be empty at the root, status.storedVersions: Invalid value: []string(nil): must have at least one stored version]

@mruoss
Copy link
Collaborator

mruoss commented Aug 31, 2022

Opened a pull request (#143) where I try to fix this. But we don't have integration tests here yet. Need to test that first.

@sleipnir
Copy link

I think we created a PR to fix the same thing :D #144

@mruoss
Copy link
Collaborator

mruoss commented Aug 31, 2022

yup. Closed yours ;)

Edit: as mine also addresses some other problems.

@sleipnir
Copy link

What do you mean exactly?

Ignore my previous comment. I had taken a test wrong

@sleipnir
Copy link

@mruoss The new version works accordingly. I think there's enough material to spawn a new version of bonny now wdyt?

sleipnir @ pop-os spawn main 
└─ $ (k8s: minikube) 🚀 ▶ make apply-k8s-manifests 
kubectl -n eigr-functions apply -f apps/operator/manifest.yaml --validate=false
deployment.apps/eigr-functions configured
customresourcedefinition.apiextensions.k8s.io/activators.spawn.eigr.io created
customresourcedefinition.apiextensions.k8s.io/actornodes.spawn.eigr.io created
customresourcedefinition.apiextensions.k8s.io/actorsystems.spawn.eigr.io created
clusterrole.rbac.authorization.k8s.io/eigr-functions unchanged
serviceaccount/eigr-functions unchanged
clusterrolebinding.rbac.authorization.k8s.io/eigr-functions unchanged
sleipnir @ pop-os spawn main 
└─ $ (k8s: minikube) 🚀 ▶ make apply-k8s-manifests 
kubectl -n eigr-functions apply -f apps/operator/manifest.yaml
deployment.apps/eigr-functions configured
customresourcedefinition.apiextensions.k8s.io/activators.spawn.eigr.io unchanged
customresourcedefinition.apiextensions.k8s.io/actornodes.spawn.eigr.io configured
customresourcedefinition.apiextensions.k8s.io/actorsystems.spawn.eigr.io unchanged
clusterrole.rbac.authorization.k8s.io/eigr-functions unchanged
serviceaccount/eigr-functions unchanged
clusterrolebinding.rbac.authorization.k8s.io/eigr-functions unchanged
sleipnir @ pop-os spawn main 
└─ $ (k8s: minikube) 🚀 ▶ k get activators
No resources found
sleipnir @ pop-os spawn main 
└─ $ (k8s: minikube) 🚀 ▶ k get actors
No resources found
sleipnir @ pop-os spawn main 
└─ $ (k8s: minikube) 🚀 ▶ k get bla
error: the server doesn't have a resource type "bla"
sleipnir @ pop-os spawn main 
└─ $ (k8s: minikube) 🚀 ▶ 

@mruoss
Copy link
Collaborator

mruoss commented Aug 31, 2022

For people looking for a solution, the generated CRD should work now (since version 0.5.2), once you have added the following entry to your config.exs:

config :bonny,
  api_version: "apiextensions.k8s.io/v1"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants