Improve consistency of errors returned from core webhooks

[killianmuldoon]

The use of field errors (from k8s.io/apimachinery/pkg/util/validation/field) is common across the webhooks in Cluster API, but which errors are used and the messages that accompany them should be made more consistent. It would be great to get some help in reviewing how these are applied in different webhooks to bring them all in line with a single standard.

Core Cluster API currently has the following webhooks:

• Cluster (@killianmuldoon) 🌱 Add field.Path aggregation for Cluster webhook #6322
• ClusterClass (@killianmuldoon) Reviewed without changes.
• MachineDeployment (@DiptoChakrabarty) 🌱 Add field.Path for MachineDeployment and MachinePool Webhooks #6377
• Machine (@Jont828) 🌱 Add field.Path aggregation for Machine webhook #6368
• MachineHealthCheck (@Jont828) 🌱 Add field.Path aggregation for MachineHealthCheck webhook #6376
• MachineSet (sachinkumarsingh092) 🌱 Add field.Path aggregation for MachineSet webhook #6352

There's three parts to this task for each webhook.

1. Use the correct type of error

Our current usage should be roughly as follows:

• field.Required: When a value that is required is not defined.
• field.Forbidden: When a value that can not be set under current conditions is set e.g. when the field is blocked by a feature gate.
• field.InternalError: An error that appears to be in the webhook rather than the information supplied to it e.g. failures in network calls.
• field.Invalid: When a validation rule fails on a field that can otherwise be populated.

There are other fieldErrors available, but I think it's better to keep the focus tight and include more information on specific validation failures in messages.

2. Use the full path to spec

• Ensure that a field.Path is included in the error
• The field.Path is the full path from the object root to the field that has caused the error.
• the root path should, where possible, be defined early with fields added using field.Path.Child.

3. Make message content and format consistent

Messages text in our field errors should follow some standards:

• Does not start with a capital letter. Does not contain a full stop.
• The message returned should give the user enough information to solve the problem
• The message should be readable in a terminal window

If you'd like to help please comment below with the name of the webhook you'd like to take on and I'll assign it in the issue here. If you've got any questions I'm happy to walk through what needs to be done here.

/help

[k8s-ci-robot]

@killianmuldoon:
This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

• Why are we solving this issue?
• To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
• Does this issue have zero to low barrier of entry?
• How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

The use of field errors (from k8s.io/apimachinery/pkg/util/validation/field) is common across the webhooks in Cluster API, but which errors are used and the messages that accompany them should be made more consistent. It would be great to get some help in reviewing how these are applied in different webhooks to bring them all in line with a single standard.

Core Cluster API currently has the following webhooks:

• Cluster
• ClusterClass (@killianmuldoon)
• MachineDeployment
• Machine
• MachineHealthCheck
• MachineSet

There's three parts to this task for each webhook.

1. Use the correct type of error

Our current usage should be roughly as follows:

• field.Required: When a value that is required is not defined.
• field.Forbidden: When a value that can not be set under current conditions is set e.g. when the field is blocked by a feature gate.
• field.InternalError: An error that appears to be in the webhook rather than the information supplied to it e.g. failures in network calls.
• field.Invalid: When a validation rule fails on a field that can otherwise be populated.

There are other fieldErrors available, but I think it's better to keep the focus tight and include more information on specific validation failures in messages.

2. Use the full path to spec

• Ensure that a field.Path is included in the error
• The field.Path is the full path from the object root to the field that has caused the error.
• the root path should, where possible, be defined early with fields added using field.Path.Child.

3. Make message content and format consistent

Messages text in our field errors should follow some standards:

• Does not start with a capital letter. Does not contain a full stop.
• The message returned should give the user enough information to solve the problem
• The message should be readable in a terminal window

If you'd like to help please comment below with the name of the webhook you'd like to take on and I'll assign it in the issue here. If you've got any questions I'm happy to walk through what needs to be done here.

/help

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.

[killianmuldoon]

There's also a number of webhooks outside the core package we could address in similar work if anyone is interested in those specifically. Those are:
In bootstrap:

• KubeadmConfig
• KubeadmConfigTemplate

In ControlPlane

• KubeadmControlPlane
• KubeadmControlPlaneTemplate

In experimental

• ClusterResourceSet
• MachinePool

In CAPD

• DockerCluster
• DockerClusterTemplate
• DockerMachineTemplate

[fabriziopandini]

/kind cleanup
/milestone v1.2

[Jont828]

Hey @killianmuldoon I'd be happy to help out with any of the webhooks. Are there any ones in particular you think would be easier to start with?

[killianmuldoon]

Thanks @Jont828 I think Machine or MachineSet are both pretty straightforward if you'd like to take one of them on.

[Jont828]

Sounds good, I'll take a look at Machines to start. To clarify, do we want to include CAPD Machines in this change, and also is #6129 a good example to follow?

[killianmuldoon]

@Jont828 I think the Cluster webhook (with the changes in #6322) is the best example we have overall.

[sachinkumarsingh092]

Hey @killianmuldoon,
I'd like to work on MachineSet. Can you please assign that to me?

[killianmuldoon]

Sure @sachinkumarsingh092 Thanks for your interest in this

/assign sachinkumarsingh092

[DiptoChakrabarty]

Hi @killianmuldoon I would like to work on MachineDeployment and exp\MachinePool

[Jont828]

@killianmuldoon Could you assign me the MachineHealthCheck one as well?

[killianmuldoon]

Sure thing - thanks @Jont828

[sbueringer]

/reopen

[k8s-ci-robot]

@sbueringer: Reopened this issue.

In response to this:

/reopen

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.

[killianmuldoon]

/close

Thanks @DiptoChakrabarty @Jont828 @sachinkumarsingh092 !

[k8s-ci-robot]

@killianmuldoon: Closing this issue.

In response to this:

/close

Thanks @DiptoChakrabarty @Jont828 @sachinkumarsingh092 !

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.