-
Notifications
You must be signed in to change notification settings - Fork 127
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
feat: added certificate to load-balancer #396
Conversation
@alekc a few answers :) |
@Sh4d1 I could but all points are still valid.
Potentially integration test could be solved by providing a certain domain names for testing purposes only, not the most elegant solution but it would work. |
@alekc yep agree. I'll let @jerome-quere answer for the testing 😄 |
That's a tricky problem. Could the use of |
Excellent idea. With The only thing remaining for now is the obfuscation of the error. We will probably need to document in bold that a user has to have a dns pointing to certificate for it to work (some may be surprised by this). Should I try to parse raw error body and extract the error to show it to end user?
we can attempt to decode resulting json with type AutoGenerated struct {
Details []struct {
Field string `json:"field"`
Message string `json:"message"`
Reason string `json:"reason"`
} `json:"details"`
Message string `json:"message"`
Type string `json:"type"`
} and return errors.new(rawError.Details[0].Message) to enduser. |
@jerome-quere I believe I found another bug with scaleway api https://developers.scaleway.com/en/products/lb/api/#put-f21555 states that I shoud be able to change certificate name with update without doing the destruction first. However, when I attempted to do so, I got an unexpected result
Notice that I am asking to rename |
@jerome-quere could you forward the issue above to the LB team please? once thats fixed it would unlock testing and I can send this off for a review. |
The issue has been escalated to the load-balancer team. |
fixed. Thx @alekc |
Hi @alekc, Don't worry, if you are too busy we can include this PR in the v1.15.0. 👍 I will make a full review of the PR today. :) |
Yep, I should be able to finish testing and send for a review today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@QuentinBrosse fixed tests and most of the changes requested, please see above for couple of those which I have replied to. Also, found another "issue" with testing related to the custom certificate. Api doesn't play very well with self signed certs (tried to issue a 10 year one). So for now I've ended up extracting a chain from scaleway.com, just to have tests passing. I suspect that once we reach end of that certificate the test will begin to fail. Any ideas in regard? potentially we can always extract cert from somewhere (scaleway.com i.e.) when we are running the test. Not ideal but would work (since we are required to have internet access anyway for these tests). |
I will escalate the issue to the LB team (cc @agirot). Can you please add the doc of the resource? |
@QuentinBrosse docs are done, all points have been solved, branch rebased on latest master and ready to be merged. |
Merged docs changes. |
@alekc about your test to upload a custom certificate. You need to upload the full certif chain. https://developers.scaleway.com/en/products/lb/api/#certificate-chain-9183b2 |
@agirot Ah! the private key and ca. That was the missing piece of the puzzle. It's all working now, thx (pushed 10y self signed cert and a fix for failing travis build). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 details and it will be OK for me 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks again for your great work @alekc 👏 !
Signed-off-by: Patrik Cyvoct <[email protected]>
Fix #310
@jerome-quere the code is ready for testing, however Scaleway's current implementation presents some challenges (especially from integration tests point of view).
When running tests, I was presented with some cryptic 400 bad request message. Rerunning it through the trace mode showed following:
Error returned in INFO mode
There are several issues: