-
Notifications
You must be signed in to change notification settings - Fork 826
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
Doc changes for TLS and loadBalancerIP changes #1784
Conversation
Build Failed 😱 Build Id: 4611f394-7681-4470-8ff8-299d169bc6ef To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: 6b2cf28f-ba21-4788-ab30-ad73df6ff50e 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: 2d2fcbbc-e68e-40b1-8e8c-8fe208b6f883 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:
|
@markmandel can you please take a look? |
Build Succeeded 👏 Build Id: 19da9e7e-f420-4d11-a86e-c58c0f8411f3 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:
|
@@ -218,6 +218,8 @@ The following tables lists the configurable parameters of the Agones chart and t | |||
| `agones.allocator.generateTLS` | Set to true to generate TLS certificates or false to provide certificates in `certs/allocator/*`| `true` | | |||
| `agones.allocator.tolerations` | Allocator [toleration][toleration] labels for pod assignment | `[]` | | |||
| `agones.allocator.affinity` | Allocator [affinity][affinity] settings for pod assignment | `{}` | | |||
| `agones.allocator.disableTLS` | Turns off the built-in encryption and authentication mechanism for the agones-allocator service. | `false` | | |||
| `agones.allocator.disableMTLS` | Just turns off client cert authentication for incoming connections to the allocator. | `false` | |
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.
Could be simplified a bit, I cannot understand the difference based on the descriptions.
| `agones.allocator.disableMTLS` | Just turns off client cert authentication for incoming connections to the allocator. | `false` | | |
| `agones.allocator.disableMTLS` | Turns off client cert authentication for incoming connections to the allocator. | `false` | |
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 clarified the text a bit.
disableMTLS - no client cert authentication
disableTLS -. full plaintext (no client authentication, and no server cert encryption)
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.
Could you please use New Configuration Features
for the new disableTLS
flag, and I assume that disableMTLS could be left as is. Or both could be moved to a section under publishVersion:
https://github.com/googleforgames/agones/blame/638a7fddecd23ec24881a738a59c45e0f8b0db7d/site/content/en/docs/Installation/Install%20Agones/helm.md#L227
I moved both disableMTLS and disableTLS to the 'New Configuration Changes' section. |
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.
Thanks for the fast fix. Someone is needed to verify wording before the final merge.
Build Failed 😱 Build Id: 59e16ebc-572b-4b0d-95b6-93b875c77c25 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: 986a07e1-6d81-4509-9843-3b15de0b94f1 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.
Thanks for the changes.
@@ -229,6 +229,8 @@ The following tables lists the configurable parameters of the Agones chart and t | |||
| Parameter | Description | Default | | |||
| --------------------------------------------------- | ----------------------------------------------------------------------------------------------- | ---------------------- | | |||
| `agones.allocator.http.loadBalancerIP` | The [Load Balancer IP][loadBalancerIP] of the Agones allocator load balancer. Only works if the Kubernetes provider supports this option. | "" | | |||
| `agones.allocator.disableMTLS` | Turns off client cert authentication for incoming connections to the allocator. | `false` | |
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.
Does this also need a release guard? Or are these options available in the current release?
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.
One of these options was present, not sure if the functionality was the same as current one.
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 is a breaking change for disableMTLS
between 1.8 and 1.9. The helm configuration was not documented because we did not want anyone get a dependency to it, until we know exactly what are the requirements for GKE ingress. If it is moved to 1.8 documentation, a user may get a dependency and the behavior changes with the next 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.
Even though it wasn't documented, should we add a 1.9 release note that mentions the breaking change?
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 don't think we have to document it as a breaking change as it has not been documented at all and off by default. Only by reading the code the feature flag could be discovered. However, if there is a best practices on what to do in this situation, we should go with it. Otherwise, I would say it is unnecessary.
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.
+1 to what Pooneh mentioned. disableMTLS was never documented earlier.
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.
Just confirming before merging this - it is good to merge?
Build Succeeded 👏 Build Id: 1fbd7eba-aae9-42df-8f50-87a90bebb9b2 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.
Well done! Thanks for your contribution.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aLekSer, devloop0, 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 Failed 😱 Build Id: e87856b8-50e5-4b7b-abe0-c91973913dc8 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: 9f7b55cf-9ff8-4d0e-a8ef-e7f4926262e4 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:
|
* Added documentation for disableMTLS and disableTLS flags. * Added documentation about the fixed load balancer IP as well. * Fixed a link typo in the allocator-service.md doc. * Fixed typo in the name of the Agones loadBalancerIP flag. * Modified wording on the docs. * Clarified the text for disableTLS vs disableMTLS. * Moved disableMTLS to the 'New Configuration Changes' section. * Fixed more wording issues. Co-authored-by: Nikhil Athreya <nathreya@google.com> Co-authored-by: pooneh-m <46979170+pooneh-m@users.noreply.github.com>
What type of PR is this?
/kind documentation
What this PR does / Why we need it:
Clarifies usage about the
agones.allocator.{disableMTLS,disableTLS,http.loadBalancerIP}
flags.Which issue(s) this PR fixes:
Closes #
Special notes for your reviewer: