feature: [vmagent] Add service discovery support for OVH Cloud VPS and dedicated server #6160
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6160 +/- ##
==========================================
- Coverage 60.37% 57.22% -3.15%
==========================================
Files 411 609 +198
Lines 76609 81208 +4599
==========================================
+ Hits 46253 46475 +222
- Misses 27794 31544 +3750
- Partials 2562 3189 +627 ☔ View full report in Codecov by Sentry. |
|
Hello @jiekun! Sorry for late response( Could you please rebase your PR on top of master branch? |
Signed-off-by: Jiekun <jiekun.dev@gmail.com>
|
Thank you for your attention. I've just rebased it. |
| ok bool | ||
| ) | ||
| if apiServer, ok = availableEndpoints[sdc.Endpoint]; !ok { | ||
| return nil, fmt.Errorf("unsupported endpoint for ovhcloud sd: %s", sdc.Endpoint) |
There was a problem hiding this comment.
Shall we point user to the docs with available endpoints?
|
|
||
| timeDelta, err := getTimeDelta(cfg) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
please add context to this error
There was a problem hiding this comment.
I would like to add context in getServerTime function, and log all possible errors it in upper level getAuthHeaders, wdyt?
| "time" | ||
| ) | ||
|
|
||
| func getAuthHeaders(cfg *apiConfig, headers http.Header, endpoint, path string) (http.Header, error) { |
There was a problem hiding this comment.
It doesn't seem that any caller of this func checks for the error. Is this intended?
There was a problem hiding this comment.
This error should have been checked, and the SD HTTP request should be canceled.
However, it appears that GetAPIResponseWithReqParams does not consider the error from RequestCallback func(req *http.Request). As a result, it sends out the service discovery request without the appropriate Auth Headers.
I am returning an error in getAuthHeaders and expecting it to be checked. Should we add a #TODO here or find a way to stop the SD request?
Edit:
| return &serverTime, nil | ||
| } | ||
|
|
||
| func getTimeDelta(cfg *apiConfig) (time.Duration, error) { |
There was a problem hiding this comment.
Would you mind adding a comment explaining that delta should be calculated only once?
|
|
||
| } | ||
|
|
||
| // ParseIPList parses ip list as they can have different formats. |
There was a problem hiding this comment.
| // ParseIPList parses ip list as they can have different formats. |
|
|
||
| // dedicatedServer struct from API. | ||
| // IP addresses are fetched independently. | ||
| type dedicatedServer struct { |
There was a problem hiding this comment.
Would be great to have link to the OVH docs with response fields
There was a problem hiding this comment.
Docs are already presented in getVPSDetails and getDedicatedServerDetails. Since multiple APIs are involved, I believe it would be beneficial to provide links to the most important one, as well as the getXxxDetails methods, so developers can easily access and review.
| ) | ||
|
|
||
| // vpsModel struct from API. | ||
| type vpsModel struct { |
There was a problem hiding this comment.
Would be great to have link to the OVH docs with response fields
Describe Your Changes
related issue: #6071
Added
Docs
CHANGELOG.md,sd_configs.md,vmagent.mdare updated.Note
Tested on OVH Cloud VPS and dedicated server.
Checklist
The following checks are mandatory: