Retry http requests to c8y fails with the response status code 401,403 and 404 #2065
Conversation
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
Robot Results
Passed Tests
|
| // get new internal id not the cached one | ||
| self.end_point.c8y_internal_id = "".into(); | ||
| let internal_id = match child_device_id { |
There was a problem hiding this comment.
I would introduce a get_fresh_internal_id() or renew_internal_id() method.
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
| let req_builder = | ||
| |token: String| HttpRequestBuilder::get(&url_get_id).bearer_auth(token); | ||
|
|
||
| self.get_and_set_jwt_token().await?; | ||
| let request_builder = | ||
| req_builder(self.cached_identifiers.token.clone().unwrap_or_default()); |
There was a problem hiding this comment.
As the closure is used along its definition, this seems to be useless.
There was a problem hiding this comment.
Refactored, not needed any more.
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
PradeepKiruvale
force-pushed
the
http-retry-closure
branch
from
e124d29
to
e6ee727
Compare
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
| let mut url_update_swlist = self.get_base_url(); | ||
| url_update_swlist.push_str("/inventory/managedObjects/"); | ||
| url_update_swlist.push_str(&self.c8y_internal_id); | ||
| url_update_swlist.push_str(&self.get_internal_id(device_id).unwrap_or_default()); |
There was a problem hiding this comment.
What's the meaning of an empty string for an internal id? This unwrap is suspicious.
There was a problem hiding this comment.
Refactored to return an error if the internal-id is an empty string.
| while self.end_point.get_internal_id(None).is_none() { | ||
| if let Err(error) = self | ||
| .try_get_and_set_internal_id(self.end_point.device_id.clone()) | ||
| .await | ||
| { |
There was a problem hiding this comment.
Is this initialization loop still required, now that the code is ready to retry on obsolete internal ids?
There was a problem hiding this comment.
Later we retry only once if there is any failure. I feel this is important because during the init phase the software list will be sent to c8y. If the call fails because of internal ID then the list will be missing on the c8y. So, it's better to retry for the first time and move on only if there is an internal ID.
| let build_request = |end_point: &C8yEndPoint| -> Result<HttpRequestBuilder, C8YRestError> { | ||
| Ok(HttpRequestBuilder::get(&url_get_id) | ||
| .bearer_auth(end_point.token.clone().unwrap_or_default())) | ||
| }; | ||
|
|
||
| async fn try_get_internal_id( | ||
| &mut self, | ||
| device_id: Option<&str>, | ||
| ) -> Result<String, C8YRestError> { | ||
| let url_get_id = self.end_point.get_url_for_get_id(device_id); | ||
| let request_builder = build_request(&self.end_point)?; | ||
|
|
||
| let request = request_builder.build()?; |
There was a problem hiding this comment.
I don't see the point to create and use the build_request closure in the same scope. Why not a simple method to build the request?
There was a problem hiding this comment.
This closure is not just used inside this function. When the first attempt fails because of 401/403 then the retries with the new token. Then it calls the try_with_fresh_token, this function takes the `closure. So, I created a closure here.
| } | ||
| } | ||
|
|
||
| pub fn get_c8y_internal_id(&self) -> &str { | ||
| &self.c8y_internal_id | ||
| pub fn get_internal_id(&self, id: Option<&str>) -> Option<String> { |
There was a problem hiding this comment.
| pub fn get_internal_id(&self, id: Option<&str>) -> Option<String> { | |
| pub fn get_internal_id(&self, child_id: Option<&str>) -> Option<String> { |
There was a problem hiding this comment.
I would keep it as device_id, which is more generic.
There was a problem hiding this comment.
I suggested child_id specifically since this API is expecting only child devices IDs as that argument and None for the main device. Keeping it generic would have been fine if this API accepted the main device's id as well. But this suggestion Didier seems like the best way to avoid all such confusion.
|
|
||
| url_update_swlist | ||
| } | ||
|
|
||
| pub fn get_url_for_get_id(&self, device_id: Option<&str>) -> String { | ||
| pub fn get_url_for_get_id(&self, device_id: String) -> String { |
There was a problem hiding this comment.
| pub fn get_url_for_get_id(&self, device_id: String) -> String { | |
| pub fn get_url_for_internal_id(&self, device_id: &str) -> String { |
| @@ -47,18 +61,18 @@ impl C8yEndPoint { | |||
| url_get_id | |||
| } | |||
|
|
|||
| pub fn get_url_for_sw_list(&self) -> String { | |||
| pub fn get_url_for_sw_list(&self, device_id: Option<&str>) -> String { | |||
There was a problem hiding this comment.
| pub fn get_url_for_sw_list(&self, device_id: Option<&str>) -> String { | |
| pub fn get_url_for_sw_list(&self, child_id: Option<&str>) -> String { |
There was a problem hiding this comment.
I would keep this generic name.
| let mut url_update_swlist = self.get_base_url(); | ||
| url_update_swlist.push_str("/inventory/managedObjects/"); | ||
| url_update_swlist.push_str(&self.c8y_internal_id); | ||
| url_update_swlist.push_str(&self.get_internal_id(device_id).unwrap_or_default()); |
There was a problem hiding this comment.
With that unwrap_or_default there's the risk of broken URLs for unknown child devices.
There was a problem hiding this comment.
Fixed, it returns error if the internal id is empty.
| let build_request = |end_point: &C8yEndPoint| -> Result<HttpRequestBuilder, C8YRestError> { | ||
| Ok(HttpRequestBuilder::get(&url_get_id) | ||
| .bearer_auth(end_point.token.clone().unwrap_or_default())) | ||
| }; |
There was a problem hiding this comment.
| let build_request = |end_point: &C8yEndPoint| -> Result<HttpRequestBuilder, C8YRestError> { | |
| Ok(HttpRequestBuilder::get(&url_get_id) | |
| .bearer_auth(end_point.token.clone().unwrap_or_default())) | |
| }; | |
| let build_request = |end_point: &C8yEndPoint| -> HttpRequestBuilder { | |
| HttpRequestBuilder::get(&url_get_id) | |
| .bearer_auth(end_point.token.clone().unwrap_or_default()) | |
| }; |
There was a problem hiding this comment.
I made it like this just to reuse the retry code when it fails. So, the try_with_fresh_jwt_token function takes a closure that returns the Result. So, this has to be consistent with that.
| self.get_and_set_jwt_token().await | ||
| } | ||
|
|
||
| async fn retry_request_with_fresh_token( |
There was a problem hiding this comment.
| async fn retry_request_with_fresh_token( | |
| async fn try_request_with_fresh_token( |
As the API is doing just that. It's just that we are using it on retries only.
| Ok(self.peers.http.await_response(request).await?) | ||
| } | ||
|
|
||
| async fn create_event( | ||
| async fn retry_request_with_fresh_internal_id( |
There was a problem hiding this comment.
| async fn retry_request_with_fresh_internal_id( | |
| async fn try_request_with_fresh_internal_id( |
| .clear_the_cached_internal_id(device_id.clone()); | ||
| // get new internal id not the cached one | ||
| self.try_get_and_set_internal_id(device_id.unwrap_or(self.end_point.device_id.clone())) | ||
| .await?; |
There was a problem hiding this comment.
Technically there's nothing wrong with this impl. But using a clear API followed by get_and_set feels a bit like exposing internal impl details.
But, i'd have introduced a new set_and_get_internal_id API that forcefully updates the internal ID, whether one is cached or not, and used that here. I'd also use that same API from inside this try_get_and_set_internal_id when the cached internal ID is empty/None.
There was a problem hiding this comment.
done, but named it as get_and_set_internal_id
| Ok(internal_id) | ||
| } | ||
|
|
||
| async fn execute( | ||
| &mut self, | ||
| request_builder: HttpRequestBuilder, | ||
| device_id: Option<String>, | ||
| build_request: impl Fn(&C8yEndPoint) -> Result<HttpRequestBuilder, C8YRestError>, |
There was a problem hiding this comment.
| build_request: impl Fn(&C8yEndPoint) -> Result<HttpRequestBuilder, C8YRestError>, | |
| build_request: impl Fn(&C8yEndPoint) -> HttpRequestBuilder, |
Wondering why that closure must return a Result and not just the HttpRequestBuilder directly?
There was a problem hiding this comment.
Yeah,its needed because get_software_list is called inside the closure and if it fails it will return an error.
There was a problem hiding this comment.
Yeah,its needed because get_software_list is called inside the closure and if it fails it will return an error.
There is no get_software_list method nor function.
A typo here, I meant get_url_for_sw_list
There was a problem hiding this comment.
Now that you introduced methods that build urls from an internal id and never fail, I would try what is suggested by @albinsuresh . I.e. to remove the error case by passing to the closure the internal id.
There was a problem hiding this comment.
Just passing the internal-id to the closure is not enough. Still the end_point is needed to get the URL for any specific HTTP call, because these URL creation functions are part of the end_point.
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
| Ok(internal_id) | ||
| } | ||
|
|
||
| async fn execute( | ||
| &mut self, | ||
| request_builder: HttpRequestBuilder, | ||
| device_id: Option<String>, | ||
| build_request: impl Fn(&C8yEndPoint) -> Result<HttpRequestBuilder, C8YRestError>, |
There was a problem hiding this comment.
Yeah,its needed because get_software_list is called inside the closure and if it fails it will return an error.
There is no get_software_list method nor function.
A typo here, I meant get_url_for_sw_list
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
| device_id: Option<&str>, | ||
| ) -> Result<String, C8YRestError> { | ||
| let url_get_id = self.end_point.get_url_for_get_id(device_id); | ||
| let request_builder = build_request(&self.end_point)?; |
There was a problem hiding this comment.
As already said, I would not use a closure to build the request to get the internal id of a device. I would simply use the url returned by get_url_for_internal_id() and not call try_request_with_fresh_token(). Sharing the later for regular requests and for the very specific request to get an internal id makes the code more complex than necessary.
There was a problem hiding this comment.
I don't see what has been updated.
There was a problem hiding this comment.
Removed the closure and used the URL returned by the get_url_for_internal_id().
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
| pub fn get_device_type(&self, device_id: Option<String>) -> DeviceType { | ||
| match device_id { | ||
| Some(device_id) => DeviceType::ChildDevice(device_id), | ||
| None => DeviceType::MainDevice, | ||
| } | ||
| } |
There was a problem hiding this comment.
This function just highlights a problem that we were trying to avoid: using magics like None to represent main device. I'd have just created those enum variants directly from the callers, which would have improved the code readability from there as well, as it would be clear if a certain function is called for the MainDevice or a ChildDevice.
There was a problem hiding this comment.
I would not do that because the APIs have to be changed. I wanted to keep the http_proxy APIs as it is and do the mapping here. From the API pov, it's calling either for the main device or for the child device.
Other way would be we must change the APIs to get the device-id always, irrespective of its for a child or the main device. For example for the send_software_list_http()`` API, this must always take a device-id`. That way things will be clearer.
There was a problem hiding this comment.
At some point we need to change those APIs to complete the cleanup what we've started here. The problem is that some of those APIs still accept request structs like UploadConfigFile, UploadLogBinary etc that has an Option field for the device-id which can be None for the main device. We should use the new DeviceType enum to those structs as well, so that we use it across the entire call stack and avoid this None magic completely.
There was a problem hiding this comment.
@PradeepKiruvale I encourage you to follow the direction given by @albinsuresh
| extras: c8y_event.extras.clone(), | ||
| } | ||
| }; | ||
| let device_type = self.end_point.get_device_type(None); |
There was a problem hiding this comment.
The device_type will always be the main device? As you're passing None here. Why? What if an event has to be created for a child device?
There was a problem hiding this comment.
Yes, this function create_event is only used by the main device. If a child device wants to use this function then it has to pass the device id, then the DeviceType can be derived out of this.
There was a problem hiding this comment.
That doesn't sound right. Even if a child device sends an event with extra fragments, it has to go through this same API call. So, we're missing something here.
| .json(&software_list)) | ||
| }; | ||
|
|
||
| let http_result = self.execute(DeviceType::MainDevice, build_request).await?; |
There was a problem hiding this comment.
Here also, hardcoded to MainDevice. Something looks wrong here. I understand that we don't support software list upload for child devices ATM. But this generic API assuming the same seems wrong. This API should be able to send software list for any device.
There was a problem hiding this comment.
Thats right. Since the Software list is supported only by the main device, I have hardcoded here. When the software list feature is supported by the child device, then one has to pass the device ID to send_software_list_http(), and based on that URL can be created.
| // Get and set child device internal id | ||
| if !device_type.eq(&DeviceType::MainDevice) { | ||
| self.get_and_set_internal_id(device_type.clone()).await?; | ||
| } |
There was a problem hiding this comment.
On a first look, it wasn't obvious to me as to why we're doing this here. But later understood.
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
| @@ -102,6 +114,14 @@ impl C8yEndPoint { | |||
| } | |||
| false | |||
| } | |||
|
|
|||
| pub fn get_device_id(&self, device_id: String) -> String { | |||
| if device_id.eq("main") { | |||
There was a problem hiding this comment.
Not sure about that special treatment for the String literal main here. Was this really necessary? With the APIs explicitly requiring the device id to be passed, I would have expected all users to pass the actual device id itself, and not the magic key "main".
If we needed to retrieve the internal id of the main device somewhere, I'd have added a get_main_device_id function instead.
| @@ -90,7 +90,7 @@ impl ConfigUploadManager { | |||
| self.upload_config_file( | |||
| Path::new(config_file_path.as_str()), | |||
| &config_upload_request.config_type, | |||
| None, | |||
| "main".into(), | |||
There was a problem hiding this comment.
We should be passing the actual device-id here rather than this special key.
| .end_point | ||
| .get_internal_id(self.end_point.device_id.clone()) |
There was a problem hiding this comment.
| .end_point | |
| .get_internal_id(self.end_point.device_id.clone()) | |
| .end_point | |
| .get_main_device_internal_id() |
Such a function would be better.
| .is_err() | ||
| { | ||
| if let Err(error) = self | ||
| .get_and_set_internal_id(self.end_point.device_id.clone()) |
There was a problem hiding this comment.
| .get_and_set_internal_id(self.end_point.device_id.clone()) | |
| .get_and_set_main_device_internal_id() |
| } else { | ||
| return Err(C8YRestError::CustomError("JWT token not available".into())); | ||
| }; | ||
| let device_id = self.end_point.get_device_id(device_id); |
There was a problem hiding this comment.
That looks really weird. Deriving the device_id from the device_id. I'm assuming that this is happening because of that special handling of "main" literal. If possible, I'd avoid passing that special char around.
| @@ -61,12 +61,12 @@ impl C8YHttpProxy { | |||
| &mut self, | |||
| log_type: &str, | |||
| log_content: &str, | |||
| child_device_id: Option<String>, | |||
| source: String, | |||
There was a problem hiding this comment.
| source: String, | |
| device_id: String, |
| @@ -79,12 +79,12 @@ impl C8YHttpProxy { | |||
| &mut self, | |||
| config_path: &Path, | |||
| config_type: &str, | |||
| child_device_id: Option<String>, | |||
| source: String, | |||
There was a problem hiding this comment.
| source: String, | |
| device_id: String, |
| @@ -47,14 +47,14 @@ pub struct GetJwtToken; | |||
| pub struct UploadLogBinary { | |||
| pub log_type: String, | |||
| pub log_content: String, | |||
| pub child_device_id: Option<String>, | |||
| pub source: String, | |||
There was a problem hiding this comment.
| pub source: String, | |
| pub device_id: String, |
| } | ||
|
|
||
| #[derive(Debug, PartialEq, Eq)] | ||
| pub struct UploadConfigFile { | ||
| pub config_path: PathBuf, | ||
| pub config_type: String, | ||
| pub child_device_id: Option<String>, | ||
| pub source: String, |
There was a problem hiding this comment.
| pub source: String, | |
| pub device_id: String, |
crates/core/c8y_api/src/json_c8y.rs
Outdated
| @@ -117,6 +118,8 @@ impl From<&SoftwareListResponse> for C8yUpdateSoftwareListResponse { | |||
|
|
|||
| Self { | |||
| c8y_software_list: Some(new_list), | |||
| // Must derive the source from the topic | |||
| source: "main".into(), | |||
There was a problem hiding this comment.
This source field should not have been implicit like this. It should have been injected/derived from outside. When we want to extend software list support for child devices, this will have to change.
There was a problem hiding this comment.
Refactored, now it gets the device id from the caller.
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
| C8YRestRequest::C8yUpdateSoftwareListResponse(request) => self | ||
| .send_software_list_http(request.source.clone(), request) | ||
| C8YRestRequest::SoftwareListResponse(request) => self | ||
| .send_software_list_http(request.device_id.clone(), request) |
There was a problem hiding this comment.
| .send_software_list_http(request.device_id.clone(), request) | |
| .send_software_list_http(request) |
When the request instance already has the device_id in it, why extract it and pass it separately?
| .await | ||
| .map(|response| response.into()), | ||
|
|
||
| C8YRestRequest::UploadLogBinary(request) => self | ||
| .upload_log_binary(request.source.clone(), request) | ||
| .upload_log_binary(request.device_id.clone(), request) |
There was a problem hiding this comment.
| .upload_log_binary(request.device_id.clone(), request) | |
| .upload_log_binary(request) |
Same as above.
| .await | ||
| .map(|response| response.into()), | ||
| C8YRestRequest::UploadConfigFile(request) => self | ||
| .upload_config_file(request.source.clone(), request) | ||
| .upload_config_file(request.device_id.clone(), request) |
There was a problem hiding this comment.
| .upload_config_file(request.device_id.clone(), request) | |
| .upload_config_file(request) |
Same as above.
PradeepKiruvale
force-pushed
the
http-retry-closure
branch
from
5d5c541
to
de86c1b
Compare
…e to expired JWT token or internal id. Signed-off-by: Pradeep Kumar K J <pkj@softwareag.com>
PradeepKiruvale
force-pushed
the
http-retry-closure
branch
from
de86c1b
to
ac3ce07
Compare
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
Proposed changes
When an HTTP interaction with
Cumulocityfails withJWTtoken and retry the HTTP request.internal-idand retry the HTTP request.C8yEndPointto cachechild_internal_idandmain_device_internal id. Also, cache thetokeninC8yEndPointTypes of changes
Paste Link to the issue
#1201
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments