Fix incorrect HTTP status from publish NC #1757
Merged
Conversation
This file contains hidden or 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
force-pushed
the
traymond/fix-missing-success-status
branch
2 times, most recently
from
2f120b6 to
e8f172e
Compare
timraymond
force-pushed
the
traymond/fix-missing-success-status
branch
3 times, most recently
from
5e84b8a to
bd0e627
Compare
ramiro-gamarra
approved these changes
Jan 18, 2023
CNS was responding with an HTTP status code of "0" from NMAgent. Successes are supposed to be 200. The C-style var block at the beginning of publishNetworkContainer was the reason for this. During refactoring, the location where this status code was set to a successful value of 200 was accidentally removed. Because the var block declared the variable and silently initialized it to 0, the compiler did not flag this bug as it otherwise would have. The status code has been removed from this block and explicitly defined and initialized to a correct value of 200. Subsequent error handling will change this as necessary. Also, despite consumers depending on this status, there were no tests to verify that the status was set correctly. Tests have been added to reflect this dependency.
DNC depends on the NMAgent body being set for its vestigial functions of retrying failed requests. Since failed requests will now be retried internally (to CNS) by the NMAgent client, this isn't really necessary anymore. There are versions of DNC out there that depend on this body though, so it needs to be present in order for NC publishing to actually work.
It was discovered that the Unpublish endpoints also omitted the status codes and bodies expected by clients. This adds those and fixes the associated tests to guarantee the expected behavior.
There were two instances where the linter was flagging dynamic errors, but this is just in a test. It's perfectly fine to bend the rules there, since we don't expect to re-use the errors (they really should be t.Fatal / t.Error anyway, but due to legacy we're returning errors here instead).
timraymond
force-pushed
the
traymond/fix-missing-success-status
branch
from
bd0e627 to
d7b68d1
Compare
rjdenney
pushed a commit
to rjdenney/azure-container-networking
that referenced
this pull request
Jan 19, 2023
* Fix incorrect HTTP status from publish NC CNS was responding with an HTTP status code of "0" from NMAgent. Successes are supposed to be 200. The C-style var block at the beginning of publishNetworkContainer was the reason for this. During refactoring, the location where this status code was set to a successful value of 200 was accidentally removed. Because the var block declared the variable and silently initialized it to 0, the compiler did not flag this bug as it otherwise would have. The status code has been removed from this block and explicitly defined and initialized to a correct value of 200. Subsequent error handling will change this as necessary. Also, despite consumers depending on this status, there were no tests to verify that the status was set correctly. Tests have been added to reflect this dependency. * Ensure that NMAgent body is always set DNC depends on the NMAgent body being set for its vestigial functions of retrying failed requests. Since failed requests will now be retried internally (to CNS) by the NMAgent client, this isn't really necessary anymore. There are versions of DNC out there that depend on this body though, so it needs to be present in order for NC publishing to actually work. * Fix missing NMAgent status for Unpublish It was discovered that the Unpublish endpoints also omitted the status codes and bodies expected by clients. This adds those and fixes the associated tests to guarantee the expected behavior. * Silence the linter There were two instances where the linter was flagging dynamic errors, but this is just in a test. It's perfectly fine to bend the rules there, since we don't expect to re-use the errors (they really should be t.Fatal / t.Error anyway, but due to legacy we're returning errors here instead).
smittal22
pushed a commit
to smittal22/azure-container-networking
that referenced
this pull request
Feb 3, 2023
* Fix incorrect HTTP status from publish NC CNS was responding with an HTTP status code of "0" from NMAgent. Successes are supposed to be 200. The C-style var block at the beginning of publishNetworkContainer was the reason for this. During refactoring, the location where this status code was set to a successful value of 200 was accidentally removed. Because the var block declared the variable and silently initialized it to 0, the compiler did not flag this bug as it otherwise would have. The status code has been removed from this block and explicitly defined and initialized to a correct value of 200. Subsequent error handling will change this as necessary. Also, despite consumers depending on this status, there were no tests to verify that the status was set correctly. Tests have been added to reflect this dependency. * Ensure that NMAgent body is always set DNC depends on the NMAgent body being set for its vestigial functions of retrying failed requests. Since failed requests will now be retried internally (to CNS) by the NMAgent client, this isn't really necessary anymore. There are versions of DNC out there that depend on this body though, so it needs to be present in order for NC publishing to actually work. * Fix missing NMAgent status for Unpublish It was discovered that the Unpublish endpoints also omitted the status codes and bodies expected by clients. This adds those and fixes the associated tests to guarantee the expected behavior. * Silence the linter There were two instances where the linter was flagging dynamic errors, but this is just in a test. It's perfectly fine to bend the rules there, since we don't expect to re-use the errors (they really should be t.Fatal / t.Error anyway, but due to legacy we're returning errors here instead).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
CNS was responding with an HTTP status code of "0" from NMAgent. Successes are supposed to be 200. The C-style var block at the beginning of publishNetworkContainer was the reason for this. During refactoring, the location where this status code was set to a successful value of 200 was accidentally removed. Because the var block declared the variable and silently initialized it to 0, the compiler did not flag this bug as it otherwise would have. The status code has been removed from this block and explicitly defined and initialized to a correct value of 200. Subsequent error handling will change this as necessary.
Also, despite consumers depending on this status, there were no tests to verify that the status was set correctly. Tests have been added to reflect this dependency.
Reason for Change:
Issue Fixed:
Requirements:
Notes: