fix: Fix to make mender-connect request a new JWT token on its own, so its not dependent on mender-update to do so #134

SebOpsahl · 2024-02-29T23:42:19Z

Changelog: Fix to make mender-connect request a new JWT token on its own, so its not dependent on mender-update to do so. Previously, mender-connect relied the Mender client to request new JWT tokens. Since the split of mender into mender-update and mender-auth, it is theoretically possible to run mender-connect without mender-update, in which case it wouldn't request JWT tokens without this fix.

Ticket: MEN-6877

mender-test-bot · 2024-02-29T23:42:44Z

@SebOpsahl, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline".

my commands and options

You can trigger a pipeline on multiple prs with:

mentioning me and start pipeline --pr mender/127 --pr mender-connect/255

You can start a fast pipeline, disabling full integration tests with:

mentioning me and start pipeline --fast

You can trigger GitHub->GitLab branch sync with:

mentioning me and sync

You can cherry pick to a given branch or branches with:

mentioning me and:

 cherry-pick to:
 * 1.0.x
 * 2.0.x

lluiscampos

The fix looks good to me. Have you experimented a bit with the fix on a QEMU run? I think this deserves some manual testing also on top of our tests, for good measure.

See my comments below.

One other thing: the changelog deserves a bit more info. I would keep the commit title short, and write a longer entry that describes more of the historical reason for this. For example:

Fix bla bla bla.
Previously, mender-connect relied the Mender client to request new JWT tokens. Since the split of mender into mender-update and mender-auth, it is theoretically possible to run mender-connect without mender-update, in which case it wouldn't request JWT tokens without this fix.

lluiscampos · 2024-03-04T12:56:04Z

client/mender/auth.go

@@ -33,7 +33,7 @@ const (

 var timeout = 10 * time.Second

-// AuthClient is the interface for the Mender Authentication Manager clilents
+// AuthClient is the interface for the Mender Authentication Manager clients


This should go on a different commit.

Specially for fixes, where we might need to cherry-pick the fix to supported branches, we don't want a typo fix to be in the way.

app/daemon_test.go

SebOpsahl · 2024-03-04T21:31:23Z

The fix looks good to me. Have you experimented a bit with the fix on a QEMU run? I think this deserves some manual testing also on top of our tests, for good measure.

Yes I have manually tested it with a QEMU run with yocto on mender integration to ensure it behaves as it should during running.

SebOpsahl · 2024-03-04T21:32:39Z

I fixed the requests you made, so it should look good now. Good catches, thanks :)

lluiscampos · 2024-03-05T07:35:22Z

@SebOpsahl

You still haven't split the typo into a different commit (ref here)
For the changelog, we are still missing to say what have we fixed. "Fix bla bla bla" was meant to symbolize the text you had, and then extend it with the historical note. So please prepend something more explicit like "Fix logic for mender-connect to request a new JTW token on its own"
- Also note that our changelogs are in markdown format, so a nice-to-have thing is to quote the binaries like mender-connect or mender-auth 😃

SebOpsahl · 2024-03-05T09:55:04Z

@mender-test-bot start pipeline

mender-test-bot · 2024-03-05T09:55:36Z

Hello 😺 I created a pipeline for you here: Pipeline-1201262197

Build Configuration Matrix

Key	Value
AUDITLOGS_REV	master
BUILD_BEAGLEBONEBLACK	true
BUILD_CLIENT	true
BUILD_QEMUX86_64_BIOS_GRUB	true
BUILD_QEMUX86_64_BIOS_GRUB_GPT	true
BUILD_QEMUX86_64_UEFI_GRUB	true
BUILD_VEXPRESS_QEMU	true
BUILD_VEXPRESS_QEMU_FLASH	true
BUILD_VEXPRESS_QEMU_UBOOT_UEFI_GRUB	true
CREATE_ARTIFACT_WORKER_REV	master
DEPLOYMENTS_ENTERPRISE_REV	master
DEPLOYMENTS_REV	master
DEVICEAUTH_ENTERPRISE_REV	master
DEVICEAUTH_REV	master
DEVICECONFIG_REV	master
DEVICECONNECT_REV	master
DEVICEMONITOR_REV	master
GENERATE_DELTA_WORKER_REV	master
GUI_REV	master
INTEGRATION_REV	master
INVENTORY_ENTERPRISE_REV	master
INVENTORY_REV	master
IOT_MANAGER_REV	master
MENDER_ARTIFACT_REV	master
MENDER_BINARY_DELTA_REV	master
MENDER_CLI_REV	master
MENDER_CONFIGURE_MODULE_REV	master
MENDER_CONNECT_REV	pull/134/head
MENDER_CONVERT_REV	master
MENDER_GATEWAY_REV	master
MENDER_REV	master
MENDER_SETUP_REV	master
MENDER_SNAPSHOT_REV	master
MONITOR_CLIENT_REV	master
MTLS_AMBASSADOR_REV	master
RUN_BACKEND_INTEGRATION_TESTS	true
RUN_INTEGRATION_TESTS	true
TENANTADM_REV	master
TEST_QEMUX86_64_BIOS_GRUB	true
TEST_QEMUX86_64_BIOS_GRUB_GPT	true
TEST_QEMUX86_64_UEFI_GRUB	true
TEST_VEXPRESS_QEMU	true
TEST_VEXPRESS_QEMU_FLASH	true
TEST_VEXPRESS_QEMU_UBOOT_UEFI_GRUB	true
USERADM_ENTERPRISE_REV	master
USERADM_REV	master
WORKFLOWS_ENTERPRISE_REV	master
WORKFLOWS_REV	master

SebOpsahl · 2024-03-08T00:01:11Z

When running the big test the “test:acceptance:qemux86_64:uefi_grub:cross-platform” test fails, even though “test:acceptance:qemux86_64:uefi_grub” does not. So some of the cross-platform tests fail and returns the following errors, which seems to be due to loss of connection.

