-
Notifications
You must be signed in to change notification settings - Fork 113
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
feat(kms): add supported providers func #277
Conversation
FYI: This might have some overlap with #276 -- I'm fine if this gets merged first, in either case one of us will have to rebase. |
require.Contains(t, supportedProviders, aws.ReferenceScheme) | ||
require.Contains(t, supportedProviders, azure.ReferenceScheme) | ||
require.Contains(t, supportedProviders, gcp.ReferenceScheme) | ||
require.Contains(t, supportedProviders, hashivault.ReferenceScheme) |
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.
Actually, after #276 this could be an interesting test that KMS impls aren't provided unless imported.
We could remove one of these imports and check that !require.Contains(..., "gcpkms")
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.
Oh, good point! I think it would be better to wait your PR to get merged first, and then I can rewrite this test.
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.
This should be ready to rewrite!
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.
Done for happy path! - but... couldn't find a proper way to test no imports case.
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.
I'd try something like this:
import (
"github.com/sigstore/sigstore/pkg/signature/kms/aws"
"github.com/sigstore/sigstore/pkg/signature/kms/azure"
// NOT import gcp
"github.com/sigstore/sigstore/pkg/signature/kms/hashivault"
)
...
require.Contains(t, supportedProviders, aws.ReferenceScheme)
require.Contains(t, supportedProviders, azure.ReferenceScheme)
require.NotContains(t, supportedProviders, "gcpkms://") // avoids gcp import, which would register it
require.Contains(t, supportedProviders, hashivault.ReferenceScheme)
It's possible the test somehow already depends on gcp
so this doesn't work, but I think it's worth a shot at least. If it's too difficult to figure out I think just testing the happy path would be fine.
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.
Ah, get it! But I think we should cover all the scheme references by importing here. Otherwise it could be false-positive pass for this unit test if we change the scheme references i.e., hashivault to vault, or if we move the package path.
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.
If you want to be exhaustive you could probably come up with some solution using build tags or requiring manual registration (not in init()
), I'm just not sure it's worth the maintenance burden.
These url schemes shouldn't change, ever, and if we move a package the test will fail anyway until someone fixes it. I think it's a good compromise to cover the general idea of selective providers. (If it works at all, TBD)
8c56f6f
to
27840b4
Compare
Signed-off-by: Furkan <furkan.turkal@trendyol.com>
27840b4
to
c7cdd22
Compare
Awesome it worked! 🎉 (just kidding, I definitely knew it would work 😅 ) |
Signed-off-by: Dan Lorenc <dlorenc@google.com>
Signed-off-by: Furkan furkan.turkal@trendyol.com
Cross ref: https://github.com/sigstore/cosign/pull/1475/files
To provide better UX for the consumer clients to check whether given provider exist or not.
Summary
Ticket Link
Fixes
Release Note