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

add wildcard support to ResourceSelectors #3823

Closed

Conversation

Laevos
Copy link
Contributor

@Laevos Laevos commented Jul 20, 2023

What type of PR is this?

/kind feature

What this PR does / why we need it:

Currently a ClusterPropagationPolicy is able to propagate both namespaced and cluster-wide resources to member clusters. However, there is not currently a way to say "I would like for all resources, both namespaced and cluster-wide, to be propagated according to this ClusterPropagationPolicy, regardless of their APIVersion or Kind, and in all Namespaces (except for the system-reserved namespaces)."

With this change, a wildcard feature is added to allow a ClusterPropagationPolicy to do just that. By Providing a "*" for any combination of the APIVersion, Kind and Namespace, a ClusterPropagationPolicy can be made to match on a wider array of

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Adds a wildcard parsing feature to the ResourceSelector, allowing more flexible matching behavior.

@karmada-bot karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 20, 2023
@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign rainbowmango after the PR has been reviewed.
You can assign the PR to them by writing /assign @rainbowmango in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@karmada-bot karmada-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 20, 2023
Signed-off-by: Lilith McMullen <lilith.mcmullen@zendesk.com>
@Laevos Laevos force-pushed the laevos/allow-wildcard-namespace branch from f6b0521 to 2b2426c Compare July 20, 2023 21:00
@codecov-commenter
Copy link

Codecov Report

Merging #3823 (2b2426c) into master (48e9a5b) will not change coverage.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##           master    #3823   +/-   ##
=======================================
  Coverage   55.64%   55.64%           
=======================================
  Files         226      226           
  Lines       21378    21378           
=======================================
  Hits        11896    11896           
  Misses       8848     8848           
  Partials      634      634           
Flag Coverage Δ
unittests 55.64% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/util/selector.go 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@XiShanYongYe-Chang
Copy link
Member

Hi @Laevos, thanks for your feedback. If we use * for exact matching, we may not be able to select which resources will not be propagated, which could pose security issues as described in the comments:

// ResourceSelectors used to select resources.
// Nil or empty selector is not allowed and doesn't mean match all kinds
// of resources for security concerns that sensitive resources(like Secret)
// might be accidentally propagated.
// +required
// +kubebuilder:validation:MinItems=1
ResourceSelectors []ResourceSelector `json:"resourceSelectors"`

How do you think?

@MingZhang-YBPS
Copy link
Contributor

Currently, ResourceSelectors supports selecting one instance or all instances of one kind. That seems good enough

@RainbowMango
Copy link
Member

There is a similar request that is for namespace part. #3656
I'm looking at them now.

@RainbowMango
Copy link
Member

However, there is not currently a way to say "I would like for all resources, both namespaced and cluster-wide, to be propagated according to this ClusterPropagationPolicy, regardless of their APIVersion or Kind, and in all Namespaces (except for the system-reserved namespaces)."

I'd like to hear your use case in more detail. We need to figure out the security risk before moving forward.
Currently, Karmada's API server is essentially a kube-apiserver which will create some resources like Secrets that might not allowed to propagate.

@Laevos
Copy link
Contributor Author

Laevos commented Jul 24, 2023

However, there is not currently a way to say "I would like for all resources, both namespaced and cluster-wide, to be propagated according to this ClusterPropagationPolicy, regardless of their APIVersion or Kind, and in all Namespaces (except for the system-reserved namespaces)."

I'd like to hear your use case in more detail. We need to figure out the security risk before moving forward. Currently, Karmada's API server is essentially a kube-apiserver which will create some resources like Secrets that might not allowed to propagate.

Sure thing @RainbowMango: Our use case is that we are a platform team who provides managed Kubernetes clusters to our internal customers. However, one business requirement we have is that we cannot request our end uses to change the way they're currently writing manifests in any significant way. Namely, we can't ask that teams add PropagationPolicies to their clusters, as those should be managed by us. At the same time, we would like to avoid having to manage every individual PropagationPolicy and ClusterPropagationPolicy by hand.

With our current architecture, we have only one member cluster, to which we want to propagate all manifests deployed against the Karmada cluster. The thought here was that by having a wildcard ClusterPropagationPolicy, we could do that as simply as possible. My understanding was that the PropagationPolicy controller already stopped some namespaces from propagating (namely, Karmada's system-reserved namespaces), is that incorrect?

An alternative solution is to create a ResourceSelector for each individual resource that needs to be propagated (Deployment, Service, etc.), but we'd prefer to avoid having to be aware of and manage a ResourceSelector for every CRD that our customers use.

@XiShanYongYe-Chang
Copy link
Member

Is it possible to consider using multiple PropagationPolicies, each selecting a specific namespace of a certain Kind? This way, compared to using a single ClusterPropagationPolicy, the control would be more precise. Additionally, the number of PropagationPolicies that administrators need to create is fixed and at a constant level.

@Laevos
Copy link
Contributor Author

Laevos commented Jul 31, 2023

@XiShanYongYe-Chang This is looking more like what we'll have to do, or possibly implement a controller in the future; the idea was to try and scope to as few PPs/CPPs as possible to avoid toilsome work of manually updating when a new Kind/CRD is introduced, but I appreciate the direction; it looks like this can be closed if the consensus is that this would be an anti-feature. Thanks for your time!

@Laevos Laevos closed this Jul 31, 2023
@a7i a7i deleted the laevos/allow-wildcard-namespace branch January 29, 2024 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants