Skip to content

Conversation

@hectorj2f
Copy link
Collaborator

Summary

closes #523

Release Note

Improve kms key validations and error messages for awskms

Documentation

@hectorj2f hectorj2f added the enhancement New feature or request label Jan 26, 2023
@hectorj2f hectorj2f requested review from mattmoor and vaikas January 26, 2023 12:11
@hectorj2f hectorj2f self-assigned this Jan 26, 2023
mattmoor
mattmoor previously approved these changes Jan 26, 2023
Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

Does this address the confusing error when someone uses awskms://{arn} instead of awskms:///{arn}?

Otherwise this looks incredibly detailed, so thanks for the better validation!

Signed-off-by: Hector Fernandez <hector@chainguard.dev>
Signed-off-by: Hector Fernandez <hector@chainguard.dev>
Signed-off-by: Hector Fernandez <hector@chainguard.dev>
Signed-off-by: Hector Fernandez <hector@chainguard.dev>
Signed-off-by: Hector Fernandez <hector@chainguard.dev>
Signed-off-by: Hector Fernandez <hector@chainguard.dev>
@hectorj2f
Copy link
Collaborator Author

Does this address the confusing error when someone uses awskms://{arn} instead of awskms:///{arn}?

I added a specific use case for awskms://{arn} which is invalid but I agree we needed a better error message for this particular scenario.

@hectorj2f hectorj2f requested a review from mattmoor January 26, 2023 18:33
@hectorj2f
Copy link
Collaborator Author

I added the last change here a4d61eb to ease the review.

@codecov-commenter
Copy link

Codecov Report

Merging #524 (a4d61eb) into main (33ed498) will decrease coverage by 0.92%.
The diff coverage is 3.92%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #524      +/-   ##
==========================================
- Coverage   56.21%   55.30%   -0.92%     
==========================================
  Files          42       42              
  Lines        4454     4459       +5     
==========================================
- Hits         2504     2466      -38     
- Misses       1747     1792      +45     
+ Partials      203      201       -2     
Impacted Files Coverage Δ
pkg/apis/policy/common/validation.go 17.46% <0.00%> (-61.12%) ⬇️
...s/policy/v1alpha1/clusterimagepolicy_validation.go 92.85% <100.00%> (+0.54%) ⬆️
...is/policy/v1beta1/clusterimagepolicy_validation.go 94.95% <100.00%> (+0.72%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@hectorj2f hectorj2f merged commit a9e677f into sigstore:main Jan 26, 2023
@hectorj2f hectorj2f deleted the improve_kms_aws_err_msgs branch January 26, 2023 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

kms doesn't reject ARNs without awskms://

4 participants