Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix blocked pipeline #4451

Merged
merged 60 commits into from
Mar 9, 2021
Merged

fix blocked pipeline #4451

merged 60 commits into from
Mar 9, 2021

Conversation

ancaantochi
Copy link
Contributor

@ancaantochi ancaantochi commented Feb 20, 2021 •
edited

Sending messages to cloud gets blocked when a leaf device is disabled, removed from scope or removed as a device because edgehub receives Unauthorized exceptions and will keep retrying.

To fix the issue:

  • when AuthenticationMode is Scope and a new flag is true it will drop messages for devices which are not in scope or in scope and disabled. New flag that was added to drop messages is false by default
  • CloudEndpoint gets a CloudProxy when trying to send a message. Before it was returning Option.None if connection failed, now it will return Try which will contain the exception when connection failed. Existing code handles the case if exception is thrown during send message the cloud proxy is closed. Next time when it tries to get the cloud connection it should resync the cache.
  • CloudConnectionProvider calls new method from device scope cache which returns DeviceInvalidStateException when device is disabled or not in scope (deleted device is treated as not is scope). device scope cache might get a response that the device is in scope, but when trying to connect it will receive Unauthorized exception, it that case it should force refresh the cache.
  • device scope cache was changed to treat as device removed from scope when HTTP error codes 400, 401, 403, 404 (before was only 400)

Tests:
CI passed: https://msazure.visualstudio.com/One/_build/results?buildId=39795314&view=results

davilu
davilu reviewed Mar 3, 2021
View reviewed changes
edge-hub/src/Microsoft.Azure.Devices.Edge.Hub.Core/ConnectionManager.cs Outdated
@@ -97,6 +97,14 @@ public async Task<Option<ICloudProxy>> GetCloudConnection(string id)
.Map(c => (ICloudProxy)new RetryingCloudProxy(id, () => this.TryGetCloudConnection(id), c));
}

public async Task<Try<ICloudProxy>> GetCloudConnectionTry(string id)
Copy link
Contributor

@davilu davilu Mar 3, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public async Task<Try<ICloudProxy>> GetCloudConnectionTry(string id)
public async Task<Try<ICloudProxy>> TryGetCloudProxy(string id)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed it to TryGetCloudConnection to be consistent with other GetCloudConnection.

@ancaantochi ancaantochi requested a review from davilu March 5, 2021 20:11
varunpuranik
varunpuranik reviewed Mar 6, 2021
View reviewed changes
edge-hub/src/Microsoft.Azure.Devices.Edge.Hub.CloudProxy/CloudConnectionProvider.cs Outdated
{
Events.ServiceIdentityNotFound(identity);
Option<IClientCredentials> clientCredentials = await this.credentialsCache.Get(identity);
return await clientCredentials
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Expect is a better pattern here?

varunpuranik
varunpuranik previously approved these changes Mar 6, 2021
View reviewed changes
davilu
davilu previously approved these changes Mar 8, 2021
View reviewed changes
@ancaantochi ancaantochi dismissed stale reviews from davilu and varunpuranik via 68d51f7 March 8, 2021 22:52
@ancaantochi ancaantochi merged commit 756b83b into Azure:release/1.1 Mar 9, 2021
@ancaantochi ancaantochi deleted the ancan/blocked-pipeline branch March 9, 2021 18:50
darobs pushed a commit to darobs/iotedge that referenced this pull request Mar 22, 2021
@ancaantochi @davilu @darobs
fail and drop message on authentication error with Scope authenticati…
e9564d1 
…on (Azure#4451)

* modified to fail and drop message on authentication error with scope authenticator

Co-authored-by: David Lu <davilu@microsoft.com>
@darobs darobs mentioned this pull request Mar 22, 2021
Merged
darobs added a commit that referenced this pull request Mar 30, 2021
@darobs @damonbarry
@arsing @ancaantochi @davilu @yophilav @kodiakhq
Update k8s to latest 1.1.1 (#4647)
9165165 
* Update pipeline triggers for release/1.1 (#4365)

* Make comments in the config.yaml for provisioning and certificates clearer. (#4542)

* fail and drop message on authentication error with Scope authentication (#4451)

* Update ASP.NET Core Runtime base images (#4573)

* Prepare for Release 1.1.1 (#4585)

* Fix NugetSecurity warning for Long Haul pipeline (#4588)

* add guid to module and device name (#4589)

* Fix typo in `always_reprovision_on_startup` config.yaml docs. (#4612)

* Prepare for Release 1.1.1 (pt2) (#4617)

Co-authored-by: Damon Barry <damonbarry@users.noreply.github.com>
Co-authored-by: Arnav Singh <arsing@microsoft.com>
Co-authored-by: Anca Antochi <ancan@microsoft.com>
Co-authored-by: David Lu <davilu@microsoft.com>
Co-authored-by: yophilav <54859653+yophilav@users.noreply.github.com>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davilu davilu davilu approved these changes

@varunpuranik varunpuranik Awaiting requested review from varunpuranik

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ancaantochi @varunpuranik @davilu