Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Etag should not depend on external state #265

Open
apuigsech opened this issue Jan 5, 2020 · 4 comments
Open

Etag should not depend on external state #265

apuigsech opened this issue Jan 5, 2020 · 4 comments

Comments

@apuigsech
Copy link
Contributor

Etag calculation is done by marshaling the payload into json and getting the md5 of that string. All values will be converted to string, and there are some specific cases in which the conversion to string depends on external state, like its time.Time. Its sting will depend on the local timezone.

This is not an issue if the item is only processed by a single computer (or a set of them on the same timezone), but I found a case in which this is not the case. I am using postgres replica through kafka events to replicate part of the state from one service to another, and both needs to be on different timezone, causing the same item in two different systems to have a different Etags. This is an issue in the moment I am considering the Etag and the Id as the replica identity.

This is the database on service A:

> select * from namespaces;
               etag               |          id          |     created     |     updated     | name
----------------------------------+----------------------+-----------------+-----------------+------
 1532840dbc17ad6b13fdd2001ae1fc99 | bo8rngfuudckd8jdh4o0 | 10:34:41.304598 | 10:34:41.304599 | ns1
 b0206828be20257bf6f85998d1e75e26 | bo8rngnuudckd8jdh4og | 10:34:42.857316 | 10:34:42.857317 | ns2
 6f0044115fd242a21404d727b9081a9f | bo8rnh7uudckd8jdh4p0 | 10:34:44.058706 | 10:34:44.058707 | ns3

And this is on service B:

> select * from namespaces;
               etag               |          id          |     created     |     updated     | name
----------------------------------+----------------------+-----------------+-----------------+------
 7a717c814f38d97989df6382622d99b5 | bo8rngfuudckd8jdh4o0 | 10:34:41.304598 | 10:34:41.304599 | ns1
 447211d08c8b2b2a45e2b2fc3e2b04d2 | bo8rngnuudckd8jdh4og | 10:34:42.857316 | 10:34:42.857317 | ns2
 81c5ba2564fc653cc508943da13847fd | bo8rnh7uudckd8jdh4p0 | 10:34:44.058706 | 10:34:44.058707 | ns3

I think we should guaranty that for same time.Time's, the Etag will be always the same independently of external states, and I suggest to do something like this on the getEtag function:

func normalizeTime(payload map[string]interface{}) map[string]interface{} {
	for k,v := range payload {
		switch v.(type){
		case time.Time:
			if t, ok := v.(time.Time) ; ok {
				payload[k] = t.UnixNano()
			}
		case map[string]interface{}:
			if t, ok := v.(map[string]interface{}) ; ok {
				normalizeTime(t)
			}
		case []interface{}: 
			if t, ok := v.([]interface{}) ; ok {
				for i,_ := range t {
					if x, ok := t[i].(time.Time) ; ok {
						t[i] = x.UnixNano()
					}
				}
			}
		}
	}
	return payload
}

If you agree with that I can work on making the a proper PR.

@smyrman
Copy link
Collaborator

smyrman commented Jan 6, 2020

Sounds good.

@apuigsech
Copy link
Contributor Author

apuigsech commented Jan 6, 2020

This is happening to me, particularly, on the "created" and "updated" fields, because they are initialised using the function Now, which uses time.Now() that considers location on the internal state.

So, other possible solutions is to prevent them to manage the location state by just adding a call to UTC() on the Now function;

	Now = func(ctx context.Context, value interface{}) interface{} {
		return time.Now()**.UTC()**
	}

Thoughts?

@smyrman
Copy link
Collaborator

smyrman commented Jan 7, 2020

Changing the Now function is defiantly the smallest change. I have no objections to it.

@Dragomir-Ivanov
Copy link
Contributor

@apuigsech I am 👍 for UTC() time as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants