enable strict JSON spec unmarshalling

[AndrewChubatiuk]

related issue VictoriaMetrics/helm-charts#2882

---

Summary by cubic

Enable strict JSON unmarshalling for all Operator CR specs. Creates now fail fast on unknown fields, while updates that only include unknown fields to this operator version are allowed to support safe downgrades.

• Bug Fixes

  • Added UnmarshalSpecStrict (lenient pass + DisallowUnknownFields) and routed all CR UnmarshalJSON (v1, v1alpha1, v1beta1) through it, setting ParsingSpecError on failure.
  • Added HasUnknownFields and updated webhooks to reject on create, but allow updates when the error is only "unknown field".
  • Fixed VMAlertmanagerConfig tests/fixtures to valid fields (msteams_configs, proper custom_fields).
  • Updated changelog entry.

• Migration

  • Manifests with unknown or misspelled spec fields are rejected on create; fix or remove extra keys before applying.
  • Existing CRs with unknown fields can be updated, but those fields are ignored by this operator version.

^{Written for commit 8bd43ae. Summary will update on new commits. Review in cubic}

[cubic-dev-ai]

No issues found across 25 files

_{Re-trigger cubic}

[vrutkovs]

I think this would break invalid Prometheus CRs:

• A CR with extra fields created
• Controller unmarshals it, sets status.ParsingSpecError
• Here we don't set the status to Failed, so error is basically invisible
• This field is not persisted to etcd, so on next reconcile we start again

Another issue - during upgrade we apply new CRDs when old operator is running:

• CRD removes a field
• Old operator is using new CRD, parses existing CR
• Extra field detected, CR is moved to failed

until operator is upgraded. In existing mode it would just be silently skipped.

I'm not convinced we should change existing behaviour, however a good documentation around this choice would be useful

[vrutkovs]

I wonder if we can limit strict parsing to new objects only - perhaps in a webhook?

[AndrewChubatiuk]

anyway in a linked issue I've described workaround, created this PR just in case

[AndrewChubatiuk]

• A CR with extra fields created
• Controller unmarshals it, sets status.ParsingSpecError
• Here we don't set the status to Failed, so error is basically invisible
• This field is not persisted to etcd, so on next reconcile we start again

seems like expected behavior when webhooks are enabled. currenty webhooks reject CRs with specs that have fields with invalid data type or value and mark resource as failed and allow to apply it when webhooks are disabled. with this change the same behaviour is added for unsupported fields.

Another issue - during upgrade we apply new CRDs when old operator is running:

• CRD removes a field
• Old operator is using new CRD, parses existing CR
• Extra field detected, CR is moved to failed

we keep noop CR properties for a long time, so this concern should be covered as well

[vrutkovs]

with this change the same behaviour is added for unsupported fields.

This is critical though - the change would apply to existing objects, however I think its safer to apply this to new objects only

[AndrewChubatiuk]

This is critical though - the change would apply to existing objects, however I think its safer to apply this to new objects only

Not sure, user may have a valid spec applied, but then he may try to upgrade it to one with unsupported fields. Should operator tolerate it?

[vrutkovs]

Its indistinguishable from "operator removed a field and now whatever user has set previously now unsupported" - I think we should allow that, giving users time to migrate instead of failing immediately.

[AndrewChubatiuk]

I think we should allow that, giving users time to migrate instead of failing immediately.

okay, seems like you mean it to be a temporary step before prohibiting unknown fields completely

[vrutkovs]

Oh yes, that is the intention - however I think we should take it slow and not break users - ideally even if they ignore deprecation warnings if possible

[AndrewChubatiuk]

updated PR, please take a look