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

Custom Schema for Property of Spec #396

Open
wasabii opened this issue Feb 22, 2022 · 15 comments
Open

Custom Schema for Property of Spec #396

wasabii opened this issue Feb 22, 2022 · 15 comments
Labels
enhancement New feature or request

Comments

@wasabii
Copy link

wasabii commented Feb 22, 2022

I have a need to specify custom open API schema for a specific property on a spec property. It cannot be generated from C# code, but I need to directly specify some OpenAPI schema.

Can the CRD generator do this?

@wasabii wasabii added the enhancement New feature or request label Feb 22, 2022
@erin-allison
Copy link
Contributor

At present, that capability is not designed into the CRD generation code.

Could you provide an example of what you're trying to define that wouldn't be able to be created using the existing attributes?

@wasabii
Copy link
Author

wasabii commented Feb 22, 2022

I'm trying to import an entire existing OpenAPI JSON schema, which is like, 9000 different sub schemas. Long story.

That said, I can say right off the bat it doesn't seem to do OneOf based on polymorphism.

I'm going to end up turning off CRD generation for this entity and just doing it elsewhere.

@wasabii
Copy link
Author

wasabii commented Feb 22, 2022

For context, because it might be interesting. My project is to try to add an AzureResource, AzureSubscriptionResource, AzureTenantResource and AzureManagementGroupResource CRD and operator, that spins up Azure resources by template, monitors their status, ensures they're up to date, deletes them when the CRD goes away. It's an experiment to see if I can get it to work, and whether it's useful.

Because declaring requirements on Azure resources (storage account, SQL server, Service Bus, etc) and managing them through K8s seems like it might be an interesting devops approach.

Basically, because I care, I want to import the entire Azure ARM REST API OpenAPI schema into the CRD. Hehe.

@erin-allison
Copy link
Contributor

That's an interesting use case.

I have a couple ideas for implementation that I'll write up later tonight and see if I can get @buehler to say which he prefers.

@erin-allison
Copy link
Contributor

This took a bit longer than planned, but my two thoughts are as follows.

First possibility would be to add an attribute CrdBodyAttribute that lets a developer define a constant value that will be used as the definition for that property.

This is very simple but wouldn't be great for something as large as the entire OpenAPI spec for Azure.

The other option would require some refactoring, but we could allow registering an IPropertyCrdMapper in the DI container which could be resolved and used as an alternative to the MapType method when creating a CRD.

private static V1JSONSchemaProps MapProperty(
PropertyInfo info,
IList<V1CustomResourceColumnDefinition> additionalColumns,
string jsonPath)
{
V1JSONSchemaProps props;
try
{
props = MapType(info.PropertyType, additionalColumns, jsonPath);
}
catch (Exception ex)
{
throw new CrdConversionException(
$@"During conversion of the property ""{info.Name}"" with type ""{info.PropertyType.Name}"", an error occured.",
ex);
}
var contextual = info.ToContextualProperty();

This could have a definition of something like below:

public interface IPropertyCrdMapper {

    // returns true if the specified property can be processed by this mapper
    public bool CanMapProperty(PropertyInfo info, string jsonPath);

    // build the schema definition for the given property
    public V1JSONSchemaProps MapProperty(PropertyInfo info, IList<V1CustomResourceColumnDefinition> additionalColumns, string jsonPath);

}

I'm pretty partial to the second solution for the flexibility it offers, though there will be a decent amount of refactoring to be done, as the code that's currently simple extension methods needs access to the DI container.

@buehler thoughts?

@wasabii
Copy link
Author

wasabii commented Feb 24, 2022

From a user's perspective, I would favor something like a [CrdSchemaProviderAttribute] or some such. Which specified a Type. Where that type implemented some interface (ICrdSchemaProvider), with a GetSchema method, which obtained the schema, and was passed some contextual information about the type and property being introspected.

I'm not sure how that integrates with ya'll's serialization/deserialization routine. Because, since we're talking arbitrary schemas attached to properties, may likewise require custom serialization logic. In fact, it might make sense to combine these two in some way. Witness the following object:

public class TestEntitySpec
{
   [CrdConverter(typeof(CustomEntitySpecContentConverter), typeof(CustomEntitySpecSchemaProvider))]
   public object Content { get;set; }
}

Two pieces of information are represented here. The "converter", which may have Read and Write methods (modeled after Newtonsoft), for taking an arbitrary data body and converting it into the real type that is loaded into the property (and back again), and a SchemaProvider, which may return an instance of JSchema, or some other object representing an OpenAPI schema.

The other option would be to simply build in serialization logic from something like JToken or ExpandoObject, and force the user to type the property that way, and deal with it dynamically. I dislike this.

I'd probably try to simply model this as closely as possible to how Newtonsoft JSON works. JsonConverterAttribute. You can put it on Types and Properties. You can also add it to the Converters collection of the JsonSerializer. Etc.

@erin-allison
Copy link
Contributor

So my thoughts shift to registering a converter in DI and/or operator configuration for a similar reason that EntityFramework doesn't expose those items as being able to be set by attribute:

  1. Doing so couples the data model very tightly to the application (the specific implementation of the schema provider and content converter is only relevant to KubeOps, and is not a generic description of the model, such as items like description would be)
  2. Only providing the type makes it so the converters/schema providers may be limited in function, as they can't as easily accept constructor parameters or other configuration that isn't defined as constant.

@wasabii
Copy link
Author

wasabii commented Feb 25, 2022

Well, EF does expose almost everything by attribute. KeyAttribute, ColumnAttribute, TableAttribute, ForeignKeyAttribute. So that's probably not the best example.

They are optional, and collected into the same runtime model as configuration by Fluent-syntax. So it merges nicely. You get to pick what makes the most sense to you.

Same as Newtonsoft, actually. You can use JsonProperty, JsonConverter, JsonObject, JsonArray, JsonIgnore, etc. But you can also add Converts to the JsonSettings.Converters property that's registered in DI, etc.

@erin-allison
Copy link
Contributor

Well, EF does expose almost everything by attribute. KeyAttribute, ColumnAttribute, TableAttribute, ForeignKeyAttribute. So that's probably not the best example.

They are optional, and collected into the same runtime model as configuration by Fluent-syntax. So it merges nicely. You get to pick what makes the most sense to you.

Yep; operative word there being almost. Items specific to how EF handles the data/schema are generally still fluent configuration only (see Value Conversions).

This would be in the same manner that KubeOps exposes details about the model as attributes (description, length, multipleof, etc) but, in my opinion, should keep that level of schema details separate.

Same as Newtonsoft, actually. You can use JsonProperty, JsonConverter, JsonObject, JsonArray, JsonIgnore, etc. But you can also add Converts to the JsonSettings.Converters property that's registered in DI, etc.

Yep; I agree that's a possibility, and you can use those attributes right now, as KubeOps calls Newtonsoft for most JSON serialization


Something else that I'm having to consider is that there is also work under consideration for slimming down the packages to allow people to export their data models. If we were to add this in as an attribute for you to specify a type converter, there would have to be a decent amount more framework exported in what is supposed to be the fairly minimal reference package.

With that in mind, regardless of how KubeOps ends up opinionated one way or another, I don't see an attribute as bringing significant benefit enough to complicate that reference package. This is compounded by the fact that as far as I can think, there isn't a real reason any other applications aside from the operator would need to access the schema details at that level.

@buehler
Copy link
Owner

buehler commented Feb 28, 2022

Hey @erin-allison and @wasabii.

If I understand the use case correctly, you want to define a custom schema for a property in an object or for an entire object. I read the conversation and tried to get my head around the different ideas. While I see the actual benefit of registering type mappers and converters via DI, I personally do not see the necessity right now. To tackle the problem at hand, a well-designed CustomSchemaAttribute would suffice. Such that a user of the SDK can define a custom schema for that part (property -> only that property, class -> that type), or even provide a path to a JSON file which contains the schema.

The other possibilities - while clever ideas - seem to add complexity that is currently not needed. Or do I interpret the problem at hand in the wrong way?

@erin-allison
Copy link
Contributor

No, I think we're on the same page about what the desired functionality is. I'll have to ponder how best to structure this so as to not make a mess when it comes time to work on #362 and #355 (comment).

I think what that will mean is there may not be a lot of helper functionality (provided by KubeOps) for those type converters. A good part of what I'm trying to avoid is a situation where a type converter / schema provider is built in a resources library and it ends up then forcing all consumers to pull in the entire KubeOps library just to access those few resource classes.

@buehler
Copy link
Owner

buehler commented Feb 28, 2022

I think what that will mean is there may not be a lot of helper functionality (provided by KubeOps) for those type converters. A good part of what I'm trying to avoid is a situation where a type converter / schema provider is built in a resources library and it ends up then forcing all consumers to pull in the entire KubeOps library just to access those few resource classes.

Good point. I tried to separate KubeOps into different libs and ended up using all entity stuff in such a .netstandard lib while nearly all other elements still reside in KubeOps.

Don't get me wrong, as soon as a real use case for custom schema providers/type-to-crd-converters arises, I'm down with it. But as far as I can judge this, it would be much more work to implement custom converters and stuff. I'm a fan of an MVP approach and "yagni" (you ain't gonna need it) ;-)

But I'm totally with you.

My approach would be: Create the mentioned custom crd scheme attribute. Further down the road when refactoring the library into multiple libraries, the crd scheme attribute still resides with the other attributes (where ever appropriate) and does not interfere with KubeOps itself. As soon as custom converters arise, an abstraction lib (like mentioned in the thread) shall contain the interfaces and other elements.

@wasabii
Copy link
Author

wasabii commented Feb 28, 2022

Not my project, but I agree with Erin's concern.

But I don't think the simple Attribute approach hurts it. The attribute, if it can live on the PROPERTY (not just the class), can be on the property, which resides in library #1, which depends on KubeOps, while the class that the property is typed on can live in a separate assembly.

In my case that Entity will be in my project, the Spec type will be in my project, and a property of the Spec type would be annotated to point to some schema. However, the property type would be JObject, which clearly isn't mine. Basically, you can pollute as much of the model as you feel comfortable polluting.

@wasabii
Copy link
Author

wasabii commented Feb 28, 2022

So, beyond the schema, is there some generic type that I can use in the Spec to represent "any object"? I tried JObject. No go. Tried "object", no go. Tried Dictionary<string, object>, ends up empty.

@wasabii
Copy link
Author

wasabii commented Feb 28, 2022

For reference, here's an example of how Newtonsoft wired it all together:

https://www.newtonsoft.com/jsonschema/help/html/GeneratingSchemas.htm

They allow you to implement one of these: https://www.newtonsoft.com/jsonschema/help/html/T_Newtonsoft_Json_Schema_Generation_JSchemaGenerationProvider.htm

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

No branches or pull requests

3 participants