fix: returning an error when vcluster config schema changed #3386
Conversation
pkg/cli/delete_helm.go
Outdated
| return err | ||
| cmd.log.Warnf("Failed to parse vcluster config from Helm values: %v. ", err) | ||
| cmd.log.Infof("Continuing with deletion...") | ||
| vclusterConfig = nil |
There was a problem hiding this comment.
This approach is error prone, the code shouldn’t continue like this, especially since we already have cleanup logic implemented for synced namespaces. Moreover, other potential configuration issues (e.g., invalid sync.toHost fields) would go undetected.
I recommend implementing a custom UnmarshalJSON method for PrivateNodesAutoNodes that attempts to unmarshal the data and, if unsuccessful, returns a dedicated error (e.g., ErrInvalidAutoNodesConfig). Later, check whether the error from yaml.Unmarshal indicates an invalid auto nodes configuration if so, continue as normal, otherwise return the error.
Additionally, please add a comment explaining that we tolerate an invalid auto nodes configuration in the delete action because no cleanup or other follow-up action is required.
There was a problem hiding this comment.
I agree, I was trying to just fix the issue by warning the user that the vcluster config could not be parsed but the vcluster chart would be deleted.
NVM it won't work when we backport to v0.29 version
If we want we could make PrivateNodes backward compatible with older versions by creating a custom UnmarshalJSON that transform PrivateNodes.AutoNodes from an object to an array, instead of returning an dedicated error, what do you think?
| func (p *PrivateNodes) UnmarshalJSON(data []byte) error { | ||
| type PrivateNodesAlias PrivateNodes | ||
| var alias PrivateNodesAlias | ||
| if err := json.Unmarshal(data, &alias); err != nil { |
There was a problem hiding this comment.
(question) Do we want just to validate the marshalling or a stricter control on the input keys (by using DisallowUnknownFields with a json decoder) ?
3 participants
What issue type does this pull request address? (keep at least one, remove the others)
/kind bugfix
What does this pull request do? Which issues does it resolve? (use
resolves #<issue_number>if possible)resolves ENG-9408
Please provide a short message that should be published in the vcluster release notes
Fixed an issue where vcluster was not being deleted due to the schema difference between vcluster v0.29 and v0.30
What else do we need to know?
Note
Gracefully handle
PrivateNodesschema decode errors during deletion by warning and continuing, enabled by a new custom error and guard checks.config.Configunmarshal failure due toPrivateNodesschema, log warning and proceed with deletion; skip namespace cleanup when config is unavailable.ErrInvalidPrivateNodesConfigand customPrivateNodes.UnmarshalJSONto wrap decode errors for clearer detection.Written by Cursor Bugbot for commit c35abd0. This will update automatically on new commits. Configure here.