search

projectcalico / calico

Cloud native networking and network security
https://docs.tigera.io/calico/latest/about/
Apache License 2.0
5.97k stars 1.33k forks source link

[CRD backend] Syncing Calico node labels cannot be disabled #5594

Open invidian opened 2 years ago

invidian commented 2 years ago

Expected Behavior

When kubecontrollersconfiguration.spec.controllers.node.syncLabels is set to Disabled, Calico Node objects do not get updated with new labels.

Current Behavior

Regardless of the value of kubecontrollersconfiguration.spec.controllers.node.syncLabels, labels for nodes are always synced to Calico Node objects.

Steps to Reproduce (for bugs)

  1. Install Calico.
  2. kubectl patch --type merge kubecontrollersconfiguration default --patch='{"spec": {"controllers": {"node": {"syncLabels": "Disabled"}}}}'
  3. kubectl label node x foo=bar
  4. calicoctl get node x | grep 'foo: bar'

Context

Findings around #5593 and #5592.

Your Environment

caseydavenport commented 2 years ago

@invidian are you running with Calico in etcd mode or using Kubernetes CRDs?

invidian commented 2 years ago

Using Kubernetes CRDs I think:

  environmentVars:
    DATASTORE_TYPE: kubernetes
    ENABLED_CONTROLLERS: node
caseydavenport commented 2 years ago

Yep, so I think that is the confusion here, and not sure how to resolve it really.

When using that mode, the Kubernetes Node API == the Calico node API. In other words, there's no controller syncing the two resources, they are one in the same, and so there's no way to disable syncing of the labels.

For etcd mode, there are two data stores in play, which means we run a controller to keep them in sync. The proper fix is most likely to validate against setting syncLabels: Disabled for CRD mode, since functionally that's the only behavior supported.

invidian commented 2 years ago

Now that you asked and explained, that all make sense, thanks!

For etcd mode, there are two data stores in play, which means we run a controller to keep them in sync. The proper fix is most likely to validate against setting syncLabels: Disabled for CRD mode, since functionally that's the only behavior supported.

The setting option could also be renamed with an etcd prefix or something.

Or even documentation could mention that this option is only applicable for etcd data store. Or perhaps controller could generate a warning log on this settings?

As I suppose the validation of that would not be trivial since it's about 2 separate keys in the CR, so it would require a validating webhook, which is non trivial to deploy right now with k8s from my experience.

caseydavenport commented 2 years ago

The setting option could also be renamed with an etcd prefix or something.

This probably would have been the right choice from the start, although it's not really feasible to rename an existing field at this point.

Docs should definitely be updated.

As I suppose the validation of that would not be trivial since it's about 2 separate keys in the CR, so it would require a validating webhook, which is non trivial to deploy right now with k8s from my experience.

Strictly speaking, we have an API server in front of the resource so this sort of validation should be possible even without a webhook. It won't apply if you edit the crd.projectcalico.org/v1 API, but that's not really meant to be user-facing. We have ways to validate projectcalico.org/v3, though!