GoogleCloudPlatform / k8s-config-connector

GCP Config Connector, a Kubernetes add-on for managing GCP resources
https://cloud.google.com/config-connector/docs/overview
Apache License 2.0
883 stars 217 forks source link

Resources references' subfields and attributes are the same for all resources during CRD generations #1931

Open ziyue-101 opened 3 months ago

ziyue-101 commented 3 months ago

Checklist

Describe the feature or resource

The new workflow uses controller-gen https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/master/dev/tasks/generate-crds#L28 to generate CRDs from go structs(SOT).

However, the generated resources references are the same for all crds which use the shared references go struct. This would create breaking changes for some resources in which projectRef has a required Kind field that they used not have. It is also hard to customize individual resource on field attributes like descriptions.

For some resources like GKEHubFeatureMembership, KCC used to get reference's description from DCL/TF such as.

field: configmanagement.configSync.metricsGcpServiceAccountRef.external
Description: "The Email of the Google Cloud Service Account (GSA) used for exporting Config Sync metrics to Cloud Monitoring. The GSA should have the Monitoring Metric Writer(roles/monitoring.metricWriter) IAM role. The Kubernetes ServiceAccount `default` in the namespace `config-management-monitoring` should be bound to the GSA. Allowed value: The `email` field of an `IAMServiceAccount` resource."

However, after removing DCL/TF layer, KCC should store the descriptions in the go structs(SOT).

For the field above, the existing go struct reuses a common ResourceRef here, which makes it hard to customize attributes like field description for individual resources. If we use the shared references, the generated CRD would look like

    properties:
      external:
        description: The external name of the referenced resource
        type: string

Instead of

properties:
    external:
              description: |-
                The Email of the Google Cloud Service Account (GSA) used for exporting Config Sync metrics to Cloud Monitoring. The GSA should have the Monitoring Metric Writer(roles/monitoring.metricWriter) IAM role. The Kubernetes ServiceAccount `default` in the namespace `config-management-monitoring` should be bound to the GSA.
                Allowed value: The `email` field of an `IAMServiceAccount` resource.
              type: string

I can think of two approaches to address the limitation.

I am leaning towards the first option and if the team agrees, we should call out that part in the new resource guide.

Additional information

No response

Importance

No response

ziyue-101 commented 3 months ago

/cc @acpana @justinsb @yuwenma WDYT?

acpana commented 3 months ago

Make each resource has its own resource reference instead of sharing the same reference struct. This allows us to have customizations easily.

This may be closer to the final stage in that we use a references definition from a go struct that we control. I think you may be on the first reference out to a GSA account as a direct resource.

yuwenma commented 3 months ago

Two suggestions:

  1. It's great to explore all possible options and compare the pros and cons. Regarding the question you have, I'm also leaning towards a dedicated resolution for each resource ref. Here's an example that may unblock your case:

    • For projectRef, the PR supports two forms: one from generic resource ref, for backward compatibility with TF generated APIs, the other is a kind-specific reference.
    • For computenetworkRef, the PR gives an example that it re-defines the spec reference and validates/resolves the actual compute network at the beginning of the reconciliation. I think that may suite your case here.
  2. We only hit 1-2 resource reference use cases in the direct controller, I would encourage to build your resource reference that best suite the use case you are dealing with and do not bother to abstract a common pattern (yet).

BTW, we shall not refer internal codenames or links in oss.