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: istio-pilot's handling of incomplete Gateway Services #263

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ca-scribner
Copy link
Contributor

This improves the handling of a gateway LoadBalancer service that does not have any ingress IPs. Previously, this could fail if the ingress attribute was None, such as when there is no LoadBalancer provider present in the cluster.

This also adds a check during reconcile which will raise an ErrorWithStatus when the Gateway Service is not ready.

This improves the handling of a gateway LoadBalancer service that does not have any ingress IPs.  Previously, this could fail if the ingress attribute was None, such as when there is no LoadBalancer provider present in the cluster.
@ca-scribner ca-scribner requested a review from a team as a code owner April 26, 2023 20:08
@ca-scribner ca-scribner enabled auto-merge (squash) April 26, 2023 20:10
@@ -822,11 +839,13 @@ def _get_address_from_loadbalancer(svc):
(str): The hostname or IP address of the LoadBalancer service
"""
ingresses = svc.status.loadBalancer.ingress

# May happen due to the Kubernetes cluster not having a LoadBalancer provider
if ingresses is None or len(ingresses) == 0:
Copy link
Member

Choose a reason for hiding this comment

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

?

Suggested change
if ingresses is None or len(ingresses) == 0:
if not ingresses:

Copy link
Member

@beliaev-maksim beliaev-maksim left a comment

Choose a reason for hiding this comment

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

also, I just think about _report_handled_errors. Do we really handle them ?

@@ -647,7 +664,7 @@ def _report_handled_errors(self, errors):
)
self._log_and_set_status(status_to_publish)
for i, error in enumerate(errors):
self.log.info(f"Handled error {i}/{len(errors)}: {error.status}")
self.log.info(f"Handled error {i+1}/{len(errors)}: {error.status}")
Copy link
Member

Choose a reason for hiding this comment

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

you can use enumerate(errors, start=1)

Suggested change
self.log.info(f"Handled error {i+1}/{len(errors)}: {error.status}")
self.log.info(f"Handled error {i}/{len(errors)}: {error.status}")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants