search

TristanCacqueray / dhall-openshift

Typecheck, template and modularize your OpenShift definitions with Dhall
Apache License 2.0
8 stars 0 forks source link

ObjectMeta are not the same in dhall-kubernetes #6

Open PierreR opened 3 years ago

PierreR commented 3 years ago

https://github.com/dhall-lang/dhall-kubernetes/blob/master/1.17/types/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta.dhall and https://github.com/TristanCacqueray/dhall-openshift/blob/master/types/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta.dhall

are not the same object:

8a9,10
> , initializers :
>     Optional ./io.k8s.apimachinery.pkg.apis.meta.v1.Initializers.dhall
10,12d11
< , managedFields :
<     Optional
<       (List ./io.k8s.apimachinery.pkg.apis.meta.v1.ManagedFieldsEntry.dhall)

Is this something that could be fixed ?

Thanks

PierreR commented 3 years ago

Well you also get the same difference between https://kubernetes.github.io/cluster-registry/reference/build/index.html#objectmeta-v1 and https://v1-17.docs.kubernetes.io/docs/reference/generated/kubernetes-api/v1.17/#objectmeta-v1-meta

I suspect the definition from dhall-kubernetes or https://v1-17.docs.kubernetes.io/docs/reference/generated/kubernetes-api/v1.17/#objectmeta-v1-meta is the correct one.

TristanCacqueray commented 3 years ago

I guess this could be fixed by using a more recent openapi schema. For Openshift-3.x, the ObjectMeta definition doesn't have that managedFields attributes.

As mentioned in https://github.com/TristanCacqueray/dhall-openshift/issues/5 I'd like to support multiple versions, but we still need static openapi schemas. In the meantime, I've updated the README with instructions on how to use the new dhall-openapi package to generate the schemas using a custom openapi file.

PierreR commented 3 years ago

I have looked at 2 openshift-openapi-spec.json files:

  1. https://github.com/openshift/origin/blob/release-4.4/api/swagger-spec/openshift-openapi-spec.json
  2. oc get --raw /openapi/v2 (using an OpenShift v4.4 cluster)
PierreR commented 3 years ago

The documentation of OpenShift have definition that match the Kubernetes one: https://docs.openshift.com/container-platform/4.4/rest_api/objects/index.html#objectmeta-meta-v1

So these two are different:

Is there a better place than https://github.com/openshift/origin/blob/release-4.4/api/swagger-spec/openshift-openapi-spec.json to look for the openshift-openapi-spec.json file ?

Which source are you using ?

PierreR commented 3 years ago

Using dhall-openapi, I have compared the 2 generations

(1) https://github.com/openshift/origin/blob/release-4.4/api/swagger-spec/openshift-openapi-spec.json (2) oc get --raw /openapi/v2 (using an OpenShift v4.4 cluster)

The differences are huge ! The generation from (2) looks much online with the official documentation from openshift v4 My undestanding is that this repo is using -more or less- (1)

@TristanCacqueray I believe there is real risk of misunderstanding here ... Could the README be more explicit about what it is about such as :

TristanCacqueray commented 3 years ago

You are correct, the openapi-spec.json file used (and linked in the README) to generate the schemas is for OpenShift 3. I was hoping to find similar openapi spec files for OpenShift 4 in order to generate per version schemas (similarly to what dhall-kubernetes do), but that doesn't seem to be possible at the moment. Your suggestion to add a stronger warning sounds good to me.

However I wonder how usable is the schema you get from the /openapi/v2 endpoint, it seems to contains every single resources and that is likely too much for most use-case. Moreover it may contain conflicting object names that needs to be taken care of, for example see: https://github.com/dhall-lang/dhall-kubernetes/pull/120.

Perhaps a good solution would be to add a selection-list parameter to the dhall-openapi so that the user can filter the relevant resources, for example com.github.openshift.api.project.v1.Project, com.github.openshift.api.route.v1.Route and io.k8s.api.core. If I understand correctly, that is the preferred solution for performance improvement. @PierreR Would such feature be useful for your use-case?

PierreR commented 3 years ago

Perhaps a good solution would be to add a selection-list parameter to the dhall-openapi so that the user can filter the relevant resources, for example com.github.openshift.api.project.v1.Project, com.github.openshift.api.route.v1.Route and io.k8s.api.core. If I understand correctly, that is the preferred solution for performance improvement. @PierreR Would such feature be useful for your use-case?

@TristanCacqueray yes ! that is exactly what I would like to be able to do ;-)

PierreR commented 3 years ago

In the meanwhile I had wondered if we was already possible to filter the input when using oc get --raw /openapi/v2 but could not find out.

TristanCacqueray commented 3 years ago

Alright, that shouldn't be too hard to improve dhall-openapi, I'm currently opening an issue.

TristanCacqueray commented 3 years ago

@PierreR Let see if we can find a generalized solution upstream ^, otherwise I guess we just need to filter that list: https://github.com/dhall-lang/dhall-haskell/blob/715ba2a8dc2d8e6f19280a9a95863119442db312/dhall-openapi/openapi-to-dhall/Main.hs#L267-L271

PierreR commented 3 years ago

For information you are right about the performance issue:

With the bigger json from oc get --raw /openapi/v2:

dhall type --quiet --file ./openshift/package.dhall  119.10s user 2.09s system 99% cpu 2:01.73 totall

With your repo:

dhall type --quiet --file ./openshift/package.dhall  73.84s user 1.28s system 99% cpu 1:15.17 total

As a note, freeze as suggested in the README does not give you any performance boost comparing to freezing the single reference to openshift/package.dhall in the client code.

TristanCacqueray commented 3 years ago

I think that is expected with such a huge package and the new caching mechanism should improve the performace a lot.

On the other hand, such package is not very ergonomic since it contains everything in a flat list, and if we can reduce it to the set of relevant objects, it should be fast and easier to use with the existing implementation.