Fix incorrect 200 for a 401 from NMAgent #1799
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
timraymond
requested review from
rsagasthya and
rbtr
and removed request for
a team
|
blocked on lints |
|
is this related to #1787? |
3 tasks
@rbtr, yes. As CNS is expected to just relay NMA's response (byte64 encoded) back to DNC for any decision making, we shouldn't be handling the error status code in CNA's server-side logic. I have closed my PR with an appropriate comment. |
|
Lints cleared. Please review. |
rbtr
previously approved these changes
Feb 10, 2023
smittal22
previously approved these changes
Feb 10, 2023
rbtr
force-pushed
the
traymond/fix-incorrect-cns-200
branch
from
2de29b7
to
02f40b4
Compare
timraymond
force-pushed
the
traymond/fix-incorrect-cns-200
branch
from
02f40b4
to
5228ceb
Compare
|
@rbtr approval needed again when you get a chance |
rbtr
previously approved these changes
Feb 13, 2023
ramiro-gamarra
requested changes
Feb 13, 2023
In scenarios where the subnet token does not match (leading to a 401 from NMAgent), CNS returns a 200 for the PublishStatusCode. This is incorrect, and a 401 should be returned instead. This leads clients of CNS to take incorrect action on, what they believe to be, a successful response.
It's important that we ensure that PublishResponseBody is, indeed, JSON. Also, that JSON needs to have an httpStatusCode property with its value set to the response code returned from NMAgent (in this test case, a 401).
The UnpublishStatusCode was not being set by CNS, which could lead to misbehavior from clients that expect it to be set appropriately. This uses the StatusCode provided by the nmagent.Error to propagate the httpStatusCode.
timraymond
force-pushed
the
traymond/fix-incorrect-cns-200
branch
from
5228ceb
to
620dd55
Compare
One of these lints is inappropriate in a test, so it's been silenced (adding context to the HTTP Request). The other one is marginal, but easy to fix, so there's now a check for an error from JSON encoding.
ramiro-gamarra
approved these changes
Feb 14, 2023
rjdenney
pushed a commit
that referenced
this pull request
Mar 13, 2023
In scenarios where the subnet token does not match (leading to a 401 from NMAgent), CNS returns a 200 for the PublishStatusCode. This is incorrect, and a 401 should be returned instead. This leads clients of CNS to take incorrect action on, what they believe to be, a successful response.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In scenarios where the subnet token does not match (leading to a 401 from NMAgent), CNS returns a 200 for the PublishStatusCode. This is incorrect, and a 401 should be returned instead. This leads clients of CNS to take incorrect action on, what they believe to be, a successful response.