-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feature: [vmagent] Add service discovery support for OVH Cloud VPS and dedicated server #6160
base: master
Are you sure you want to change the base?
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
A few minor comments
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we point user to the docs with available endpoints?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
|
||
timeDelta, err := getTimeDelta(cfg) | ||
if err != nil { | ||
return nil, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add context to this error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't seem that any caller of this func checks for the error. Is this intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
if modifyRequest != nil { |
return &serverTime, nil | ||
} | ||
|
||
func getTimeDelta(cfg *apiConfig) (time.Duration, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind adding a comment explaining that delta should be calculated only once?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
} | ||
|
||
// ParseIPList parses ip list as they can have different formats. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great to have link to the OVH docs with response fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great to have link to the OVH docs with response fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
Describe Your Changes
related issue: #6071
Added
Docs
CHANGELOG.md
,sd_configs.md
,vmagent.md
are updated.Note
Tested on OVH Cloud VPS and dedicated server.
Checklist
The following checks are mandatory: