search

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
894 stars 226 forks source link

[WIP] Return a warning when the state-into-spec annotation will be defaulted to 'merge' #2323

Closed maqiuyujoyce closed 1 month ago

maqiuyujoyce commented 3 months ago

Change description

This is an experiment PR that checks the default value of state-into-spec annotation in the webhook via retrieving the ConfigConnector object and potentially the ConfigConnectorContext object, and returns a warning if the default value is merge.

In general, we shouldn't make API calls in the webhook because it has a short timeout (10s), though it should be fairly cheap to do the CC/CCC object retrievals.

Tests you have done

Ran make deploy-controller locally with basic CCC and CC objects (no spec.stateIntoSpec configuration), then applied a PubsubTopic resource (no state-into-spec annotation). Got the following warning as expected:

kubectl apply -f pubsubtopic.yaml 
Warning: 'cnrm.cloud.google.com/state-into-spec: merge' is unsupported for CRDs added in 1.114.0 and later. Use 'absent' instead. More details can be found at https://cloud.google.com/config-connector/docs/concepts/ignore-unspecified-fields.
pubsubtopic.pubsub.cnrm.cloud.google.com/pubsubtopic-sample-2 created
google-oss-prow[bot] commented 3 months ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please ask for approval from maqiuyujoyce. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
maqiuyujoyce commented 3 months ago

I think it's nice to have the warning in the webhook, though making API calls in the webhook seems to be violating the best practices and may have consequence about the performance and etc. IMO the risk is higher than the benefit, and it should be okay if we don't merge this PR (e.g. users don't receive warnings if their CRs are defaulted with state-into-spec: merge). @yuwenma WDYT?

maqiuyujoyce commented 1 month ago

Discussed offline and decided not to add this feature.