Preserve an instance's enabled status for its two ports

[seh]

Addressing both #58 and #29, this patch series started with the former and expanded to cover the latter as well. My testing confirms that fargo can communicate with a Eureka server of version era both pre-1.2.2 and post-1.2.2 using both XML and JSON, which is when the change in port value encoding came into service.

The challenges communicating with Eureka on each side of this version boundary involve several fields in the JSON encoding for both instances and the application-level query response:

• instance
  • port.$
    Pre-Eureka v1.2.2: string
    Post-Eureka v1.2.2: number
  • securePort.$
    Pre-Eureka v1.2.2: string
    Post-Eureka v1.2.2: number
• versions__delta
  Pre-Eureka v1.2.2: number
  Post-Eureka v1.2.2: string

Furthermore, when registering instances with Eureka, post version 1.2.2 the server started requiring that JSON-encoded instances include the "dataCenterInfo.@Class" field, so we have to synthesize that, while still accommodating custom data center types for which a caller will need to specify a class value.

Finally, I discovered that fargo couldn't decode an instance's data center metadata that contains values that look like numbers; the Eureka server will encode them as JSON numbers rather than strings, requiring us to decode them flexibly.

There are a few disparate solutions combined here, but I figured that they sit well enough together under the umbrella of better compatibility with the server. Again, I believe this patch series resolves the longstanding incompatibility with fargo, Eureka post-version 1.2.2, and JSON encoding. We should be able to both register and query applications and instances now.

[seh]

I had forgotten to resolve the conflict here after #60 merged.
PTAL.

[seh]

fargo has been quiet for a while. Is anyone available to review this proposal?
Maybe @ryansb or @cquinn? You two have been through these areas of the code.
@damtur, by now, you know it all.

[damtur]

Ideally I would like to have second reviewer on that, but from my point of view it all looks ok.

[damtur]

Why is this float64 and not int? Have you checked it's coming as a fload? Any thoughts of adding inte there as well?

[seh]

