OCPBUGS-1428: Fix service account token secret reference #2862
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: awgreene 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
pkg/lib/scoped/token_retriever.go
Outdated
| annotations := ref.GetAnnotations() | ||
| value := annotations[corev1.ServiceAccountNameKey] | ||
| if value == saName { | ||
| secretMap[ref.Name] = &ref | ||
| secretMap[ref.Name] = &secrets.Items[i] |
There was a problem hiding this comment.
We could use a deepCopy here:
- Pro: Object in the client cache isn't returned. As it is, users should never edit the secrets returned by this function for risk of concurrent map writes. Also prevents changes to these secrets if updated on cluster.
- Con: Memory footprint.
There was a problem hiding this comment.
I'd vote for DeepCopy here since we potentially leave ourselves open to race conditions in the future where the secrets list gets changed on cluster in the middle of this operation. In that case, we could output a reference that is invalid. How big of an extra memory footprint are we thinking here?
There was a problem hiding this comment.
The canonical solution I've seen (and typically use) for this is:
for _, ref := range secrets.Items {
ref := ref
...
}There was a problem hiding this comment.
Updated per @joelanford's suggestion, but moved it inside the if statement.
pkg/lib/scoped/token_retriever.go
Outdated
| for i, ref := range secrets.Items { | ||
| annotations := ref.GetAnnotations() | ||
| value := annotations[corev1.ServiceAccountNameKey] | ||
| if value == saName { | ||
| secretMap[ref.Name] = &ref | ||
| secretMap[ref.Name] = &secrets.Items[i] | ||
| } | ||
| } |
There was a problem hiding this comment.
I created this go playground code to showcase the issue.
awgreene
force-pushed
the
zsecret
branch
3 times, most recently
from
1dc817b to
51b5854
Compare
|
Looks like we need to rebase. Then I'll LGTM. |
|
@timflannagan Yes, it is 2094303 in BZ and that produced a new bug in Jira with an ID of 1428. |
Problem: The filterSecretsBySAName function attempts to identify all service account token secrets related to a serviceAccount. To do so, the filterSecretsBySAName function uses a range-for loop to iterate over entries in the secrets argument. If a valid service account token secret is found, a pointer to the range-for loop's value variable is added to a map of results. Unfortunately, if a valid entry is found in the middle of the list of secrets, the value returned by the range-for loop is updated, causes the entry in the map to change. Solution: Add a pointer to the actual secret instead of the range-for loop's value variable. Signed-off-by: Alexander Greene <[email protected]>
awgreene
changed the title
openshift-ci-robot
added
the
jira/invalid-bug
|
@awgreene: This pull request references Jira Issue OCPBUGS-1428, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In 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 |
openshift-ci-robot
added
jira/valid-bug
|
@awgreene: This pull request references Jira Issue OCPBUGS-1428, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: In 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-ci-robot
removed
the
jira/invalid-bug
|
@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: Xia-Zhao-rh. Note that only operator-framework members and repo collaborators can review this PR, and authors cannot review their own PRs. In 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. |
|
/retest |
| // the secret stored in the map will change if there are more | ||
| // entries in the list of secrets that the range-for loop is | ||
| // iterating over. | ||
| ref := ref |
There was a problem hiding this comment.
I really prefer not forcing unintuitive inner/outer variable collisions to avoid this.
I'll LGTM, but IMHO the better approach is to use a non-colliding loop-scoped variable with a note.
I concede that this yields a better code delta, but if it requires a substantial comment like the one provided, is it really an improvement?
There was a problem hiding this comment.
This has already merged, but I 100% agree with the sentiment.
In this case, @joelanford pointed out that this is the canonical solution that he usually sees to address this intuitive problem. As you said, we could create our own way of avoiding the issue and forego the comment, but I would rather:
- Adhere to standards found in multiple go projects.
- Not "hide" the problem by coming up with our own solution.
- Document the reason for the approach with a comment so our team is aware of this pattern and can apply it when writing/reviewing code later.
Again, I don't know that I would have supported this approach had I been there when the pattern was created, but now that it is established I will respect it.
|
/lgtm |
|
@awgreene: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-1428 has been moved to the MODIFIED state. In 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. |
Problem: The filterSecretsBySAName function attempts to identify all service account token secrets related to a serviceAccount. To do so, the filterSecretsBySAName function uses a range-for loop to iterate over entries in the secrets argument. If a valid service account token secret is found, a pointer to the range-for loop's value variable is added to a map of results. Unfortunately, if a valid entry is found in the middle of the list of secrets, the value returned by the range-for loop is updated, causes the entry in the map to change.
Solution: Add a pointer to the actual secret instead of the range-for loop's value variable.
Signed-off-by: Alexander Greene [email protected]