-
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
Use ServerCA from GameServerAllocationPolicy instead of client secret ca.crt #1545
Conversation
Build Failed 😱 Build Id: 302468ce-30c3-447f-b7d5-e8c847863feb To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
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 only found one small documentation issue, but outside of that, this look good to go 👍
priority: 1 | ||
weight: 100 | ||
EOF | ||
``` | ||
|
||
To define the local cluster priority, similarly, an allocation rule should be defined, while leaving allocationEndpoints unset. If the local cluster priority is not defined, the allocation from the local cluster happens only if allocation from other clusters with the existing allocation rules is unsuccessful. | ||
|
||
`sercerCA` is the server TLS CA public certificate, set only if the remote server certificate is not signed by a public CA (e.g. self-signed). |
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.
Can we please wrap the references to serverCA, with a feature shortcode?
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.
Done. Thanks!
@@ -67,7 +71,7 @@ EOF | |||
|
|||
The certificates are base 64 string of the certificate file e.g. `cat ${CERT_FILE} | base64 -w 0` | |||
|
|||
`ca.crt` is the server TLS public certificate if it is self-signed. For simplicity, it is recommended to use one client secret per cluster and make `ca.crt` bundle of server certificates. | |||
Agones recommends using [cert-manager.io](https://cert-manager.io/) solution for generating client certificates. |
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.
Not for this PR, but do we want to provide some docs on this in some way down the line?
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.
There are already documentation on allocator-service.md for cert-manager.io. I'll try to provide snippets in this doc as well, but in a different PR.
Build Succeeded 👏 Build Id: 64c1cbc0-8c6e-47f4-a68a-9171a1170886 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:
|
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.
Done!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: markmandel, 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. |
Build Succeeded 👏 Build Id: 5548fe0d-51d4-4382-8deb-ccb3ca3374bc 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:
|
Build Succeeded 👏 Build Id: e63d7d74-bfa6-4e0b-b8eb-e021cfa4b6fc 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:
|
Build Succeeded 👏 Build Id: 6bfabc8a-bc14-4ac4-8404-c4724a9eef3c 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:
|
Build Succeeded 👏 Build Id: 2f381365-9556-4bf6-879d-54651e979dd6 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:
|
Co-authored-by: Mark Mandel <markmandel@google.com>
What type of PR is this?
/kind feature
What this PR does / Why we need it:
This is adding ServerCA field to GameServerAllocationPolicy to be used instead of client ca.crt field. This unblocks cert manager adaptation for multi-cluster allocation client certs.
Which issue(s) this PR fixes:
Closes #1517
Special notes for your reviewer: