refactor: simplify c8y_http_proxy#3224
Merged
didier-wenzek merged 17 commits intothin-edge:mainfrom Nov 21, 2024
Merged
Conversation
Contributor
Robot Results
|
didier-wenzek
commented
Nov 7, 2024
crates/extensions/c8y_mapper_ext/src/operations/handlers/config_update.rs
Show resolved
Hide resolved
3aa9fad to
a1ed6f6
Compare
albinsuresh
reviewed
Nov 21, 2024
Contributor
albinsuresh
left a comment
There was a problem hiding this comment.
Everything looks fine, except for dropping the get_internal_id retry logic which may have other side effects. Happy to approve once that comment is resolved.
Comment on lines
+34
to
+39
| let request = HttpRequestBuilder::get(&url_get_id).build()?; | ||
|
|
||
| let mut attempt = 0; | ||
| loop { | ||
| attempt += 1; | ||
| let request = HttpRequestBuilder::get(&url_get_id).build()?; | ||
| let endpoint = request.uri().path().to_owned(); | ||
| let method = request.method().to_owned(); | ||
|
|
||
| match self.http.await_response(request).await? { | ||
| Ok(response) => match response.status() { | ||
| StatusCode::OK => { | ||
| let internal_id_response: InternalIdResponse = response.json().await?; | ||
| let internal_id = internal_id_response.id(); | ||
|
|
||
| return Ok(internal_id); | ||
| } | ||
| StatusCode::NOT_FOUND | StatusCode::UNAUTHORIZED | StatusCode::FORBIDDEN => { | ||
| if attempt > 3 { | ||
| error!("Failed to fetch internal id for {device_id} even after multiple attempts"); | ||
| response.error_for_status()?; | ||
| } | ||
| info!("Re-fetching internal id for {device_id}, attempt: {attempt}"); | ||
| sleep(self.config.retry_interval).await; | ||
| continue; | ||
| } | ||
| code => { | ||
| return Err(C8YRestError::FromHttpError( | ||
| tedge_http_ext::HttpError::HttpStatusError { | ||
| code, | ||
| endpoint, | ||
| method, | ||
| }, | ||
| )) | ||
| } | ||
| }, | ||
|
|
||
| Err(e) => return Err(C8YRestError::FromHttpError(e)), | ||
| } | ||
| } | ||
| let http_result = self.http.await_response(request).await?; | ||
| let http_response = http_result.error_for_status()?; | ||
| let internal_id_response: InternalIdResponse = http_response.json().await?; | ||
| let internal_id = internal_id_response.id(); | ||
| Ok(internal_id) |
Contributor
There was a problem hiding this comment.
From what I remember, the retry mechanism was added to prevent the mapper from failing on startup if the device did not have internet connection at that very moment. Wouldn't that be broken with this simplification? Or we're hoping that the service manager would restart the mapper on this failure anyway and hence it will recover eventually?
Contributor
There was a problem hiding this comment.
Ignore that comment. That internal id lookup was there earlier when we were eagerly fetching the internal id of the main device during the startup (in the C8yHttpActor::init()). Now that we don't have that init logic and internal id is always fetched on-demand, this shouldn't be an issue.
Contributor
Author
There was a problem hiding this comment.
Yes, this was one of the intent, however the removed piece of code was useless in case of internet disruptions: TCP and TLS errors breaking the retry loop.
The only place where a retry might is useful is when publishing the software list on start. But this should be done at a different level - in the c8y mapper and not blindly at the HTTP level.
Comment on lines
+47
to
+50
| let c8y_host = tedge_config | ||
| .c8y | ||
| .try_get(c8y_profile)? | ||
| .http | ||
| .or_config_not_set()? | ||
| .to_string(); | ||
| let c8y_mqtt_host = tedge_config | ||
| .c8y | ||
| .try_get(c8y_profile)? | ||
| .mqtt | ||
| .or_config_not_set()? | ||
| .to_string(); | ||
|
|
||
| let c8y_config = tedge_config.c8y.try_get(c8y_profile)?; | ||
| let auth_proxy_addr = c8y_config.proxy.client.host.clone(); |
Contributor
There was a problem hiding this comment.
Suggested change
| let c8y_host = tedge_config | |
| .c8y | |
| .try_get(c8y_profile)? | |
| .http | |
| .or_config_not_set()? | |
| .to_string(); | |
| let c8y_mqtt_host = tedge_config | |
| .c8y | |
| .try_get(c8y_profile)? | |
| .mqtt | |
| .or_config_not_set()? | |
| .to_string(); | |
| let c8y_config = tedge_config.c8y.try_get(c8y_profile)?; | |
| let auth_proxy_addr = c8y_config.proxy.client.host.clone(); | |
| let c8y_config = tedge_config.c8y.try_get(c8y_profile)?; | |
| let c8y_host = c8y_config | |
| .http | |
| .or_config_not_set()? | |
| .to_string(); | |
| let c8y_mqtt_host = c8y_config | |
| .mqtt | |
| .or_config_not_set()? | |
| .to_string(); | |
| let auth_proxy_addr = c8y_config.proxy.client.host.clone(); |
| StatusCode::OK => { | ||
| let internal_id_response: InternalIdResponse = response.json().await?; | ||
| let internal_id = internal_id_response.id(); | ||
| pub async fn try_get_internal_id(&mut self, device_id: &str) -> Result<String, C8YRestError> { |
Contributor
There was a problem hiding this comment.
I'm assuming that even the remaining functions in this "non-actor" would be moved to handle in the next step.
Bravo555
approved these changes
Nov 21, 2024
Member
Bravo555
left a comment
There was a problem hiding this comment.
Happy to approve as it is and defer the full removal of c8y_http_proxy to another PR, as this one is already quite big.
This is an intermediate step. C8yEndPoint has been enriched with a ProxyUrlGenerator and local_proxy_url method. But the clients has not been changed beyond the miminum to get the C8yEndPoint. They are still using a combination of C8yEndPoint and ProxyUrlGenerator to generate the urls to the local auth proxy. The next step will be to make the ProxyUrlGenerator private and enforce the use of C8yEndPoint::local_proxy_url() Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
This is done my making private the method `maybe_tenant_url` and let the c8y mapper uses `local_proxy_url` instead. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
The C8YHttpProxyActor was adding a level of indirection with little value. Message generated by C8YHttpProxy was pushed to C8YHttpProxyActor and then to an HttpActor. This commit is an intermediate step. The C8YHttpProxyActor is no more an actor but has been removed. The next step will be to merge C8YHttpProxy and C8YHttpProxyActor. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
This endpoint is duplicated into CumulocityConverter::http_proxy Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
This endpoint is already provided by OperationHandler::http_proxy Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
No more used. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
The retry logic was unnecessary complicated (with nested loops, caches and cache invalidation. As a first step these loops are replaced by simple sequences: get the c8y internal id for the device, then use this id to proceed (i.e. to post a software list or to create an event). Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
272d17f to
09b0c2a
Compare
This cache is no more used and in any case this is not right place for a cache as the role of this struct is to name the misc urls used by C8Y REST API. If a cache is re-introduce it should be in the C8Y HTTP proxy. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Now that the local c8y auth proxy is used for all HTTP interaction with c8y, the HTTP actor needs no more JWT token support but has to be configured for TLS in case mTLS is enabled for the local c8y auth proxy. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
09b0c2a to
2c69cdb
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
Can
c8y_http_proxybe fully deprecated and replaced byc8y_auth_proxy?As noted here the c8y mapper is already bypassing
c8y_http_proxyin favor ofc8y_auth_proxybut only partially: to upload the file attached to an event but not to create that event.Actually,
c8y_mapper_ext(and alsoc8y_firmware_manager) can be independent ofc8y_http_proxyandc8y_auth_proxyaltogether. Indeed, the key point is related to url rewriting.c8y_api::http_proxy::C8yEndPointusing thec8y_auth_proxy::url::ProxyUrlGenerator. There is no need for a direct dependency on thec8y_auth_proxycrate beyond that url generator.c8y_http_proxycan be simplified to focus only one URL generation.Tasks:
c8y_http_proxyc8y_http_proxyfromc8y_auth_proxy,c8y_firmware_plugin, andc8y_firmware_manager(this is only to get some struct definitions).c8y_http_proxy.Types of changes
Paste Link to the issue
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments