-
Notifications
You must be signed in to change notification settings - Fork 843
Fix JSON Schema validation for ServiceAccount annotations #4122
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
Conversation
- Updated `serviceaccount.*.annotations` schema to allow key-value string pairs. - Changed `additionalProperties` from `type: string` to `type: object`. - Resolves issue preventing Helm chart installation when annotations are provided. - Closes googleforgames#4119.
Build Succeeded 🥳 Build Id: 24722a91-abd1-4d5c-8505-049d7bb8089f 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.
For this PR we only want to change the agones.serviceaccount.sdk.annotations
. From our Helm install docs agones.serviceaccount.sdk.annotations
is A map of namespaces to maps of [Annotations](https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/) added to the Agones SDK service account for the specified namespaces
, whereas the rest of the annotations are a simple map of Annotations, and not a map of a map.
@@ -213,7 +213,7 @@ | |||
"annotations": { | |||
"type": "object", | |||
"additionalProperties": { | |||
"type": "string" | |||
"type": "object" |
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.
"type": "object" | |
"type": "object" | |
"additionalProperties": { | |
"type": "string" | |
} |
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.
@igooch The additionalProperties keyword is used to control the handling of extra fields, but it may not be needed for annotations. i hope, it does not work like a map of maps. please correct me if i'm wrong.
Also, when I tried using boolean, string, and number values in additionalProperties, I got these errors during installation:
Error: INSTALLATION FAILED: values don't meet the specifications of the schema(s) in the following chart(s):
agones:
- agones.serviceaccount.sdk.annotations: Additional property default is not allowed
Error: INSTALLATION FAILED: values don't meet the specifications of the schema(s) in the following chart(s):
agones:
- agones.serviceaccount.sdk.annotations.default: Invalid type. Expected: number, given: object
Error: INSTALLATION FAILED: values don't meet the specifications of the schema(s) in the following chart(s):
agones:
- agones.serviceaccount.sdk.annotations.default: Invalid type. Expected: string, given: object
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.
The service account annotations for sdk
is a map of maps. The service account annotations for controller
and allocator
are maps as defined in https://agones.dev/site/docs/installation/install-agones/helm/.
So a sample section of a values.yaml file might look like:
serviceaccount:
allocator:
name: agones-allocator
annotations:
"foo": bar
controller:
name: agones-controller
annotations:
"baz": qux
sdk:
name: agones-sdk
annotations:
default:
"annotation": value
In Kubernetes Annotations (and labels) are required to be strings https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/. So here we want to verify that the user only passes in string values.
@0xaravindh can you verify the fix by following the reproduce step in #4119? |
yes @gongmax .I was able to reproduce the issue. it’s fixed now. The fix works as expected. |
@@ -213,7 +213,7 @@ | |||
"annotations": { | |||
"type": "object", | |||
"additionalProperties": { | |||
"type": "string" | |||
"type": "object" |
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.
The service account annotations for sdk
is a map of maps. The service account annotations for controller
and allocator
are maps as defined in https://agones.dev/site/docs/installation/install-agones/helm/.
So a sample section of a values.yaml file might look like:
serviceaccount:
allocator:
name: agones-allocator
annotations:
"foo": bar
controller:
name: agones-controller
annotations:
"baz": qux
sdk:
name: agones-sdk
annotations:
default:
"annotation": value
In Kubernetes Annotations (and labels) are required to be strings https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/. So here we want to verify that the user only passes in string values.
@@ -173,13 +173,13 @@ | |||
"annotations": { | |||
"type": "object", | |||
"additionalProperties": { | |||
"type": "string" | |||
"type": "object" |
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.
"type": "object" | |
"type": "string" |
} | ||
}, | ||
"labels": { | ||
"type": "object", | ||
"additionalProperties": { | ||
"type": "string" | ||
"type": "object" |
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.
"type": "object" | |
"type": "string" |
@@ -196,7 +196,7 @@ | |||
"annotations": { | |||
"type": "object", | |||
"additionalProperties": { | |||
"type": "string" | |||
"type": "object" |
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.
"type": "object" | |
"type": "string" |
Build Succeeded 🥳 Build Id: 96457c05-bae8-4848-8d4f-ab784fcbd62e 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:
|
@igooch please take a look! |
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
What type of PR is this?
/kind bug
What this PR does / Why we need it:
serviceaccount.*.annotations
schema to allow key-value string pairs.additionalProperties
fromtype: string
totype: object
.Which issue(s) this PR fixes:
Closes #4119
Special notes for your reviewer: