-
Notifications
You must be signed in to change notification settings - Fork 78
[release-4.8] OCPBUGS-5527: Fix label key truncation for subscription annotations (#2731) #427
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
Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com> Co-authored-by: Per Goncalves da Silva <pegoncal@redhat.com> Upstream-repository: operator-lifecycle-manager Upstream-commit: 1bdf969e6dff317e975de21078d60dd1406235e0
|
@openshift-cherrypick-robot: Jira Issue OCPBUGS-4948 has been cloned as Jira Issue OCPBUGS-5527. Retitling PR to link against new bug. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@openshift-cherrypick-robot: No Bugzilla bug is referenced in the title of this pull request. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@openshift-cherrypick-robot: This pull request references Jira Issue OCPBUGS-5527, which is valid. The bug has been moved to the POST state. 6 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@grokspawn we spoke about this before Holiday Shutdown, IIRC we had planned to backport this to 4.8 as some operator could hit this issue and SPAM the api server. Can you confirm that we had planned to backport this to 4.8? |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
@openshift-cherrypick-robot: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/label backport-risk-assessed |
|
/approve |
|
@jianzhangbjz could you review and add the cherry-pick-approved label? |
|
/lgtm |
tmshort
left a comment
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.
Depends on whether one has to worry about illegal characters on short names, or only when it's truncated.
|
|
||
| // Remove trailing illegal characters | ||
| idx := len(name) - 1 | ||
| for ; idx >= 0; idx-- { | ||
| lastChar := name[idx] | ||
| if lastChar != '.' && lastChar != '_' && lastChar != '-' { | ||
| break | ||
| } | ||
| } | ||
|
|
||
| // Just being defensive. This is unlikely to happen since the operator name should | ||
| // be compatible kubernetes naming constraints | ||
| if idx < 0 { | ||
| return "", fmt.Errorf(componentLabelKeyError, "unsupported name field") | ||
| } | ||
|
|
||
| // Update Label | ||
| name = name[0 : idx+1] |
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 sure why no one seems to:
if name = strings.TrimRight(name[0:63], "._-"); len(name) <= 0 {
return ....
}
Should the illegal characters be checked outside the len(name) check? Is it possible to set a name with an illegal trailing character? Could that happen?
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 sure why no one seems to:
if name = strings.TrimRight(name[0:63], "._-"); len(name) <= 0 {
return ....
}
I actually prefer your suggestion, but this is a backport of a fix and we attempt to avoid altering the code. We can always create a PR on master to do this though.
Is it possible to set a name with an illegal trailing character? Could that happen?
The kube api server will reject the create request and OLM will constantly try to create it even with the error, which is why we're backporting this.
| expectedLabel: "", | ||
| returnsError: true, | ||
| }, | ||
| } |
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.
This doesn't have the case for a small label key with illegal characters...
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.
We'll need to fix this in master.
tmshort
left a comment
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
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: awgreene, openshift-cherrypick-robot, tmshort The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Test pass, details: https://issues.redhat.com/browse/OCPBUGS-5527 |
|
@openshift-cherrypick-robot: An error was encountered updating to the MODIFIED state for bug OCPBUGS-5527 on the Jira server at https://issues.redhat.com/. No known errors were detected, please see the full error message for details. Full error message.
No transition status with name `MODIFIED` could be found. Please select from the following list: []
Please contact an administrator to resolve this issue, then request a bug refresh with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/jira refresh |
|
@awgreene: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-5527 has been moved to the MODIFIED state. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
…ift#427) Upstream-repository: api Upstream-commit: 56393459804e944ea1107fb8e23e3ef7def801b7
…ift#427) Upstream-repository: api Upstream-commit: 56393459804e944ea1107fb8e23e3ef7def801b7
…ift#427) Upstream-repository: api Upstream-commit: 56393459804e944ea1107fb8e23e3ef7def801b7
…ift#427) Upstream-repository: api Upstream-commit: 56393459804e944ea1107fb8e23e3ef7def801b7
This is an automated cherry-pick of #423
/assign awgreene