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

Validate config before saving changes after config reset #14203

Merged
merged 1 commit into from
Jan 28, 2022

Conversation

poornas
Copy link
Contributor

@poornas poornas commented Jan 27, 2022

Description

Motivation and Context

mc admin config reset alias notify_webhook should fail when actively registered buckets exist. Currently, this succeeds but leaves behind stale bucket notification metadata on the registered buckets. It would be cleaner to throw an error at the time of notify target reset so that mc event remove can be performed before removing the targets

How to test this PR?

mc admin config set sitea notify_webhook enable=on endpoint=http://localhost:8080
mc admin service restart sitea
mc event add sitea/bucket
mc admin config reset sitea notify_webhook    
### with this PR, `reset` should report mc: <ERROR> Unable to reset 'notify_webhook:_' on the server. Unable to disable configured targets '_:webhook'.
### with master, this would succeed - an attempt to re-add the target fails 

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Optimization (provides speedup with no functional changes)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • Fixes a regression (If yes, please add commit-id or PR # here)
  • Documentation updated
  • Unit tests added/updated

cmd/admin-handlers-config-kv.go Outdated Show resolved Hide resolved
@poornas poornas changed the title Avoid deleting notification target if configured targets exist Validate config before saving changes after config reset Jan 28, 2022
@minio-trusted
Copy link
Contributor

Mint Automation

Test Result
mint-large-bucket.sh ✔️
mint-fs.sh ✔️
mint-gateway-s3.sh ✔️
mint-erasure.sh ✔️
mint-dist-erasure.sh ✔️
mint-gateway-nas.sh ✔️
mint-compress-encrypt-dist-erasure.sh ✔️
mint-pools.sh ✔️
Deleting image on docker hub
Deleting image locally

@harshavardhana harshavardhana merged commit a4be47d into minio:master Jan 28, 2022
@harshavardhana harshavardhana deleted the webhook branch January 28, 2022 02:28
@pfriedland
Copy link

pfriedland commented Jan 30, 2022

something is not right, maybe with this fix.
I can't add any web hooks right now. I updated to the latest minio on a distributed configuration that has been working PERFECT. Until I tried to add a new webhook:

mc admin config set develop notify_webhook:abc enable=on endpoint=http://ercot-qenel8.develop.svc.cluster.local:5000/health

mc: Unable to set 'notify_webhook:abc enable=on endpoint=http://ercot-qenel8.develop.svc.cluster.local:5000/health' to server. Unable to disable configured targets 'minio-ercot-cop:webhook'.

I've tried to reset/remove minio-ercot-cop:webhook but it's not going away. It's not a valid webhook that is being complained about. minio-ercot-cop:webhook isn't a valid webhook, I don't think. maybe minio-ercot-cop:notify_webhook would be valid but not plain old webhook in the name. Am I missing something really obvious please?

@poornas
Copy link
Contributor Author

poornas commented Jan 30, 2022

@pfriedland , you probably have stale event notifications on buckets that need to be deleted first -

for i in $(mc --json ls alias/ | jq -r .key); do echo $i; mc event list alias/$i; done

Above script should help identify buckets using this old webhook minio-ercot-cop:notify_webhook. If you do `mc event remove / minio-ercot-cop:notify_webhook for such buckets, you should be able to add a new target - this PR is just ensuring that the config is valid at the time of saving and not deleting an actively referenced target

@harshavardhana
Copy link
Member

Above script should help identify buckets using this old webhook minio-ercot-cop:notify_webhook. If you do `mc event remove / minio-ercot-cop:notify_webhook for such buckets, you should be able to add a new target - this PR is just ensuring that the config is valid at the time of saving and not deleting an actively referenced target

@pfriedland please do not comment on pull requests make new issues - this is merged and released as per requirement.

mc: Unable to set 'notify_webhook:abc enable=on endpoint=http://ercot-qenel8.develop.svc.cluster.local:5000/health' to server. Unable to disable configured targets 'minio-ercot-cop:webhook'.

You should make sure to disable and enable new targets that replace existing targets should be unregistered from existing bucket configurations.

@pfriedland
Copy link

pfriedland commented Jan 30, 2022 via email

@harshavardhana
Copy link
Member

I had to —force remove all events on the bucket I suspected it to be attached. This odd event didn’t show up in any list. Things are back to normal, I think.

👍🏽 we added this PR to potentially avoid the issue you had, so you shouldn't have future problems in this manner because unset or deletion of notification targets should require prior removal on the bucket notification configuration first.

The PR ensures that an active setting is not allowed to be "reset" or "removed"

@bh4t bh4t added the bugfix label Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants