Skip to content
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

fix(endpoints): do not reset organization's secrets when an endpoint is created or updated #19082

Merged
merged 3 commits into from
Jul 31, 2020

Conversation

sranka
Copy link
Contributor

@sranka sranka commented Jul 26, 2020

Closes #19081

This PR fixes the endpoint service so that it does not reset all organization secrets when a notification endpoint is created or updated. SecretService.PutSecrets call, which that "deletes" all existing secrets, is replaced by PatchSecrets herein. A new test was added.

@sranka sranka changed the title fix(endpoints): do not override all secrets when an endpoint is creat… fix(endpoints): do not reset organization's secrets when an endpoint is created or updated Jul 26, 2020
@sranka
Copy link
Contributor Author

sranka commented Jul 26, 2020

There is a good test that explains the semantics of PutSecrets. It would be IMHO even better to rename the SecretService's PutSecrets method to ResetSecrets to clearly indicate that all existing secrets are deleted and the new are set. I can offer this refactoring upon reviewer's request, it might be opinionated.

I guess that every code that now uses PutSecrets is suspicious to do the same mistake. I found also

influxdb/pkger/service.go

Lines 2424 to 2426 in 88cdf43

createFn := func(ctx context.Context, i int, orgID, userID influxdb.ID) *applyErrBody {
err := s.secretSVC.PutSecrets(ctx, orgID, secrets)
if err != nil {

It should probably use PatchSecrets, @jsteenb2 ?

@sranka sranka marked this pull request as ready for review July 26, 2020 07:47
Copy link
Contributor

@glinton glinton left a comment

Choose a reason for hiding this comment

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

Can you add tests to show the old/new behavior

@sranka
Copy link
Contributor Author

sranka commented Jul 29, 2020

@glinton .. a test was added herein to validate the correct behavior

@sranka sranka requested review from glinton and jsteenb2 July 29, 2020 11:24
@sranka sranka force-pushed the 19081/lost_secrets branch from 7cd3695 to dce1d0a Compare July 29, 2020 14:06
@sranka sranka force-pushed the 19081/lost_secrets branch from 0c14344 to 1939eec Compare July 30, 2020 00:38
@sranka sranka merged commit d77ccf9 into master Jul 31, 2020
@sranka sranka deleted the 19081/lost_secrets branch July 31, 2020 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update or creation of a notification endpoint deletes the secrets of all existing endpoints
2 participants