Truncate annotations when generating kubernetes yaml files #14023
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan 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 |
openshift-ci
bot
added
the
approved
|
@umohnani8 PTAL |
|
@nivekuil PTAL |
|
I'd just as soon error instead of truncate, but I don't mind enough to not merge. LGTM |
cmd/podman/play/kube.go
Outdated
| kubeOptions.Annotations[splitN[0]] = splitN[1] | ||
| annotation := splitN[1] | ||
| if len(annotation) > define.MaxKubeAnnotation { | ||
| return errors.Errorf("annotation exheeds maximum size, %d, of kubernetes annotation: %s", define.MaxKubeAnnotation, annotation) |
There was a problem hiding this comment.
typo, should be exceeds with a 'c'
libpod/define/annotations.go
Outdated
| @@ -135,6 +135,8 @@ const ( | |||
| // creating a checkpoint image to specify the name of host distribution on | |||
| // which the checkpoint was created. | |||
| CheckpointAnnotationDistributionName = "io.podman.annotations.checkpoint.distribution.name" | |||
| // MaxKubeAnnotation is the max length of Annotions allowed by Kubernetes. | |||
pkg/domain/infra/abi/play.go
Outdated
|
|
||
| for name, val := range podYAML.Annotations { | ||
| if len(val) > define.MaxKubeAnnotation { | ||
| return nil, errors.Errorf("invalid annotation %q=%q value length exheeds Kubernetetes max %d", name, val, define.MaxKubeAnnotation) |
test/system/700-play.bats
Outdated
| mkdir -p $TESTDIR | ||
| echo "$testYaml" | sed "s|TESTDIR|${TESTDIR}|g" > $PODMAN_TMPDIR/test.yaml | ||
| run_podman 125 play kube --annotation "name=$RANDOMSTRING" $PODMAN_TMPDIR/test.yaml | ||
| assert "$output" =~ "annotation exheeds maximum size, 63, of kubernetes annotation:" "Expected to fail with Length greater then 63" |
There was a problem hiding this comment.
typo, exceeds.
Also, should be "greater than", not then
test/system/700-play.bats
Outdated
| echo "$testBadYaml" | sed "s|TESTDIR|${TESTDIR}|g" > $PODMAN_TMPDIR/test.yaml | ||
|
|
||
| run_podman 125 play kube - < $PODMAN_TMPDIR/test.yaml | ||
| assert "$output" =~ "invalid annotation \"test\"=\"$RANDOMSTRING\"" "Expected to fail with annotation length greater then 63" |
| @@ -288,6 +289,16 @@ func newServicePortState() servicePortState { | |||
| } | |||
| } | |||
|
|
|||
| func TruncateKubeAnnotation(str string) string { | |||
| str = strings.TrimSpace(str) | |||
| if utf8.RuneCountInString(str) < define.MaxKubeAnnotation { | |||
There was a problem hiding this comment.
Okay, one more. The only spec I can find (I didn't look too hard) mentions 63 characters but doesn't actually indicate if we're talking new-style Unicode or old-style ASCII C chars. Does anyone know how to figure this one out?
There was a problem hiding this comment.
IDK, but I"m interested in seeing the answer....
There was a problem hiding this comment.
It is also inconsistent since in cmd/podman/play/kube.go the byte len is checked and not utf8 len.
|
LGTM |
|
Changes LGTM once comments addressed |
Kubernetes only allows 63 characters in an annotation. Make sure that we only add 63 or less charaters when generating kube. Warn if containers or pods have longer length and truncate. Discussion: containers#13901 Fixes: containers#13962 Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
|
@rhatdan Why did you merge this? #14023 (comment) is not resolved! It is inconsistent, on the one part you check for 63 bytes and on the other you check for 63 utf8 characters |
|
I merged with two LGTM and fixed comments. We can open other PRs on the inconsistency. |
github-actions
bot
added
the
locked - please file new issue/PR
Kubernetes only allows 63 characters in an annotation. Make sure
that we only add 63 or less charaters when generating kube. Warn
if containers or pods have longer length and truncate.
Discussion: #13901
Fixes: #13962
Signed-off-by: Daniel J Walsh dwalsh@redhat.com