-
Notifications
You must be signed in to change notification settings - Fork 817
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
Adding SAN to fleet autoscaler certs and updating documentation #1910
Conversation
Build Succeeded 👏 Build Id: 2edd1844-0f6b-4fb6-9267-a86ffe02e20f The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
h2ccwwKBgQCha4ox+SaXzlYROr1qge8xgqK+lpg1i2f32PgK3Ipodjk3esAuzeNM | ||
L5o5pDLorHu4EFtveIneRsV+kf8YPVuid18lYzMAJBqlXvcUik2Izk57cWB/P9so | ||
3/03jXI8iT8BbIU+PoII2EvQEPeAI07BYMU9cvsiFvFoB6z162DJhw== | ||
MIIEpQIBAAKCAQEAzTtFY02SAY4jHiryJbBRT4+2wn1OlqL4WTWUFtKaWEjm+gAn |
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.
Oh that's awesome - so we now also know that using a SAN works with the current Agones install and Go toolchain.
Fantastic!
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.
Thank you for doing this!!!!
Minor doc note, but otherwise this is awesome.
site/content/en/docs/Getting Started/create-webhook-fleetautoscaler.md
Outdated
Show resolved
Hide resolved
[v3_req] | ||
keyUsage = digitalSignature | ||
extendedKeyUsage = serverAuth | ||
subjectAltName = @alt_names |
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.
You can also use the existing configurations in openssl.cnf and only set SAN. Here is an example of doing it in the old version of Agones documentations: https://1-4-0.agones.dev/site/docs/advanced/allocator-service/#server-tls-certificate
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.
oooh!
I have no strong opinions either way. Whichever is easier to explain?
Do we feel like having a file better, because it's easier for automation tools to utilise?
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.
I am happy to change it to use the /etc/ssl/openssl.cnf
, but my thinking is that if you use that we are appending extra config to a fairly chunk example file. Whereas if we use a custom cert.conf
it is easier for people to follow what is actually happening.
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.
I am happy to change it to use the /etc/ssl/openssl.cnf, but my thinking is that if you use that we are appending extra config to a fairly chunk example file. Whereas if we use a custom cert.conf it is easier for people to follow what is actually happening.
Yeah, I would agree. Also wouldn't want people editing/thinking they have to edit the /etc/ssl/openssl.cnf
file either.
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.
Shall I keep it as is then?
…caler.md Co-authored-by: Mark Mandel <markmandel@google.com>
Build Succeeded 👏 Build Id: e1f72257-3790-43ba-a920-019b566705cb The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Am I OK to merge this or is there somenthing I should fix up before hand? |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kdima, pooneh-m The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
New changes are detected. LGTM label has been removed. |
Making this PR as breaking, not because it actually breaks things, but to remind us to make a note in the release notes to get people to update their webhook certs. |
Build Succeeded 👏 Build Id: 5e0e56fd-a023-4792-8c62-609c4e06f593 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
What type of PR is this?
/kind cleanup
What this PR does / Why we need it:
This PR solves part of #1899. I have updated the certs used for webhook integration test to include SAN record and also updated the docs with the description on how to generate such certs.
Which issue(s) this PR fixes:
It partially completes #1899
Special notes for your reviewer:
I have tested this change by running the
TestFleetAutoscalerTLSWebhook
integration test on go 1.15.5.