Please note that we're already in Test Freeze for the release-1.24 branch. This means every merged PR has to be cherry-picked into the release branch to be part of the upcoming v1.24.0 release.

@mdbooth: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

I have now tested this manually and verified that it fixes the problem.

To describe the problem: cloud-provider-openstack adds an annotation to the Service object during reconciliation. In our environment we were incorrectly deploying cloud-provider-openstack without the required permission to patch the service object, which resulted in an error message like:

I0422 10:30:04.823353   64011 event.go:294] "Event occurred" object="lb-test-ns/lb-test-svc2" fieldPath="" kind="Service" apiVersion="v1" type="Warning" reason="SyncLoadBalancerFailed" message="Error syncing load balancer: failed to ensure load balancer: failed to patch service object lb-test-ns/lb-test-svc2: services \"lb-test-svc2\" is forbidden: User \"system:serviceaccount:kube-system:cloud-controller-manager\" cannot patch resource \"services\" in API group \"\" in the namespace \"lb-test-ns\

However, what we observed was that the next time the Service was reconciled after the failure, the reconcile would succeed but the annotation had not been added to the Service object.

The reason for this was that the Service object passed to cloud-provider-openstack from syncService was dirty from the previous invocation. As the annotation was still present from the previous run there was no difference for the Patcher to patch, and therefore the reconcile erroneously succeeds.

I have verified that with:

• cloud-provider-openstack deployed without the required permissions to patch Service objects
• this patch in place

cloud-provider-openstack will now fail with the above permission error on every reconcile attempt, not just the first one.

I would appreciate advice on how to write an automated test for this. I think the ideal test would be:

• Have the informer return a Service object
• Modify the object in a reconciler, but don't push the modification to apiserver and return an error
• sync the Service a second time, retrieving the object from the informer
• assert that the object is not dirty

It might be possible to construct a unit test to do this but it would be quite complicated and probably not very easy to understand or maintain. It feels more like envtest territory?

Alternatively, we could argue that this would really be a test of the behaviour of informers, not the service controller, and doesn't require a separate test?

controller-runtime does this here, btw: https://github.com/kubernetes-sigs/controller-runtime/blob/c162794a9b12dea31c076820938a779873e93957/pkg/cache/internal/cache_reader.go#L84-L90

lgtm :)

@andrewsykim the impact of this that I've noticed is fairly minor and by fixing the underlying permission issue we're unlikely to see it often, although the problem still exists if there's a transient failure to patch the Service object.

I haven't been following the 1.24 release schedule, so I don't know how frozen we are right now. Is it worth trying to merge this, or wait until after 1.24?

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

/remove-lifecycle stale

@andrewsykim This is still a thing, btw. Any idea who can approve?

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mdbooth, thockin

The full list of commands accepted by this bot can be found here.

The pull request process is described here