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

GM Version checking doesn't work #1049

Closed
jiivan opened this issue Jan 4, 2019 · 6 comments
Closed

GM Version checking doesn't work #1049

jiivan opened this issue Jan 4, 2019 · 6 comments

Comments

@jiivan
Copy link

jiivan commented Jan 4, 2019

To reproduce, run pytest -vs --maxfail=1 --lf scripts/concent_acceptance_tests/force_payment/test_payment.py
Result:

DEBUG    [golem.network.concent.client       ] send_to_concent(): POST 'https://staging.concent.golem.network/api/v1/send/' hdr: {'Content-Type': 'application/octet-stream', 'X-Golem-Messages': '2.21.1'} 
DEBUG    [golem.network.concent.client       ] Headers received from Concent: {'Server': 'nginx/1.15.0', 'Date': 'Fri, 04 Jan 2019 09:02:59 GMT', 'Content-Type': 'application/json', 'Content-Length': '192', 'Connection': 'keep-alive', 'Concent-Version': 'v0.10.2-101-gbecec05d', 'Concent-Golem-Messages-Version': '2.17.0', 'X-Frame-Options': 'SAMEORIGIN'} 
WARNING  [golem.network.concent.client       ] Concent request failed with status 400 and response: '{"error": "Golem Message contains wrong fields. Should be an instance of <class \'golem_messages.message.tasks.SubtaskResultsAccepted\'> not <class \'bool\'> [subtask_results_accepted_list:True]"}'

Analysis:
Local GM version is 2.21.1, remote version is 2.17.0. Concent service acts as if those versions are compatible, hence it doesn't understand new message nesting protocol.

@PaweuB
Copy link

PaweuB commented Jan 7, 2019

@kbeker
Copy link
Contributor

kbeker commented Jan 7, 2019

Sanity check needs to be added on django side (middleware). There are 3 cases:

  • If X-Golem-Messages is not added or it's None in header:
    Pass and do nothing,
  • If X-Golem-Messages is added and it contains correct GM version:
    Pass and do nothing,
  • If X-Golem-Messages is added but contains wrong version of GM or it's contains version which could not be parsed:
    Respond with error

@cameel
Copy link
Contributor

cameel commented Jan 7, 2019

Just to clarify: this check should be performed by the router that kicks in before you even get to the cluster. But currently that can only be enabled if we have exactly two clusters. Otherwise routing is disabled and there are no checks. On staging we have only a single version right now. I've added an issue to make it work the same way no matter whether we have one or two versions available: golemfactory/concent-deployment#289. When the requested version is not available, the router will now respond with HTTP 404.

The change proposed by @kbeker above is an additional sanity check in the Concent app - if you, for whatever reason, manage to get through to a Concent cluster with an incompatible version of GM in your X-Golem-Messages header, Concent should respond with ServiceRefused. This would indicate a cluster misconfiguration so you should never get this in production.

@kbeker The header is called X-Golem-Messages and contains GM version used by the Golem client. That's the one we should check. Concent-Golem-Messages-Version is a completely different header Concent puts on its HTTP responses and contains GM version used by Concent.

Also, this should be obvious, but just in case: note that (as stated in the #857 blueprint) versions do not need to be exactly the same. It's enough if they're compatible:

Versions are considered compatible if they share the minor and major version number. E.g 2.18.5 is compatible with 2.18.1 but not with 2.17.5 or 3.0.0.

Note that the way we do it in the router matches semver only for release versions but it does not really matter because this mechanism is meant only for production. We just look at first two numbers and ignore the rest: golemfactory/concent-deployment#258 (comment). It's best if the Python code checks it exactly the same way.

@kbeker
Copy link
Contributor

kbeker commented Jan 7, 2019

@kbeker The header is called X-Golem-Messages and contains GM version used by the Golem client. That's the one we should check. Concent-Golem-Messages-Version is a completely different header Concent puts on its HTTP responses and contains GM version used by Concent.

@cameel You're right, my bad, edited.

@kbeker kbeker self-assigned this Jan 7, 2019
@cameel
Copy link
Contributor

cameel commented Jan 7, 2019

Update: on the standup today we agreed that instead of ServiceRefused, Concent should return HTTP 404 when the version does not match.

@kbeker kbeker removed their assignment Jan 9, 2019
@pawelkisielewicz pawelkisielewicz self-assigned this Jan 9, 2019
@pawelkisielewicz
Copy link
Contributor

Fixed in concent and should work properly. It was fixed in #1057, didn't close automatically

@kbeker kbeker added this to the next milestone Feb 20, 2019
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

5 participants