#7746 jwtfile path correction

[ssonthal]

Fixes #7746, #5948

Changes

• List the changes

Types of changes

What types of changes does your code introduce?

• Bugfix (a non-breaking change that fixes an issue)
• New feature (a non-breaking change that adds functionality)
• Breaking change (a change that causes existing functionality not to work as expected)
• Optimization
• Refactoring
• Documentation update
• Build-related changes
• Other: Description

Testing

Requires testing

• Yes
• No

If yes, did you write tests?

• Yes
• No

Notes on testing

Optional. Remove if not applicable.

Documentation

Requires documentation update

• Yes
• No

If yes, link the PR to the docs update or the issue with the details labeled docs. Remove if not applicable.

Requires explanation in Release Notes

• Yes
• No

If yes, fill in the details here. Remove if not applicable.

Remarks

Optional. Remove if not applicable.

[ssonthal]

@rubo / @LukaszRozmej this PR includes the previously suggested changes. Can continue the thread here.

[MarekM25]

I think this change might break setups in one case: if someone used dataDir + jwtSecret, it won’t read the old JWT anymore.

There was a more interesting approach in this pull request: #5979.

This approach will have to be discussed with other teams.

[obasekiosa]

I think this change might break setups in one case: if someone used dataDir + jwtSecret, it won’t read the old JWT anymore.

There was a more interesting approach in this pull request: #5979.

This approach will have to be discussed with other teams.

@ssonthal

True it would break setups, (same as changing the JwtSecretFile config would break setups unless you move the secrete to the new location and tell the consensus client about it).

Note:
if (x) implies that the result of line x is true the proceed otherwise do not.

So A way around that

1. if jwt-secret file path isn't specified then
2. if (1) check if jwt-secret file already exists in previous default directory (which was hard coded so should be a straightforward check).
3. if (2) then log a warning with the specified expected change and the continue with the old secret (or maybe move it to expected dir, and log that too!, moving makes sense because its a file managed by the client not the user.)
4. if (2) is not true then continue with the auto generation in the data directory path if it doesn't already exist.
5. if (1) is not true then continue as expected in (4).

Also, note there a bit of other things being generated in this directory, even after the change, you might want to change them too in another pr after a bit of discussion on what they are.

[ssonthal]

I think this change might break setups in one case: if someone used dataDir + jwtSecret, it won’t read the old JWT anymore.
There was a more interesting approach in this pull request: #5979.
This approach will have to be discussed with other teams.

@ssonthal

True it would break setups, (same as changing the JwtSecretFile config would break setups unless you move the secrete to the new location).

Note: if (x) implies that the result of line x is true the proceed otherwise do not.

So A way around that

1. if jwt-secret file path isn't specified then
2. if (1) check if jwt-secret file already exists in previous default directory (which was hard coded so should be a straightforward check).
3. if (2) then log a warning with the specified expected change and the continue with the old secret (or maybe move it to expected dir, and log that too!, moving makes sense because its a file managed by the client not the user.)
4. if (2) is not true then continue with the auto generation in the data directory path if it doesn't already exist.
5. if (1) is not true then continue as expected in (4).

Also, note there a bit of other things being generated in this directory, even after the change, you might want to change them too in another pr after a bit of discussion on what they are.

I will start working in this direction, then.

[ssonthal]

@obasekiosa can you check once now?

[obasekiosa]

@ssonthal
still making changes?

[ssonthal]

Nope. You can review now.

[obasekiosa]

Nope. You can review now.

@ssonthal

there are still requested but pending changes.
let me know if you can't see any on your end.

[ssonthal]

@obasekiosa no I can't see any request changes on my end.

[obasekiosa]

@obasekiosa no I can't see any request changes on my end.

apologies, seems they've been on pending on my end.

you should see them now.

[ssonthal]

@obasekiosa can you check now?

[ssonthal]

@MarekM25 / @LukaszRozmej / @rubo can you confirm if the changes look fine to you?

[obasekiosa]

a side note: if you intend to take corrective action then they are info logs and not warning logs.

for now until we decide if we need to move or not based on @MarekM25 response,
we can just leave it as is.

[obasekiosa]

@ssonthal
you don't have to always merge master.

once you do that once, and you don't add any additional changes, then its very unlikely that you'd have conflicts with master for a long long while.

[ssonthal]

Got it!