add support for upgrade_settings in both node config and NAP config

[davidquarles]

No description provided.

[davidquarles]

This fixes #5580.

[davidquarles]

@rileykarson @danawillow I'm still finishing up writing tests here, but had two quick questions:

1. Is it actually preferable to factor the shared upgrade settings config out into its own file, as was done with e.g. google/node_config.go?
2. Should we do any sort of validation here of the upstream constraint that max_surge + max_unavailable <= 20, or does it make more sense to just let the failure occur and propagate down? If we prefer the former, any guidance is appreciated, as I'm not sure how/where to do that sort of composite validation.

EDIT: cc @slevenick @ndmckinley

[davidquarles]

2. Should we do any sort of validation here of the upstream constraint that max_surge + max_unavailable <= 20, or does it make more sense to just let the failure occur and propagate down? If we prefer the former, any guidance is appreciated, as I'm not sure how/where to do that sort of composite validation.

Additional context:
After testing, it appears the google API will allow you to violate this constraint at both the node pool and NAP layers and silently fails in subtly different ways. NAP config successfully applies (I used maxUnavailable=1, maxSurge=20), but NAP autoscale is subsequently broken. I tried this same config at the node pool layer and terraform emitted no errors, but the node pool itself didn't update and nothing change in terraform state.

[nat-henderson]

This looks great.

1. Is it actually preferable to factor the shared upgrade settings config out into its own file, as was done with e.g. google/node_config.go?

In my opinion, no - I'd prefer it if you put it in container_cluster.

2. Should we do any sort of validation here of the upstream constraint that max_surge + max_unavailable <= 20, or does it make more sense to just let the failure occur and propagate down? If we prefer the former, any guidance is appreciated, as I'm not sure how/where to do that sort of composite validation.

I think it's probably best to add this validation - our general approach is that validation is desirable unless it would impose an undue maintenance burden. In this case where the server-side can break if the validation isn't added, it seems extremely useful to add.

[rileykarson]

Closing PR as stale

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.