Merge pull request GoogleCloudPlatform#2610 from yuwenma/CR-validation

doc: add direct resource API validation guide

docs/archive/README.NewResourceFromTerraform.md

@@ -165,7 +165,7 @@ ServiceMappings file. Add the `ResourceConfig` for your resource:
 1.  Replace the value for `name` with the name of the Terraform resource
     associated with your GCP resource.
 
-1.  Replace the value for `kind` with the ConfigConnector `Kind` for your
+1.  Replace the value for `kind` with the Config Connector `Kind` for your
     resource. The name should be in pascal case, i.e. ServiceNameResourceName,
     but with acronyms in uppercase.
 

docs/develop-resources/README.md

@@ -1,6 +1,6 @@
 # Developing the Direct Resource Guide 
 
-We are thrilled to introduce the new way to add a ConfigConnector resource: the direct resource guide!  
+We are thrilled to introduce the new way to add a Config Connector resource: the direct resource guide!  
 
 In the past few months, we have made a tremendous amount of efforts to make adding a new resource (or a new field) much faster and more manageable, we have changed some key resource reconciliation processs to be more reliable and Kubernetes-native. What's more, we have made a revolutionary change to the test driven development and PR review process to improve the test coverage for every single field. 
 

docs/develop-resources/api-conventions/validations.md

@@ -0,0 +1,158 @@
+# Direct Resource API Validation
+
+
+# TL;DR
+
+Config Connector Direct Resource shall have each `spec` field validated.
+
+
+# Basic Rules
+
+Each Config Connector resource `spec` field shall be validated by at least one of the following approach:
+
+
+## 1. CRD validation
+
+* This throws standard OpenAPI validation errors without requiring the Config Connector controller to run.
+* This validation shall take the responsibility to give users a cheap and self-guiding check. It does not contain complicated logic. 
+
+
+## 2. Config Connector controller validation
+
+
+* This throws the Config Connector-defined errors in runtime.
+* This validation shall take the responsibility to guardrail config issues that otherwise could cause ambiguous or unexpected Config Connector behavior.
+* We shall build and publish a Config Connector error code table to future explain the issue and give a triage guide.
+
+
+## 3. GCP Service Validation
+
+
+* This throws GCP defined errors in runtime. Config Connector shall propagate the GCP response errors to the Config Connector object’s `status` field. 
+* This validation shall take the responsibility to guide users on fixing the issues due to GCP service requirements, like `SpannerInstance` has `spec.processingUnit` whose value can only increase but not decrease. 
+
+
+# CRD validation
+
+
+## Rule 1: Required/Optional
+
+Required field should use tag `+required`. 
+
+Optional field does not need the tag.
+
+
+```
+type CloudBuildWorkerPoolSpec struct {
+  // +required   
+  PrivatePoolConfig *PrivatePoolV1Config `json:"privatePoolV1Config,omitempty"`
+}
+```
+
+
+## Rule 2: Immutable field
+
+Immutable field should be validated via CEL rule using kubebuilder tag 
+
+
+```
+type PrivatePoolV1Config_NetworkConfigSpec struct {
+        // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="the field is immutable"
+        // Immutable. The network definition that the workers are peered
+        //  to. If this section is left empty, the workers will be peered to
+        //  `WorkerPool.project_id` on the service producer network.
+        PeeredNetworkRef refv1beta1.ComputeNetworkRef `json:"peeredNetworkRef,omitempty"`
+}
+```
+
+Note: 
+
+
+* Kubernetes supports CEL since 1.26 which is the oldest GKE cluster version (written at ). Using CEL shall not cause GKE cluster version issues, but we shall reevaluate this when the condition changes.
+* Some Config Connector resources use webhook to validate immutable fields. We can continue using that, but for external contributors CEL is much easier for self-learning. 
+
+We only apply CEL on immutable fields.** More complicated CEL validation requires further discussion.**
+
+One future improvement can be having generate-types detect the “Immutable.” comment and add the kubebuilder tag automatically. 
+
+
+## Rule 3: Parent
+
+Parent field should be required and immutable.
+
+
+### Required
+
+Suggest using the following `inline` struct
+
+
+```
+type CloudBuildWorkerPoolSpec struct {
+  // +required   
+  Parent `json:",inline"`
+}
+
+type Parent struct {
+    // +required      
+    ProjectRef *refv1beta1.ProjectRef `json:"projectRef"`
+
+    // +required
+    Location string `json:"location"`
+}
+```
+
+
+### Immutable
+
+Since most parent is a reference to another resource which can either be `<kind>Ref.external `or `<kind>Ref.name` , the immutable validation needs to be done at the controller validation level. For example, switching `<kind>Ref.external `and `<kind>Ref.name `are allowed if referring to the same resource.
+
+See this [resource reference guide](https://github.com/yuwenma/k8s-config-connector/blob/resource-ref/docs/develop-resources/resource-reference.md) (todo update once PR merged) for general rules;
+
+See this [external reference guide](https://github.com/yuwenma/k8s-config-connector/blob/real-id/docs/develop-resources/realID.md#statusexternalref-format) (todo update once PR merged) for parent validation with `status.externalRef`.   
+
+
+## Rule 4: No `anyOf`, `oneOf`, or` allOf`
+
+Short answer: Not now.
+
+Full answer: 
+
+OpenAPI supports `oneOf`, `anyOf` and `allOf`. Thus, in theory these should be the CRD level rules. The Direct Resource CRD is auto-generated by controller-gen, which only reads the kubebuilder tag rules. But the kubebuilder tags [do not (and most likely won’t)](https://github.com/kubernetes-sigs/controller-tools/issues/461) support `oneOf`, `anyOf` or `allOf`.
+
+To set this as CRD level rule, Config Connector shall build its own CRD transformer.  Ideally, this could be a Config Connector tag similar to kubebuilder, but this requires integrating with controller-gen which is a non-trivial amount of work. Other options like webhook, OpenAPI schema modifier are not self-explaining and easy enough to use, considering that Direct Resource shall be open to external contributors. Thus, Config Connector does not use CRD level validation for `oneOf`, `anyOf` and `allOf`.
+
+One future improvement can be having a Direct Resource tag that adds CRD level checks for   `oneOf`, `anyOf` or `allOf`.
+
+
+```
+// +Config Connector:validation:oneOf:serviceAttachmentRef,targetGRPCProxyRef,targetHTTPProxyRef,targetHTTPSProxyRef"
+type target struct {
+        ServiceAttachmentRef *refs.ComputeServiceAttachmentRef `json:"serviceAttachmentRef,omitempty"`
+
+        TargetGRPCProxyRef *refs.ComputeTargetGrpcProxyRef `json:"targetGRPCProxyRef,omitempty"`
+
+        TargetHTTPProxyRef *refs.ComputeTargetHTTPProxyRef `json:"targetHTTPProxyRef,omitempty"`
+
+        TargetHTTPSProxyRef *refs.ComputeTargetHTTPSProxyRef `json:"targetHTTPSProxyRef,omitempty"`
+}
+```
+
+
+
+# Controller validation
+
+Advanced Kubernetes contributors can use webhook to validate the resource CRs. 
+
+To easily ramp-up external contributors, we recommend using the controller-level validation instead. This validation mostly happens on Adapter [initialization](https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/91aac5186eb0aa2c5c3def94d7a7c79f948ac9a9/pkg/controller/direct/directbase/directbase_controller.go#L226), GCP object [creation](https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/91aac5186eb0aa2c5c3def94d7a7c79f948ac9a9/pkg/controller/direct/directbase/directbase_controller.go#L283) and [update](https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/91aac5186eb0aa2c5c3def94d7a7c79f948ac9a9/pkg/controller/direct/directbase/directbase_controller.go#L291) steps. 
+
+
+## Rule 1: Resource Reference
+
+Config Connector shall validate the resource reference based on the [resource reference guide](https://github.com/yuwenma/k8s-config-connector/blob/resource-ref/docs/develop-resources/resource-reference.md). 
+
+
+## Rule 2: Resource ID
+
+`spec.resourceID` is an optional and  immutable field. 
+
+Config Connector shall validate the `resourceID` (if present) matches the `status.externalRef` (if present) based on the  reconciliation 4 steps.

docs/develop-resources/guides/1-add-mockgcp-tests.md

@@ -4,10 +4,11 @@
 MockGCP test is required for the Direct resource development. 
 
 1. This is required no matter if the feature request is small or big. For example, both adding a single new field or adding an entire new resource require the MockGCP test coverage. If the MockGCP does not exist yet when adding a single field, developers should add the corresponding MockGCP coverage for the entire resource.  
-1. Each ConfigConnector `spec` field should be covered in both create and update(if applicable) cases. 
-1. MockGCP check is auto-enabled in Presubmit check and shall not be skipped. Code changes to a ConfigConnector resource must not merge if not covered by MockGCP check. 
+1. Each Config Connector `spec` field should be covered in both create and update(if applicable) cases. 
+1. MockGCP check is auto-enabled in Presubmit check and shall not be skipped. Code changes to a Config Connector resource must not merge if not covered by MockGCP check. 
 1. New PRs should give steady golden log output before they can merge in. Reasonable golden object and http log changes are okay and sometimes expected.
 1. MockGCP should be up-to-date when migrating resource from TF/DCL based controller to the Direct Controller
+1. Real GCP record should be uploaded to reflect and validate the change in scenarios like adding a new resource, migrating from TF or DCL based controller to the direct controller, adding a new field, etc. 
 
 ### Suggestion
 

docs/develop-resources/guides/2-define-apis.md

@@ -1,6 +1,6 @@
 # 2. Define API
 
-ConfigConnector builds the API using the Google Cloud Client proto.   
+Config Connector builds the API using the Google Cloud Client proto.   
 
 ## 2.1 Build the Google Cloud Proto (one-off step)
 
@@ -17,7 +17,7 @@ cd $REPO_ROOT/dev/tools/proto-to-mapper
 make generate-pb
 ```
 
-## 2.2 Generate the ConfigConnector Types (repeat-run safe) 
+## 2.2 Generate the Config Connector Types (repeat-run safe) 
 
 Run the following command
 
@@ -48,15 +48,15 @@ The apis directory to where to write the result to. Shall always be   $REPO_ROO
 
 * `--kind`
 
-The ConfigConnector resource kind, camel case. Normally it should contain the service name for example `SpannerInstance`, `SQLInstance`.
+The Config Connector resource kind, camel case. Normally it should contain the service name for example `SpannerInstance`, `SQLInstance`.
 
 * `--proto-source`
 
 The proto name of the resource, you can find them in [https://github.com/googleapis/googleapis.git](https://github.com/googleapis/googleapis.git). For example, the SQLInstance is named `instance` under [https://github.com/googleapis/googleapis/tree/master/google/cloud/sql/v1beta4](https://github.com/googleapis/googleapis/tree/master/google/cloud/sql/v1beta4). The proto-source should be `instance` instead of `SQLInstance`
 
 * `--api-version`
 
-The ConfigConnector apiVersion value, shall be <code>&lt;service>.[cnrm.cloud.google.com/](http://cnrm.cloud.google.com/)&lt;version></code>, where the generated file will be placed under<code>$REPO_ROOT/apis/&lt;service>/&lt;version></code> (if the dir does not exist, the command will create one).  
+The Config Connector apiVersion value, shall be <code><service>.[cnrm.cloud.google.com/](http://cnrm.cloud.google.com/)<version></code>, where the generated file will be placed under<code>$REPO_ROOT/apis/<service>/<version></code> (if the dir does not exist, the command will create one).  
 
 
 ## 2.3 Generate CRD (repeat-run safe)
@@ -81,7 +81,7 @@ cd $REPO_ROOT
 
 For scenarios #1 and #2, we accept breaking changes. So it is fine if the CRD outcome in 2.3 is different from the already released CRDs.
 
-For scenarios #3, ConfigConnector has to be backward compatible. So we have to keep the CRD **existing** spec and status the same.
+For scenarios #3, Config Connector has to be backward compatible. So we have to keep the CRD **existing** spec and status the same.
 
 * The API field name must **not** change. You need to manually modify the field if it’s changed. 
 * The field go comment is the CRD’s field description, this can be modified.
@@ -92,15 +92,15 @@ According to the above principles, you shall decide how to process the following
 
 * Add the parent field and mark as required
 
-    * See detailed [requirements and example](https://github.com/yuwenma/k8s-config-connector/blob/CR-validation/docs/develop-resources/api-validations.md#rule-3-parent)  (TODO: update link)
+    * See detailed [requirements and example](../api-conventions/validations.md#rule-3-parent)  (TODO: update link)
 
 * Replace any field that is a resource reference to `<resource>Ref `and add the resource to `./apis/refs` if not exist.` `
 
     * See [direct resource reference validation rules](https://github.com/yuwenma/k8s-config-connector/blob/resource-ref/docs/develop-resources/resource-reference.md)  (TODO: update link). This validation shall consider the actual resource field’ usage and can be done in follow-up PRs 
 
 * Add **non** output-only fields to `spec`, excluding imperative fields.
 
-* Add other CR validations according to the [Direct Resource API Validation Guide](https://github.com/yuwenma/k8s-config-connector/blob/CR-validation/docs/develop-resources/api-validations.md) (TODO: update link)
+* Add other CR validations according to the [Direct Resource API Validation Guide](../api-conventions/validations.md)
 
 * (Only for TF/DCL Beta) Existing `spec` and `status` fields should still be there, except the [output-only spec](https://paste.googleplex.com/4694303066030080) should be removed. 
 

docs/develop-resources/guides/4-add-controller.md

@@ -16,7 +16,7 @@ The controller template has implemented the model interface` find, create, updat
 
 ## 4.2 Resolve resource references
 
-Most ConfigConnector resource need references like `spec.projectRef. `You should add those references in `AdapterForObject` using functions `Resolve<RefResource>`
+Most Config Connector resource need references like `spec.projectRef. `You should add those references in `AdapterForObject` using functions `Resolve<RefResource>`
 
 if there is no previous reference method, You may need to add a new` Resolve<RefResource> `
 
@@ -25,7 +25,7 @@ Check  to make sure your validation is complete.
 
 ## 4.3 Register your controller
 
-To wire your controller in the ConfigConnector operator, you need to register the controller [here](https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/master/pkg/controller/direct/register/register.go)
+To wire your controller in the Config Connector operator, you need to register the controller [here](https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/master/pkg/controller/direct/register/register.go)
 
 
 ## 4.4 Verify your controller

docs/develop-resources/scenarios/alpha-to-beta.md

@@ -7,5 +7,4 @@ If the resource is TF-based or DCL-based, you shall migrate them to [the Direct
 
 ## From Direct Alpha to Beta
 
-If the resource is already a Direct resource, you can follow [step 5.2](https://github.com/yuwenma/k8s-config-connector/blob/scifi-guide/docs/develop-resources/guides/5-releases.md#52-bump-from-v1alpha1-to-v1beta1)  (TODO: update the link)
- and submit a single PR 
+If the resource is already a Direct resource, you can follow [step 5.2](../guides/5-releases.md#52-bump-from-v1alpha1-to-v1beta1) and submit a single PR 

docs/develop-resources/scenarios/migrate-tf-resource-alpha.md

@@ -2,7 +2,7 @@
 
 ## Add MockGCP tests
 
-Follow [Step 1](https://github.com/yuwenma/k8s-config-connector/blob/scifi-guide/docs/develop-resources/guides/1-add-mockgcp-tests.md)
+Follow [Step 1](../guides/1-add-mockgcp-tests.md)
 
 1. The 1st PR should set the `create.yaml `and  `update.yaml `fields the same value for both test suites, with `_http.log `telling the matching HTTP request/response, and `_generated_object_<resource>.golden.yaml` telling the output-only fields. It shall record against real GCP 
 
@@ -17,7 +17,7 @@ Follow [Step 1](https://github.com/yuwenma/k8s-config-connector/blob/scifi-guide
 
 ## Add API
 
-Follow [Step 2](https://github.com/yuwenma/k8s-config-connector/blob/scifi-guide/docs/develop-resources/guides/2-define-apis.md)
+Follow [Step 2](../guides/2-define-apis.md)
 
 The PR shall contain the types and deepcopy codes. It shall follow the Direct resource recommended styles and conventions. (TODO add the link) **It can change the existing fields since this is a Alpha resource**.
 
@@ -31,13 +31,13 @@ The PR shall contain the types and deepcopy codes. It shall follow the Direct re
 
 ## Add the mapper
 
-Follow [Step 3](https://github.com/yuwenma/k8s-config-connector/blob/scifi-guide/docs/develop-resources/guides/3-add-mapper.md)
+Follow [Step 3](../guides/3-add-mapper.md)
 
 This PR adds the Direct mapper. You can do this together with the previous step or the next step if no additional manual changes are needed.
 
 ## Add the controller 
 
-Follow [Step 4](https://github.com/yuwenma/k8s-config-connector/blob/scifi-guide/docs/develop-resources/guides/4-add-controller.md).
+Follow [Step 4](../guides/4-add-controller.md).
 
 * Use the `KCC_USE_DIRECT_RECONCILERS` flag [exampe](https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/0bbac86ace6ab2f4051b574f026d5fe47fa05b75/dev/tasks/run-e2e#L27). This will override the tf2crd and dcl2crd label to force using the Direct controller. 
 

docs/develop-resources/scenarios/migrate-tf-resource-beta.md

@@ -2,7 +2,7 @@
 
 ## Add MockGCP tests
 
-Follow [Step 1](https://github.com/yuwenma/k8s-config-connector/blob/scifi-guide/docs/develop-resources/guides/1-add-mockgcp-tests.md)
+Follow [Step 1](../guides/1-add-mockgcp-tests.md)
 
 1. The 1st PR should set the `create.yaml `and  `update.yaml `fields the same value for both test suites, with `_http.log `telling the matching HTTP request/response, and `_generated_object_<resource>.golden.yaml` telling the output-only fields.
 
@@ -17,7 +17,7 @@ Follow [Step 1](https://github.com/yuwenma/k8s-config-connector/blob/scifi-guide
 
 ## Add the backward compatible API
 
-Follow [Step 2](https://github.com/yuwenma/k8s-config-connector/blob/scifi-guide/docs/develop-resources/guides/2-define-apis.md)
+Follow [Step 2](../guides/2-define-apis.md)
 
 The PR shall contain the types and deepcopy codes. It shall make modifications to make sure the CRDs are the same, because Beta resource has to be backward compatible. 
 
@@ -35,13 +35,13 @@ Note: *Do not use* `excluded_resources`, we want the presubmit to validate the e
 
 ## Add the backward compatible mapper
 
-Follow [Step 3](https://github.com/yuwenma/k8s-config-connector/blob/scifi-guide/docs/develop-resources/guides/3-add-mapper.md)
+Follow [Step 3](../guides/3-add-mapper.md)
 
 The PR adds the Direct mapper. You can do this together with the previous step or the next step if no additional manual changes are needed. Using  `/*NOTYET .. */` to comment out new functions, same as the last step.
 
 ## Add the direct controller 
 
-Follow [Step 4](https://github.com/yuwenma/k8s-config-connector/blob/scifi-guide/docs/develop-resources/guides/4-add-controller.md)
+Follow [Step 4](../guides/4-add-controller.md)
 
 * Use the `KCC_USE_DIRECT_RECONCILERS` flag [exampe](https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/0bbac86ace6ab2f4051b574f026d5fe47fa05b75/dev/tasks/run-e2e#L27).