-
Notifications
You must be signed in to change notification settings - Fork 131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
defaultAndValidateMachineSpec() only validates, doesn't default #1670
Comments
Hey @multi-io, that analysis feels correct to me. I think it would make at least sense to try out what happens if we fix it, given that people usually implemented defaulting for a reason when they implemented providers. |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with /lifecycle stale |
/remove-lifecycle stale |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with /lifecycle stale |
/remove-lifecycle stale |
@multi-io are you still interested in fixing this bug? |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with /lifecycle stale |
/remove-lifecycle stale |
Unless I'm very confused right now, this code in defaultAndValidateMachineSpec() (part the of the mutating webhooks)
machine-controller/pkg/admission/machines.go
Line 185 in 94e2005
...only updates the passed-in pointer to the to-be-defaulted MachineSpec, but doesn't update the MachineSpec it points to. So the updated object will be garbage collected and the caller's MachineSpec will not be updated (i.e. defaulted).
Changing the line to something like
*spec = defaultedSpec
should fix it (or changing some function signatures so we pass pointers rather than values). I'm not sure about the implications because it seems like this bug has been in there since 2018. So maybe nobody really needed the provider-specific defaulting functionality?The text was updated successfully, but these errors were encountered: