The Wayback Machine - https://web.archive.org/web/20220421031725/https://github.com/loft-sh/vcluster/issues/386
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

Syncer option to persist labels on physical resources #386

Open
bschwenn opened this issue Feb 25, 2022 · 7 comments
Open

Syncer option to persist labels on physical resources #386

bschwenn opened this issue Feb 25, 2022 · 7 comments
Labels

Comments

@bschwenn
Copy link
Contributor

@bschwenn bschwenn commented Feb 25, 2022

Is your feature request related to a problem?

Mutating webhooks often use label selectors to determine which resources to process. Since vcluster mutates all the labels on synced resources, this can prevent webhooks in the physical cluster from being able to mutate the physical resources.

In particular we've encountered this issue for synced pods.

Which solution do you suggest?

Add a configuration option to persist the labels on resources to the syncer. The option could disable label mutation entirely for some resource types, or add both the vcluster labels and the original labels.

Which alternative solutions exist?

  • This could be implemented by consumers via a vcluster plugin. However, this seems onerous and would require a lot of wrapping of the standard syncers.
  • Consumers could update their webhooks to catch vcluster mutated labels. However, when using vcluster only as a development workflow this would entail updating production services to handle development cases, which is a bit of an antipattern.
  • Consumers can deploy the webhooks into the vcluster. This complicates the process of spinning up clusters and could lead to quite a sprawl.

Additional context

My apologies if there is already a way to achieve this (or a reason it's impossible), I didn't see it in my perusal of the docs and code if so.

@FabianKramm
Copy link
Member

@FabianKramm FabianKramm commented Feb 28, 2022

@bschwenn thanks for creating this issue! vcluster hashes labels to avoid conflicts between resources in the host cluster, the main problem would be that for example a vcluster workload could become a vcluster endpoint if it would just mirror the matching labels of the vcluster service.

We could think about an option, but it would be interesting for me to explore the use case for this a little bit more as a possible solution probably depends a lot on what resources you want to actually select. We could think for example about an option where vcluster would rewrite the labels like it does currently (as they are required for other resources such as services, networkpolices etc that selects resources by labels) as well as add the original labels or only parts of them to the resource as well.

I agree, currently a vcluster plugin would be not a good fit here and adjusting the webhooks is probably also not a good idea as each vcluster has different labels as it uses the vcluster name to create the label to avoid conflicts between multiple vclusters in the same namespace. So you would need to match all pods that are created by a vcluster and then do the filter logic within the admission controller itself.

So would be great if you could elaborate a little bit more on your specific use case here in terms of what resources need to be matched and what needs to be mutated there.

@bschwenn
Copy link
Contributor Author

@bschwenn bschwenn commented Feb 28, 2022

Thanks for the response @FabianKramm, makes sense.

To be more specific about our use case here, say we have a mutating webhook configured similarly to the following:

apiVersion: admissionregistration.k8s.io/v1
kind: MutatingWebhookConfiguration
metadata:
  name: pod-mutating-webhook
webhooks:
  - name: ...
    admissionReviewVersions:
      - "v1beta1"
    rules:
      - apiGroups:
          - "*"
        apiVersions:
          - "v1"
        operations:
          - "CREATE"
        resources:
          - pods
    ...
    objectSelector:
      matchExpressions:
      - key: example.com/enable-pod-mutating-webhook
        operator: In
        values:
        - "true"

where the example.com/enable-pod-mutating-webhook label is used to "opt-in" to having the webhook operate on a pod. (Our real world use case is operating on pods + pod labels in particular.) Since vcluster translates this label, the webhook will not operate on the mirrored pods. In our case, this prevents some configuration from being injected into the pod, which prevents it from properly initializing.

an option where vcluster would rewrite the labels like it does currently ... as well as add the original labels or only parts of them to the resource as well.

Yeah this would be great for our purposes. The presence of the translated labels doesn't cause any issue, rather the absence of one particular label on pods. So the smallest change to unblock this use case would be an option to sync specific untranslated labels onto the real cluster pods.

@FabianKramm
Copy link
Member

@FabianKramm FabianKramm commented Mar 1, 2022

@bschwenn thanks for the detailed explanation! Yes we could do a syncer flag like --sync-labels=*.my-label-1,... which would then just sync labels that match those regexes alongside the translated ones and which should work for your use case. So you could just do:

--sync-labels: my-domain.com/.+

and vcluster would sync the labels as well as still translate them

@FabianKramm FabianKramm added the good first issue label Mar 1, 2022
@bschwenn
Copy link
Contributor Author

@bschwenn bschwenn commented Mar 2, 2022

Thanks Fabian, that sounds great! I may be able to take a crack at contributing this if I find time soon

@FabianKramm
Copy link
Member

@FabianKramm FabianKramm commented Mar 2, 2022

@bschwenn sounds great, I'm happy to review a PR from you!

@bschwenn
Copy link
Contributor Author

@bschwenn bschwenn commented Mar 2, 2022

@FabianKramm gave it a shot in #391! I didn't use regex-based flags for the moment in the interest of simplicity.

@FabianKramm
Copy link
Member

@FabianKramm FabianKramm commented Mar 9, 2022

@bschwenn I guess this can be closed now correct?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 participants