This repository has been archived by the owner on Jul 26, 2022. It is now read-only.
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.
Standardize CRD Spec #477
Standardize CRD Spec #477
Changes from 11 commits
9e31ff4
dd3ec01
1036d2d
dabcf4c
b593efa
13be99b
36ac681
ccebb36
d4c9deb
17633cf
13145cc
08f0a4b
e2522d7
310e8bd
2819108
03f82b1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@Flydiverny I like the idea of a universal injector in addition to the secret sync to allow injection via multiple sources to runtime env or file, without the need of using (potentially) 3+ separate upstream injectors with varying installation and usage
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.
Should we have a default value for refreshInterval? This can be a sensitive field, since it might make users reach their provider API limits.
If not, we need to mark this as required (and might give some advice on how to estimate it).
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.
Default reconcile loops happen between 10h (if we don't change anything to the generated stuff), right?
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.
A
message
field with the stringified error would be great for diagnostics.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.
Agree, any reason not to go with a condition array block here for readiness?
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.
Sure, conditions would fit in here nicely!
What about:
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 good, a few comments
lastHeartbeatTime
doesnt need to be used here, that is generally only for conditions which need explicit heart beatsfailureReason
,failureMessage
already fit within the condition, those would better fit with events in this case rather than explicitly in the base status.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.
Regarding
lastHeartbeatTime
: i think this is the case here. TheInSync
condition should be updated with every sync. We should consider renaming this tolastSyncTime
.failure*
as attributes are duplicates, true. Ill remove them.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.
Should
SecretStore
have a status? W/ secret stores implementing some sort of liveliness check so you can validate aSecretStore
is healthy before adding secrets?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 think this would be useful but slightly challenging. Some SecretStore types may not have permissions to do inspection, for instance if Vault role isn't granted lookup-self permissions then the SecretStore may report failed liveness but still have permission to fetch ExternalSecrets.
I think my recommendation would be to implement this with best effort and documenting the potential permissions store backends may need to support the liveness check.
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.
Agree, determining the
status
of an SecretStore is challenging. I think it would be great to haveEvents
on theSecretStore
CRD this way it is easier to observe if a single ES or multiple ES are having problems.A basic
is ready
/is running
could look like this: