Support the partial observed state in ContainerCluster, ContainerNodePool and RedisInstance

[maqiuyujoyce]

Change description

Fixes b/315227901

• Supported the following observed fields:

  • status.observedState.masterAuth.clientCertificate in ContainerCluster
  • status.observedState.version in ContainerNodePool
  • status.authString in RedisInstance

• Fixed the issue that when the parent of the observed field exists but the observed field itself doesn't, there is still an empty observed state. Added test cases to cover this scenario.

Tests you have done

• Run make ready-pr to ensure this PR is ready for review.
• Perform necessary E2E testing for changed resources.

Did local integration test using the following test cases.

• ContainerCluster and ContainerNodePool

  1. Applied the following YAML:

     apiVersion: container.cnrm.cloud.google.com/v1beta1
     kind: ContainerCluster
     metadata:
       name: test-observed-state-1
       namespace: test
     spec:
       location: us-central1-a
       initialNodeCount: 1
       loggingService: logging.googleapis.com/kubernetes
       monitoringService: monitoring.googleapis.com/kubernetes
     ---
     apiVersion: container.cnrm.cloud.google.com/v1beta1
     kind: ContainerNodePool
     metadata:
       name: test-observed-state-1
       namespace: test
     spec:
       location: us-central1-a
       autoscaling:
         minNodeCount: 1
         maxNodeCount: 3
       clusterRef:
         name: test-observed-state-1
     ---
     apiVersion: container.cnrm.cloud.google.com/v1beta1
     kind: ContainerCluster
     metadata:
       name: test-observed-state-2
       namespace: test
     spec:
       location: us-central1-a
       initialNodeCount: 1
       loggingService: logging.googleapis.com/kubernetes
       monitoringService: monitoring.googleapis.com/kubernetes
       masterAuth:
         clientCertificateConfig:
           issueClientCertificate: true
     

  2. Verified that after the reconciliation completed, in the output of kubectl describe, the values under status.observedState in the CRs matched the corresponding fields under spec.

      # No `status.observedState` in ContainerCluster test-observed-state-1.
     
      # Had `status.observedState` that matched the spec in ContainerNodePool test-observed-state-1.
      Status:
        ...
        Observed State:
          Version:  1.27.3-gke.100
      
      # Had `status.observedState` that matched the spec in ContainerCluster test-observed-state-2.
      Status:
        ...
        Observed State:
          Master Auth:
            Client Certificate: [truncated]
     

• RedisInstance

  1. Applied the following YAML:

     apiVersion: redis.cnrm.cloud.google.com/v1beta1
     kind: RedisInstance
     metadata:
       name: test-observed-state-1
       namespace: test
     spec:
       authEnabled: true
       displayName: Sample Redis Instance
       region: us-central1
       tier: BASIC
       memorySizeGb: 16
       maintenanceSchedule:
       - endTime: 2023-08-01T16:29:22.045123456Z
         scheduleDeadlineTime: 2023-08-01T17:29:22.045123456Z
         startTime: 2023-08-01T15:29:22.045123456Z
     

  2. Verified that after the reconciliation completed, in the output of kubectl describe, status.observedState.authString can be found.

  3. Updated the resource with the following YAML:

     apiVersion: redis.cnrm.cloud.google.com/v1beta1
     kind: RedisInstance
     metadata:
       name: test-observed-state-1
       namespace: test
     spec:
       authEnabled: false
       displayName: Sample Redis Instance
       region: us-central1
       tier: BASIC
       memorySizeGb: 16
       maintenanceSchedule:
       - endTime: 2023-08-01T16:29:22.045123456Z
         scheduleDeadlineTime: 2023-08-01T17:29:22.045123456Z
         startTime: 2023-08-01T15:29:22.045123456Z
     

  4. Verified that after the reconciliation completed, in the output of kubectl describe, the entire status.observedState struct is gone.

[google-oss-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maqiuyujoyce

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 [maqiuyujoyce]

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

[justinsb]

So given that e.g. masterVersion is status.masterVersion, do we want to introduce status.observedState? i.e. should it be status.observedState.masterAuth.clientCertificate vs status.masterAuth.clientCertificate?

The way I see it:

1. If we introduce status.observedState we have to deal with the status.masterVersion vs status.observedState.masterVersion inconsistency at some time in the future
2. If we don't introduce status.observedState we (maybe) have a naming conflict if there's a status field in the API called "conditions"

Personally, I'd rather deal with the hypothetical work of (2) than the known work of (1), but that's just my intuition

[maqiuyujoyce]

Thank you for the suggestions, @justinsb !

1. If we introduce status.observedState we have to deal with the status.masterVersion vs status.observedState.masterVersion inconsistency at some time in the future

2. If we don't introduce status.observedState we (maybe) have a naming conflict if there's a status field in the API called "conditions"

IMO, if we introduce status.observedState.masterVersion one day, then status.masterVersion and status.observedState.masterVersion will always share the same value. I'd consider it a redundancy but not inconsistency. Or are you referring to some other scenarios?

If we need to deal with naming conflict, then it seems to be a true inconsistency: possibly we either need to rename the observed field (inconsistent with its original field name), or rename the existing status field (may impact all CRDs).

And in general I find redundancy safer to the users.

[cheftako]

My 2 cents. I think grouping the observed state has additional benefits in that we're grouping it. We can tell customers (and future devs) that all cloud response state can be found under .spec.observedState. More importantly I think we should insist that only cloud response state is allowed into .spec.observedState. That allows anyone looking at a value under .spec.observedState to trust its a value from the cloud provider and not say an artifact from the controller. I think it will make a docs easier and clearer. I think the fact that it essentially name spaces the response data and hence provides protection from future conflicts (either from new fields, newly cared about response fields or other non cloud response status fields which happen to have a name collision) is valuable but possibly not the most valuable.

[diviner524]

I agree with @cheftako, my view on a dedicated status.observedState is it gives our users a new mental model on how to identify status changes caused by the external cloud provider, but not KCC.

if there's a status field in the API called "conditions"

Also I want to mention this is not a hypothetical scenario. We have already seen it in a few CRDs. See the following two examples in our servicemapping config. We had to ignore a bunch of fields because they have conflict naming with the reserved K8s status name fields.

https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/master/config/servicemappings/run.yaml#L27

https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/master/config/servicemappings/run.yaml#L147

Should customers require support for these ignored fields in the future, we have the flexibility to add them to status.observedState.

[maqiuyujoyce]

Thank you for the input, @cheftako and @diviner524 !

@justinsb I'll probably merge the implementation as is for now. If you think not having status.observedState is important, then let's discuss offline about a revised design.

[justinsb]

I spoke to @cheftako , who persuaded me round to your collective point of view. My hope is that we can remove the duplicated fields in v1, so if we do that I think we should proceed with observedState :-)

[diviner524]

/lgtm