Add MeshSync spec validation to reconciliation loop#743
Add MeshSync spec validation to reconciliation loop#743ahmedmadbouly186 wants to merge 2 commits into
Conversation
Signed-off-by: ahmedmadbouly186 <66012617+ahmedmadbouly186@users.noreply.github.com>
miacycle
left a comment
miacycle
left a comment
There was a problem hiding this comment.
@ahmedmadbouly186, positive progress so far.
It would be helpful to users if these validation errors aren't just logged to stdout, but are also sent to Meshery Server for users to be aware of.
| // validateSpec validates the MeshSync spec before reconciliation | ||
| func (r *MeshSyncReconciler) validateSpec(_ context.Context, log logr.Logger, baseResource *mesheryv1alpha1.MeshSync) error { | ||
| spec := baseResource.Spec | ||
| nullNative := mesheryv1alpha1.NativeMeshsyncBroker{} |
There was a problem hiding this comment.
to represent that the native broker is empty and doesn't have either Name or Namespace
| hasCustom := spec.Broker.Custom.URL != "" | ||
|
|
||
| // 1. Broker: at least one must be configured | ||
| if !hasNative && !hasCustom { |
There was a problem hiding this comment.
What does "Native" mean in this context?
There was a problem hiding this comment.
This refers to a broker that Meshery itself deployed and manages inside the cluster, and Meshery Operator manages the lifecycle of the broker that Meshery uses for data streaming across the cluster and the outside world.
|
|
||
| // Validate spec before any side effects | ||
| if err := r.validateSpec(ctx, log, baseResource); err != nil { | ||
| log.Error(err, "MeshSync spec validation failed") |
There was a problem hiding this comment.
"MeshSync spec validation failed"
It would be a little more accurate and informative to operators of Meshery if:
- The name of the MeshSync custom resource was included.
- The message is changed to something like "MeshSync resource '' configuration invalid."
ahmedmadbouly186
force-pushed
the
feat/ahmed_madbouly/375
branch
from
63a4b52 to
0c084cc
Compare
Signed-off-by: ahmedmadbouly186 <66012617+ahmedmadbouly186@users.noreply.github.com>
| Complete(r) | ||
| } | ||
|
|
||
| func (r *MeshSyncReconciler) validateBrokerConfig(Broker mesheryv1alpha1.MeshsyncBroker) error { |
There was a problem hiding this comment.
this is different from reconcileBrokerConfig func, as it is fine with one of them set the custom url or the native, also the checks are embeded later for safety checks, you might want to adjust the funcs
| } | ||
|
|
||
| // Get broker configuration | ||
| if err := r.reconcileBrokerConfig(ctx, baseResource); err != nil { |
There was a problem hiding this comment.
refactore this func to remove the validaiton inline
|
Strong direction here, and your reasoning for why validation belongs in the operator rather than in meshsync is correct. We are keeping this and iterating it inside Workstreams 2 and 3, which introduce the v1alpha2 API, server-side-apply reconciliation, and proper status conditions. Two pieces of earlier review feedback should land as part of that: include the CR name in the message (something like We will rebase this onto the modernized reconciler rather than supersede it, and the new errors will be declared as MeshKit structured errors so they carry a stable code and remediation. |
4 participants
Description
This PR fixes #375
Notes for Reviewers
This issue opened in MeshSync, but I see it needs to be implemented here for the following reasons
Why this belongs in
meshery-operator, notmeshsyncThe MeshSync CRD is defined and owned by
meshery-operator. Its controller (MeshSyncReconciler)is the first process to handle a MeshSync resource — running before the MeshSync pod is ever scheduled. This means:
ValidationFailedstatus condition back to the CR, visible viakubectl describe meshsyncmeshsyncapp only runs after the operator has already deployed it — by then it's too late to prevent a broken Deployment from being createdmeshsyncalready has its own config validation incrd_config.go(validateLists), but that validateswatch-list content at runtime, not the CR spec at admission time. These are complementary layers, not duplicates.
This issue tracks the operator-side gap. The
meshsync-side validation (crd_config.go) is complete as-is.Signed commits