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(reports) skip mesh when namespace-limited #3336

Merged
merged 1 commit into from
Jan 6, 2023
Merged

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Jan 5, 2023

What this PR does / why we need it:

Disable mesh detection if the configuration uses --watch-namespaces. The mesh detector attempts to scan all namespaces and cannot be configured to use the multi-namespace cache used by the manager. Since it will fail when --watch-namespaces is set, disable it altogether.

Which issue this PR fixes:

Fix #3158

Special notes for your reviewer:

Disabling altogether since the report data isn't critical. Alternatives are:

  • Find some way to use the manager client for mesh detection. I don't think this is easily doable without taking it out of the reports system and instead building it into the Service controller. controller-runtime does not expose the manager client and I don't particularly want to roll my own MNCB/Kubernetes API client integration just for this.
  • Only scan the controller namespace. We should always have access to this, but it's not guaranteed to have a mesh enabled if there is one.

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@codecov
Copy link

codecov bot commented Jan 5, 2023

Codecov Report

Base: 74.0% // Head: 74.1% // Increases project coverage by +0.1% 🎉

Coverage data is based on head (e44612e) compared to base (9bce9e3).
Patch coverage: 0.0% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #3336     +/-   ##
=======================================
+ Coverage   74.0%   74.1%   +0.1%     
=======================================
  Files        110     110             
  Lines      13263   13268      +5     
=======================================
+ Hits        9822    9842     +20     
+ Misses      2808    2798     -10     
+ Partials     633     628      -5     
Impacted Files Coverage Δ
internal/manager/run.go 47.6% <0.0%> (-1.2%) ⬇️
internal/manager/utils/reports.go 0.0% <0.0%> (ø)
...nternal/controllers/gateway/udproute_controller.go 73.6% <0.0%> (-2.8%) ⬇️
internal/dataplane/kongstate/service.go 66.0% <0.0%> (-1.3%) ⬇️
internal/dataplane/parser/parser.go 91.3% <0.0%> (+1.6%) ⬆️
...nternal/controllers/gateway/tcproute_controller.go 75.6% <0.0%> (+2.6%) ⬆️
...ternal/controllers/gateway/httproute_controller.go 55.8% <0.0%> (+3.3%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@rainest rainest marked this pull request as ready for review January 5, 2023 21:11
@rainest rainest requested a review from a team as a code owner January 5, 2023 21:11
Disable mesh detection if the configuration uses --watch-namespaces. The
mesh detector attempts to scan all namespaces and cannot be configured
to use the multi-namespace cache used by the manager. Since it will fail
when --watch-namespaces is set, disable it altogether.
@randmonkey randmonkey merged commit 9130508 into main Jan 6, 2023
@randmonkey randmonkey deleted the fix/mesh-namespace branch January 6, 2023 05:17
@pmalek pmalek added the fix label Mar 1, 2023
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.

watchNamespaces ClusterRoles Error
3 participants