Skip to content
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

Open
multi-io opened this issue Jun 21, 2023 · 8 comments
Open

defaultAndValidateMachineSpec() only validates, doesn't default #1670

multi-io opened this issue Jun 21, 2023 · 8 comments
Labels
kind/bug Categorizes issue or PR as related to a bug. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. sig/cluster-management Denotes a PR or issue as being assigned to SIG Cluster Management.

Comments

@multi-io
Copy link
Contributor

multi-io commented Jun 21, 2023

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

spec = &defaultedSpec

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

@embik
Copy link
Member

embik commented Jul 4, 2023

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 embik added kind/bug Categorizes issue or PR as related to a bug. sig/cluster-management Denotes a PR or issue as being assigned to SIG Cluster Management. labels Jul 4, 2023
@kubermatic-bot
Copy link
Contributor

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 kubermatic-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 2, 2023
@embik
Copy link
Member

embik commented Oct 2, 2023

/remove-lifecycle stale

@kubermatic-bot kubermatic-bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 2, 2023
@kubermatic-bot
Copy link
Contributor

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 kubermatic-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 31, 2023
@embik
Copy link
Member

embik commented Jan 2, 2024

/remove-lifecycle stale

@kubermatic-bot kubermatic-bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 2, 2024
@embik
Copy link
Member

embik commented Jan 2, 2024

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

@kubermatic-bot
Copy link
Contributor

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 kubermatic-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 1, 2024
@embik
Copy link
Member

embik commented Apr 2, 2024

/remove-lifecycle stale

@kubermatic-bot kubermatic-bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 2, 2024
@embik embik added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. sig/cluster-management Denotes a PR or issue as being assigned to SIG Cluster Management.
Projects
None yet
Development

No branches or pull requests

3 participants