🌱 Improve dry run for topology changes to dry run server side apply

[chrischdi]

What this PR does / why we need it:

This proposes an improvement on how we implement dry run for detecting changes by actually using dry run server side apply.

The proposed implementation has the following advantages

• It uses the same logic as using the real server side apply patch operation
• The diff logic relies on comparing the current object to the object how it would look like after server side apply
• The diff logic does not rely on how managed fields are used by the kube-apiserver and requires no knowledge about the handled object or used schema.

Disadvantages:

• Using dry run server side apply requires more requests to the kube-apiserver because it requires an additional request for each comparison.

TODOs:

• Add provider implementation documentation:
  • for template resources which get rotated (known types are: InfrastructureMachineTemplate or BootstrapTemplate): If there are existing validating webhooks which block due to immutability when a resource gets updated (ValidateUpdate) they need to implement the sigs.k8s.io/cluster-aip/util/webhooks.TopologyAwareValidator instead which provides a bool to decide skipping immutability checks

[sbueringer]

Very high level review & ignored the tests for now

Looks good

[chrischdi]

/hold

competes with #6709

/test all

[chrischdi]

/test all

[chrischdi]

e2e currently fails due to:

error reconciling the Cluster topology: failed to create patch helper for DockerMachineTemplate/k8s-upgrade-with-runtimesdk-1uasfg-control-plane-52rwq: failed to determine changes via dryRunSSAPatch: failed to request dry-run server side apply: admission webhook "validation.dockermachinetemplate.infrastructure.cluster.x-k8s.io" denied the request: DockerMachineTemplate.infrastructure.cluster.x-k8s.io "k8s-upgrade-with-runtimesdk-1uasfg-control-plane-52rwq" is invalid: spec.template.spec: Invalid value: v1beta1.DockerMachineTemplate{TypeMeta:v1.TypeMeta{Kind:"DockerMachineTemplate", APIVersion:"infrastructure.cluster.x-k8s.io/v1beta1"}, ObjectMeta:v1.ObjectMeta{Name:"k8s-upgrade-with-runtimesdk-1uasfg-control-plane-52rwq", GenerateName:"", Namespace:"k8s-upgrade-with-runtimesdk-t58uv4", SelfLink:"", UID:"a52c48ad-71e8-423b-98f5-5c4f0861479b", ResourceVersion:"2014", Generation:2, CreationTimestamp:time.Date(2022, time.June, 24, 8, 12, 20, 0, time.Local), DeletionTimestamp:, DeletionGracePeriodSeconds:(*int64)(nil), Labels:map[string]string{"cluster.x-k8s.io/cluster-name":"k8s-upgrade-with-runtimesdk-1uasfg", "topology.cluster.x-k8s.io/owned":""}, Annotations:map[string]string{"cluster.x-k8s.io/cloned-from-groupkind":"DockerMachineTemplate.infrastructure.cluster.x-k8s.io", "cluster.x-k8s.io/cloned-from-name":"quick-start-control-plane"}, OwnerReferences:[]v1.OwnerReference{v1.OwnerReference{APIVersion:"cluster.x-k8s.io/v1beta1", Kind:"Cluster", Name:"k8s-upgrade-with-runtimesdk-1uasfg", UID:"1836653c-8570-43a8-b20b-a677d9f9c689", Controller:(*bool)(nil), BlockOwnerDeletion:(*bool)(nil)}}, Finalizers:[]string(nil), ZZZ_DeprecatedClusterName:"", ManagedFields:[]v1.ManagedFieldsEntry{v1.ManagedFieldsEntry{Manager:"capi-topology", Operation:"Apply", APIVersion:"infrastructure.cluster.x-k8s.io/v1beta1", Time:time.Date(2022, time.June, 24, 8, 18, 27, 0, time.Local), FieldsType:"FieldsV1", FieldsV1:(*v1.FieldsV1)(0x4000d99140), Subresource:""}}}, Spec:v1beta1.DockerMachineTemplateSpec{Template:v1beta1.DockerMachineTemplateResource{ObjectMeta:v1beta1.ObjectMeta{Labels:map[string]string(nil), Annotations:map[string]string(nil)}, Spec:v1beta1.DockerMachineSpec{ProviderID:(*string)(nil), CustomImage:"kindest/node:v1.24.0", PreLoadImages:[]string(nil), ExtraMounts:[]v1beta1.Mount{v1beta1.Mount{ContainerPath:"/var/run/docker.sock", HostPath:"/var/run/docker.sock", Readonly:false}}, Bootstrapped:false}}}}: DockerMachineTemplate spec.template.spec field is immutable. Please create a new resource instead.

We have to catch immutability some more...

[sbueringer]

Hm. That's an interesting issue. Any ideas on how to work around this? A lot of InfrastructureMachineTemplates are immutable

[chrischdi]

Member

Yes, catch the error (what we already do) but detect it as return true, true, nil in this case

[sbueringer]

Member

Yes, catch the error (what we already do) but detect it as return true, true, nil in this case

I probably misunderstood. My impression is we can't run SSA dryrun on immutable templates because the Validation Webhook will block it so then we won't get a meaningful diff?

Does the SSA dryrun give us a result despite the validation webhook blocking the request?

[chrischdi]

Member

Yes, catch the error (what we already do) but detect it as return true, true, nil in this case

I probably misunderstood. My impression is we can't run SSA dryrun on immutable templates because the Validation Webhook will block it so then we won't get a meaningful diff?

Does the SSA dryrun give us a result despite the validation webhook blocking the request?

See the implementation. Thats imho the best we can do here: identify if the error causes are only of spec fields which got are marked as invalid.

[sbueringer]

Member

Yes, catch the error (what we already do) but detect it as return true, true, nil in this case

I probably misunderstood. My impression is we can't run SSA dryrun on immutable templates because the Validation Webhook will block it so then we won't get a meaningful diff?
Does the SSA dryrun give us a result despite the validation webhook blocking the request?

See the implementation. Thats imho the best we can do here: identify if the error causes are only of spec fields which got are marked as invalid.

Just that I interpret the code correctly. If we get an CauseTypeFieldValueInvalid error from a field in spec. we assume we have changes?

So as long as immutable errors are returned as CauseTypeFieldValueInvalid and with the correct path we detect immutable errors as changes. Which is correct because without changes no immutable error.

I think this doesn't cover cases where e.g. the CAPA provider returns a BadRequest error?

• CAPA https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/main/api/v1beta1/awsmachinetemplate_webhook.go#L141-L143
• CAPZ seems to be fine: https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/main/api/v1beta1/azuremachinetemplate_webhook.go
• CAPO as well: https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/main/api/v1alpha5/openstackmachinetemplate_webhook.go#L64-L66

[sbueringer]

Just a short status update: we discussed the issue a bit and evaluated options. We will explore them and then make a suggestion

[sbueringer]

lgtm pending squash + #6710 (comment)

I would be fine with moving #6710 (comment) to a follow-up issue so we get this PR merged ASAP and still get a few days time in CI.

[chrischdi]

lgtm pending squash + #6710 (comment)

I would be fine with moving #6710 (comment) to a follow-up issue so we get this PR merged ASAP and still get a few days time in CI.

Regarding squash: @fabriziopandini do we still want to keep the CAPD change as seperate commit?

Sounds good to move it to a follow-up. I think we could even link the commit for the CAPD change if we want (which we can do after merge only)?!

[chrischdi]

/test pull-cluster-api-e2e-full-main
/test pull-cluster-api-e2e-informing-ipv6-main
/test pull-cluster-api-e2e-informing-main
/test pull-cluster-api-e2e-workload-upgrade-1-21-1-22-main

[k8s-ci-robot]

@chrischdi: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-e2e-workload-upgrade-1-24-latest-main	d6f0297	link	false	/test pull-cluster-api-e2e-workload-upgrade-1-24-latest-main

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[chrischdi]

Runtime SDK flake: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api/6710/pull-cluster-api-e2e-full-main/1544943529262845952

Getting addressed by #6817 ? @killianmuldoon

/retest

[sbueringer]

Sounds good to move it to a follow-up. I think we could even link the commit for the CAPD change if we want (which we can do after merge only)?!

I would prefer having relevant documentation directly in the documentation vs. linking to commits on merged PRs. No objections to an additional link. I think the link to the commit stays the same (now vs after merge)

[chrischdi]

• Added unit tests for the ShouldSkipImmutabilityChecks func
• Squashed commits

cc @fabriziopandini @sbueringer

/test pull-cluster-api-e2e-full-main
/test pull-cluster-api-e2e-informing-ipv6-main
/test pull-cluster-api-e2e-informing-main
/test pull-cluster-api-e2e-workload-upgrade-1-21-1-22-main

[chrischdi]

/cherry-pick release-1.2

[k8s-infra-cherrypick-robot]

@chrischdi: once the present PR merges, I will cherry-pick it on top of release-1.2 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.2

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[chrischdi]

/test pull-cluster-api-e2e-full-main
/test pull-cluster-api-e2e-informing-ipv6-main
/test pull-cluster-api-e2e-informing-main
/test pull-cluster-api-e2e-workload-upgrade-1-21-1-22-main

[sbueringer]

Thx
/lgtm

[fabriziopandini]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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

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

[sbueringer]

/hold cancel

[k8s-infra-cherrypick-robot]

@chrischdi: new pull request created: #6861

In response to this:

/cherry-pick release-1.2

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.