test: add test coverage for missing permissions #1123
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There are multiple issues with missing permission, though they all share the same KNV2004 error code. This commit adds test coverage for the following scenarios: 1. A GSA doesn't exist when using gcpserviceaccount 2. A GSA exists, but it is not impersonated with a KSA. 3. A GSA exists and is impersonated, but it doesn't have reader permission. 4. A WI+KSA doesn't have reader permission.
| Namespace: configsync.ControllerNamespace, | ||
| Name: core.RootReconcilerName(rs.Name), | ||
| } | ||
| require.NoError(nt.T, |
There was a problem hiding this comment.
most of the e2e tests use nt.Must(...) instead of require.NoError(nt.T, ...).
| gsaEmail: tc.gsaEmail, | ||
| } | ||
| mustConfigureRootSync(nt, rs, tenant, spec) | ||
| nt.WaitForRootSyncSourceError(rs.Name, status.SourceErrorCode, tc.expectedErrMsg) |
There was a problem hiding this comment.
All these auth errors makes me wonder if we shouldnt add a new error code. I would normally expect the problem with a source error to be in the source, but these are all auth problems fetching the source.
|
|
||
| // The exact error message returned by AR for the Helm chart is: | ||
| // unexpected error rendering chart, will retry","Err":"rendering helm chart: invoking helm: Error: failed to authorize: failed to fetch oauth token: unexpected status from GET request to https://us-docker.pkg.dev/v2/token?scope=repository%3Astolos-dev%2Fconfig-sync-e2e-test--nanyu-cluster-1%2Fcoredns-test-helm-argke-workload-identit%3Apull\u0026service=us-docker.pkg.dev: 403 Forbidden | ||
| missingHelmARPermissionError = "403 Forbidden" |
There was a problem hiding this comment.
Would it make sense to change the test validation to allow regex so we could test for more of the error message without needing to know/encode the url? Like maybe rendering helm chart: invoking helm: Error: failed to authorize: failed to fetch oauth token: unexpected status from GET request to (.*): 403 Forbidden
| // The exact error message returned for GKE WI is: | ||
| // credential refresh failed: auth URL returned status 500, body: \"error retrieveing TokenSource.Token: compute: Received 403 `Unable to generate access token; IAM returned 403 Forbidden: Permission 'iam.serviceAccounts.getAccessToken' denied on resource (or it may not exist).\\nThis error could be caused by a missing IAM policy binding on the target IAM service account.\\nFor more information, refer to the Workload Identity documentation:\\n\\thttps://cloud.google.com/kubernetes-engine/docs/how-to/workload-identity#authenticating_to | ||
| // The exact error message returned for Fleet WI is: | ||
| // credential refresh failed: auth URL returned status 500, body: \"error retrieveing TokenSource.Token: oauth2/google: status code 403: {\\n \\\"error\\\": {\\n \\\"code\\\": 403,\\n \\\"message\\\": \\\"Permission 'iam.serviceAccounts.getAccessToken' denied on resource (or it may not exist).\\\",\\n \\\"status\\\": \\\"PERMISSION_DENIED\\\",\\n \\\"details\\\": [\\n {\\n \\\"@type\\\": \\\"type.googleapis.com/google.rpc.ErrorInfo\\\",\\n \\\"reason\\\": \\\"IAM_PERMISSION_DENIED\\\",\\n \\\"domain\\\": \\\"iam.googleapis.com\\\",\\n \\\"metadata\\\": {\\n \\\"permission\\\": \\\"iam.serviceAccounts.getAccessToken\\\"\\n }\\n }\\n ]\\n }\\n}\\n\\n\" |
There was a problem hiding this comment.
This error is really messy with all the multi-line json escaping. Is there any way we can clean up the error a bit for the user to make it more readable?
There was a problem hiding this comment.
error retrieving TokenSource.Tokenis from our askpass sidecar code.oauth2/google: status code %d: %sis from one of 3 places in the oauth2/google internal library.
I wonder if we could add some code to the askpass sidecar to detect and parse a JSON payload in the error string and do some formatting to pretty-print it so it's more readable like the GKE WI error.
There was a problem hiding this comment.
Or even better, maybe we can modify the oauth2 library to parse it for us...
|
I accidentally pushed from the wrong fork branch. I'll push a new patch after addressing these comments. |
|
@nan-yu: The following tests failed, say
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. I understand the commands that are listed here. |
There are multiple issues with missing permission, though they all share the same KNV2004 error code.
This commit adds test coverage for the following scenarios: