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

Relax ingress await restrictions #1832

Merged
merged 7 commits into from Dec 10, 2021
Merged

Relax ingress await restrictions #1832

merged 7 commits into from Dec 10, 2021

Conversation

viveklak
Copy link
Contributor

Proposed changes

Currently an ingress object is marked available when the backend endpoints are available and when the loadbalancer IP or host is available. The change here relaxes some assumptions made previously around ingress objects only referring to service backends or paths referencing endpoints that would emit events. The loadbalancer still a strict requirement but endpoint checks have been relaxed.

While I think this is the better default behavior, I wonder if we should add an annotation to opt-in to the previous stricter await logic?

Related issues (optional)

Fixes #1810

@@ -212,7 +212,7 @@ func newHelmReleaseProvider(
}

func debug(format string, a ...interface{}) {
logger.V(3).Infof("[DEBUG] %s", fmt.Sprintf(format, a...))
logger.V(6).Infof("[DEBUG] %s", fmt.Sprintf(format, a...))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sneaking this in. Happy to move to a separate PR if necessary.

@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

provider/pkg/await/ingress.go Outdated Show resolved Hide resolved
provider/pkg/await/ingress.go Outdated Show resolved Hide resolved
provider/pkg/await/ingress.go Outdated Show resolved Hide resolved
provider/pkg/await/ingress_test.go Outdated Show resolved Hide resolved
@viveklak
Copy link
Contributor Author

@lblackstone Thoughts on the following?

While I think this is the better default behavior, I wonder if we should add an annotation to opt-in to the previous stricter await logic?

@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@lblackstone
Copy link
Member

@lblackstone Thoughts on the following?

While I think this is the better default behavior, I wonder if we should add an annotation to opt-in to the previous stricter await logic?

I think it's ok to loosen the behavior unconditionally for now, and we can reevaluate if we get user feedback about it. The result of an error here is most likely just a transient update failure if something downstream gets updated too early.

@viveklak viveklak merged commit 839fa82 into master Dec 10, 2021
@pulumi-bot pulumi-bot deleted the vl/IngressAwait branch December 10, 2021 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ingress await semantics are too strict
2 participants