Create secrets for serviceaccounts #125
Conversation
|
🤖 Created branch: z_pr125/vthapar/secret-tokens |
vthapar
requested review from
Jaanki,
Oats87,
skitt and
sridhargaddam
as code owners
vthapar
dismissed
tpantelis’s stale review
Reworked the code, addressed comments
|
@skitt @tpantelis This is ready for review. I have only tweaked the UTs just enough to pass so that we can get this in and unblock submariner-io/shipyard#825 I can add UTs in a subsequent PR, unless you think UTs need to come in this patch itself. |
|
@vthapar is this ready for review? It looks like two CI jobs are failing |
Upgrade job is the only one that failed more than once, e2e has failed just once. Re-ran the jobs to see if e2e fails again. Looking into upgrade job issue. |
vthapar
force-pushed
the
secret-tokens
branch
3 times, most recently
from
543b847 to
678c374
Compare
|
🤖 I had an issue pushing the updated branch: already up-to-date |
|
@skitt @sridhargaddam @tpantelis this one needs review |
pkg/broker/ensure.go
Outdated
| return nil, err | ||
| } | ||
|
|
||
| _, err = serviceaccount.EnsureSecretFromSA(kubeClient, sa.Name, inNamespace) |
There was a problem hiding this comment.
My previous comment still applies, unless I’ve missed something:
I would prefer the secret handling to be hidden in the SA handling — callers shouldn’t have to know that to set up a service account correctly, they have to set up a secret.
In other words, there should only be a call to serviceaccount.Ensure() in this function, and serviceaccount.Ensure() should handle the secret (perhaps by calling EnsureSecretFromSA(), of course, but that should be transparent to the caller).
There was a problem hiding this comment.
My previous comment still applies, unless I’ve missed something:
I would prefer the secret handling to be hidden in the SA handling — callers shouldn’t have to know that to set up a service account correctly, they have to set up a secret.
In other words, there should only be a call to
serviceaccount.Ensure()in this function, andserviceaccount.Ensure()should handle the secret (perhaps by callingEnsureSecretFromSA(), of course, but that should be transparent to the caller).
Got it. I missed the context of comment for Ensure and only addressed for EnsureFromYaml.
|
@vthapar can you please address Stephen's comment? |
Done. |
pkg/serviceaccount/ensure.go
Outdated
| if err == nil { | ||
| return true, nil | ||
| } |
There was a problem hiding this comment.
We should distinguish NotFound error
| if err == nil { | |
| return true, nil | |
| } | |
| if apierrors.IsNotFound(err) | |
| return nil | |
| } | |
| if err != nil { | |
| return err | |
| } |
vthapar
force-pushed
the
secret-tokens
branch
3 times, most recently
from
f152db6 to
3b61f57
Compare
|
🤖 I had an issue pushing the updated branch: already up-to-date |
K8s 1.24+ doesn't autocreate secret for serviceaccount. This breaks submariner deployments which expects secret to be created and associated to the corresponding serviceaccount. The fix ensures the Secret of type service-account-token exists, is annotated with ServiceAccount.Name, and is added to Secrets list in the ServiceAccount. Fixes submariner-io#102 Signed-off-by: Vishal Thapar <[email protected]>
Signed-off-by: Vishal Thapar <[email protected]>
pkg/broker/ensure.go
Outdated
| if apierrors.IsNotFound(err) { | ||
|
|
||
| } |
There was a problem hiding this comment.
There’s something missing here, I presume.
There was a problem hiding this comment.
There’s something missing here, I presume.
Missed cleanup after refactor.
| if err != nil { | ||
| newSecret := &corev1.Secret{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: fmt.Sprintf("%s-token-%s", sa.Name, generateRandomString(5)), |
There was a problem hiding this comment.
We can fix this up later, this shouldn’t block the PR.
There was a problem hiding this comment.
I used GenerateName in intial draft, but then needed to do another Get to find the generated name. Went with this coz similar to what OCP's controller is doing.
Refactor serviceaccount.Ensure so that creation of secret is transparent to caller. Signed-off-by: Vishal Thapar <[email protected]>
skitt
added
the
backport
|
🤖 Closed branches: [z_pr125/vthapar/secret-tokens] |
K8s 1.24+ doesn't autocreate secret for serviceaccount. This
breaks submariner deployments which expects secret to be created
and associated to the corresponding serviceaccount.
The fix ensures the Secret of type service-account-token exists,
is annotated with ServiceAccount.Name, and is added to Secrets list in
the ServiceAccount.
Fixes #102
Signed-off-by: Vishal Thapar [email protected]