machine-controller icon indicating copy to clipboard operation
machine-controller copied to clipboard

defaultAndValidateMachineSpec() only validates, doesn't default

Open multi-io opened this issue 2 years ago • 8 comments

Unless I'm very confused right now, this code in defaultAndValidateMachineSpec() (part the of the mutating webhooks)

https://github.com/kubermatic/machine-controller/blob/94e2005cbd847a05e72d04ce97dc2d3bab59fa1c/pkg/admission/machines.go#L185

...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?

multi-io avatar Jun 21 '23 11:06 multi-io

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.

embik avatar Jul 04 '23 06:07 embik

Issues go stale after 90d of inactivity. After a furter 30 days, they will turn rotten. Mark the issue as fresh with /remove-lifecycle stale.

If this issue is safe to close now please do so with /close.

/lifecycle stale

kubermatic-bot avatar Oct 02 '23 11:10 kubermatic-bot

/remove-lifecycle stale

embik avatar Oct 02 '23 12:10 embik

Issues go stale after 90d of inactivity. After a furter 30 days, they will turn rotten. Mark the issue as fresh with /remove-lifecycle stale.

If this issue is safe to close now please do so with /close.

/lifecycle stale

kubermatic-bot avatar Dec 31 '23 23:12 kubermatic-bot

/remove-lifecycle stale

embik avatar Jan 02 '24 07:01 embik

@multi-io are you still interested in fixing this bug?

embik avatar Jan 02 '24 07:01 embik

Issues go stale after 90d of inactivity. After a furter 30 days, they will turn rotten. Mark the issue as fresh with /remove-lifecycle stale.

If this issue is safe to close now please do so with /close.

/lifecycle stale

kubermatic-bot avatar Apr 01 '24 12:04 kubermatic-bot

/remove-lifecycle stale

embik avatar Apr 02 '24 06:04 embik