Secret create - add ignore option to allow noop#26467
Conversation
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
| // If ignore flag is set, check if secret already exists | ||
| if options.Ignore { | ||
| secret, err := manager.Lookup(name) | ||
| if err == nil { | ||
| // Secret exists, return its ID without error | ||
| return &entities.SecretCreateReport{ | ||
| ID: secret.ID, | ||
| }, nil | ||
| } | ||
| if !errors.Is(err, secrets.ErrNoSuchSecret) { | ||
| // Some other error occurred during lookup | ||
| return nil, err | ||
| } | ||
| // Secret doesn't exist, continue with creation | ||
| } |
There was a problem hiding this comment.
This is not "atomic", which means this check might say secret does not exist and then before we call manager.Store() another process might have created it causing the --ignore command to fail with secret already exists which would be totally unexpected.
Logically I think the right thing is to handle that after manager.Store() and inspect if the error is errSecretNameInUse and then return no error. Of course the issue is that errSecretNameInUse is not public so we would first have to export it in c/common
There was a problem hiding this comment.
Good catch, thanks.
QQ if we're changing c/common and the change is in the SecretsManager, should we just move the change there? replace is already handled there
There was a problem hiding this comment.
sure implementing it there sounds reasonable to me.
Signed-off-by: Ygal Blum <ygal.blum@gmail.com>
0d51999 to
bfc327a
Compare
|
@Luap99 I've updated the code to use the new flag from the secret manager in c/common |
Luap99
left a comment
There was a problem hiding this comment.
LGTM, comment is non blocking as I don't think we would have to backport such a feature ever.
| github.com/containernetworking/plugins v1.7.1 | ||
| github.com/containers/buildah v1.40.1-0.20250604193037-b8d8cc375f30 | ||
| github.com/containers/common v0.63.2-0.20250604184922-bb2062b6265c | ||
| github.com/containers/common v0.63.2-0.20250624163146-1bc9d1737003 |
There was a problem hiding this comment.
not blocking but in general (unless there is a API breakage that prevents the compile) it is best to to the vendor bump in a separate commit.
That ensure that the actual code change can be reviewed more easily but also most important if we have to backport this for whatever reason we would like to cherry-pick it and it can only work conflict free without vendor changes.
There was a problem hiding this comment.
Thanks for the info. I'll make sure to separate them in the future.
Just for me to understand, this change will not work without the vendor change (as it relies on new code in c/common). So, I don't understand your comment:
it can only work conflict free without vendor changes
There was a problem hiding this comment.
Correct it needs the vendor change, however it only needs that one specific new secret option. So in case of a RHEL backport or whatever one would backport your c/common change into an older c/common branch then tag a new release with it and then vendor this into an older podman branch. Then that person still has to backport the podman code change and because the vendor/go.mod stuff differs we get conflicts. With two commits we could most likely cleanly cherry-pick the podman code change which then works with the backported c/common
There was a problem hiding this comment.
I see. Thanks for the explanation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99, ygalblum The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@containers/podman-maintainers PTAL |
|
LGTM |
4942875
into
podman-container-tools:main
Does this PR introduce a user-facing change?
Yes