-
Notifications
You must be signed in to change notification settings - Fork 173
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
improve federated cred naming #3997
base: master
Are you sure you want to change the base?
improve federated cred naming #3997
Conversation
/azp run ci, e2e |
Azure Pipelines successfully started running 2 pipeline(s). |
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.
Implementation LGTM, but now that I'm seeing it I have 1 concern with the new naming format - service account names take the form system:serviceaccount:${NAMESPACE}:${WORKLOAD_NAME}
, those two parameters often have dashes within them (e.g. system:serviceaccount:openshift-cloud-controller-manager:cloud-controller-manager
).
If we suffix -${CLUSTER_NAME}
to that, we'll end up with e.g. system:serviceaccount:openshift-cloud-controller-manager:cloud-controller-manager-aro-cluster
or equivalent.
Perhaps we should use a different separator between the SA name and the cluster name? For example --
, or _
? According to https://learn.microsoft.com/en-us/entra/workload-id/workload-identity-federation-considerations, we're limited to alphanumeric, dash, and underscore characters.
@@ -18,7 +18,7 @@ const ( | |||
) | |||
|
|||
func GetPlatformWorkloadIdentityFederatedCredName(clusterResourceId, identityResourceId azure.Resource, serviceAccountName string) string { | |||
clusterResourceKey := fmt.Sprintf("%s-%s-%s", clusterResourceId.SubscriptionID, clusterResourceId.ResourceGroup, clusterResourceId.ResourceName) | |||
clusterResourceKey := fmt.Sprintf("%s_%s", serviceAccountName, clusterResourceId.ResourceName) |
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 replace any occurrences of :
in the serviceAccountName as well, :
is not a valid character for federated credential names. We may want to consider parsing the name and trimming off the first two sections, as these service account names will generally start with system:serviceaccount:
which is redundant for the purposes of naming the fedcred.
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.
Left comments on how to fix the unit test issues. thanks for your efforts here!
sanitizedServiceAccountName := strings.ReplaceAll(serviceAccountName, ":", "-") | ||
parts := strings.Split(sanitizedServiceAccountName, "-") |
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.
The unit tests are failing because the tests themselves don't supply a valid service account name for you to sanitize. For example, on line 670 of pkg/cluster/workloadidentityresources_test.go:
serviceAccount: "openshift-cloud-controller-manager:cloud-controller-manager",
Should be this:
serviceAccount: "system:serviceaccount:openshift-cloud-controller-manager:cloud-controller-manager",
The same is true on pkg/validate/dynamic/platformworkloadidentityprofile_test.go line 427. This:
expectedPlatformIdentity1ExtraFederatedCredName := platformworkloadidentity.GetPlatformWorkloadIdentityFederatedCredName(clusterResourceId, platformIdentity1ResourceId, "something else")
Should be a valid service account name like this:
expectedPlatformIdentity1ExtraFederatedCredName := platformworkloadidentity.GetPlatformWorkloadIdentityFederatedCredName(clusterResourceId, platformIdentity1ResourceId, "system:serviceaccount:something:else")
Hopefully this fixes the unit test issues.
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
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.
Thanks again, here are our notes from code review.
@@ -8,6 +8,8 @@ import ( | |||
"fmt" | |||
"math/big" | |||
|
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.
@@ -18,16 +20,17 @@ const ( | |||
) | |||
|
|||
func GetPlatformWorkloadIdentityFederatedCredName(clusterResourceId, identityResourceId azure.Resource, serviceAccountName string) string { | |||
clusterResourceKey := fmt.Sprintf("%s-%s-%s", clusterResourceId.SubscriptionID, clusterResourceId.ResourceGroup, clusterResourceId.ResourceName) | |||
name := fmt.Sprintf("%s-%s-%s", clusterResourceKey, serviceAccountName, identityResourceId.ResourceName) | |||
sanitizedServiceAccountName := strings.Join((strings.Split(strings.ReplaceAll(serviceAccountName, ":", "-"), "-"))[2:], "-") |
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.
Consider using strings.TrimPrefix() instead, since the service account name will always start with system:serviceaccount:
.
Consider also dropping the namespace from the federated credential name. You could do this by splitting the service account name string like you do now, but keep only the last element.
// the base-36 encoded string of a SHA-224 hash will typically be around 43 to 44 characters long. | ||
hash := sha256.Sum224([]byte(name)) | ||
encodedName := (&big.Int{}).SetBytes(hash[:]).Text(base36Encoding) | ||
remainingChars := maxFederatedCredNameLength - len(encodedName) - numberOfDelimiters | ||
|
||
if remainingChars < len(clusterResourceKey) { | ||
return fmt.Sprintf("%s-%s", clusterResourceKey[:remainingChars], encodedName) | ||
return fmt.Sprintf("%s_%s", clusterResourceKey, encodedName)[:remainingChars] |
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.
Instead of trimming the whole resulting string, we only want to trim the clusterResourceKey (which if trimmed will be the service account name).
return fmt.Sprintf("%s_%s", clusterResourceKey, encodedName)[:remainingChars] | |
return fmt.Sprintf("%s_%s", clusterResourceKey[:remainingChars], encodedName) |
@@ -29,7 +29,7 @@ func TestGetPlatformWorkloadIdentityFederatedCredName(t *testing.T) { | |||
}) | |||
|
|||
t.Run("has expected key as prefix", func(t *testing.T) { | |||
wantPrefix := fmt.Sprintf("%s-%s-%s", subscriptionId, resourceGroup, clusterName) | |||
wantPrefix := fmt.Sprintf("%s_%s", clusterName, strings.Join((strings.Split(strings.ReplaceAll(saName, ":", "-"), "-"))[2:], "-")) |
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.
Consider being explicit here rather than duplicating the strings logic.
wantPrefix := fmt.Sprintf("%s_%s", clusterName, strings.Join((strings.Split(strings.ReplaceAll(saName, ":", "-"), "-"))[2:], "-")) | |
wantPrefix := fmt.Sprintf("%s_%s", clusterName, "openshift-cloud-controller-manager") |
c050056
to
d954915
Compare
Which issue this PR addresses:
ARO-13251
Fixes
What this PR does / why we need it:
The customer should have a clear idea of what federated credentials are associated with a given cluster.
Currently, we use subscription ID + service account name + hash.
We want to move the naming to cluster name + service account name (without the namespace) + hash instead, so it’s clear what cluster a federated credential is associated with.
Test plan for issue:
Updated unit tests
Tested by creating a MIWI cluster
Is there any documentation that needs to be updated for this PR?
Not yet