Replace the NMAgent client in CNS with the one from the nmagent package #1643

timraymond · 2022-10-03T18:44:59Z

The NMAgent client used previously in CNS is older, and a duplication of the one added in nmagent/. In order to maintain one nmagent client and prevent duplication / wasted effort, this changes CNS to use the "official" client instead.

uses conventional commit messages
includes documentation
adds unit tests

Notes:

rbtr · 2022-10-18T17:58:26Z

cns/restserver/util.go

-	var err error
-	joinResponse, joinErr := nmagent.JoinNetwork(networkID)
+func (service *HTTPRestService) joinNetwork(ctx context.Context, networkID string) error {
+	jnr := nma.JoinNetworkRequest{


Interesting that you abbreviate in this way. When I'm only dealing with a single request, I prefer using req, and imo that is the more common/readable pattern.

yeah... that's a habit I picked up from a C++ codebase lol. I like req too.

Okay, this was done in 7519cbd

rbtr · 2022-10-18T18:03:42Z

cns/service/main.go

 	fmt.Printf("Version %v\n", version)
 }

+type NodeInquirer interface {


I'm imagining this with a top hat and monocle. I would have gone with "Interrogator" 😅

I like Interrogator. I wanted to separate out the read ops from the write ops. NodeReader sounded... weird and misleading. Interrogator does say what it does what it says on the tin... it interrogates nodes for information. I guess that means the writing side should be NodeInceptor 😆

Updated in 9381406

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.

The existing unpublish endpoints within CNS used the older nmagent client. This converts them to use the newer one.

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.

The CNS client uses this SupportedAPIs method to proxy requests from DNC and detect capabilities of the node.

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.

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.

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).

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.

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.

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.

The linter rightly pointed out that context could be trivially propagated further than it was. This commit does exactly that.

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.

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.

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.

Surprisingly, a return was missing here. This was caught by the linter, and was most certainly incorrect.

There was nothing incorrect about this shadowed err variable, but linters complain about it and fail CI.

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.

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.

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.

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.

"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).

ramiro-gamarra · 2022-10-19T17:44:24Z

cns/service/main.go

 }

+// NodeInterrogator is functionality necessary to read information about nodes.
+// It is intended to be strictly read-only.


Not sure I understand this line. Does it mean, not to expand the interface with methods that have side effects?

Yeah, the intention is to set some opinions about what this set of behavior means. If you have some methods which aren't about digging information out of Nodes, then it might belong in another interface.

ramiro-gamarra · 2022-10-19T19:15:54Z

cns/restserver/api.go

 	"github.com/Azure/azure-container-networking/cns/types"
 	"github.com/Azure/azure-container-networking/cns/wireserver"
-	"github.com/Azure/azure-container-networking/common"
+	nma "github.com/Azure/azure-container-networking/nmagent"


package is used as nmagent in other places, any reason to rename it in some places and not on others? personally, I think renaming packages should be done as little as possible. if it's widely understood that nma refers to nmagent, then the package should just be nma.

Any instance of this is an oversight from when there were old and new packages both named nmagent. I agree, but I won't fix this in this PR because it is blocking other feature work. I'll add this to remediation items for a future PR.

ramiro-gamarra · 2022-10-19T19:18:34Z

cns/restserver/internalapi.go

 		req, _ = http.NewRequest(http.MethodPost, "", bytes.NewBuffer(body))
 		req.Header.Set(common.ContentType, common.JsonContent)
+
+		w := httptest.NewRecorder()


I know this is just moving existing lines, but this is... interesting, to say the least. i suppose its a symptom of all routing + business logic living in the same function.

Not a fan of httptest in something that's not a test anyway... but that's a battle for another day.

ramiro-gamarra · 2022-10-19T19:32:52Z

nmagent/client.go

 	return out, err // nolint:wrapcheck // wrapping just introduces noise here
 }

+// GetNetworkContainerVersion gets the current goal state version of a Network


godoc is off

Noted. Will fix in a future cleanup PR.

cns/NetworkContainerContract.go

…ge (Azure#1643) * 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).

This reverts commit 9cc0b0b. Performing these changes uncovered unknown dependencies that clients had on specific formations of responses from CNS. Until these can be properly documented and understood, the legacy implementation must remain. A subsequent approach may instead introduce alternative endpoints with known properties that clients can migrate to piecemeal.

timraymond requested a review from a team as a code owner October 3, 2022 18:44

timraymond requested review from rbtr and removed request for a team October 3, 2022 18:45

timraymond force-pushed the nma-switchover branch 3 times, most recently from 98e2d17 to 459a378 Compare October 17, 2022 22:57

rbtr previously approved these changes Oct 18, 2022

View reviewed changes

timraymond dismissed rbtr’s stale review via 9381406 October 18, 2022 22:45

timraymond force-pushed the nma-switchover branch from 459a378 to 9381406 Compare October 18, 2022 22:45

timraymond requested a review from rbtr October 18, 2022 22:45

timraymond enabled auto-merge (squash) October 18, 2022 22:45

timraymond force-pushed the nma-switchover branch 2 times, most recently from 06a4ab1 to 992adda Compare October 19, 2022 14:26

timraymond added 17 commits October 19, 2022 15:04

Switch PublishNetworkContainer to nmagent package

da24d70

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

10f3d1b

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

8ebe248

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

e63161d

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

ae2d605

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.

Add GetNCVersionList endpoint to NMAgent client

461050f

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

4c912e6

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.

Use context for joinNetwork

757d8be

The linter rightly pointed out that context could be trivially propagated further than it was. This commit does exactly that.

Silence complaints about dynamic errors in tests

4f2da3e

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

b51f3b1

Surprisingly, a return was missing here. This was caught by the linter, and was most certainly incorrect.

Remove yet another shadowed err

e862f6c

There was nothing incorrect about this shadowed err variable, but linters complain about it and fail CI.

Finish wiring in the NMAgent Client

e07f2fc

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.

timraymond added 4 commits October 19, 2022 15:04

Silence the linter

827f7d4

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

4a2658a

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

6ab0a2e

"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).

timraymond force-pushed the nma-switchover branch from 992adda to 6ab0a2e Compare October 19, 2022 19:04

rbtr approved these changes Oct 19, 2022

View reviewed changes

ramiro-gamarra reviewed Oct 19, 2022

View reviewed changes

timraymond merged commit 9cc0b0b into Azure:master Oct 19, 2022

pjohnst5 reviewed Nov 23, 2022

View reviewed changes

cns/NetworkContainerContract.go Show resolved Hide resolved

timraymond deleted the nma-switchover branch December 12, 2022 16:35

Replace the NMAgent client in CNS with the one from the nmagent package #1643

Replace the NMAgent client in CNS with the one from the nmagent package #1643

Uh oh!

Conversation

timraymond commented Oct 3, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timraymond Oct 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

timraymond Oct 19, 2022 •

edited

Loading