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 Envoy service status watching #2605

Merged
merged 1 commit into from
Jun 19, 2020

Conversation

youngnick
Copy link
Member

We all missed in #2583 that not all informers handle Unstructured objects; wrap the service status informer in the dynamicHandler to ensure that no Unstructured objects make it through.

Service status watching was broken before this, as it didn't handle Unstructured.

Signed-off-by: Nick Young [email protected]

@youngnick youngnick requested a review from jpeach June 18, 2020 05:47
@youngnick youngnick self-assigned this Jun 18, 2020
@codecov
Copy link

codecov bot commented Jun 18, 2020

Codecov Report

Merging #2605 into master will decrease coverage by 0.09%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2605      +/-   ##
==========================================
- Coverage   76.88%   76.79%   -0.10%     
==========================================
  Files          71       71              
  Lines        5498     5502       +4     
==========================================
- Hits         4227     4225       -2     
- Misses       1185     1190       +5     
- Partials       86       87       +1     
Impacted Files Coverage Δ
cmd/contour/serve.go 0.00% <0.00%> (ø)
internal/k8s/syncer.go 0.00% <0.00%> (ø)
internal/dag/cache.go 98.09% <0.00%> (-0.96%) ⬇️

Copy link
Contributor

@jpeach jpeach left a comment

Choose a reason for hiding this comment

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

Regression from #2583.

How about doing a more extensive refactor to automatically prepend a DynamicClientHandler on every cache handler? That way we remove the pitfalls for future code changes.

@youngnick youngnick force-pushed the fix-service-watching branch from c38d8e1 to 8ae50f2 Compare June 19, 2020 01:42
We all missed in projectcontour#2583 that not all informers handle Unstructured objects; wrap the service status informer in the dynamicHandler to ensure that no Unstructured objects make it through.

Service status watching was broken before this, as it didn't handle Unstructured.

Signed-off-by: Nick Young <[email protected]>
@youngnick youngnick force-pushed the fix-service-watching branch from 8ae50f2 to 3db40fd Compare June 19, 2020 02:55
@youngnick youngnick requested a review from jpeach June 19, 2020 03:13
Copy link
Contributor

@jpeach jpeach left a comment

Choose a reason for hiding this comment

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

LGTM

@youngnick youngnick merged commit d75219a into projectcontour:master Jun 19, 2020
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.

2 participants