Per the documentation for json.Unmarshal, numbers coming from JSON into Go are always of type float64—barring use of json.(*Decoder).UseNumber. (That's implemented here.) As it stands, fargo does not set this option on a Decoder when decoding a fargo.GetAppsResponse.

[damtur]

How can we be so sure that PortJ has not been used by library-consuemers. What would be the downside of leaving PortJ here so it's backwards compatible?
Are we planning major version update with this change?

[seh]

I thought I had written about this earlier, but apparently not. I apologize for the delay in addressing your concern.

The message for commit 9352b64 in this patch series mentions this removal:

None of these were exposed deliberately for use by callers; they were
all exposed only as accidental consequences of the JSON and XML
marshalling library's idiosyncrasies.

We can't be sure whether anyone wrote to or read the "PortJ" and "SecurePortJ" fields. Removing them introduces an incompatibility, in the sense that code that used to compile will now fail to compile. I could add the fields back in, though the values read from those fields would be different than they were before, and writing to those fields wouldn't have the intended effect.

I had first tried to solve this problem by making use of those fields as they are, transferring values between them and making guesses as to whether setting a port to a nonzero value meant that it should be enabled, but I gave up on the idea and backed out those changes. As I wrote tests, I found that I couldn't even convince myself reliably of how it should work. It felt like we were going to just barely make it possible to register or detect which ports are enabled, but leave it too easy to get it wrong.

I am not sure that it's any more fair to our clients to allow their code to continue to compile as it did, but not work properly at run time. One thing I considered doing—and will do, if you think it's better than not doing it—is to leave those fields there, and when we read an Instance from the Eureka server to transcribe the port number and whether it's enabled into those 'J' fields, duplicating the information from the "Port," "PortEnabled," "SecurePort," and "SecurePortEnabled" fields. However, I would not want to try something different in reverse: namely, I do not want to try to merge values from these different fields when marshaling an Instance to send to the Eureka server. To do so introduces too much confusion for callers: What happens if the values conflict? Which one do I need to set? Do I need to set both?

I doubted that fargo had any clients accessing these fields profitably, but I did find one public use over in stv3v/cfkit. There is also st3v/pcf-micro-examples, but then I realized that one is a vendored copy of fargo. The former has a legitimate use. @st3v is the author of the commit that introduced those lines on 1 October 2015. (Note that that setting of the "PortJ" and "SecurePortJ" fields would not have any effect if EurekaConnection.UseJson had been false instead.)

The st3v/cfkit project has fargo's version pinned via Godeps at commit ac37d05, which is from 10 September 2015. Hence, that project wouldn't be broken by this change unless it tried to advance its dependency version without changing its calling code.

@st3v Do you have an opinion on this matter?

[st3v]

@seh Don't worry about cfkit and pcf-micro-examples. Those were pet projects I played around with quite a while ago. They aren't used anywhere.

[damtur]

I'm a little bit confused here. Previously we were unmarshaling here with *r = GetAppsResponse(ra) but now it seams to me that we are only unmarshaling VersionsDelta here?
Also minor, what does the auxArray stand for?

[seh]

Note line 42, where we populate the "auxArray" variable's "getAppsResponse" (anonymous|embedded) field with variable "r." When we supply "auxArray' to json.Unmarshal at line 45, it will write directly to a "r" (dereferencing the pointer as necessary), which is analogous to the previous assignment you recall. It will write to "r" and also write into the auxiliary struct's "VersionsDelta" field, which we then convert and assign back into "r"'s "VersionsDelta" field. The outer struct's field (of type interface{}) shadows the field with the same name in the embedded "getAppsResponse" field (of type int).

Here the variable name "auxArray" means "auxiliary structure for the "array of applications" case, versus the "single application" case handled at line 52. The variable name "aux" comes up frequently with this encoding or decoding technique in Go, where one introduces some separate, temporary, and only locally significant struct to handle the finicky details that Go's struct tags can't accommodate.

[damtur]

Thx for finding that ⭐️

[damtur]

Just a thinking exercise. How hard will it be to detect eureka version and serve what's the most appropriate version of date depending on version. Will that be even cleaner? Are we worried that eureka might stop accepting old version?

[seh]

The server doesn't send anything indicative of its version—not even in an HTTP header. If we had already received some responses from a server and been able to inspect the fields that differ, we could then remember which "style" or "version" of the encoding we've seen the server use and then mimic that in our future requests. There's still that problem of how our client starts that conversation safely.

As for whether Eureka will stop accepting the old version, I think they'd have to introduce some new resource paths in order to be fair to all the possible clients out there of various versions. My impression is that their handling of the encoding here has been haphazard; it came down to whatever XStream happened to do, and the Java clients could still talk well enough with the servers, so it doesn't appear that they cared enough to document or even prevent these format changes. I don't think they were deliberate.

[damtur]

Why have you decided to remove XMLName here? What if it's used by someone?

[seh]

When using an xml.Encoder to encode an element, one must supply an xml.StartElement that determines the element's name. The "XMLName" field has no effect in that case. I struggled with a bug here for a while, trying to figure out why my "XMLName" field's value wasn't being honored. I finally realized that the value passed to xml.(*Encoder).EncodeElement is what determines the element name.

If someone was setting this field's value to something other than "instance," the Eureka server would have rejected the resulting message body. If they were to try to set it now—if I restored the field to the fargo.Instance struct —it would have no effect. To leave it in would confuse both our maintainers and our users.

Now, it's possible that someone's code would stop compiling if they were accessing the "XMLName" field of fargo.Instance. I didn't think it was worth retaining this cruft just to preserve that illusion of compatibility. I think that this does warrant bumping at least the minor version number in the next release that would contain these changes.

[damtur]

That's a great explanation! Thank you for taking your time to answer to all of my concerns in so much of a detail!
Let's go with the solution as it is right now. One thing, which is bothering me still is that we should try to write really short note (maybe just links to the discussion we were having here) what has changed and how to migrate when someone's code doesn't compile.
I'm happy with those changes and I'm leaning to merge this as it is right now.
This will be a major version bump since there are some breaking changes, with a note that migration ought to be painless.
Thank you again @seh for your great work and time!

[seh]

You are most welcome, @damtur.

If we're contemplating a major version bump, this might also be our one chance to fix these longstanding go lint violations:

dns_discover.go:15:5: exported var ErrNotInAWS should have comment or be unexported
dns_discover.go:79:21: error strings should not end with punctuation
dns_discover.go:92:6: func findDnsServerAddr should be findDNSServerAddr
dns_discover.go:98:9: if block ends with a return statement, so drop this else and outdent its block
errors.go:31:6: exported type AppNotFoundError should have comment or be unexported
net.go:31:9: if block ends with a return statement, so drop this else and outdent its block
net.go:686:45: method parameter insId should be insID
net.go:805:1: exported method Instance.Id should have comment or be unexported
net.go:805:20: method Id should be ID
rpc.go:14:5: exported var HttpClient should have comment or be unexported
rpc.go:14:5: var HttpClient should be HTTPClient
rpc.go:26:46: func parameter isJson should be isJSON
rpc.go:62:29: func parameter isJson should be isJSON
rpc.go:90:37: func parameter isJson should be isJSON
struct.go:27:2: struct field discoveryTtl should be discoveryTTL
struct.go:28:2: struct field UseJson should be UseJSON
struct.go:32:6: type GetAppsResponseJson should be GetAppsResponseJSON
struct.go:44:6: type GetAppResponseJson should be GetAppResponseJSON
struct.go:73:6: type RegisterInstanceJson should be RegisterInstanceJSON
struct.go:93:2: struct field HomePageUrl should be HomePageURL
struct.go:94:2: struct field StatusPageUrl should be StatusPageURL
struct.go:95:2: struct field HealthCheckUrl should be HealthCheckURL
struct.go:97:2: struct field CountryId should be CountryID

If you're interested in fixing those—breaking lots of callers in the process, yes—let me know and I'll file a separate PR with those renaming and documentation additions.

[damtur]

Hey @seh I think keeping migration process as simple as possible with this version bump should be the goal. I don't see enough value in changing those right now.

[seh]

Weird: I thought I had responded to this 13 days ago when I also wrote a parallel reply:

One thing, which is bothering me still is that we should try to write really short note (maybe just links to the discussion we were having here) what has changed and how to migrate when someone's code doesn't compile.

Where would you like this documentation? Posted here in this proposal, or in note attached to a GitHub release, or maybe in the top-level README.md file?

I was waiting on your response before writing that documentation, and then I thought that maybe I had missed your reply. It turns out that I didn't post my question properly, despite me being sure that I did write it.

[damtur]

@seh I've merged the code now as it's good. I'll create a release while I get your note. Please write it anywhere so I have it and I will include it in a release note. Thank you for all your work!