fix: Decouple label propagation #1908

enxebre · 2025-01-09T23:47:26Z

Description
nodeClaim Labels are the source for core karpenter to sync Node Labels in a centralized fashion.
Some bootstrap userdata provider implementations also consume these labels and pass them through kubelet self setting.

That results in a coupling between a centralized and a kubelet self setting approach.
This coupling results in conflicting criteria for validations and degraded UX.
This PR removes the coupling.

As a consequence, Node Labels are not unncessarily restricted for the centralized sync anymore.
This better empowers administrators reducing the reliance on self setting kubelet and minimizing the risk of a Node steering privileged workloads to itself.

A subset of labels, excluding those disallowed by the node restriction admission, is now stored in json within the "karpenter.sh/node-restricted-labels" NodeClaim annotation. This enables bootstrap userdata providers to continue utilizing the labels if needed.

How was this change tested?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

k8s-ci-robot · 2025-01-09T23:47:33Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: enxebre
Once this PR has been reviewed and has the lgtm label, please assign maciekpytel for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

enxebre · 2025-01-09T23:48:25Z

If we are happy with the approach I'll sort out the tests.

enxebre · 2025-01-09T23:50:44Z

cc @elmiko @javanthropus

elmiko · 2025-01-10T00:21:09Z

thanks Alberto, i'll take a look tomorrow or monday

sftim

Do we want to restrict any of these, even as we loosen restrictions:

kubernetes.io/hostname
kubernetes.io/arch
kubernetes.io/os
node.kubernetes.io/instance-type
node.kubernetes.io/windows-build

?

pkg/controllers/nodeclaim/lifecycle/launch.go

enxebre · 2025-01-10T12:20:10Z

Do we want to restrict any of these, even as we loosen restrictions:
kubernetes.io/hostname
kubernetes.io/arch
kubernetes.io/os
node.kubernetes.io/instance-type
node.kubernetes.io/windows-build
?

That sounds reasonable. We can even make this a configurable --flag being restrictive as default if we want to. And with * allowing everything (note that in any case, now that we have decoupling, KubeletLabels will filter out everything not admitted). We are most likely moving in a similar direction in capi with the flag. Thoughts?

elmiko

thanks @enxebre , this is definitely going the direction i wanted to see. i think @sftim's comments about the restricted labels is good too, i think having it configurable is nice perhaps with a sane default that covers the labels Tim mentioned?

pkg/controllers/nodeclaim/lifecycle/launch.go

pkg/apis/v1/labels.go

enxebre · 2025-01-13T12:19:12Z

Thanks for feedback. I addressed comments and I'll look into sorting out the tests next.

coveralls · 2025-01-13T20:44:42Z

Pull Request Test Coverage Report for Build 12755426911

Details

48 of 49 (97.96%) changed or added relevant lines in 3 files are covered.
4 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.07%) to 81.267%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/controllers/nodeclaim/lifecycle/launch.go	6	7	85.71%

Files with Coverage Reduction	New Missed Lines	%
pkg/scheduling/requirements.go	2	98.1%
pkg/apis/v1/labels.go	2	95.92%

Totals
Change from base Build 12718181288:	0.07%
Covered Lines:	9119
Relevant Lines:	11221

💛 - Coveralls

enxebre · 2025-01-14T12:21:08Z

addressed comments and tests, PTAL @elmiko @javanthropus @sftim

elmiko · 2025-01-14T14:42:39Z

i think this is generally looking good, but i'm curious about @sftim 's comment #1908 (review)

i think it makes sense to consider preventing some of those labels, especially if there is overlap with what the ccm might manage.

enxebre · 2025-01-14T15:03:42Z

i think this is generally looking good, but i'm curious about @sftim 's comment #1908 (review)
i think it makes sense to consider preventing some of those labels, especially if there is overlap with what the ccm might manage.

This is now implemented in the PR now via a74f2fd#diff-ae13a9bbcddb80fed52372de837e250e0cec57957b6b58bc06c98130d0aed84fR123-R129

pkg/apis/v1/labels.go

sftim · 2025-01-14T15:34:54Z

pkg/apis/v1/labels.go

+	labelDomain := GetLabelDomain(key)
+	for restrictedLabelDomain := range RestrictedLabelDomains {
+		if labelDomain == restrictedLabelDomain || strings.HasSuffix(labelDomain, "."+restrictedLabelDomain) {
+			return fmt.Errorf("label %s is restricted; specify a well known label: %v, or a custom label that does not use a restricted domain: %v", key, sets.List(WellKnownLabels), sets.List(RestrictedLabelDomains))


Not sure that this is the right advice. Maybe just tell people that Karpenter doesn't manage these labels on Nodes because that's left to other components.

This func is actually validating requirements input and not allowing those within .karpenter.sh (except for the well known CapacityTypeLabelKey). That's afaict a Karpenter design choice.
FWIW It should never return err for the nodepool/nodeclaim apis since this is validated with cel as well. It would return error for a pod with a conflicting node selector req.

So I think the current error message is actually accurate?
IsValidToSyncCentrallyLabel is what dictates what gets propagated to Nodes.

the error message makes sense to me given the context, but i do wonder if we should disambiguate from the restricted label error that is produced in the following conditional. looking at the error message on line 117 makes me think we should alter the other message.

I updated the error in both places to clarify the conflicting label is not allowed because it might interfere with the internal provisioning logic. Let me know if that's still not clear.

pkg/apis/v1/labels.go

sftim · 2025-01-14T15:35:56Z

pkg/controllers/nodeclaim/lifecycle/launch.go

+	if err != nil {
+		panic(err)
+	}
+	kubeletLabelsAnnotation := map[string]string{apis.Group + "/node-restricted-labels": string(json)}


Maybe add a comment about why we set this.

Thanks! Added, please let me know if that's still not clear.

sftim · 2025-01-14T15:37:18Z

pkg/scheduling/requirements.go

+	return labels
+}
+
+// KubeletLabels filters out labels from the requirements that will be rejected by the node restriction admission.


Suggested change

// KubeletLabels filters out labels from the requirements that will be rejected by the node restriction admission.

// KubeletLabels identifies the labels that we expect the kubelet to manage

// Do not include labels here that the node restriction admission controller would reject.

?

Thanks! Added a variation of the above, please let me know if that's still not clear.

elmiko · 2025-01-14T16:00:48Z

This is now implemented in the PR now via a74f2fd#diff-ae13a9bbcddb80fed52372de837e250e0cec57957b6b58bc06c98130d0aed84fR123-R129

awesome, thanks!

elmiko

left a couple comments

elmiko · 2025-01-14T18:08:06Z

pkg/apis/v1/labels.go

+	labelDomain := GetLabelDomain(key)
+	for restrictedLabelDomain := range RestrictedLabelDomains {
+		if labelDomain == restrictedLabelDomain || strings.HasSuffix(labelDomain, "."+restrictedLabelDomain) {
+			return fmt.Errorf("label %s is restricted; specify a well known label: %v, or a custom label that does not use a restricted domain: %v", key, sets.List(WellKnownLabels), sets.List(RestrictedLabelDomains))


the error message makes sense to me given the context, but i do wonder if we should disambiguate from the restricted label error that is produced in the following conditional. looking at the error message on line 117 makes me think we should alter the other message.

pkg/apis/v1/labels.go

elmiko

i like the new error text. i had thought we might want to differentiate those 2 error conditions, but thinking about it i'm not sure what would be a meaningful distinction.

enxebre · 2025-01-17T07:48:29Z

@sftim @elmiko @javanthropus I think I addressed most comments. Please Let me know if there's more feedback

elmiko · 2025-01-17T21:40:59Z

looking good to me

nodeClaim Labels are the source for core karpenter to sync Node Labels in a centralized fashion. Some bootstrap userdata provider implementations also consume this labels and pass them through kubelet self setting. That results in a coupling between a centralized and a kubelet self setting approach. This coupling results in conflicting criteria for validations and degraded UX. This PR removes the coupling. As a consequence, Node Labels are not unncessarily restricted for the centralized sync anymore. This better empowers administrators reducing the reliance on self setting kubelet and minimizing the risk of a Node steering privileged workloads to itself. A subset of labels, excluding those disallowed by the node restriction admission, is now stored in json within the "karpenter.sh/node-restricted-labels" NodeClaim annotation. This enables bootstrap userdata providers to continue utilizing the labels if needed.

engedaam · 2025-01-21T18:26:18Z

/assign @elmiko

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 9, 2025

k8s-ci-robot requested review from engedaam and tallaxes January 9, 2025 23:47

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 9, 2025

enxebre mentioned this pull request Jan 9, 2025

allow karpenter to filter and create nodes with any labels #1046

Open

sftim reviewed Jan 10, 2025

View reviewed changes

pkg/controllers/nodeclaim/lifecycle/launch.go Outdated Show resolved Hide resolved

elmiko reviewed Jan 10, 2025

View reviewed changes

pkg/controllers/nodeclaim/lifecycle/launch.go Outdated Show resolved Hide resolved

javanthropus reviewed Jan 10, 2025

View reviewed changes

pkg/apis/v1/labels.go Outdated Show resolved Hide resolved

pkg/apis/v1/labels.go Outdated Show resolved Hide resolved

enxebre force-pushed the decouple-label-propagation branch from 597f443 to c91b821 Compare January 13, 2025 12:17

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 13, 2025

enxebre force-pushed the decouple-label-propagation branch 3 times, most recently from 9a2e195 to 8fef8c6 Compare January 13, 2025 20:22

enxebre force-pushed the decouple-label-propagation branch from 8fef8c6 to 860af35 Compare January 14, 2025 12:19

enxebre force-pushed the decouple-label-propagation branch from 860af35 to d2630f5 Compare January 14, 2025 13:40

javanthropus reviewed Jan 14, 2025

View reviewed changes

pkg/apis/v1/labels.go Outdated Show resolved Hide resolved

enxebre force-pushed the decouple-label-propagation branch from d2630f5 to 28a7364 Compare January 14, 2025 15:31

sftim reviewed Jan 14, 2025

View reviewed changes

pkg/apis/v1/labels.go Outdated Show resolved Hide resolved

sftim reviewed Jan 14, 2025

View reviewed changes

enxebre force-pushed the decouple-label-propagation branch from 28a7364 to b35abac Compare January 14, 2025 17:49

elmiko reviewed Jan 14, 2025

View reviewed changes

enxebre force-pushed the decouple-label-propagation branch 2 times, most recently from fe07959 to 2fa417c Compare January 15, 2025 08:26

elmiko reviewed Jan 15, 2025

View reviewed changes

enxebre added 2 commits January 21, 2025 10:41

Relax cel and fix tests to satisfy new labels constraints

1bf5f15

enxebre force-pushed the decouple-label-propagation branch from 2fa417c to 1bf5f15 Compare January 21, 2025 09:41

k8s-ci-robot assigned elmiko Jan 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Decouple label propagation #1908

fix: Decouple label propagation #1908

enxebre commented Jan 9, 2025 •

edited

Loading

k8s-ci-robot commented Jan 9, 2025

enxebre commented Jan 9, 2025

enxebre commented Jan 9, 2025

elmiko commented Jan 10, 2025

sftim left a comment

enxebre commented Jan 10, 2025 •

edited

Loading

elmiko left a comment

enxebre commented Jan 13, 2025

coveralls commented Jan 13, 2025

enxebre commented Jan 14, 2025

elmiko commented Jan 14, 2025

enxebre commented Jan 14, 2025

sftim Jan 14, 2025

enxebre Jan 14, 2025

elmiko Jan 14, 2025

enxebre Jan 15, 2025

sftim Jan 14, 2025

enxebre Jan 14, 2025

sftim Jan 14, 2025

enxebre Jan 14, 2025

elmiko commented Jan 14, 2025

elmiko left a comment

elmiko Jan 14, 2025

elmiko left a comment

enxebre commented Jan 17, 2025

elmiko commented Jan 17, 2025

engedaam commented Jan 21, 2025

	// KubeletLabels filters out labels from the requirements that will be rejected by the node restriction admission.
	// KubeletLabels identifies the labels that we expect the kubelet to manage
	// Do not include labels here that the node restriction admission controller would reject.

fix: Decouple label propagation #1908

Are you sure you want to change the base?

fix: Decouple label propagation #1908

Conversation

enxebre commented Jan 9, 2025 • edited Loading

k8s-ci-robot commented Jan 9, 2025

enxebre commented Jan 9, 2025

enxebre commented Jan 9, 2025

elmiko commented Jan 10, 2025

sftim left a comment

Choose a reason for hiding this comment

enxebre commented Jan 10, 2025 • edited Loading

elmiko left a comment

Choose a reason for hiding this comment

enxebre commented Jan 13, 2025

coveralls commented Jan 13, 2025

Pull Request Test Coverage Report for Build 12755426911

Details

💛 - Coveralls

enxebre commented Jan 14, 2025

elmiko commented Jan 14, 2025

enxebre commented Jan 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elmiko commented Jan 14, 2025

elmiko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elmiko left a comment

Choose a reason for hiding this comment

enxebre commented Jan 17, 2025

elmiko commented Jan 17, 2025

engedaam commented Jan 21, 2025

enxebre commented Jan 9, 2025 •

edited

Loading

enxebre commented Jan 10, 2025 •

edited

Loading