Skip to content
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

Jump to bottom

Fix of key vault import and late initialization #536

Merged
merged 5 commits into from
Sep 21, 2023
Merged

Fix of key vault import and late initialization #536

merged 5 commits into from
Sep 21, 2023

Conversation

ytsarev
Copy link
Collaborator

@ytsarev ytsarev commented Sep 8, 2023 •
edited by turkenf

Description of your changes

I have:

  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Tested both fresh Key creation and the import. Tested creation with and without rotation_policy. All green. The uptest will follow.

k get -f reproduce8.yaml
NAME                                               READY   SYNCED   EXTERNAL-NAME      AGE
vault.keyvault.azure.upbound.io/test-vault8-yury   True    True     test-vault8-yury   51m

NAME                                                      READY   SYNCED   EXTERNAL-NAME                                                AGE
key.keyvault.azure.upbound.io/test-vault81                True    True     test-vault81/5605c12ed02c4d6db2332e1eb21d0539                51m
key.keyvault.azure.upbound.io/test-vault82                True    True     test-vault82/bc6811e8da1b49faa7573f4d06f61962                51m
key.keyvault.azure.upbound.io/test-vault83                True    True     test-vault83/8d336bbb74754af2ad6eb244b327d2b4                51m
key.keyvault.azure.upbound.io/test-vault84                True    True     test-vault84/a381b641a10f4157849a8bfabf67faa5                51m
key.keyvault.azure.upbound.io/test-vault85                True    True     test-vault85/fee0373a300f4182b9d9539d56720619                51m
key.keyvault.azure.upbound.io/test-vault86                True    True     test-vault86/ed9afd15f0bb4e039a3c2af823e7df8a                51m

key.keyvault.azure.upbound.io/test-vault-8-no-rotation3   True    True     test-vault-8-no-rotation3/7987f8f6e4f040a78220f2cf678834fd   2m32s

NAME                                  READY   SYNCED   EXTERNAL-NAME   AGE
resourcegroup.azure.upbound.io/test   True    True     test            110m

NAME                                                      READY   SYNCED   EXTERNAL-NAME                          AGE
accesspolicy.keyvault.azure.upbound.io/test-vault8-sp     True    True     09809340-6f59-4d37-9cf3-030766d318a5   51m
accesspolicy.keyvault.azure.upbound.io/test-vault8-user   True    True     836042ec-9c85-479c-8f32-ecc8a06fda7c   51m

Uptest:

@ytsarev ytsarev mentioned this pull request Sep 8, 2023
Closed
@ytsarev
Copy link
Collaborator Author

ytsarev commented Sep 8, 2023

Import also works properly, see #479 (comment)

@ytsarev ytsarev changed the title Fix of key vault key first observe failure Fix of key vault key import Sep 11, 2023
@ytsarev ytsarev changed the title Fix of key vault key import Fix of key vault import Sep 11, 2023
@ytsarev @sergenyalcin
Fix of key vault key import
3409255 
* Fix key vault import
* Use special function to keep name + version as part of externalname
  so it will have a format of
`key-name/84faa4674826492e9b16095719740a00`
* Fixes crossplane-contrib#479

Co-authored-by: Sergen Yalçın <yalcinsergen97@gmail.com>
Signed-off-by: Yury Tsarev <yury@upbound.io>
@ytsarev ytsarev changed the title Fix of key vault import Fix of key vault import and late initialization Sep 15, 2023
@ytsarev ytsarev marked this pull request as ready for review September 15, 2023 10:22
@ytsarev
key_vault_key: Fix late initialization for rotation_policy
e005e9d 
Signed-off-by: Yury Tsarev <yury@upbound.io>
@ytsarev
Copy link
Collaborator Author

ytsarev commented Sep 15, 2023

/test-examples="examples/keyvault/keyvault-all-in-one.yaml"

@ytsarev
Copy link
Collaborator Author

ytsarev commented Sep 15, 2023 •
edited

@turkenf just to note it here, the uptest is failing with access issues

@turkenf
Copy link
Collaborator

turkenf commented Sep 15, 2023

/test-examples="examples/keyvault/secret.yaml"

@turkenf
Copy link
Collaborator

turkenf commented Sep 16, 2023

/test-examples="examples/keyvault/key.yaml"

@turkenf
Copy link
Collaborator

turkenf commented Sep 16, 2023

/test-examples="examples/keyvault/vault.yaml"

@turkenf
Copy link
Collaborator

turkenf commented Sep 16, 2023

/test-examples="examples/keyvault/key.yaml"

