Skip to content

OCPBUGS-1428: Fix service account token secret reference #2862

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Sep 21, 2022
Merged

OCPBUGS-1428: Fix service account token secret reference #2862

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions pkg/lib/scoped/token_retriever.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,12 @@ func getAPISecret(logger logrus.FieldLogger, kubeclient operatorclient.ClientInt
func filterSecretsBySAName(saName string, secrets *corev1.SecretList) map[string]*corev1.Secret {
secretMap := make(map[string]*corev1.Secret)
for _, ref := range secrets.Items {
// Avoid referencing the "ref" created by the range-for loop as
// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


annotations := ref.GetAnnotations()
value := annotations[corev1.ServiceAccountNameKey]
if value == saName {
Expand Down
117 changes: 117 additions & 0 deletions pkg/lib/scoped/token_retriever_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
package scoped

import (
"testing"

"github.com/stretchr/testify/require"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

const serviceAccountName = "foo"

func TestFilterSecretsBySAName(t *testing.T) {
tests := []struct {
name string
secrets *corev1.SecretList
wantedSecretNames []string
}{
{
name: "NoSecretFound",
secrets: &corev1.SecretList{
Items: []corev1.Secret{
*newSecret("aSecret"),
*newSecret("someSecret"),
*newSecret("zSecret"),
},
},
wantedSecretNames: []string{},
},
{
name: "FirstSecretFound",
secrets: &corev1.SecretList{
Items: []corev1.Secret{
*newSecret("aSecret", withAnnotations(map[string]string{corev1.ServiceAccountNameKey: serviceAccountName})),
*newSecret("someSecret"),
*newSecret("zSecret"),
},
},
wantedSecretNames: []string{"aSecret"},
},
{
name: "SecondSecretFound",
secrets: &corev1.SecretList{
Items: []corev1.Secret{
*newSecret("aSecret"),
*newSecret("someSecret", withAnnotations(map[string]string{corev1.ServiceAccountNameKey: serviceAccountName})),
*newSecret("zSecret"),
},
},
wantedSecretNames: []string{"someSecret"},
},
{
name: "ThirdSecretFound",
secrets: &corev1.SecretList{
Items: []corev1.Secret{
*newSecret("aSecret"),
*newSecret("someSecret"),
*newSecret("zSecret", withAnnotations(map[string]string{corev1.ServiceAccountNameKey: serviceAccountName})),
},
},
wantedSecretNames: []string{"zSecret"},
},
{
name: "TwoSecretsFound",
secrets: &corev1.SecretList{
Items: []corev1.Secret{
*newSecret("aSecret"),
*newSecret("someSecret", withAnnotations(map[string]string{corev1.ServiceAccountNameKey: serviceAccountName})),
*newSecret("zSecret", withAnnotations(map[string]string{corev1.ServiceAccountNameKey: serviceAccountName})),
},
},
wantedSecretNames: []string{"someSecret", "zSecret"},
},
{
name: "AllSecretsFound",
secrets: &corev1.SecretList{
Items: []corev1.Secret{
*newSecret("aSecret", withAnnotations(map[string]string{corev1.ServiceAccountNameKey: serviceAccountName})),
*newSecret("someSecret", withAnnotations(map[string]string{corev1.ServiceAccountNameKey: serviceAccountName})),
*newSecret("zSecret", withAnnotations(map[string]string{corev1.ServiceAccountNameKey: serviceAccountName})),
},
},
wantedSecretNames: []string{"aSecret", "someSecret", "zSecret"},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := filterSecretsBySAName(serviceAccountName, tt.secrets)
require.Equal(t, len(tt.wantedSecretNames), len(got))
for _, wantedSecretName := range tt.wantedSecretNames {
require.NotNil(t, got[wantedSecretName])
require.Equal(t, wantedSecretName, got[wantedSecretName].GetName())
}
})
}
}

type secretOption func(*corev1.Secret)

func withAnnotations(annotations map[string]string) secretOption {
return func(s *corev1.Secret) {
s.SetAnnotations(annotations)
}
}

func newSecret(name string, opts ...secretOption) *corev1.Secret {
s := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: name,
},
}
for _, opt := range opts {
opt(s)
}
return s
}