Prevent definition of same gvk in custom resource configuration

[chrischdi]

I hope it is okay that I directly created a PR for this. I can also create an issue for that first if this is preferred 👍

What this PR does / why we need it:

This PR adjusts the options for custom resource configuration to allow defining them multiple times.
This allows to split potentially large configurations to multiple files.

It also adds validation to detect if the configuration includes multiple definitions for the same resource name.
This prevents the following unexpected behaviour:

Configuration:

kind: CustomResourceStateMetrics
spec:
  resources:
  - groupVersionKind:
      group: cluster.x-k8s.io
      kind: Cluster
      version: v1beta1
    metrics:
    - each:
        info: {}
        type: Info
      name: foo
  - groupVersionKind:
      group: cluster.x-k8s.io
      kind: Cluster
      version: v1beta1
    metrics:
    - each:
        info: {}
        type: Info
      name: bar

Expected result:

• either return an error and fail to start (like proposed in the PR)
• or the following metrics:

  # HELP kube_cluster_x_k8s_io_v1beta1_Cluster_bar
  # TYPE kube_cluster_x_k8s_io_v1beta1_Cluster_bar gauge
  kube_cluster_x_k8s_io_v1beta1_Cluster_bar 1
  kube_cluster_x_k8s_io_v1beta1_Cluster_bar 1
  # HELP kube_cluster_x_k8s_io_v1beta1_Cluster_foo
  # TYPE kube_cluster_x_k8s_io_v1beta1_Cluster_foo gauge
  kube_cluster_x_k8s_io_v1beta1_Cluster_foo 1
  kube_cluster_x_k8s_io_v1beta1_Cluster_foo 1
  

Actual result metrics:

# HELP kube_cluster_x_k8s_io_v1beta1_Cluster_bar
# TYPE kube_cluster_x_k8s_io_v1beta1_Cluster_bar gauge
kube_cluster_x_k8s_io_v1beta1_Cluster_bar 1
kube_cluster_x_k8s_io_v1beta1_Cluster_bar 1
# HELP kube_cluster_x_k8s_io_v1beta1_Cluster_bar
# TYPE kube_cluster_x_k8s_io_v1beta1_Cluster_bar gauge
kube_cluster_x_k8s_io_v1beta1_Cluster_bar 1
kube_cluster_x_k8s_io_v1beta1_Cluster_bar 1

How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality)

No changes to cardinality

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

[dgrisonnet]

I have a feeling that this will lead to some unexpected behavior. It is always safer to override the data completely to avoid having data leaking from past configurations.

Generally speaking, I am not a fan of complexifying how configuration is consumed. Reading a configuration file should be really straightforward for a software. I'd rather have the configuration file generated from a data templating language than adding additional complexity to the code.

[chrischdi]

I have a feeling that this will lead to some unexpected behavior. It is always safer to override the data completely to avoid having data leaking from past configurations.

Generally speaking, I am not a fan of complexifying how configuration is consumed. Reading a configuration file should be really straightforward for a software. I'd rather have the configuration file generated from a data templating language than adding additional complexity to the code.

Seems to be a fair point.

Where I was coming from was: using kustomize to provision kube-state-metrics by using the official helm-chart of kube-state-metrics. When using kustomize I would then have to use some other pre-processing to create the single CRD configuration (e.g. maybe via helm?). My first thought was about having a single config file for each CR I want to produce metrics for and use a single configmap which has multiple files for providing them to the pod.

However: I'm ok to drop that cli parts 👍

At least the check in https://github.com/kubernetes/kube-state-metrics/pull/1810/files#diff-bf91d16f16c99a97ad09eb6587f27d8fc075a68825d396e035c8100f864a7582R174-R176 should still get added to the FromConfig to prevent misconfigurations (duplicates) when using a single file.

[mrueg]

Would having CustomResourceStateMetrics CRDs and a way for kube-state-metrics to discover them, solve this problem as well? This might make things way more complex but would allow other deployments to ship their own CRSM config.

At least the check in https://github.com/kubernetes/kube-state-metrics/pull/1810/files#diff-bf91d16f16c99a97ad09eb6587f27d8fc075a68825d396e035c8100f864a7582R174-R176 should still get added to the FromConfig to prevent misconfigurations (duplicates) when using a single file.

👍

[chrischdi]

Would having CustomResourceStateMetrics CRDs and a way for kube-state-metrics to discover them, solve this problem as well? This might make things way more complex but would allow other deployments to ship their own CRSM config.

At least the check in https://github.com/kubernetes/kube-state-metrics/pull/1810/files#diff-bf91d16f16c99a97ad09eb6587f27d8fc075a68825d396e035c8100f864a7582R174-R176 should still get added to the FromConfig to prevent misconfigurations (duplicates) when using a single file.

👍

Yes, that even seems to be the best method to provide the configuration 👍

[chrischdi]

Updated to only prevent the configuration issue.

[fpetkovski]

Thanks, just a small nit

[mrueg]

/approve

[fpetkovski]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrischdi, fpetkovski, mrueg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [fpetkovski,mrueg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[sbueringer]

Would having CustomResourceStateMetrics CRDs and a way for kube-state-metrics to discover them, solve this problem as well? This might make things way more complex but would allow other deployments to ship their own CRSM config.

@mrueg @chrischdi Would it make sense to track this in a new issue? If I saw correctly, it's currently not tracked in an issue.