@turkenf turkenf force-pushed the vault-key-fix branch 2 times, most recently from f162c21 to 5055f8d Compare September 18, 2023 11:27
@turkenf
Add seperate examples for keyvault group for:
af65763 
- azurerm_key_vault
- azurerm_key_vault_key
- azurerm_key_vault_certificate
@ytsarev
Update certificate uptest example
775d03b 
* Update example with known working configuration
* Remove manual intervention

Signed-off-by: Yury Tsarev <yury@upbound.io>
@ytsarev
Copy link
Collaborator Author

ytsarev commented Sep 19, 2023

/test-examples="examples/keyvault/certificate.yaml"

@ytsarev
Copy link
Collaborator Author

ytsarev commented Sep 19, 2023 

 k get -f examples/keyvault/certificate.yaml
NAME                                                      READY   SYNCED   EXTERNAL-NAME                                        AGE
certificate.keyvault.azure.upbound.io/uptest-yury-test1   True    True     uptest-yury-test1/216ef8ca492b4db1b118dedbf8c3da45   6m25s

NAME                                                  READY   SYNCED   EXTERNAL-NAME                          AGE
accesspolicy.keyvault.azure.upbound.io/example-cert   True    True     09809340-6f59-4d37-9cf3-030766d318a5   6m25s

NAME                                                READY   SYNCED   EXTERNAL-NAME       AGE
vault.keyvault.azure.upbound.io/uptest-yury-test1   True    True     uptest-yury-test1   6m25s

NAME                                               READY   SYNCED   EXTERNAL-NAME       AGE
resourcegroup.azure.upbound.io/uptest-yury-test1   True    True     uptest-yury-test1   6m25s

Certificate is actually good. We need to adjust uptest datasource for proper AccessPolicy config in the pipeline. CC @turkenf @jeanduplessis

@turkenf
Copy link
Collaborator

turkenf commented Sep 20, 2023

/test-examples="examples/keyvault/certificate.yaml"

3 similar comments
@turkenf
Copy link
Collaborator

turkenf commented Sep 20, 2023

/test-examples="examples/keyvault/certificate.yaml"

@turkenf
Copy link
Collaborator

turkenf commented Sep 20, 2023

/test-examples="examples/keyvault/certificate.yaml"

@turkenf
Copy link
Collaborator

turkenf commented Sep 20, 2023

/test-examples="examples/keyvault/certificate.yaml"

@turkenf
Copy link
Collaborator

turkenf commented Sep 20, 2023

/test-examples="examples/keyvault/key.yaml"

@turkenf
Copy link
Collaborator

turkenf commented Sep 20, 2023

/test-examples="examples/keyvault/key.yaml"

@turkenf
Copy link
Collaborator

turkenf commented Sep 20, 2023

/test-examples="examples/keyvault/secret.yaml"

@turkenf
Copy link
Collaborator

turkenf commented Sep 20, 2023 •
edited

All uptest runs passed successfully and I added them to the description.

I also did an upgrade test and created the Key.keyvault resource on the main branch. As expected the resource was not ready:

NAME                                              READY   SYNCED   EXTERNAL-NAME       AGE
key.keyvault.azure.upbound.io/upgrade-55ft-test   False   True     upgrade-55ft-test   10m

Then I updated the provider according to this PR and updated crossplane.io/external-name according to the ID in the error received.

NAME                                                READY   SYNCED   EXTERNAL-NAME                                        AGE
key.keyvault.azure.upbound.io/upgrade-55ft-test     True    True     upgrade-55ft-test/16a0af0b959b4181b88803dbcc49807e   49m

We need to document this in the next release.

sergenyalcin
sergenyalcin approved these changes Sep 21, 2023
View reviewed changes
Copy link
Collaborator

@sergenyalcin sergenyalcin left a comment

Choose a reason for hiding this comment

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

Thanks @ytsarev LGTM!

turkenf
turkenf approved these changes Sep 21, 2023
View reviewed changes
Copy link
Collaborator

@turkenf turkenf left a comment

Choose a reason for hiding this comment

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

Thank you @ytsarev, LGTM.

@ytsarev ytsarev merged commit 980aa58 into crossplane-contrib:main Sep 21, 2023
9 checks passed
@ytsarev ytsarev deleted the vault-key-fix branch September 21, 2023 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sergenyalcin sergenyalcin sergenyalcin approved these changes

@turkenf turkenf turkenf approved these changes

@ulucinar ulucinar Awaiting requested review from ulucinar ulucinar is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error creating vault key in azure azure.keyvault: Certificate observe and deletion issues
3 participants
@ytsarev @turkenf @sergenyalcin