-
Notifications
You must be signed in to change notification settings - Fork 544
Add Akeyless Secrets Store Component #4036
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
base: main
Are you sure you want to change the base?
Conversation
a56fc84 to
b2a72bf
Compare
sicoyle
left a comment
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.
Thank you for iterating with me on this! Here's another batch of feedback for ya - I still have a bit more to review on this, but this is the main I think so far :) 🙌
tuvia-akeyless
left a comment
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.
LGTM. Please verify (manually/tests) it is working for various types of dynamic and rotated secrets.
This comment was marked as outdated.
This comment was marked as outdated.
bc57bc0 to
2718c8f
Compare
|
@sicoyle - can you please review this again? There were a bunch of merge conflicts and it happens every I update the branch. |
850536c to
4226c9f
Compare
Hi! Yes, thank you for your patience 🙏 I've been OOO for the past two weeks on vacation. I'm back now :) |
|
can you please rebase onto main? There are over 10k lines changed now in this PR with a ton of unrelated changes... |
79d16fc to
e7bc2f8
Compare
Signed-off-by: Kobbi Gal <[email protected]>
Signed-off-by: Kobbi Gal <[email protected]>
Signed-off-by: Kobbi Gal <[email protected]>
Signed-off-by: Kobbi Gal <[email protected]>
Signed-off-by: Kobbi Gal <[email protected]>
Signed-off-by: Kobbi Gal <[email protected]>
Signed-off-by: Kobbi Gal <[email protected]>
Signed-off-by: Kobbi Gal <[email protected]>
Signed-off-by: Kobbi Gal <[email protected]>
Signed-off-by: Kobbi Gal <[email protected]>
e7bc2f8 to
bb84293
Compare
@sicoyle should be all cleaned up now. FYI, since your last review I added a few things:
|
Signed-off-by: kgal-akl <[email protected]>
Signed-off-by: Kobbi Gal <[email protected]>
sicoyle
left a comment
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.
few comments so far with the additions. I will circle back again either later today or tomorrow for updates - thank you! :) Also as long as we get this merged sometime this month then this should be in the clear imo to make the next official dapr 1.17 release set for January🎉
secretstores/akeyless/akeyless.go
Outdated
| // akeylessMetadata contains the metadata for the Akeyless secret store. | ||
| type akeylessMetadata struct { | ||
| GatewayURL string `json:"gatewayUrl" mapstructure:"gatewayUrl"` | ||
| GatewayTLSCA string `json:"gatewayTLSCA" mapstructure:"gatewayTLSCA"` |
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.
GatewayTLSCA -> GatewayTlsCa please
Also, can any of these become unexported fields instead of exported?
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.
GatewayTLSCA -> GatewayTlsCa please
Rename resolved in commit 91b9871.
can any of these become unexported fields instead of exported?
As far as I can tell, metadata fields need to be exported. The fields must remain exported. Inmetadata/utils.go(lines 219-221):
// fields that are not exported cannot be set via the mapstructure metadata decoding mechanism
if !currentField.IsExported() {
continue
}This means:
kitmd.DecodeMetadata(used inparseMetadata) usesmapstructure, which requires exported fields for reflection.GetComponentMetadatausesGetMetadataInfoFromStructType, which skips unexported fields.
If we lowercase these fields:
parseMetadatawould fail to decode metadata from the configuration.GetComponentMetadatawould skip them, breaking metadata documentation generation.
So I believe it's better to leave the fields as is.
However, I did find that some of the secret store receiver functions can be unexported since they're only used within the akeyless package (in test and other functions). I unexported them in commit ff08e2a.
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.
Looks like the linter is actually asking me to rename GatewayTlsCa to GatewayTLSCa:
secretstores/akeyless/utils.go:281:22 stylecheck ST1003: func parameter gatewayTlsCa should be gatewayTLSCa
Should I revert the change or just add annotate/ignore it?
secretstores/akeyless/akeyless.go
Outdated
| if reauthErr := a.ensureValidToken(ctx); reauthErr != nil { | ||
| return fmt.Errorf("failed to re-authenticate after 401: %w", reauthErr) | ||
| } |
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.
honestly this seems to me like it belongs elsewhere and we should have a dynamic client. Can you reference
| func (x *X509) RefreshX509(ctx context.Context) error { |
see here using "latest" clients creds https://github.com/dapr/components-contrib/blob/main/common/component/kafka/clients.go#L14
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 refactored this method, see #4036.
Are you suggesting that I create a new client in common/akeyless/client.go and move this logic there? Are we expecting other components to utilize Akeyless clients and this is why we want to move this logic to common?
Signed-off-by: Kobbi Gal <[email protected]>
Signed-off-by: Kobbi Gal <[email protected]>
Signed-off-by: Kobbi Gal <[email protected]>
Signed-off-by: Kobbi Gal <[email protected]>
Signed-off-by: Kobbi Gal <[email protected]>
Signed-off-by: Kobbi Gal <[email protected]>
Signed-off-by: Kobbi Gal <[email protected]>
Signed-off-by: Kobbi Gal <[email protected]>
Hahah yeah she's my twin actually! So after this PR is merged then in your PR in dapr/dapr you have to bump the components-contrib go mod reference and push that as well so dapr/dapr knows about your changes here. Essentially it will be a: |
Description
Added a new Secret Store component for Akeyless.
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list:
Issue reference
#4063
Requirements