update for new home az contract from nma #3151

ZetaoZhuang · 2024-11-14T23:17:56Z

Reason for Change:

NMA introduces a new field apiVersion in home az endpoint for the purpose of fixing the ipv6 issue in swift azr, we need to make changes on cns to correspond to this contract change.

Issue Fixed:

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
relevant PR labels added

Notes:

timraymond · 2024-11-15T16:40:46Z

nmagent/responses.go

@@ -38,7 +38,8 @@ type NCVersionList struct {
 }

 type AzResponse struct {
-	HomeAz uint `json:"homeAz"`
+	HomeAz     uint `json:"homeAz"`
+	APIVersion uint `json:"apiVersion"`


How is this used? Nothing else seems to change, so why does anything care what APIVersion is present?

This is going to be consumed in dnc and used for indicating a Dualstack fix from NMA

Can you describe the ipv6 fix needed in a little more detail? My concern with this is that nothing about the API is actually changing. We're just adding a cryptic field to it that clients are just supposed to understand. What does it mean when APIVersion becomes 3, 4, so on? A more obvious field would be something like NeedsIPv6Fix = true or something, but I don't have enough context on the entire problem to recommend a good API (hence the request for more detail).

Currently NMA is rollout a new fix for the azr swift regarding dualstacks secnarios. they are adding a new field in home az api to indicate the fix already there, so dnc will check this apiVersion coming from NMA response to decide enable azr or not. This APIVersion will always be 2 for now. we should raise alert if we saw apiVersion other than 2.

but NMA mentioned this value might change in the future for different purpose

If NMAgent is trying to indicate whether or not AZR should be enabled based on some programming they do, then the field should be EnableAZR = true / false. We shouldn't care about some fix or whatever that they have to do. Can we push back on this? The fact of NMAgent's APIs being generally not great isn't justification for allowing them to continue to make them worse.

If the answer is "no, the decision is firm," then we should fix this at the system border so we don't have to spread the knowledge about what a 2 means through the entire ACN stack. The NMAgent client (whose designed responsibility is a respository of all of the weird things NMAgent does) should translate this to something meaningful like EnableAZR in the response.

While I agree with your comment, Tim, NMA wants us to really use APIVersion here where they can probably change version to reuse this in case, they need to provide such a fix for another issue later (just having a boolean for one case might warrant more changes later from their side). That being said, @ZetaoZhuang we should check with NMA if 0 and 2 are the only values we should expect from them.

I agree this is not clean at all, but this is what they're pushing us to use from their side. Currently, this is a hotfix like change for us and ideally, we shouldn't need to use this later.

I checked with Vishwesh, he confirmed that 2 is the only valid value for now. we should also allow 0 as well. that is the default value of the field when NMA is still running the old version, and apiVersion does not exist

Okay, thanks for clarifying that we don't have any control over what they do. I'll make suggestions to isolate their bad design, though, so we don't have to deal with it forever.

timraymond · 2024-11-15T21:53:26Z

cns/restserver/homeazmonitor.go

+	// validate APIVersion, APIVersion is a uint, so its value >=0
+	// 0 should be valid when NMA version is old and does not have the apiVersion value in home az response
+	if azResponse.APIVersion > 0 && azResponse.APIVersion != 2 {
+		returnMessage := fmt.Sprintf("[HomeAzMonitor] invalid APIVersion value from nmagent: %d", azResponse.APIVersion)
+		returnCode := types.UnexpectedError
+		h.update(returnCode, returnMessage, cns.HomeAzResponse{IsSupported: true})
+		return
+	}


All of this knowledge about NMAgent should move to a method of nmagent.AzResponse. Something like:

func (a AzResponse) Valid() bool { return a.APIVersion >= 0 && a.APIVersion < 3 } func (a AzResponse) ShouldEnableAZR() bool { return a.Valid() && a.APIVersion == 2 }

Further, in the HomeAz method of the client, you can test for validity of the response and return an error instead stating that there is an invalid APIVersion from NMAgent.

Yep, we can move the validation part to the nmagent.AzResponse as you suggested. but I dont think we should return the ShouldEnableAZR here. I think we return whatever the apiversion we get from nma to DNC, and dnc should be the one to make decision to enable azr or not based on apiVersion, couple dnc feature flags, etc.

Hard disagree. APIVersion = 2 has a specific meaning, designed for this specific purpose. There is no room for interpretation on behalf of clients. I'm flexible on the name (AZREnablementRequested, NMAgentAppliedTheIPv6Fix, or something), but the name must convey the true meaning so that it is self-documenting for future readers and for other users of the nmagent package.

timraymond · 2024-11-15T21:56:44Z

cns/api.go

@@ -356,6 +356,7 @@ type NmAgentSupportedApisResponse struct {
 type HomeAzResponse struct {
 	IsSupported bool `json:"isSupported"`
 	HomeAz      uint `json:"homeAz"`
+	APIVersion  uint `json:"apiVersion"`


This should indicate whether or not we should Enable AZR (per earlier discussion). We can set this from doing:

resp, err := nmaClient.HomeAz(ctx) if err != nil { return errors.Wrap(err, "getting home az") } return cns.HomeAzResponse{ EnableAZR: resp.ShouldEnableAZR(), }

Then this can be directly consumed in clients without further interpretation about the meaning of codes.

msvik · 2024-11-19T23:22:44Z

nmagent/responses.go

+
+func (az AzResponse) NmaAppliedTheIPV6Fix() bool {
+	//nolint:gomnd // this magic number is made by nma design
+	return az.APIVersion == 2


Shouldn't this be az.APIVersion >= 2 instead of az.APIVersion == 2 since it can change in future?

good question. Vishwesh mentioned that for now we only want to allow APIVersion as 2, and he want us to raise alerts if we get APIVersion other than 2. from what he means, when we get APIVersion as "3", we should mark it as an invalid version number and then raise alert on it. That way, we may not want it to indicate the fix is applied.

Since this API version field is being introduced as general purpose field which can be used for future bugs also, we should account for what would be AZ behavior if it becomes 3 some day.

msvik · 2024-11-19T23:29:38Z

cns/restserver/homeazmonitor.go

@@ -154,7 +154,14 @@ func (h *HomeAzMonitor) Populate(ctx context.Context) {
 		h.update(returnCode, returnMessage, cns.HomeAzResponse{IsSupported: true})
 		return
 	}
-	h.update(types.Success, "Get Home Az succeeded", cns.HomeAzResponse{IsSupported: true, HomeAz: azResponse.HomeAz})
+	// validate APIVersion value
+	if !azResponse.Valid() {


Is it ok to IsValid check after checking AZ? Should we do Valid check before it?

imo, its ok. the order does not matter here. the APIversion is for indicating the ipv6 fix which is independent of the value of home az. so it could be home az is 0, but APIversion is either valid or invalid.

update for new home az contract

0f11f3a

ZetaoZhuang requested review from a team as code owners November 14, 2024 23:17

ZetaoZhuang requested a review from rjdenney November 14, 2024 23:17

fix lint

25e7ef1

timraymond reviewed Nov 15, 2024

View reviewed changes

ZetaoZhuang requested a review from timraymond November 15, 2024 18:01

timraymond requested changes Nov 15, 2024

View reviewed changes

ZetaoZhuang requested a review from timraymond November 18, 2024 19:41

ZetaoZhuang added 2 commits November 19, 2024 11:39

address comment

90301c2

fix lint

a1d5a07

ZetaoZhuang requested review from smittal22 and timraymond and removed request for timraymond November 19, 2024 20:05

msvik reviewed Nov 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update for new home az contract from nma #3151

update for new home az contract from nma #3151

ZetaoZhuang commented Nov 14, 2024 •

edited

Loading

timraymond Nov 15, 2024

ZetaoZhuang Nov 15, 2024

timraymond Nov 15, 2024

ZetaoZhuang Nov 15, 2024 •

edited

Loading

ZetaoZhuang Nov 15, 2024

timraymond Nov 15, 2024

smittal22 Nov 15, 2024

ZetaoZhuang Nov 15, 2024

timraymond Nov 15, 2024

timraymond Nov 15, 2024

ZetaoZhuang Nov 18, 2024 •

edited

Loading

timraymond Nov 18, 2024

timraymond Nov 15, 2024

msvik Nov 19, 2024

ZetaoZhuang Nov 19, 2024 •

edited

Loading

msvik Nov 20, 2024

msvik Nov 19, 2024

ZetaoZhuang Nov 19, 2024

update for new home az contract from nma #3151

Are you sure you want to change the base?

update for new home az contract from nma #3151

Conversation

ZetaoZhuang commented Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZetaoZhuang Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZetaoZhuang Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZetaoZhuang Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZetaoZhuang commented Nov 14, 2024 •

edited

Loading

ZetaoZhuang Nov 15, 2024 •

edited

Loading

ZetaoZhuang Nov 18, 2024 •

edited

Loading

ZetaoZhuang Nov 19, 2024 •

edited

Loading