Issue-654, Prohibiting cluster spec updating until they are running #672
Conversation
…y to be updated was added
worryg0d
requested review from
testisnullus,
ribaraka,
OleksiienkoMykyta and
tengu-alt
and removed request for
testisnullus and
ribaraka
worryg0d
changed the title
worryg0d
changed the title
| // ensuring if the cluster is ready for the spec updating | ||
| if (c.Status.CurrentClusterOperationStatus != models.NoOperation || c.Status.State != models.RunningStatus) && c.Generation != oldCluster.Generation { |
There was a problem hiding this comment.
Code should be self documented, I don't think that we need such comments
There was a problem hiding this comment.
I discussed it with teammates and we decided to keep it as it is because it enhances the readability of the following check
There was a problem hiding this comment.
Strange to write comment for check if no operation or not running status, but ok
There was a problem hiding this comment.
This comment refers to spec update only, no the general k8s resource update
| // ensuring if the cluster is ready for the spec updating | ||
| if (c.Status.CurrentClusterOperationStatus != models.NoOperation || c.Status.State != models.RunningStatus) && c.Generation != oldCluster.Generation { | ||
| return models.ErrClusterIsNotReadyToUpdate | ||
| } | ||
|
|
There was a problem hiding this comment.
Do we really need this check? c.Generation != oldCluster.Generation
We already have it in SetupWithManager and there will be no update in false case.
And check all such cases where you use this check
if event.ObjectNew.GetGeneration() == event.ObjectOld.GetGeneration() { return false }
There was a problem hiding this comment.
The following logic prohibits updating the resource spec field. Validating webhooks trigger to update requests only on Patch and Update operation. However, we need to prevent any updations of the spec, so we have to understand what triggered an update. If it changes the spec it also changes the metadata.generation field. If we don't ensure that this is a spec update then it will denies any Update requests, but controllers of the following resources also update resources by adding/deleting annotations/finalizers by using client.Update and client.Patch.
In summary, It helps to understand if it was an update of the resource spec field or any other update, but we have to prohibit only updates of the spec.
There was a problem hiding this comment.
Validating webhook can validate the request and potentially reject it based on our business logic.
During this phase, the Generation field of the resource has not yet been incremented because the update has not been persisted. So, why should we check this?
There was a problem hiding this comment.
For the incoming new object it is already changed, because this is a part of it and it is changes only by k8s
| // ensuring if the cluster is ready for the spec updating | ||
| if (c.Status.CurrentClusterOperationStatus != models.NoOperation || c.Status.State != models.RunningStatus) && c.Generation != oldCluster.Generation { | ||
| return models.ErrClusterIsNotReadyToUpdate | ||
| } | ||
|
|
There was a problem hiding this comment.
Validating webhook can validate the request and potentially reject it based on our business logic.
During this phase, the Generation field of the resource has not yet been incremented because the update has not been persisted. So, why should we check this?
| // ensuring if the cluster is ready for the spec updating | ||
| if (c.Status.CurrentClusterOperationStatus != models.NoOperation || c.Status.State != models.RunningStatus) && c.Generation != oldCluster.Generation { | ||
| return models.ErrClusterIsNotReadyToUpdate | ||
| } | ||
|
|
The PR adds validation to the update webhooks to prevent cluster spec updating until their state is
runningor there is no cluster operation.Also, it cleans up the codebase and adds some minor fixes.
closes #654