FAILED test_mender-connect.py::TestMenderConnect::test_mender_connect_auth_changes - Failed: Timed out waiting for 'Connection established with http://localhost:5000/' (URL to view test: https://github.com/mendersoftware/meta-mender/blob/2ec7f2e0e83c6557fd1b8ad74f88c733c3bf28c6/tests/acceptance/test_mender-connect.py#L274)
FAILED test_mender-connect.py::TestMenderConnect::test_mender_connect_reconnect - Failed: Timed out waiting for 'connection manager failed to connect' (URL to view test: https://github.com/mendersoftware/meta-mender/blob/2ec7f2e0e83c6557fd1b8ad74f88c733c3bf28c6/tests/acceptance/test_mender-connect.py#L274)

How may it be that it works for the other tests, but not testing for cross platform?

lluiscampos · 2024-03-08T09:00:29Z

(...) How may it be that it works for the other tests, but not testing for cross platform?

The test is only run for that platform. If you look into the details of the test report you will find them mark as skip.

The name might be confusing, but the "cross platform" tests are the ones that are independent of the platform so we run them only once. The tests at hand are runtime tests for the mender-connect software, so these we consider "cross platform" because it doesn't matter how you integrate Mender on the platform, the software will run the same way once on the device. (Does this explanation make sense?)

We need to figure out if the tests are failing due to a failure in the software or is the test that needs some modification. For example if we have modified the text in the logs or the order of them it is possible that the test expectations are not met but the software is still fine.

SebOpsahl · 2024-03-08T19:09:41Z

Oh okey, I understand now. Will look into it 👍

SebOpsahl · 2024-03-08T21:40:34Z

@mender-test-bot start pipeline

mender-test-bot · 2024-03-08T21:41:06Z

Hello 😺 I created a pipeline for you here: Pipeline-1206911451

Build Configuration Matrix

Key	Value
AUDITLOGS_REV	master
BUILD_BEAGLEBONEBLACK	true
BUILD_CLIENT	true
BUILD_QEMUX86_64_BIOS_GRUB	true
BUILD_QEMUX86_64_BIOS_GRUB_GPT	true
BUILD_QEMUX86_64_UEFI_GRUB	true
BUILD_VEXPRESS_QEMU	true
BUILD_VEXPRESS_QEMU_FLASH	true
BUILD_VEXPRESS_QEMU_UBOOT_UEFI_GRUB	true
CREATE_ARTIFACT_WORKER_REV	master
DEPLOYMENTS_ENTERPRISE_REV	master
DEPLOYMENTS_REV	master
DEVICEAUTH_ENTERPRISE_REV	master
DEVICEAUTH_REV	master
DEVICECONFIG_REV	master
DEVICECONNECT_REV	master
DEVICEMONITOR_REV	master
GENERATE_DELTA_WORKER_REV	master
GUI_REV	master
INTEGRATION_REV	master
INVENTORY_ENTERPRISE_REV	master
INVENTORY_REV	master
IOT_MANAGER_REV	master
MENDER_ARTIFACT_REV	master
MENDER_BINARY_DELTA_REV	master
MENDER_CLI_REV	master
MENDER_CONFIGURE_MODULE_REV	master
MENDER_CONNECT_REV	pull/134/head
MENDER_CONVERT_REV	master
MENDER_GATEWAY_REV	master
MENDER_REV	master
MENDER_SETUP_REV	master
MENDER_SNAPSHOT_REV	master
MONITOR_CLIENT_REV	master
MTLS_AMBASSADOR_REV	master
RUN_BACKEND_INTEGRATION_TESTS	true
RUN_INTEGRATION_TESTS	true
TENANTADM_REV	master
TEST_QEMUX86_64_BIOS_GRUB	true
TEST_QEMUX86_64_BIOS_GRUB_GPT	true
TEST_QEMUX86_64_UEFI_GRUB	true
TEST_VEXPRESS_QEMU	true
TEST_VEXPRESS_QEMU_FLASH	true
TEST_VEXPRESS_QEMU_UBOOT_UEFI_GRUB	true
USERADM_ENTERPRISE_REV	master
USERADM_REV	master
WORKFLOWS_ENTERPRISE_REV	master
WORKFLOWS_REV	master

mender-test-bot · 2024-03-10T19:54:46Z

Merging these commits will result in the following changelog entries:

Changelogs

SebOpsahl · 2024-03-10T21:15:00Z

@mender-test-bot start pipeline

mender-test-bot · 2024-03-10T21:15:32Z

Hello 😺 I created a pipeline for you here: Pipeline-1208047869

Build Configuration Matrix

Key	Value
AUDITLOGS_REV	master
BUILD_BEAGLEBONEBLACK	true
BUILD_CLIENT	true
BUILD_QEMUX86_64_BIOS_GRUB	true
BUILD_QEMUX86_64_BIOS_GRUB_GPT	true
BUILD_QEMUX86_64_UEFI_GRUB	true
BUILD_VEXPRESS_QEMU	true
BUILD_VEXPRESS_QEMU_FLASH	true
BUILD_VEXPRESS_QEMU_UBOOT_UEFI_GRUB	true
CREATE_ARTIFACT_WORKER_REV	master
DEPLOYMENTS_ENTERPRISE_REV	master
DEPLOYMENTS_REV	master
DEVICEAUTH_ENTERPRISE_REV	master
DEVICEAUTH_REV	master
DEVICECONFIG_REV	master
DEVICECONNECT_REV	master
DEVICEMONITOR_REV	master
GENERATE_DELTA_WORKER_REV	master
GUI_REV	master
INTEGRATION_REV	master
INVENTORY_ENTERPRISE_REV	master
INVENTORY_REV	master
IOT_MANAGER_REV	master
MENDER_ARTIFACT_REV	master
MENDER_BINARY_DELTA_REV	master
MENDER_CLI_REV	master
MENDER_CONFIGURE_MODULE_REV	master
MENDER_CONNECT_REV	pull/134/head
MENDER_CONVERT_REV	master
MENDER_GATEWAY_REV	master
MENDER_REV	master
MENDER_SETUP_REV	master
MENDER_SNAPSHOT_REV	master
MONITOR_CLIENT_REV	master
MTLS_AMBASSADOR_REV	master
RUN_BACKEND_INTEGRATION_TESTS	true
RUN_INTEGRATION_TESTS	true
TENANTADM_REV	master
TEST_QEMUX86_64_BIOS_GRUB	true
TEST_QEMUX86_64_BIOS_GRUB_GPT	true
TEST_QEMUX86_64_UEFI_GRUB	true
TEST_VEXPRESS_QEMU	true
TEST_VEXPRESS_QEMU_FLASH	true
TEST_VEXPRESS_QEMU_UBOOT_UEFI_GRUB	true
USERADM_ENTERPRISE_REV	master
USERADM_REV	master
WORKFLOWS_ENTERPRISE_REV	master
WORKFLOWS_REV	master

SebOpsahl · 2024-04-24T09:20:08Z

@kacf Problem to fix:
Make mender-connect request a new JWT token on its own, so its not dependent on mender-update to do so.

Proposed solution (PR):
Previously FetchJWTToken() wasn’t called which wouldn’t let it call the JWT token independently, which made it dependent on mender-update. The PR proposes a solution where FetchJWTToken() is used to fetch the token, to then for WaitForJwtTokenStateChange() to retrieve it in it’s waiting. I and Lluis have talked an the solution should look good.

Proposed change in test:
Since the tests is based on mocks it is proposed to set the token, wait for a connection and then send a signal, simulating the task of FetchJWTToken()

With these changes the test_mender_connect_auth_changes() which previously didn’t work now is successful. This test case however from the test_mender_connect_reconnect() test function fails. All the other test cases from the function is successful. The one thing separating the failing test case from the others is that this test case focuses on killing the server and then waiting for the error.
Any idea as to what might be causing the error? The error returned is Failed: Timed out waiting for 'error reconnecting:'

…on its own, so its not dependent on `mender-update` to do so. Previously `FetchJWTToken()` wasn’t called which wouldn’t let it call the JWT token independently, which made it dependent on `mender-update`. The PR to do this 👉 mendersoftware/mender-connect#134 proposed to fix this issue using `FetchJWTToken()` to fetch the token, to then for `WaitForJwtTokenStateChange()` to retrieve it in it’s waiting. The old mocks for the tests seemed to be more tailored to test the functionality with `GetJWTToken()`. This PR proposes a change in the mocks and test flow to better test the new implementation (where the 'new implementation' is the one proposed in the other PR for ticket MEN-6877) Changelog: None Ticket: MEN-6877 Signed-off-by: Sebastian Opsahl <sebastian.opsahl@northern.tech>

SebOpsahl · 2024-05-26T20:44:53Z

app/daemon.go

@@ -312,57 +312,49 @@ func (d *MenderShellDaemon) dbusEventLoop(client mender.AuthClient) {
 			break
 		}

-		if d.needsReconnect() {


In this newest commit I removed this extra check "if d.needsReconnect" early in the function which set the needsReconnect value to True. Rather the code now only have one check. Having these two checks doing the same thing must have delayed the tests and made it fail.

SebOpsahl · 2024-05-26T20:47:15Z

app/daemon.go


 					// If the server (Mender client proxy) closed the connection, it is likely that
 					// both messageLoop is asking for a reconnection and we got JwtTokenStateChange
 					// signal. So drain here the reconnect channel and reconnect only once
 					if d.needsReconnect() {
-						log.Debug("dbusEventLoop: drained reconnect req channel")
+						log.Trace("dbusEventLoop: daemon needs to reconnect")


Reference point for message above. Only now one check for setting needsReconnect to True

Okey. But I think now the comment above with regards to "draining" the channel is obsolete, as with the current code this is the only point where the message loop can signal a re-connection.

I think the code is good but we need to adjust the miss-leading comment (or remove?)

…connect This PR is made to change the mocks and testflow in tests to better fit the new implementation of the PR 👉 mendersoftware/mender-connect#134. This PR fixes it so `mender-connect` requests a new JWT token on its own, so its not dependent on `mender-update` to do so. Previously `FetchJWTToken()` wasn’t called which wouldn’t let it call the JWT token independently, which made it dependent on `mender-update`. The proposed fix is using `FetchJWTToken()` to fetch the token, to then for `WaitForJwtTokenStateChange()` to retrieve it in it’s waiting. The old mocks and test flow for the tests seemed to be more tailored to test the functionality with `GetJWTToken()`. This PR proposes a change in the mocks and test flow to better test the new implementation. Changelog: None Ticket: MEN-6877 Signed-off-by: Sebastian Opsahl <sebastian.opsahl@northern.tech>

lluiscampos · 2024-05-28T07:53:02Z

app/daemon.go

+					d.serverJwt = token
+					d.serverUrl = serverURL
+					d.postEvent(e)
+					log.Tracef("(dbusEventLoop) posting Event: %s", e.event)


Either we move this entry to before the d.postEvent(e) line or we change the text for something like "posted Event: ..."

lluiscampos · 2024-05-28T07:55:30Z

app/daemon.go


 					// If the server (Mender client proxy) closed the connection, it is likely that
 					// both messageLoop is asking for a reconnection and we got JwtTokenStateChange
 					// signal. So drain here the reconnect channel and reconnect only once
 					if d.needsReconnect() {
-						log.Debug("dbusEventLoop: drained reconnect req channel")
+						log.Trace("dbusEventLoop: daemon needs to reconnect")


Okey. But I think now the comment above with regards to "draining" the channel is obsolete, as with the current code this is the only point where the message loop can signal a re-connection.

I think the code is good but we need to adjust the miss-leading comment (or remove?)

Fix to make `mender-connect` request a new JWT token on its own, so its not dependent on `mender-update` to do so. Previously, `mender-connect` relied the Mender client to request new JWT tokens. Since the split of `mender` into `mender-update` and `mender-auth`, it is theoretically possible to run `mender-connect` without `mender-update`, in which case it wouldn't request JWT tokens without this fix. Changelog: None Ticket: MEN-6877 Signed-off-by: Sebastian Opsahl <sebastian.opsahl@northern.tech>

SebOpsahl force-pushed the master branch from 2baab53 to c0e2284 Compare February 29, 2024 23:52

SebOpsahl requested a review from a user March 1, 2024 00:05

lluiscampos self-requested a review March 1, 2024 10:19

lluiscampos requested changes Mar 4, 2024

View reviewed changes

SebOpsahl force-pushed the master branch 2 times, most recently from ac6db0c to d7063f8 Compare March 4, 2024 21:12

SebOpsahl force-pushed the master branch 2 times, most recently from 4521af3 to d8556ae Compare March 5, 2024 09:49

lluiscampos approved these changes Mar 5, 2024

View reviewed changes

SebOpsahl requested a review from lluiscampos March 8, 2024 00:04

SebOpsahl force-pushed the master branch from d8556ae to 3ae0411 Compare March 8, 2024 21:13

SebOpsahl force-pushed the master branch 3 times, most recently from 515506c to 9c8b3a8 Compare March 10, 2024 19:54

SebOpsahl force-pushed the master branch 2 times, most recently from ac207fb to f7bff76 Compare March 11, 2024 08:27

SebOpsahl force-pushed the master branch 5 times, most recently from 4db0a34 to 2e6bd05 Compare March 11, 2024 15:12

SebOpsahl closed this Apr 15, 2024

SebOpsahl reopened this Apr 15, 2024

SebOpsahl force-pushed the master branch from 2e6bd05 to a72ee75 Compare April 22, 2024 07:12

lluiscampos mentioned this pull request May 3, 2024

chore: updating mocks for tests to test mender-connect JWT request handling mendersoftware/meta-mender#2108

Open

SebOpsahl force-pushed the master branch from a72ee75 to b6b84b9 Compare May 26, 2024 20:13

SebOpsahl commented May 26, 2024

View reviewed changes

lluiscampos approved these changes May 28, 2024

View reviewed changes

SebOpsahl force-pushed the master branch 2 times, most recently from 620e97f to e8ac42f Compare May 29, 2024 21:08

SebOpsahl force-pushed the master branch from e8ac42f to 8cfe7c5 Compare June 6, 2024 09:14

SebOpsahl force-pushed the master branch from 8cfe7c5 to 0630c41 Compare June 6, 2024 10:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fix to make mender-connect request a new JWT token on its own, so its not dependent on mender-update to do so #134

fix: Fix to make mender-connect request a new JWT token on its own, so its not dependent on mender-update to do so #134

SebOpsahl commented Feb 29, 2024 •

edited

Loading

mender-test-bot commented Feb 29, 2024

lluiscampos left a comment

lluiscampos Mar 4, 2024

SebOpsahl commented Mar 4, 2024 •

edited

Loading

SebOpsahl commented Mar 4, 2024

lluiscampos commented Mar 5, 2024

SebOpsahl commented Mar 5, 2024

mender-test-bot commented Mar 5, 2024

SebOpsahl commented Mar 8, 2024

lluiscampos commented Mar 8, 2024

SebOpsahl commented Mar 8, 2024

SebOpsahl commented Mar 8, 2024

mender-test-bot commented Mar 8, 2024

mender-test-bot commented Mar 10, 2024

SebOpsahl commented Mar 10, 2024

mender-test-bot commented Mar 10, 2024

SebOpsahl commented Apr 24, 2024

SebOpsahl May 26, 2024

SebOpsahl May 26, 2024

lluiscampos May 28, 2024

lluiscampos May 28, 2024

lluiscampos May 28, 2024

fix: Fix to make mender-connect request a new JWT token on its own, so its not dependent on mender-update to do so #134

Are you sure you want to change the base?

fix: Fix to make mender-connect request a new JWT token on its own, so its not dependent on mender-update to do so #134

Conversation

SebOpsahl commented Feb 29, 2024 • edited Loading

mender-test-bot commented Feb 29, 2024

lluiscampos left a comment

Choose a reason for hiding this comment

lluiscampos Mar 4, 2024

Choose a reason for hiding this comment

SebOpsahl commented Mar 4, 2024 • edited Loading

SebOpsahl commented Mar 4, 2024

lluiscampos commented Mar 5, 2024

SebOpsahl commented Mar 5, 2024

mender-test-bot commented Mar 5, 2024

SebOpsahl commented Mar 8, 2024

lluiscampos commented Mar 8, 2024

SebOpsahl commented Mar 8, 2024

SebOpsahl commented Mar 8, 2024

mender-test-bot commented Mar 8, 2024

mender-test-bot commented Mar 10, 2024

Changelogs

SebOpsahl commented Mar 10, 2024

mender-test-bot commented Mar 10, 2024

SebOpsahl commented Apr 24, 2024

SebOpsahl May 26, 2024

Choose a reason for hiding this comment

SebOpsahl May 26, 2024

Choose a reason for hiding this comment

lluiscampos May 28, 2024

Choose a reason for hiding this comment

lluiscampos May 28, 2024

Choose a reason for hiding this comment

lluiscampos May 28, 2024

Choose a reason for hiding this comment

SebOpsahl commented Feb 29, 2024 •

edited

Loading

SebOpsahl commented Mar 4, 2024 •

edited

Loading