Cherrypick latest master to release-0.18 branch #377
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes crossplane-contrib#361 Signed-off-by: Hasan Turken <[email protected]> (cherry picked from commit 59ccb14)
These are not KMS Cloud Keys at all. Signed-off-by: Nic Cope <[email protected]> (cherry picked from commit 199992a)
Signed-off-by: Nic Cope <[email protected]> (cherry picked from commit b4c9720)
Signed-off-by: Nic Cope <[email protected]> (cherry picked from commit d439f1e)
See crossplane/crossplane-runtime#291 for context. Signed-off-by: Nic Cope <[email protected]> (cherry picked from commit 0cd8367)
https://github.com/golang/go/wiki/CodeReviewComments#named-result-parameters This is generally something we avoid in Crossplane code, in favor of more explicit returns. Guidance from upstream is that it should really only be done when it makes the function signature easier to read in GoDoc, which is not relevant in this function. Signed-off-by: Nic Cope <[email protected]> (cherry picked from commit 8b18b53)
See https://github.com/crossplane/crossplane/blob/b7ce021/CONTRIBUTING.md This commit mostly addresses style. The exceptions is that we remove a little defensive programming around missing external names and SA names, which should in practice always be set when resourcePath is called. The largest change is to the tests, where we remove a layer of abstraction to bring them a little closer to typical Crossplane tests and remove a few utils that were added to compare errors and conditions. Such utilities already exist in the crossplane-runtime/pkg/test package. Signed-off-by: Nic Cope <[email protected]> (cherry picked from commit 0ab0e81)
It appears that deleting a service account makes the service account key Get call return HTTP 403 Forbidden for some period of time. In this situation the service account (and thus all keys) are in practice gone. Unfortunately we can't really distinguish between the key being gone and an actual permission error, but presumably if we treat a 403 during Observe as the key not existing we'd get another 403 when we tried to create it (in cases where the ServiceAccountKey managed resource is not being deleted). ``` Events: Type Reason Age From Message ---- ------ ---- ---- ------- Normal CreatedExternalResource 62s managed/serviceaccountkey.iam.gcp.crossplane.io Successfully requested creation of external resource Warning CannotObserveExternalResource 1s (x2 over 1s) managed/serviceaccountkey.iam.gcp.crossplane.io cannot get GCP ServiceAccountKey object via IAM API: googleapi: Error 403: Permission iam.serviceAccountKeys.get is required to perform this operation on service account key projects/crossplane-playground/serviceAccounts/[email protected]/keys/747816818ee73171aa92f9c4071170a1f993696a., forbidden ``` You can see the same workaround in the Terraform provider at https://github.com/hashicorp/terraform-provider-google/blob/a5e55351/google/resource_google_service_account_key.go#L164 Signed-off-by: Nic Cope <[email protected]> (cherry picked from commit bfb4c9c)
Currently reference resolution results in the following error:
```
cannot resolve references: spec.forProvider.serviceAccount: could not resolve ServiceAccount reference: {<nil> <nil> 0xc003906e90}: referenced field was empty (referenced resource may not yet be ready)
```
Signed-off-by: Nic Cope <[email protected]>
(cherry picked from commit dd9f401)
Closes crossplane-contrib#352 Signed-off-by: Hasan Turken <[email protected]> (cherry picked from commit 56aee82)
Fixes crossplane-contrib#371 Signed-off-by: Hasan Turken <[email protected]> (cherry picked from commit 67baa55)
Signed-off-by: Hasan Turken <[email protected]> (cherry picked from commit 673aa23)
Signed-off-by: Hasan Turken <[email protected]> (cherry picked from commit 947bcac)
Signed-off-by: Hasan Turken <[email protected]> (cherry picked from commit 18fa1f4)
Signed-off-by: Hasan Turken <[email protected]> (cherry picked from commit a8a3a0f)
Signed-off-by: Hasan Turken <[email protected]> (cherry picked from commit fe5130e)
Signed-off-by: Hasan Turken <[email protected]> (cherry picked from commit 32a246f)
Signed-off-by: Hasan Turken <[email protected]> (cherry picked from commit 3bf9f72)
jbw976
approved these changes
Sep 20, 2021
There was a problem hiding this comment.
LGTM - this looks to have the same set of commits from my rebase attempt that we closed in favor of doing these cherry picks (https://github.com/crossplane/provider-gcp/pull/376/commits). I scanned to compare the commits between that PR and this one, and the only ones missing are the merge commits, which are not needed when doing these cherry picks. Thanks @turkenh!
ulucinar
approved these changes
Sep 20, 2021
|
@turkenh, maybe "rebase and merge" could just fast-forward? |
Yes @ulucinar, I think that could work in Jared's PR to bring both branches into the same state. But given we cherry-picked commits in this PR, I believe it doesn't matter too much (already had different commit ids). Preferred this way since this is consistent with the process that we are using to take changes to release branches. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of your changes
This PR cherry picks all changes to release-0.18 branch in latest master:
ServiceAccountKeydoes not exist #373I have:
make reviewable testto ensure this PR is ready for review.How has this code been tested
N/A