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

Fix resetting service type to default when not specified (#8165) #8169

Merged
merged 2 commits into from
Oct 31, 2024

Conversation

thbkrkr
Copy link
Contributor

@thbkrkr thbkrkr commented Oct 31, 2024

These changes ensure that when deleting the type of an http(s) external service declaration in an Elastic resource definition, the service type is properly reset to the default ClusterIP value.

This can be broken down into 3 changes:
- When the type of an external service is not set, it defaults to ClusterIP. Thats seems mandatory to me to be able to detect to reset ClusterIP when no service is configured while there is still the version of the service typed LoadBalancer on the server side. It's ok to do this change because the default value of the Type field is this value (v1/types.go). So, this will not change the current type for existing clusters that have not set the type.
- When an external service is reconciled, we requeue if we get an alreadyExists error. This is because the deletion of the service can take many seconds (e.g.: gcp lb deletion takes >30s), resulting in the creation while the resource is still being deleted.
- When the type of a service changes from something different to the default ClusterIP value, server side changes are not applied. We may lose some case where we could update instead of recreate but it will avoid some cases where certain fields have no sense with the new type.
@thbkrkr thbkrkr added >bug Something isn't working backport For backport PRs v2.15.0 labels Oct 31, 2024
@thbkrkr thbkrkr marked this pull request as ready for review October 31, 2024 11:15
@thbkrkr thbkrkr enabled auto-merge (squash) October 31, 2024 11:15
Copy link
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, not sure what is going on with the build.

@thbkrkr
Copy link
Contributor Author

thbkrkr commented Oct 31, 2024

In the backport commit, the new code uses an import (k8serrors) added by ae1f62c which is not in the 2.15 branch. I will fix the import.

@thbkrkr thbkrkr merged commit fa9234e into 2.15 Oct 31, 2024
5 checks passed
@thbkrkr thbkrkr deleted the backport-8165-to-2.15 branch October 31, 2024 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport For backport PRs >bug Something isn't working v2.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants