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.

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

feat: make ConnectivityListener configurable, and remove unnecessary network availability checks #1100

Merged
merged 17 commits into from
Aug 1, 2023

Conversation

gkc
Copy link
Contributor

@gkc gkc commented Jul 29, 2023

Closes #1099

See analysis here which led to the changes in this PR

  • Eliminate direct use of InternetConnectivityChecker from at_client package except for a method in RemoteSecondary which (a) is unused and now marked @Deprecated and where (b) the usage is non-problematic (checks if atServer's host and port are reachable)
  • Eliminate wrapper class at_client.NetworkUtil and all its usages in at_client
  • Eliminate indirect (via at_client.ConnectivityListener) use of InternetConnectivityChecker from at_client package
  • For backwards compatibility, still exports ConnectivityListener (which is in use by multiple programs including sshnpd), but
    • (a) adds optional constructor parameters which programs can use if they wish to continue to use ConnectivityListener
    • (b) ConnectivityListener is marked deprecated as connectivity listening is not functionality which at_client should export

- What I did

  • Deprecate RemoteSecondary.isAvailable()
  • Remove Monitor's unnecessary use of NetworkUtil.isNetworkAvailable
  • Remove unnecessary use of ConnectivityListener from NotificationServiceImpl
  • Remove SyncServiceImpl's unnecessary use of NetworkUtil.isNetworkAvailable
  • Remove AtClientValidation.validateAtKey's unnecessary use of NetworkUtil.isNetworkAvailable
  • Adjust any unit tests which currently use mock network availability checks to simulate atServer reachability to instead use other mock approaches instead, without changing those tests's other setup or expects
  • Remove the now-unused NetworkUtil class
  • Add hostname, port and a checkInterval as optional parameter to ConnectivityListener constructor and use accordingly
  • Deprecate ConnectivityListener class as it's not a core concern of the at_client package
  • Some software supply chain management
    • adjusted dependency configuration for the crypto packages we depend on
    • defensive measures to prevent the next release of the encrypt package from causing us problems, as the behaviour of IV.fromLength has changed in the main branch (unpublished)

- How I did it

- How to verify it
Tests pass

- Description for the changelog
feat: Make ConnectivityListener configurable by adding optional parameters to its constructor.
feat: Remove unnecessary use of network availability checks in various places

@gkc gkc marked this pull request as ready for review July 29, 2023 17:28
Copy link
Member

@cconstab cconstab left a comment

Choose a reason for hiding this comment

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

I hate that connectivity listener uses tcp rather than ICMP

@cconstab
Copy link
Member

cconstab commented Jul 29, 2023

Another process wrap but well done

https://pub.dev/packages/dart_ping

And some discussion on the topic

dart-lang/sdk#33764

@gkc
Copy link
Contributor Author

gkc commented Jul 29, 2023

I hate that connectivity listener uses tcp rather than ICMP

ConnectivityListener is ours; it wraps InternetConnectivityChecker

Neither are being used by at_client any more so 🤷‍♂️

…gurable-network-checks

# Conflicts:
#	packages/at_client/CHANGELOG.md
@gkc
Copy link
Contributor Author

gkc commented Jul 29, 2023

Another process wrap but well done

https://pub.dev/packages/dart_ping

And some discussion on the topic

dart-lang/sdk#33764

Now that at_client makes no use of it, I'd be happy to mark at_client's ConnectivityListener class as Deprecated and leave it to programs to use whatever they want to instead, since really it's nothing to do with AtClient

docs: update `@Deprecated` message for `RemoteSecondary.isAvailable`
@gkc gkc requested a review from cconstab July 29, 2023 19:54
gkc added 2 commits July 30, 2023 12:22
- add direct dependency on asn1lib
- pin versions for asnlib, crypton and encrypt packages
@gkc
Copy link
Contributor Author

gkc commented Jul 30, 2023

Checks currently failing because one of our cicd hosts is down

@gkc
Copy link
Contributor Author

gkc commented Jul 30, 2023

Checks complete thank you @cpswan

@@ -111,7 +111,8 @@ class AESDecryptionSink extends ChunkedConversionSink<List<int>> {

IV getIV(String? ivBase64) {
if (ivBase64 == null) {
return IV.fromLength(16);
// From the bad old days when we weren't setting IVs
return IV(Uint8List(16));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is a defensive measure to prevent the next release of the encrypt package from causing us problems, as the behaviour of IV.fromLength has changed

@@ -12,7 +12,8 @@ class EncryptionUtil {

static IV getIV(String? ivBase64) {
if (ivBase64 == null) {
return IV.fromLength(16);
// From the bad old days when we weren't setting IVs
return IV(Uint8List(16));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is a defensive measure to prevent the next release of the encrypt package from causing us problems, as the behaviour of IV.fromLength has changed

@gkc gkc requested a review from cpswan July 30, 2023 17:04
@cconstab
Copy link
Member

cconstab commented Jul 30, 2023

Everything works as expected thanks @gkc

Screenshot 2023-07-30 at 10 05 59

Copy link
Member

@cconstab cconstab left a comment

Choose a reason for hiding this comment

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

Tested by hand as well as via Ci/CD

@gkc gkc merged commit 65fb64b into trunk Aug 1, 2023
@gkc gkc deleted the gkc-fewer-more-configurable-network-checks branch February 4, 2024 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement changes proposed in https://github.com/atsign-foundation/sshnoports/issues/292
2 participants