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

Respect groupVersion when dynamically loading resource-type metadata from cluster API #139

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

tintoy
Copy link
Owner

@tintoy tintoy commented Sep 9, 2021

Currently, resource type metadata for the dynamic resource client ignores groupVersion (e.g. networking.istio.io/v1beta1) in favour of version (e.g. v1beta1) when loading resource type metadata from the k8s API (loading from type annotations on model metadata works correctly however).

The end result of this is that resource types that have a groupVersion rather than just a version (i.e. version != groupVersion) don't seem to work correctly with the dynamic client.

I think the original idea was to enable consumers to specify kind and apiVersion without having to specify group as well (i.e. kind and apiGroupVersion) but this was probably due to a lack of understanding (on my part, I suspect) of the semantics of API group and version.

I have some mild concerns that this may constitute a breaking change but considering that the existing behaviour was both wrong and non-functional I think I am OK with that 🙂

PS. @felixfbecker - figured you might care since the powershell stuff was one of the original consumers of this functionality 🙂

Relates to #114 and #138

@tintoy tintoy added the enhancement New feature or request label Sep 9, 2021
@tintoy tintoy self-assigned this Sep 9, 2021
@linjmeyer
Copy link

Hey @tintoy I'm unfortunately still getting the same error when using this branch (#138)

@tintoy
Copy link
Owner Author

tintoy commented Sep 10, 2021

Ok, I’ll have a look this morning and see if I can figure out what’s going on.

@tintoy
Copy link
Owner Author

tintoy commented Sep 10, 2021

Just to confirm - you’re calling ApiMetadata.Load() to get metadata from the cluster API before trying to call List()?

@linjmeyer
Copy link

Ah I wasn't, but with that added I am getting a different error for both the custom resource and HorizontalPodAutoscalers which was previously working:

HPA:

Cannot find resource API for kind 'HorizontalPodAutoscaler  apiVersion 'autoscaling/v1

Istio CRD:

Cannot determine the list model type that corresponds to networking.istio.io/v1beta1/DestinationRule.

I've tried tweaking a few things, like not passing in the API group in the version (ie just v1beta1) to the annotations and List call, same result in all cases. Interesting this does work tho:

var cached = _kubeClient.ApiMetadata.Get(ResourceKind, ResourceApi);
// ApiMetadata has 400+ resources when using breakpoints, so the data seems to be there

Any ideas? Thanks again for the time, no real rush on this.

Istio classes (tried using networking.istio.io/v1beta1 and just v1beta1)

[KubeListItem("DestinationRule", "networking.istio.io/v1beta1")]
[KubeObject("DestinationRuleList", "networking.istio.io/v1beta1")]
public class DestinationRuleListV1 : KubeResourceListV1<DestinationRuleV1>
{
    public override List<DestinationRuleV1> Items { get; }
}

---

[KubeObject("DestinationRule", "networking.istio.io/v1beta1")]
[KubeApi(KubeAction.List, "apis/networking.istio.io/v1beta1/destinationrules")]
[KubeApi(KubeAction.List, "apis/networking.istio.io/v1beta1/namespaces/{namespace}/destinationrules")]
[KubeApi(KubeAction.Get, "apis/networking.istio.io/v1beta1/destinationrules/status")]
[KubeApi(KubeAction.Get, "apis/networking.istio.io/v1beta1/namespaces/{namespace}/destinationrules/status")]
public class DestinationRuleV1 : KubeResourceV1
{
    
}

@tintoy
Copy link
Owner Author

tintoy commented Sep 10, 2021

The error Cannot determine the list model type that corresponds to networking.istio.io/v1beta1/DestinationRule is saying that the client has received a response from the K8s API but doesn't know how to deserialise it (because the client doesn't know what CLR type to use as the deserialisation target). So declaring your type (as you've done above) is the right approach, you just need to tell the client where your model type lives:

clientOptions.ModelTypeAssemblies.Add(
        typeof(DestinationRuleV1).Assembly
);

@tintoy
Copy link
Owner Author

tintoy commented Sep 10, 2021

To be honest, I'm not entirely happy with how the dynamic resource client works; it was originally built to support the Powershell provider (where specifying type info is more painful) and has plenty of room for improvement in terms of ergonomics :)

Philosophically speaking, KubeClient is mainly designed for use with strongly-typed models and clients and I suspect what has led you to the dynamic client is that there is no client defined for the Istio resource types (feel free to correct me if I'm wrong on this).

The way the library was originally envisioned, you'd extend the client by creating some models and an additional KubeResourceClient implementation (see https://github.com/tintoy/dotnet-kube-client#extensibility for an example of this). And possibly even package that up as a library called KubeClient.Extensions.Istio (if you're feeling particularly exuberant 😉).

But perhaps the dynamic client provides a halfway point in terms of investment of effort so it might be worth refining its ergonomics for this sort of use case.

I'll keep thinking about that 🙂

@linjmeyer
Copy link

ahh got it, yeah this was a while ago I think there were no statically typed options for HPA v2 so I ended up using the dynamic client. I forget exactly as it was a while ago, but this is a new but similar use case for Istio resources so I started with the same approach. I took a look at the docs you linked and switched to the statically typed approach, may as well since I need to declare classes for the CRD resources anyway. It's working well now, so thank you for the help and suggestions!

@tintoy
Copy link
Owner Author

tintoy commented Sep 13, 2021

Great, glad to hear it! 🙂

I’ll see what I can do to improve that error message, BTW - it doesn’t really give a good indication of how to resolve the problem.

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

Successfully merging this pull request may close these issues.

2 participants