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

Reverse Breaking Changes, Introduce beforeTemplate #544

Open
erikerikson opened this issue Sep 14, 2018 · 7 comments
Open

Reverse Breaking Changes, Introduce beforeTemplate #544

erikerikson opened this issue Sep 14, 2018 · 7 comments

Comments

@erikerikson
Copy link
Contributor

erikerikson commented Sep 14, 2018

As noted in the deprecated artillery-core repository (and antecedent of ~/core here), the change to the ordering of templating was breaking.

I recognize the importance of pre-templating customization/logic needs (it would have made artillery-core-uuid to produce variable values far easier to use and implement). However, it was a breaking change. Please reverse the breaking change.

To maintain support for the use cases you enabled with this change, I advise adding a beforeTemplate hook that appropriately names the timing and facilitates those use cases.

If you prefer not to retract the change, perhaps you can re-enable the ability to execute logic after templating but before request sending via a different hook.

@hassy
Copy link
Member

hassy commented Sep 20, 2018

Thanks @erikerikson! We will need to consider the options. Simply reversing the change runs the risk of breaking a lot of scripts out there that have come to rely on the current behavior of beforeRequest. Another hook is the best option in my opinion, but I'm not sure as to what the clearest / least ambiguous name might be - do you have any suggestions?

@erikerikson
Copy link
Contributor Author

erikerikson commented Sep 25, 2018

It's a tricky circumstance for sure with bad results regardless of choice. Those newer script creators may be more invested in updating them but aside from that I'd think that the size of the investment over time prior to this relatively recent change would outweigh the post-change investment. I don't have data to back that up on hand and it's clearly not my call to make. FWIW, given our prior investments, this occurrence calls to question the prudence of or stability of our investments.

I personally liked the semantics of the beforeRequest being just before the making of the request because the name and action were so tightly correlated. That was part of the reason I recommended beforeTemplate (although that's a code based name and may be a poor choice for communications to user as a result) given the subsequent use of the template method to render variables. Perhaps beforeVariableResolution would be more appropriate albeit a bit long. beforeResolution or beforeRender are shorter alternatives.

I suppose if you are asking for recommendations on the name for a re-introduction of the old semantics, something like beforeTransmission or beforeSend might suffice. While we will have to adopt the choice of the tool, I'd suggest that'd be the wrong direction to go.

@elgalu
Copy link

elgalu commented Mar 31, 2019

Thanks for reporting this.

Had to pin previous artillery:

  "dependencies": {
    "artillery": "1.6.0-26",
    "artillery-plugin-aws-sigv4": "0.0.3",
    "artillery-plugin-expect": "1.2.1"
  },

In order to solve this error:

The request signature we calculated does not match the signature you provided.

The Canonical String for this request should have been

'POST\n/endpoints/some-sm-endpoint/invocations\n\nhost:runtime.sagemaker.eu-central-1.amazonaws.com\nx-amz-date:20190331T055402Z\nx-amz-security-token:***==\n\nhost;x-amz-date;x-amz-security-token\n***'

The String-to-Sign should have been
'AWS4-HMAC-SHA256\n20190331T055402Z\n20190331/eu-central-1/sagemaker/aws4_request\n***'

@lukecleland-myfiziq
Copy link

lukecleland-myfiziq commented Apr 24, 2019

This is still not working for me, can't add any variables to the request. Have tried @elgalu's comment above, also I've tried @galvinhsiu's fork. Nothing works. The beforeRequest is being run before the variables hence signature mismatch.

@Lillypot
Copy link

Lillypot commented Mar 18, 2020

Hi - I'm having the same issue where I have installed the aws-sigv4 plugin and added the lines in my script. However, on executing using debug mode I get '"The request signature we calculated does not match the signature you provided. Check your AWS Secret Access Key and signing method. Consult the service documentation for details.\n\nThe Canonical String for this request should have been' Can anyone help me with this please? step by step instructions on how to resolve this issue as I have been struggling with this for several days. Thanks in advance

@erikerikson
Copy link
Contributor Author

@Lillypot please go to the https://github.com/Nordstrom/artillery-plugin-aws-sigv4 repo. The instructions there should help but it not feel free to open an issue.

@Lillypot
Copy link

Lillypot commented Mar 24, 2020

@Lillypot please go to the https://github.com/Nordstrom/artillery-plugin-aws-sigv4 repo. The instructions there should help but it not feel free to open an issue.

Hi @erikerikson -- yes I have used the instruction from these sites but the problem I have is that I connect via command line and assume my role and then when I run my script using debug mode I get mismatch of signatures. I have installed the plugin as per the instructions in the document link and add the plugin into the script.
I still get the '"The request signature we calculated does not match the signature you provided. Check your AWS Secret Access Key and signing method. Consult the service documentation for details.The Canonical String for this request should have been". Please advise ... Thanks again in advance
I notice that the beforerequest is not taking the body .. just an observation

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

No branches or pull requests

5 participants