NETOBSERV-857 After some time, it fails to retrieve flows #310
Conversation
|
/ok-to-test |
openshift-ci
bot
added
the
ok-to-test
Codecov Report
@@ Coverage Diff @@
## main #310 +/- ##
==========================================
+ Coverage 58.49% 58.51% +0.02%
==========================================
Files 148 148
Lines 6536 6532 -4
Branches 783 783
==========================================
- Hits 3823 3822 -1
+ Misses 2497 2494 -3
Partials 216 216
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
New image: ["quay.io/netobserv/network-observability-console-plugin:5cf517e"]. It will expire after two weeks. |
|
/ok-to-test |
| } else if cfg.Authorization != "" { | ||
| headers[auth.AuthHeader] = []string{cfg.Authorization} | ||
| } else if cfg.TokenPath != "" { | ||
| bytes, err := os.ReadFile(cfg.TokenPath) |
There was a problem hiding this comment.
That's a bit less straightforward, but we could keep the token in memory for something like 5 minutes and then make it expire? Like it's suggested there: https://kubernetes.io/docs/tasks/configure-pod-container/configure-service-account/#launch-a-pod-using-service-account-token-projection
That would avoid reading files at every request.
Currently we use the default expiration time which is 1 hour if I'm correct, so reloading every 5min is a comfortable margin.
There was a problem hiding this comment.
what about fsnotify ?
There was a problem hiding this comment.
I'm more for keeping it simple ... don't think we need something that sophisticated here? There's more complexity by having a watcher, error handling might be different and more tricky (e.g. if an error makes you "miss" a change event, should you just report the error? reload the file anyway? I think the answer isn't obvious)
There was a problem hiding this comment.
(and also not introducing a new dependency just for that)
|
/lgtm |
|
/label qe-approved Verified no longer running into the cert issue with this change when |
|
@jpinsonneau are we good to merge? |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jpinsonneau The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Reload HOST token from file on every query