This PR makes some minor changes to the NMAgent client package as a part of a larger work item. This commit adds a NotFound method to the error returned by the NMAgent client. It also adds special handling to treat a 400 BadRequest as a NotFound when returned by the DeleteNetwork API call, since this case should be interpreted as a NotFound by the caller.
* Add DeleteNetwork call to NMAgent client
* Clean up comments and unused struct tags
---------
Co-authored-by: Diego Becerra <diegobecerra@microsoft.com>
* initial delete NC changes to include body
* initial delete NC changes to include body
* lint error
* initial delete NC changes to include body
* initial delete NC changes to include body
* lint error
* test change
* test change
* add comment
* tmp commit for onbaording nma v2
* Remove test output file
* Remove unnecessary code when CNS onboard get nc version list without
token
* tmp commit to fix getnc version tests when onboarding nc version api v2
from nmagent.
* Fix the unit test for nmagent v2 api change.
* Fix unit test TestGetNetworkContainerVersionStatus
* Revert back to GetNCVersionF test.
* Roll back to get nc version api v1 for test.
* Continue revert back and store nc version url
* Onboard nmagent get nc version api v2.
* Address pr feedback of returning early and remove comment out code.
* Remove unnecessary ncVersionURLs and NCVersionRequest.
* Remove unnecessary variables.
* Update nmagent get nc version api v2 to v2 url
* Remove comment out code.
* tmp commit for onbaording nma v2
* Remove test output file
* Remove unnecessary code when CNS onboard get nc version list without
token
* tmp commit to fix getnc version tests when onboarding nc version api v2
from nmagent.
* Fix the unit test for nmagent v2 api change.
* Fix unit test TestGetNetworkContainerVersionStatus
* Revert back to GetNCVersionF test.
* Roll back to get nc version api v1 for test.
* Continue revert back and store nc version url
* Onboard nmagent get nc version api v2.
* Address pr feedback of returning early and remove comment out code.
* Remove unnecessary ncVersionURLs and NCVersionRequest.
* Remove unnecessary variables.
* Update nmagent get nc version api v2 to v2 url
* Remove comment out code.
* Update nmagent get nc version list.
* Address feedback and fix golint
* Fix lint issue.
* Fix the remaining 2 lint issues.
* Revert back test error generation to address feedback.
In cases where clients using a WireserverTransport (namely the NMAgent
one) do not wish to provide a body, one must be provided to satisfy
quirks of Wireserver. Initially, an empty Go string was thought to be
sufficient, but this is not. Wireserver expects an empty JSON string:
specifically the 2-byte sequence "".
Furthermore, this logic was only being triggered on cases where a nil
body was presented to the WireserverTransport's logic. Given that there
are now more linters enforcing the use of `http.NoBody`, this is
distinct from `nil` and must be separately checked for (but treated in
the same way).
Tests and inline documentation have been updated to reflect this new
understanding.
* Add Content-Type for outgoing Wireserver POSTs
Because the Body of the request was replaced, Go also has no idea what
the content type should be. It can't read from the reader to perform the
detection, so this just sets it to JSON manually. As mentioned in the
comments, this is a good idea anyway since we're being explicit rather
than relying on heuristic methods to do the right thing.
* Switch PublishNetworkContainer to nmagent package
This removes the PublishNetworkContainer method from the CNS client's
nmagent package in favor of using the one from the top-level nmagent
client package. This is to ensure that there's only one nmagent client
to maintain.
* Use Unpublish method from nmagent package
The existing unpublish endpoints within CNS used the older nmagent
client. This converts them to use the newer one.
* Use JoinNetwork from new nmagent client
The API in CNS was using the old nmagent client endpoints which we want
to phase out so that there is exactly one nmagent client across all
systems.
* Add SupportedAPIs method to nmagent and use it
The CNS client uses this SupportedAPIs method to proxy requests from DNC
and detect capabilities of the node.
* Add GetNCVersion to nmagent and use it in CNS
CNS previously used its own client method for accessing the GetNCVersion
endpoint of nmagent. In the interest of having a singular NMAgent
client, this adopts the behavior into the nmagent package and uses it
within CNS instead.
* Use test helpers for context and checking errs
Checking whether the error was present and should it be present was
annoying and repetitive, so there's a helper now to take care of that.
Similarly, contexts are necessary but it's nice to create them from the
test's deadline itself. There's a helper to do that now as well.
* Fix broken tests & improve the suite
There were some broken tests left over from implementing new NMAgent
interface methods. This makes those tests pass and also improves the
test suite by detangling mixed concerns of the utility functions within.
Many of them returned errors and made assertions, which made them
confusing to use. The utility functions now only do things you request,
and the tests themselves are the only ones making assertions (sometimes
through helpers that were added to make those assertions easier).
* Add GetNCVersionList endpoint to NMAgent client
This is the final endpoint that was being used in CNS without being
present in the "official" NMAgent client. This moves that
implementation, more-or-less, to the nmagent package and consumes it in
NMAgent through an interface.
* Remove incorrect error shadowing
There were a few places where errors were shadowed. Also removed the
C-ism of pre-declaring variables for the sake of pre-declaring
variables.
* Replace error type assertions with errors.As
In two instances, type assertions were used instead of errors.As. Apart
from being less obvious, there are apparently instances where this can
be incorrect with error wrapping.
Also, there was an instance where errors.As was mistakenly used in the
init clause of an if statement, instead of the predicate. This was
corrected to be a conjunctive predicate converting the error and then
making assertions using that error. This is safe because short-circuit
evaluation will prevent the second half of the predicate from being
evaluated if the error is not of that type.
* Use context for joinNetwork
The linter rightly pointed out that context could be trivially
propagated further than it was. This commit does exactly that.
* Add error wrapping to an otherwise-opaque error
The linter highlighted this error, showing that the error returned would
be confusing to a user of this function. This wraps the error indicating
that a join network request was issued at the point where the error was
produced, to aid in debugging.
* Remove error shadowing in the tests
This is really somewhat unnecessary because it's just a test. It
shouldn't impact correctness, since the errors were properly scoped to
their respective if statements. However, to prevent other people's
linters from complaining, this corrects the lint.
* Silence complaints about dynamic errors in tests
This is an unnecessary lint in a test, because it's useful to be able to
define errors on the fly when you need them in a test. However, the
linter demands it presently, so it must be silenced.
* Add missing return for fmt.Errorf
Surprisingly, a return was missing here. This was caught by the linter,
and was most certainly incorrect.
* Remove yet another shadowed err
There was nothing incorrect about this shadowed err variable, but
linters complain about it and fail CI.
* Finish wiring in the NMAgent Client
There were missing places where the nmagent client wasn't wired in
properly in main. This threads the client through completely and also
alters some tests to maintain compatibility.
* Add config for Wireserver to NMAgent Client
This was in reaction to a lint detecting a vestigial use of
"WireserverIP". This package variable is no longer in use, however, the
spirit of it still exists. The changes adapt the provided configuration
into an nmagent.Config for use with the NMAgent Client instead.
* Silence the linter
The linter complained about a shadowed err, which is fine since it's
scoped in the `if`. Also there was a duplicate import which resulted
from refactoring. This has been de-duped.
* Rename jnr -> req and nma -> nmagent
The "jnr" variable was confusing when "req" is far more revealing of
what it is. Also, the "nma" alias was left over from a prior iteration
when the legacy nmagent package and the new one co-existed.
* Rename NodeInquirer -> NodeInterrogator
"Interrogator" is more revealing about the set of behavior encapsulated
by the interface. Depending on that behavior allows a consumer to
interrogate nodes for various information (but not modify anything about
them).
* Add implementation for GetNetworkConfiguration
Previously the NMAgent client did not have support for the
GetNetworkConfiguration API call. This adds it and appropriate coverage.
* Refactor retry loops to use shared function
The cancellable retry was common enough that it made sense to extract it
to a separate BackoffRetry function in internal. This made its
functionality easier to test and reduced the number of tests necessary
for each new endpoint
* Slight re-org
The client had enough extra stuff in it that it made sense to start
separating things into different files
* Add retries for Unauthorized responses
In the original logic, unauthorized responses are treated as temporary
for a specific period of time. This makes the nmagent.Error consider
Unauthorized responses as temporary for a configurable time. Given that
BackoffRetry cares only whether or not an error is temporary, this
naturally causes them to be retried.
Additional coverage was added for these scenarios as well.
* Add a WireserverTransport
This deals with all the quirks of proxying requests to NMAgent through
Wireserver, without spreading that concern through the NMAgent client
itself.
* Reorganize the nmagent internal package
The wireserver transport became big enough to warrant its own file
* Use WireserverTransport
This required some changes to the test so that the WireserverTransport
middleware could take effect always
* Add PutNetworkContainer method to NMAgent client
This is another API that must be implemented
* Switch NMAgent client port to uint16
Ports are uint16s by definition.
* Add missing body close and context propagation
* Add DeleteNetworkContainer endpoint
* Move internal imports to another section
It's a bit clearer when internal imports are isolated into one section,
standard library imports in another, then finally external imports in
another section.
* Additional Validation / Retry improvements
This is a bit of a rollup commit, including some additional validation
logic for some nmagent requests and also some improvements to the
internal retry logic. The retry logic is now a struct, and the client
depends only on an interface for retrying. This is to accommodate the
existing retry package (which was unknown).
The internal Retrier was enhanced to add a configurable Cooldown
function with strategies for Fixed backoff, Exponential, and a Max
limitation.
* Move GetNetworkConfig request params to a struct
This follows the pattern established in other API calls. It moves
validation to the request itself and also leaves the responsibility for
constructing paths to the request.
* Add Validation and Path to put request
To be consistent with the other request types, this adds Validate and
Path methods to the PutNetworkContainerRequest
* Introduce Request and Option
Enough was common among building requests and validating them that it
made sense to formalize it as an interface of its own. This allowed
centralizing the construction of HTTP requests in the nmagent.Client. As
such, it made adding TLS disablement trivial. Since there is some
optional behavior that can be configured with the nmagent.Client,
nmagent.Option has been introduced to handle this in a clean manner.
* Add additional error documentation
The NMAgent documentation contains some additional documentation as to
the meaning of particular HTTP Status codes. Since we have this
information, it makes sense to enhance the nmagent.Error so it can
explain what the problem is.
* Fix issue with cooldown remembering state
Previously, cooldown functions were able to retain state across
invocations of the "Do" method of the retrier. This adds an additional
layer of functions to allow the Retrier to purge the accumulated state
* Move Validation to reflection-based helper
The validation logic for each struct was repetitive and it didn't help
answer the common question "what fields are required by this request
struct." This adds a "validate" struct tag that can be used to annotate
fields within the request struct and mark them as required.
It's still possible to do arbitrary validation within the Validate
method of each request, but the common things like "is this field a zero
value?" are abstracted into the internal helper. This also serves as
documentation to future readers, making it easier to use the package.
* Housekeeping: file renaming
nmagent.go was really focused on the nmagent.Error type, so it made
sense to rename the file to be more revealing. The same goes for
internal.go and internal_test.go. Both of those were focused on retry
logic.
Also added a quick note explaining why client_helpers_test.go exists,
since it can be a little subtle to those new to the language.
* Remove Vim fold markers
While this is nice for vim users, @ramiro-gamarra rightly pointed out
that this is a maintenance burden for non-vim users with little benefit.
Removing these to reduce the overhead.
* Set default scheme to http for nmagent client
In practice, most communication for the nmagent client occurs over HTTP
because it is intra-node traffic. While this is a useful option to have,
the default should be useful for the common use case.
* Change retry functions to return durations
It was somewhat limiting that cooldown functions themselves would block.
What was really interesting about them is that they calculated some
time.Duration. Since passing 0 to time.Sleep causes it to return
immediately, this has no impact on the AsFastAsPossible strategy
Also improved some documentation and added a few examples at the request
of @aegal
* Rename imports
The imports were incorrect because this client was moved from another
module.
* Duplicate the request in wireserver transport
Upon closer reading of the RoundTripper documentation, it's clear that
RoundTrippers should not modify the request. While effort has been made
to reset any mutations, this is still, technically, modifying the
request. Instead, this duplicates the request immediately and re-uses
the context that was provided to it.
* Drain and close http ResponseBodies
It's not entirely clear whether this is needed or not. The documentation
for http.(*Client).Do indicates that this is necessary, but
experimentation in the community has found that this is maybe not 100%
necessary (calling `Close` on the Body appears to be enough).
The only harm that can come from this is if Wireserver hands back
enormous responses, which is not the case--these responses are fairly
small.
* Capture unexpected content from Wireserver
During certain error cases, Wireserver may return XML. This XML is
useful in debugging, so we want to capture it in the error and surface
it appropriately. It's unclear whether Wireserver notes the
Content-Type, so we use Go's content type detection to figure out what
the type of the response is and clean it up to pass along to the NMAgent
Client. This also introduces a new ContentError which semantically
represents the situation where we were given a content type that we
didn't expect.
* Don't return a response with an error in RoundTrip
The http.Client complains if you return a non-nil response and an error
as well. This fixes one instance where that was happening.
* Remove extra vim folding marks
These were intended to be removed in another commit, but there were some
stragglers.
* Replace fmt.Errorf with errors.Wrap
Even though fmt.Errorf provides an official error-wrapping solution for
Go, we have made the decision to use errors.Wrap for its stack
collection support. This integrates well with Uber's Zap logger, which
we also plan to integrate.
* Use Config struct instead of functional Options
We determined that a Config struct would be more obvious than the
functional options in a debugging scenario.
* Remove validation struct tags
The validation struct tags were deemed too magical and thus removed in
favor of straight-line validation logic.
* Address Linter Feedback
The linter flagged many items here because it wasn't being run locally
during development. This addresses all of the feedback.
* Remove the UnauthorizedGracePeriod
NMAgent only defines 102 processing as a temporary status. It's up to
consumers of the client to determine whether an unauthorized status
means that it should be retried or not.
* Add error source to NMA error
One of the problems with using the WireserverTransport to modify the
http status code is that it obscures the source of those errors. Should
there be an issue with NMAgent or Wireserver, it will be difficult (or
impossible) to figure out which is which. The error itself should tell
you, and WireserverTransport knows which component is responsible. This
adds a header to the HTTP response and uses that to communicate the
responsible party. This is then wired into the outgoing error so that
clients can take appropriate action.
* Remove leftover unauthorizedGracePeriod
These blocks escaped notice when the rest of the UnauthorizedGracePeriod
logic was removed from the nmagent client.
* Remove extra validation tag
This validation tag wasn't noticed when the validation struct tags were
removed in a previous commit.
* Add the body to the nmagent.Error
When errors are returned, it's useful to have the body available for
inspection during debugging efforts. This captures the returned body and
makes it available in the nmagent.Error. It's also printed when the
error is converted to its string representation.
* Remove VirtualNetworkID
This was redundant, since VNetID covered the same key. It's actually
unclear what would happen in this circumstance if this remained, but
since it's incorrect this removes it.
* Add StatusCode to error
Clients still want to be able to communicate the status code in logs, so
this includes the StatusCode there as well.
* Add GreKey field to PutNetworkContainerRequest
In looking at usages, this `greKey` field is undocumented but critical
for certain use cases. This adds it so that it remains supported.
* Add periods at the end of all docstrings
Docstrings should have punctuation since they're documentation. This
adds punctuation to every docstring that is exported (and some that
aren't).
* Remove unused Option type
This was leftover from a previous cleanup commit.
* Change `error` to a function
The `nmagent.(*Client).error` method wasn't actually using any part of
`*Client`. Therefore it should be a function. Since we can't use `error`
as a function name because it's a reserved keyword, we're throwing back
to the Perl days and calling this one `die`.
Santhosh Prabhu
ZetaoZhuang
Diego Becerra
Saksham Mittal
shchen
Paul Johnston
Timothy J. Raymond