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(kuma-cp) possible to delete resources on Zone CP #2665

Merged
merged 7 commits into from
Aug 31, 2021

Conversation

lobkovilya
Copy link
Contributor

@lobkovilya lobkovilya commented Aug 24, 2021

Summary

Webhook didn't check DELETE operations on Zone CP

Full changelog

  • fix webhook for DELETE
  • add unit tests
  • extend e2e test

Issues resolved

N/A

Documentation

N/A

Testing

  • Unit tests
  • E2E tests
  • Manual testing on Universal
  • Manual testing on Kubernetes

Backwards compatibility

  • Add backport-to-stable label if the code is backwards compatible. Otherwise, list breaking changes.

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
@lobkovilya lobkovilya requested a review from a team as a code owner August 24, 2021 17:55
@codecov-commenter
Copy link

codecov-commenter commented Aug 24, 2021

Codecov Report

Merging #2665 (d95f8fb) into master (3665e51) will decrease coverage by 0.00%.
The diff coverage is 57.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2665      +/-   ##
==========================================
- Coverage   51.97%   51.96%   -0.01%     
==========================================
  Files         871      871              
  Lines       49512    49521       +9     
==========================================
  Hits        25733    25733              
- Misses      21699    21711      +12     
+ Partials     2080     2077       -3     
Impacted Files Coverage Δ
test/e2e/deploy/kuma_deploy_global_zone.go 0.00% <0.00%> (ø)
pkg/plugins/runtime/k8s/webhooks/validation.go 87.80% <68.42%> (+0.40%) ⬆️
pkg/insights/components.go 70.00% <0.00%> (-30.00%) ⬇️
pkg/core/resources/store/customizable_store.go 77.77% <0.00%> (-11.12%) ⬇️
.../core/managers/apis/ratelimit/ratelimit_manager.go 35.48% <0.00%> (-9.68%) ⬇️
pkg/plugins/resources/memory/store.go 77.71% <0.00%> (-4.35%) ⬇️
app/kumactl/cmd/root.go 70.17% <0.00%> (-3.51%) ⬇️
pkg/insights/resyncer.go 66.16% <0.00%> (-1.51%) ⬇️
api/observability/v1/mads.pb.go 35.56% <0.00%> (+1.03%) ⬆️
pkg/xds/generator/direct_access_proxy_generator.go 83.90% <0.00%> (+1.14%) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3665e51...d95f8fb. Read the comment docs.

@parkanzky
Copy link
Contributor

Was it intentional to remove the "Skip validation on default mesh" logic?

Apart from that question, looks good to me.

@subnetmarco
Copy link
Contributor

Are we sure the update operations on zone CPs are also disabled?

Also, what is the error message that we return when an UPDATE/DELETE operation is performed on the zone CPs?

@lobkovilya
Copy link
Contributor Author

@parkanzky yes, it was intentional. Before service accounts, we were using kuma.io/synced annotation to allow creating resources only synced from Global CP. The default mesh was created by Zone CP itself, and that's why we had to make an exception for it. So we made this exception not to allow users to create default meshes from CLI, but to allow Zone CP to create them. Thanks to the service accounts approach we don't need this exception anymore.

@lobkovilya
Copy link
Contributor Author

@subnetmarco

  • yes, it UPDATE operations were covered by test even before this PR
  • UPDATE - You are trying to update a Dataplane on global CP. In multizone setup, it should be only updated on zone CP and synced to global CP.
  • DELETE - You are trying to delete a Dataplane on global CP. In multizone setup, it should be only deleted on zone CP and synced to global CP.

@subnetmarco
Copy link
Contributor

Let's update the message to: "Operation not allowed. {project-name} resources like {resource-name} can be updated or deleted only from the GLOBAL control plane and not from a ZONE control plane."

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
@lobkovilya lobkovilya merged commit b636a9a into master Aug 31, 2021
@lobkovilya lobkovilya deleted the fix/zone-cp-delete branch August 31, 2021 09:05
nikita15p pushed a commit to nikita15p/kuma that referenced this pull request Sep 28, 2021
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
nikita15p pushed a commit to nikita15p/kuma that referenced this pull request Sep 28, 2021
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
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.

None yet

5 participants