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

Bug: MySQL Flexible Server property sourceServerResourceId has wrong type #3772

Closed
theunrepentantgeek opened this issue Feb 11, 2024 · 6 comments · Fixed by #3832
Closed
Assignees
Labels
bug 🪲 Something isn't working high-priority Issues we intend to prioritize (security, outage, blocking bug)
Milestone

Comments

@theunrepentantgeek
Copy link
Member

While responding to a question on Slack, I spotted the property FlexibleServer sourceServerResourceId which looks to be an ARM resource ID, and therefore should be a genruntime.ResourceReference not a string.

We can't really change the existing version, but I think we should import the latest version(s) of the API (2023-06-30 and maybe 2023-10-01-Preview) and change it there.

@matthchr matthchr added bug 🪲 Something isn't working and removed needs-triage 🔍 labels Feb 12, 2024
@matthchr
Copy link
Member

We should investigate how the heuristic missed this, and see if we can improve it - for example looking for properties that end in "resourceId" as I don't think it's doing that

@matthchr matthchr added the high-priority Issues we intend to prioritize (security, outage, blocking bug) label Feb 12, 2024
@theunrepentantgeek
Copy link
Member Author

Trying out some improvements to our heuristic and finding other properties we missed:

Group Version Kind Property
authorization v1api20200801preview RoleAssignmentProperties DelegatedManagedIdentityResourceId
authorization v1api20220401 RoleAssignmentProperties DelegatedManagedIdentityResourceId
dbformysql v1api20210501 ServerProperties SourceServerResourceId
machinelearningservices v1api20210701 IdentityForCmk UserAssignedIdentity
machinelearningservices v1api20210701 KeyVaultProperties KeyVaultArmId

We need to decide whether fixing these is important enough to have a breaking change, or whether we can do so without breaking.

@theunrepentantgeek theunrepentantgeek added this to the v2.7.0 milestone Feb 22, 2024
@matthchr matthchr modified the milestone: v2.7.0 Feb 22, 2024
@theunrepentantgeek theunrepentantgeek self-assigned this Feb 26, 2024
@theunrepentantgeek
Copy link
Member Author

We could make this less of a breaking change if we modified genruntime.ResourceReference to allow deserialization from a plain string. We then wouldn't break existing resources, so in-place upgrades of ASO in an existing cluster would work fine. Users would still need to modify their resources to apply them though.

@matthchr
Copy link
Member

We could make this less of a breaking change if we modified genruntime.ResourceReference to allow deserialization from a plain string. We then wouldn't break existing resources, so in-place upgrades of ASO in an existing cluster would work fine. Users would still need to modify their resources to apply them though.

I think we've looked into this in the past and determined that it won't work because of Kubernetes structured schema requirements, specifically that they won't let you model that in the Swagger so even if the underlying Go type can do it, APIServer will (I believe) reject the request because it doesn't match the OpenAPI spec.

@matthchr
Copy link
Member

Synced up and decided to first check what new apiversions for these resource types are supported - possibly we can fix there in a totally nonbreaking way.

@theunrepentantgeek
Copy link
Member Author

For the three identified groups, the latest available versions are:

Group Stable Preview
authorization 2022-04-01 2020-10-01-preview
dbformysql 2023-06-30 2023-10-01-preview
machinelearningservices 2023-10-01 2023-08-01-preview

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🪲 Something isn't working high-priority Issues we intend to prioritize (security, outage, blocking bug)
Projects
Development

Successfully merging a pull request may close this issue.

2 participants