tests: csau test

[ziyue-101]

Change description

Remove version filed if auto-upgrade is turned on.

Tests you have done

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

[ziyue-101]

/assign @acpana

[acpana]

/lgtm
/approve

thanks for splitting the tests!

cc @maqiuyujoyce

[maqiuyujoyce]

Could you make the test case be updating from one management mode (e.g. automatic) to another management mode (e.g. manual)? We want to test the update of this field as well.

[maqiuyujoyce]

I suggest we don't update from using version to using management (yet) due to this test gap.

[yuwenma]

For the management and version, suggest making sure the direct controller can do the following (we'd like to see this is reflected in the test suites)

1. If using CSAU, the GCP service returned version is recorded in status.observedState.version (or status.version if it already exists)
2. If using CSAU, and passing in the version in the HTTP request, does GCP service give errors? Or no-op? or take into account the version as well?

• if it is error. the current KCC API is good. users know how to fix their config if given both management AUTOMATIC and version.
• If it is a no-op, the direct controller should add additional logic that if management= AUTOMATIC, unset version before sending the HTTP request.
• If GCP service take into account the version, KCC can treat it as a no-op.

[maqiuyujoyce]

If using CSAU, the GCP service returned version is recorded in status.observedState.version (or status.version if it already exists)

Good point.

If using CSAU, and passing in the version in the HTTP request, does GCP service give errors? Or no-op? or take into account the version as well?

Note that KCC pass in the version in the HTTP request because version is also specified in the KRM object. Check #3344 (comment) for more details. So basically this means user specified both management and version.

If it is a no-op, the direct controller should add additional logic that if management= AUTOMATIC, unset version before sending the HTTP request.

If no-op, then it means the live state is considered SoT and the desired intention is ignored. We should find the diff in between the desired state and live state and return an error accordingly if there is a diff.

[yuwenma]

Note that KCC pass in the version in the HTTP request because version is also specified in the KRM object. Check #3344 (comment) for more details. So basically this means user specified both management and version.

Right. Passing in both version and management could be okay, it defers to how the GKEHub service responses. For the 3 possible results I gave, KCC only need additional logic if GKEHub service allows both values and ignore the version.

[ziyue-101]

Answer: the GCP server returns an error if both version and csau is specified in the spec.

2. If using CSAU, and passing in the version in the HTTP request, does GCP service give errors? Or no-op? or take into account the version as well?

• if it is error. the current KCC API is good. users know how to fix their config if given both management AUTOMATIC and version.
• If it is a no-op, the direct controller should add additional logic that if management= AUTOMATIC, unset version before sending the HTTP request.
• If GCP service take into account the version, KCC can treat it as a no-op.

[ziyue-101]

I suggest we don't update from using version to using management (yet) due to this test gap.

Thanks for the digging. I don't think this PR should be blocked on the test gap. I can remove the version field in the created.yaml

[ziyue-101]

@maqiuyujoyce the gap is in test, I wonder what's the behavior for base controller when doing update, does it patch or update? Does he base controller have the same gap?

[ziyue-101]

Answer

If using CSAU, the GCP service returned version is recorded in status.observedState.version (or status.version if it already exists)

If this is desired, this means the status.version field should be optional because it should only be populated when csau is disabled. This PR only adds test cases, I think we can address the status.version in followups.

[maqiuyujoyce]

/lgtm
/approve

/hold
Do you have any other concerns, @yuwenma ?

[google-oss-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: acpana, 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

[yuwenma]

Answer

If using CSAU, the GCP service returned version is recorded in status.observedState.version (or status.version if it already exists)

If this is desired, this means the status.version field should be optional because it should only be populated when csau is enabled. This PR only adds test cases, I think we can address the status.version in followups.

Do you infer the version is required now and not in status? If so, yes it should be changed from required to optional (this is common when the GCP service adds automatic support for some previously configurable only fields).

[yuwenma]

/hold cancel

Looks good on my side.
Thanks! @ziyue-101

[ziyue-101]

Answer

If using CSAU, the GCP service returned version is recorded in status.observedState.version (or status.version if it already exists)

If this is desired, this means the status.version field should be optional because it should only be populated when csau is enabled. This PR only adds test cases, I think we can address the status.version in followups.

Do you infer the version is required now and not in status? If so, yes it should be changed from required to optional (this is common when the GCP service adds automatic support for some previously configurable only fields).

My bad, I corrected my previous statement. The version field is prohibited when management field is set as automatic, it is optional If management field is set with other values. The GCP sever has validation logic to deny any request with version field and csau field set together. I think we should delegate the validation to the GCP server and don't emulate GCP server behavior in the direct controller because that could change over time.

[yuwenma]

Answer

If using CSAU, the GCP service returned version is recorded in status.observedState.version (or status.version if it already exists)

If this is desired, this means the status.version field should be optional because it should only be populated when csau is enabled. This PR only adds test cases, I think we can address the status.version in followups.

Do you infer the version is required now and not in status? If so, yes it should be changed from required to optional (this is common when the GCP service adds automatic support for some previously configurable only fields).

My bad, I corrected my previous statement. The version field is prohibited when management field is set as automatic, it is optional If management field is set with other values. The GCP sever has validation logic to deny any request with version field and csau field set together. I think we should delegate the validation to the GCP server and don't emulate GCP server behavior in the direct controller because that could change over time.

Right, that's option 3 I mentioned. If If GCP service take into account the version, KCC can treat it as a no-op, and let users take actions according to the GCP error
Can you add a scenario test to record the real GCP behavior? Step 1. version configured Step 2 version and managed (auto) configured 3. version unset and only managed(auto) configured.