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

Update tplink config to include aes keys #125685

Merged
merged 4 commits into from
Sep 10, 2024
Merged

Update tplink config to include aes keys #125685

merged 4 commits into from
Sep 10, 2024

Conversation

sdb9696
Copy link
Contributor

@sdb9696 sdb9696 commented Sep 10, 2024 •
edited
Loading

Proposed change

PR for storing aes keys in the config entry:

  • Addresses an issue whereby if the device is in an error state keys are generated repeatedly using up a lot of cpu time
  • No longer stores the whole device config to allow separation between values updated via discovery from those updated after connection.
  • Passes the actual device from discovery through to the config flow to prevent an extra discovery request.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

@home-assistant
Copy link

Hey there @rytilahti, @bdraco, mind taking a look at this pull request as it has been labeled with an integration (tplink) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of tplink can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign tplink Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

@bdraco
Copy link
Member

bdraco commented Sep 10, 2024 •
edited
Loading

This is larger than I would like to go in a patch release.. its a hard call here since when it gets into this state, most of the run time of the whole event loop is regenerating the RSA key over and over again.
Screenshot 2024-09-10 at 12 13 25 PM

@bdraco
Copy link
Member

bdraco commented Sep 10, 2024

init changes look good
config flow changes look good

@bdraco
Copy link
Member

bdraco commented Sep 10, 2024

testing this now

@bdraco
Copy link
Member

bdraco commented Sep 10, 2024

all devices working

@bdraco
Copy link
Member

bdraco commented Sep 10, 2024

config entry migrations look good

@bdraco
Copy link
Member

bdraco commented Sep 10, 2024

keys are saved for working devices

@bdraco
Copy link
Member

bdraco commented Sep 10, 2024

restarting now

@sdb9696 sdb9696 marked this pull request as ready for review September 10, 2024 18:32
bdraco
bdraco reviewed Sep 10, 2024
View reviewed changes
tests/components/tplink/test_init.py
Comment on lines +719 to +720
"homeassistant.components.tplink.async_create_clientsession",
return_value="Foo",
Copy link
Member

@bdraco bdraco Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is it possible to patch the lib here instead of HA internals? I realize this is the public API exposed to the integration, and its unlikely to change, but it would still be better to avoid patching it.

Copy link
Member

@bdraco bdraco Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This isn't great but we are patching the API being exposed to the integration so its less concerning. We should clean this up in the future so we don't make this PR any larger as it likely will need another lib bump to make it patchable in the lib, and I'm already concerned about the size of this PR for a patch release. I think we need to do it for a patch release given the impact of the problem though.

@bdraco
Copy link
Member

bdraco commented Sep 10, 2024

1 comment for the tests, doing profiles now

@bdraco
Copy link
Member

bdraco commented Sep 10, 2024

profile is clean

confirmed issue is solved

@bdraco
Copy link
Member

bdraco commented Sep 10, 2024

only thing aes related in purple and thats just loading the existing key

Screenshot 2024-09-10 at 1 42 11 PM

@sdb9696
Copy link
Contributor Author

sdb9696 commented Sep 10, 2024

only thing aes related in purple and thats just loading the existing key

In a subsequent PR maybe we could generate the first key in an executor thread?

@bdraco
Copy link
Member

bdraco commented Sep 10, 2024

only thing aes related in purple and thats just loading the existing key

In a subsequent PR maybe we could generate the first key in an executor thread?

Thats a good idea. Even a single generation blocks the loop for longer than we usually would like

@bdraco bdraco added this to the 2024.9.2 milestone Sep 10, 2024
@bdraco bdraco merged commit 40ee39f into home-assistant:dev Sep 10, 2024
30 checks passed
@sdb9696 sdb9696 deleted the tplink/fix/aes_keys branch September 10, 2024 21:39
@github-actions github-actions bot locked and limited conversation to collaborators Sep 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bdraco bdraco bdraco approved these changes

@rytilahti rytilahti Awaiting requested review from rytilahti rytilahti is a code owner

Assignees

@bdraco bdraco

@rytilahti rytilahti

Labels
by-code-owner cherry-picked cla-signed code-quality core has-tests integration: tplink Quality Scale: platinum
Projects
None yet
Milestone
2024.9.2
Development

Successfully merging this pull request may close these issues.
4 participants
@sdb9696 @bdraco @frenck @rytilahti