Add NMAgent Client #1305
Merged
Add NMAgent Client #1305
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
Contributor
ramiro-gamarra
left a comment
ramiro-gamarra
left a comment
There was a problem hiding this comment.
Few initial comments
rbtr
reviewed
Apr 1, 2022
Collaborator
rbtr
left a comment
rbtr
left a comment
There was a problem hiding this comment.
globally, docstrings need punctuation.
very nice package but I feel YAGNI - we aren't providing the official nma-go library, just writing a little client for our personal use
Comment on lines
104
to
171
| // set the response status code with the *real* status code | ||
| realCode, err := wsResp.StatusCode() | ||
| if err != nil { | ||
| return resp, fmt.Errorf("retrieving status code from wireserver response: %w", err) | ||
| } | ||
|
|
||
| resp.StatusCode = realCode |
Collaborator
There was a problem hiding this comment.
is this a change of behavior vs our current nma usage? making this not-a-drop-in change?
Member
Author
There was a problem hiding this comment.
I'll take a closer look and report back.
Member
Author
|
@ramiro-gamarra Take a look at 088b710. It now reads the entire response and handles unexpected content types. |
Member
Author
|
Here's the remaining items that we decided last week on this PR:
|
Member
Author
|
I made a few attempts to remove the I ran all this by @ramiro-gamarra yesterday on a call, and the only addition was the body of the request in an error case to aid in debugging. |
Collaborator
|
@timraymond I think you still need punctuation on docstrings everywhere, otherwise I have no significant comments |
ramiro-gamarra
previously approved these changes
Apr 29, 2022
Contributor
ramiro-gamarra
left a comment
ramiro-gamarra
left a comment
There was a problem hiding this comment.
I think this is good to get started with testing. We can make tweaks as needed in later PRs.
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != http.StatusOK { |
Contributor
There was a problem hiding this comment.
It doesn't seem like we read the body in some operations (maybe that's existing DNC behavior anyways). Would be nice to do something like io.Copy(io.Discard, resp.Body) to ensure any payload is consumed.
Member
Author
There was a problem hiding this comment.
The standard library will recycle connections if you close the body. I suspect that requiring package authors to drain the bodies was too obtuse, so they made Close() affirmatively mean "I'm done with this connection" here:
https://cs.opensource.google/go/go/+/refs/tags/go1.18.1:src/net/http/transport.go;l=2135-2140
The check to see whether an EOF was seen is still present, but it's short-circuited by alive in this conditional:
https://cs.opensource.google/go/go/+/refs/tags/go1.18.1:src/net/http/transport.go;l=2150-2153
alive is basically a latch that makes the connection eligible for recycling.
Contributor
There was a problem hiding this comment.
Connection re-use (or lack of) can be shown with this example:
package main
import (
"context"
"io"
"io/ioutil"
"log"
"net/http"
"net/http/httptrace"
)
func main() {
clientTrace := &httptrace.ClientTrace{
GotConn: func(info httptrace.GotConnInfo) {
log.Printf("conn: %+v", info)
},
}
ctx := httptrace.WithClientTrace(context.Background(), clientTrace)
req, err := http.NewRequestWithContext(ctx, http.MethodGet, "http://example.com", nil)
if err != nil {
log.Fatal(err)
}
res, err := http.DefaultClient.Do(req)
if err != nil {
log.Fatal(err)
}
if _, err := io.Copy(ioutil.Discard, res.Body); err != nil {
log.Fatal(err)
}
res.Body.Close()
req, err = http.NewRequestWithContext(ctx, http.MethodGet, "http://example.com", nil)
if err != nil {
log.Fatal(err)
}
_, err = http.DefaultClient.Do(req)
if err != nil {
log.Fatal(err)
}
}You can comment out the code that consumes the body, and the trace should show no connection reuse. This is still the behavior in 1.18. It's possible that there are some scenarios where this won't be the case but this has bit me enough in the past to make me want to be extra careful :)
Member
Author
There was a problem hiding this comment.
Mind if I reuse this example as a repro for a Go issue? (I'll mention you on it) IMO, this violates least surprise, and it appears there's been some effort to make Close() do the right thing... it's apparently not complete though
Contributor
There was a problem hiding this comment.
Feel free. Code slightly modified from this article btw. Would be nice to see some clarification around the code you pointed to, though it seems to me that this behavior is still consistent with the docs. Maybe Gopher slack or golang-nuts would be a good place to ask first?
nmagent/client.go
Outdated
| } | ||
|
|
||
| // GetNetworkConfiguration retrieves the configuration of a customer's virtual | ||
| // network. Only subnets which have been delegated will be returned |
Contributor
There was a problem hiding this comment.
My understanding is that all subnets in vnet are returned, not just delegated ones. We mark the subnets as delegated at later steps in DNC.
Member
Author
There was a problem hiding this comment.
I pulled most of these doc comments straight from the NMA docs just so users wouldn't have to cross reference them all the time... This one comes from section 8.2.6. We should find out if that's true and update the upstream docs if all the subnets are returned.
Member
Author
|
@ramiro-gamarra I apparently vacated your approval by pushing those commits. PTAL :) @rbtr Docstrings addressed in 4fcbdd5 . I wrote a linter for that if we want to look at incorporating it. |
ramiro-gamarra
previously approved these changes
May 2, 2022
Previously the NMAgent client did not have support for the GetNetworkConfiguration API call. This adds it and appropriate coverage.
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
The client had enough extra stuff in it that it made sense to start separating things into different files
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.
This deals with all the quirks of proxying requests to NMAgent through Wireserver, without spreading that concern through the NMAgent client itself.
The wireserver transport became big enough to warrant its own file
This required some changes to the test so that the WireserverTransport middleware could take effect always
This is another API that must be implemented
Ports are uint16s by definition.
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.
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.
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
The imports were incorrect because this client was moved from another module.
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.
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.
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.
The http.Client complains if you return a non-nil response and an error as well. This fixes one instance where that was happening.
These were intended to be removed in another commit, but there were some stragglers.
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.
We determined that a Config struct would be more obvious than the functional options in a debugging scenario.
The validation struct tags were deemed too magical and thus removed in favor of straight-line validation logic.
The linter flagged many items here because it wasn't being run locally during development. This addresses all of the feedback.
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.
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.
These blocks escaped notice when the rest of the UnauthorizedGracePeriod logic was removed from the nmagent client.
This validation tag wasn't noticed when the validation struct tags were removed in a previous commit.
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.
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.
Clients still want to be able to communicate the status code in logs, so this includes the StatusCode there as well.
In looking at usages, this `greKey` field is undocumented but critical for certain use cases. This adds it so that it remains supported.
Docstrings should have punctuation since they're documentation. This adds punctuation to every docstring that is exported (and some that aren't).
This was leftover from a previous cleanup commit.
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`.
timraymond
force-pushed
the
traymond/nmagent-client
branch
from
11586d1 to
ba69161
Compare
rbtr
approved these changes
May 5, 2022
rsagasthya
pushed a commit
to rsagasthya/azure-container-networking
that referenced
this pull request
May 18, 2022
* 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`.
matmerr
pushed a commit
to matmerr/azure-container-networking
that referenced
this pull request
Jun 29, 2022
* 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`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
This adds a new nmagent package containing a Client for interacting with NMAgent. There's also some additional supporting logic in the nmagent/internal package for handling Wireserver, automatic retries, and validating requests.
Each endpoint that we use has a method on the client that accepts a context and some kind of request struct (see requests.go). The client deals with any asynchronous behavior and presents a synchronous abstraction to callers (consequently it's important to set timeouts on the inbound context if that's what's desired).
nmagent/internalcontains anhttp.RoundTripperused to deal with the idiosyncrasies of proxying requests through Wireserver. It also contains a few other utilities for struct validation and retries also used here.Requirements:
Notes: