Skip to content

feat: add tags field to lease, to allow user set metadata #622

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.

Sign up for GitHub

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

Draft
bennyz wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

feat: add tags field to lease, to allow user set metadata #622

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
aa72235
feat: add tags field to lease protobuf and CRD types
bennyz Apr 19, 2026
430d296
feat: implement lease tag validation, storage, and filtering
bennyz Apr 19, 2026
d7e9a3c
feat: add configurable maxTags via operator LeasePolicy CR
bennyz Apr 19, 2026
2fafd2e
feat: add lease tags to Python CLI, client, and MCP
bennyz Apr 19, 2026
385bb2b
docs: add JEP-0012 for custom user tags on leases
bennyz Apr 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/python-tests.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ jobs:
echo "::error::No coverage.xml files found"
exit 1
fi
uv run diff-cover $coverage_files --compare-branch=origin/${{ github.base_ref }} --fail-under=80
uv run diff-cover $coverage_files --compare-branch=origin/${{ github.base_ref }} --fail-under=80 --exclude '*_pb2.py' '*_pb2_grpc.py'

# https://github.com/orgs/community/discussions/26822
pytest:
Expand Down
47 changes: 46 additions & 1 deletion controller/api/v1alpha1/lease_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package v1alpha1
import (
"context"
"fmt"
"strings"
"time"

cpb "github.com/jumpstarter-dev/jumpstarter-controller/internal/protocol/jumpstarter/client/v1"
Expand All @@ -16,6 +17,7 @@ import (
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/selection"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/validation"
"k8s.io/utils/ptr"
kclient "sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/log"
Expand Down Expand Up @@ -169,6 +171,26 @@ func ParseLabelSelector(selectorStr string) (*metav1.LabelSelector, error) {
}, nil
}

// ValidateLeaseTags validates user-defined lease tags against the given maxTags limit.
func ValidateLeaseTags(tags map[string]string, maxTags int) error {
if len(tags) > maxTags {
return fmt.Errorf("too many tags: %d (maximum %d)", len(tags), maxTags)
}
for k, v := range tags {
if strings.HasPrefix(k, LeaseTagMetadataPrefix) || strings.HasPrefix(k, "jumpstarter.dev/") {
return fmt.Errorf("tag key %q must not use reserved prefix", k)
}
prefixedKey := LeaseTagMetadataPrefix + k
if errs := validation.IsQualifiedName(prefixedKey); len(errs) > 0 {
return fmt.Errorf("tag key %q is not a valid label key: %s", k, strings.Join(errs, "; "))
}
if errs := validation.IsValidLabelValue(v); len(errs) > 0 {
return fmt.Errorf("tag value for key %q is not a valid label value: %s", k, strings.Join(errs, "; "))
}
}
return nil
}

func LeaseFromProtobuf(
req *cpb.Lease,
key types.NamespacedName,
Expand All @@ -195,16 +217,38 @@ func LeaseFromProtobuf(
return nil, err
}

// Build ObjectMeta labels: start with selector matchLabels (excluding reserved prefix), then add prefixed user tags
metaLabels := make(map[string]string)
for k, v := range selector.MatchLabels {
if strings.HasPrefix(k, LeaseTagMetadataPrefix) {
continue
}
metaLabels[k] = v
}
for k, v := range req.Tags {
metaLabels[LeaseTagMetadataPrefix+k] = v
}

// Store user tags in spec (without prefix)
var specTags map[string]string
if len(req.Tags) > 0 {
specTags = make(map[string]string, len(req.Tags))
for k, v := range req.Tags {
specTags[k] = v
}
}

return &Lease{
ObjectMeta: metav1.ObjectMeta{
Namespace: key.Namespace,
Name: key.Name,
Labels: selector.MatchLabels,
Labels: metaLabels,
},
Spec: LeaseSpec{
ClientRef: clientRef,
Duration: duration,
Selector: *selector,
Tags: specTags,
ExporterRef: func() *corev1.LocalObjectReference {
if req.ExporterName == nil || *req.ExporterName == "" {
return nil
Expand Down Expand Up @@ -238,6 +282,7 @@ func (l *Lease) ToProtobuf() *cpb.Lease {
Selector: metav1.FormatLabelSelector(&l.Spec.Selector),
Client: ptr.To(fmt.Sprintf("namespaces/%s/clients/%s", l.Namespace, l.Spec.ClientRef.Name)),
Conditions: conditions,
Tags: l.Spec.Tags,
}
if l.Spec.ExporterRef != nil {
lease.ExporterName = ptr.To(l.Spec.ExporterRef.Name)
Expand Down
180 changes: 180 additions & 0 deletions controller/api/v1alpha1/lease_helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ limitations under the License.
package v1alpha1

import (
"fmt"
"strings"
"testing"
"time"

Expand Down Expand Up @@ -331,5 +333,183 @@ var _ = Describe("LeaseFromProtobuf", func() {
Expect(lease.Spec.ExporterRef).NotTo(BeNil())
Expect(lease.Spec.ExporterRef.Name).To(Equal(exporterName))
})

It("should store tags in spec and prefix them in ObjectMeta labels", func() {
pbLease := &cpb.Lease{
Selector: "board=rpi4",
Duration: durationpb.New(time.Hour),
Tags: map[string]string{
"team": "devops",
"ci-job": "12345",
},
}
key := types.NamespacedName{Name: "test-lease", Namespace: "default"}
clientRef := corev1.LocalObjectReference{Name: "test-client"}

lease, err := LeaseFromProtobuf(pbLease, key, clientRef)

Expect(err).NotTo(HaveOccurred())
// Tags in spec (unprefixed)
Expect(lease.Spec.Tags).To(HaveKeyWithValue("team", "devops"))
Expect(lease.Spec.Tags).To(HaveKeyWithValue("ci-job", "12345"))
// Tags in ObjectMeta.Labels (prefixed)
Expect(lease.Labels).To(HaveKeyWithValue("metadata.jumpstarter.dev/team", "devops"))
Expect(lease.Labels).To(HaveKeyWithValue("metadata.jumpstarter.dev/ci-job", "12345"))
// Selector matchLabels still present
Expect(lease.Labels).To(HaveKeyWithValue("board", "rpi4"))
})

It("should handle lease with no tags", func() {
pbLease := &cpb.Lease{
Selector: "board=rpi4",
Duration: durationpb.New(time.Hour),
}
key := types.NamespacedName{Name: "test-lease", Namespace: "default"}
clientRef := corev1.LocalObjectReference{Name: "test-client"}

lease, err := LeaseFromProtobuf(pbLease, key, clientRef)

Expect(err).NotTo(HaveOccurred())
Expect(lease.Spec.Tags).To(BeNil())
// Only selector matchLabels in ObjectMeta
Expect(lease.Labels).To(HaveKeyWithValue("board", "rpi4"))
})
})
})

var _ = Describe("ValidateLeaseTags", func() {
It("should accept empty tags", func() {
Expect(ValidateLeaseTags(nil, 10)).To(Succeed())
Expect(ValidateLeaseTags(map[string]string{}, 10)).To(Succeed())
})

It("should accept valid tags", func() {
tags := map[string]string{
"team": "devops",
"ci-job": "12345",
"env": "staging",
}
Expect(ValidateLeaseTags(tags, 10)).To(Succeed())
})

It("should reject more than 10 tags", func() {
tags := make(map[string]string)
for i := 0; i < 11; i++ {
tags[fmt.Sprintf("key%d", i)] = "value"
}
err := ValidateLeaseTags(tags, 10)
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("too many tags"))
})

It("should reject tag key longer than 63 chars", func() {
tags := map[string]string{
strings.Repeat("a", 64): "value",
}
err := ValidateLeaseTags(tags, 10)
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("tag key"))
Expect(err.Error()).To(ContainSubstring("not a valid label key"))
})

It("should reject tag value longer than 63 chars", func() {
tags := map[string]string{
"key": strings.Repeat("v", 64),
}
err := ValidateLeaseTags(tags, 10)
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("tag value"))
Expect(err.Error()).To(ContainSubstring("not a valid label value"))
})

It("should reject invalid label key characters", func() {
tags := map[string]string{
"invalid key!": "value",
}
err := ValidateLeaseTags(tags, 10)
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("not a valid label key"))
})

It("should reject invalid label value characters", func() {
tags := map[string]string{
"key": "invalid value!",
}
err := ValidateLeaseTags(tags, 10)
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("not a valid label value"))
})

It("should reject reserved prefix jumpstarter.dev/", func() {
tags := map[string]string{
"jumpstarter.dev/custom": "value",
}
err := ValidateLeaseTags(tags, 10)
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("reserved prefix"))
})

It("should reject reserved prefix metadata.jumpstarter.dev/", func() {
tags := map[string]string{
"metadata.jumpstarter.dev/team": "value",
}
err := ValidateLeaseTags(tags, 10)
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("reserved prefix"))
})

It("should accept exactly 10 tags", func() {
tags := make(map[string]string)
for i := 0; i < 10; i++ {
tags[fmt.Sprintf("key%d", i)] = "value"
}
Expect(ValidateLeaseTags(tags, 10)).To(Succeed())
})

It("should accept key and value of exactly 63 chars", func() {
tags := map[string]string{
strings.Repeat("k", 63): strings.Repeat("v", 63),
}
Expect(ValidateLeaseTags(tags, 10)).To(Succeed())
})
})

var _ = Describe("Lease.ToProtobuf", func() {
It("should include tags in protobuf output", func() {
lease := &Lease{
ObjectMeta: metav1.ObjectMeta{
Name: "test-lease",
Namespace: "default",
},
Spec: LeaseSpec{
ClientRef: corev1.LocalObjectReference{Name: "test-client"},
Duration: &metav1.Duration{Duration: time.Hour},
Selector: metav1.LabelSelector{MatchLabels: map[string]string{"board": "rpi4"}},
Tags: map[string]string{"team": "devops", "ci-job": "12345"},
},
}

pb := lease.ToProtobuf()

Expect(pb.Tags).To(HaveKeyWithValue("team", "devops"))
Expect(pb.Tags).To(HaveKeyWithValue("ci-job", "12345"))
})

It("should handle nil tags", func() {
lease := &Lease{
ObjectMeta: metav1.ObjectMeta{
Name: "test-lease",
Namespace: "default",
},
Spec: LeaseSpec{
ClientRef: corev1.LocalObjectReference{Name: "test-client"},
Duration: &metav1.Duration{Duration: time.Hour},
Selector: metav1.LabelSelector{MatchLabels: map[string]string{"board": "rpi4"}},
},
}

pb := lease.ToProtobuf()

Expect(pb.Tags).To(BeEmpty())
})
})
9 changes: 7 additions & 2 deletions controller/api/v1alpha1/lease_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ type LeaseSpec struct {
Selector metav1.LabelSelector `json:"selector"`
// Optionally pin this lease to a specific exporter name.
ExporterRef *corev1.LocalObjectReference `json:"exporterRef,omitempty"`
// User-defined tags for the lease. Immutable after creation.
// Maximum 10 entries. Keys and values must conform to Kubernetes label rules.
// +kubebuilder:validation:MaxProperties=10
Tags map[string]string `json:"tags,omitempty"`
Comment on lines +38 to +41
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify whether generated CRDs/webhooks currently enforce tag immutability or tag key/value rules.
rg -n -C3 'Tags map\[string\]string|spec\.tags|x-kubernetes-validations|ValidateLeaseTags|ValidateCreate|ValidateUpdate|ValidatingWebhook' controller/api controller/deploy/operator/config/crd controller/deploy/helm

Repository: jumpstarter-dev/jumpstarter

Length of output: 10337

🏁 Script executed:

#!/bin/bash
# Search for ValidateCreate/ValidateUpdate methods on Lease type
rg -n -A10 'func \(l \*Lease\).*Validate(Create|Update)' controller/api/v1alpha1/

Repository: jumpstarter-dev/jumpstarter

Length of output: 53

🏁 Script executed:

#!/bin/bash
# Get the full tags field definition from the generated CRD
rg -n -A15 'name: tags' controller/deploy/operator/config/crd/bases/jumpstarter.dev_leases.yaml | head -30

Repository: jumpstarter-dev/jumpstarter

Length of output: 53

🏁 Script executed:

#!/bin/bash
# Search for ValidatingWebhook or MutatingWebhook configurations
find controller/deploy -name '*.yaml' -type f -exec grep -l 'ValidatingWebhook\|MutatingWebhook\|admissionWebhooks' {} \;

Repository: jumpstarter-dev/jumpstarter

Length of output: 131

🏁 Script executed:

#!/bin/bash
# Get the complete tags field section from generated CRD
grep -n -A20 '"name": "tags"' controller/deploy/operator/config/crd/bases/jumpstarter.dev_leases.yaml

Repository: jumpstarter-dev/jumpstarter

Length of output: 53

🏁 Script executed:

#!/bin/bash
# Check what webhook configs are in the kustomization
cat controller/deploy/operator/config/default/kustomization.yaml

Repository: jumpstarter-dev/jumpstarter

Length of output: 7658

🏁 Script executed:

#!/bin/bash
# Search for webhook marker annotations or webhook manifests
find controller/deploy/operator/config -name '*webhook*' -type f

Repository: jumpstarter-dev/jumpstarter

Length of output: 53

🏁 Script executed:

#!/bin/bash
# Search for tags in the CRD file without assuming specific format
cat controller/deploy/operator/config/crd/bases/jumpstarter.dev_leases.yaml | grep -n -i tags | head -20

Repository: jumpstarter-dev/jumpstarter

Length of output: 175

🏁 Script executed:

#!/bin/bash
# Find where ValidateLeaseTags is actually called
rg -n 'ValidateLeaseTags' controller/ --type go

Repository: jumpstarter-dev/jumpstarter

Length of output: 1704

🏁 Script executed:

#!/bin/bash
# Check the spec schema section of the Lease CRD to see what fields are validated
sed -n '/spec:/,/^[[:space:]]*[a-z]/p' controller/deploy/operator/config/crd/bases/jumpstarter.dev_leases.yaml | head -100

Repository: jumpstarter-dev/jumpstarter

Length of output: 187

🏁 Script executed:

#!/bin/bash
# Get the complete tags field definition from the CRD
sed -n '149,165p' controller/deploy/operator/config/crd/bases/jumpstarter.dev_leases.yaml

Repository: jumpstarter-dev/jumpstarter

Length of output: 926

Add CRD-level immutability and label-rule validation for the Tags field.

The Tags field is documented as immutable and label-rule constrained, but the generated CRD enforces only maxProperties: 10. ValidateLeaseTags validates all constraints comprehensively, but it's only called from the gRPC service layer. Direct Kubernetes writes via kubectl or controllers can mutate spec.tags or persist invalid keys/values, bypassing validation and potentially desynchronizing tags from the prefixed metadata labels used for filtering.

Suggested fix 
 	// User-defined tags for the lease. Immutable after creation.
 	// Maximum 10 entries. Keys and values must conform to Kubernetes label rules.
 	// +kubebuilder:validation:MaxProperties=10
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="lease tags are immutable"
 	Tags map[string]string `json:"tags,omitempty"`

Add CEL validation rules in the generated CRD to enforce immutability and label-rule conformance for keys and values, or implement ValidateCreate/ValidateUpdate webhook methods on the Lease type.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// User-defined tags for the lease. Immutable after creation.
// Maximum 10 entries. Keys and values must conform to Kubernetes label rules.
// +kubebuilder:validation:MaxProperties=10
Tags map[string]string `json:"tags,omitempty"`
// User-defined tags for the lease. Immutable after creation.
// Maximum 10 entries. Keys and values must conform to Kubernetes label rules.
// +kubebuilder:validation:MaxProperties=10
// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="lease tags are immutable"
Tags map[string]string `json:"tags,omitempty"`
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@controller/api/v1alpha1/lease_types.go` around lines 38 - 41, The CRD
currently only limits Tags with MaxProperties=10 but does not enforce
immutability or Kubernetes label rules; update the API so spec.tags cannot be
mutated and invalid keys/values are rejected by either adding CEL validation
rules to the generated CRD for the Tags field (immutability check comparing to
oldObject.spec.tags and regex checks for keys/values) or implement webhook-based
validation on the Lease type by adding ValidateCreate and ValidateUpdate methods
that call the existing ValidateLeaseTags helper and return errors when tags
violate immutability or label rules; refer to the Tags field, the Lease type,
and the existing ValidateLeaseTags function when making these changes.

// The release flag requests the controller to end the lease now
Release bool `json:"release,omitempty"`
// Requested start time. If omitted, lease starts when exporter is acquired.
Expand Down Expand Up @@ -70,8 +74,9 @@ const (
type LeaseLabel string

const (
LeaseLabelEnded LeaseLabel = "jumpstarter.dev/lease-ended"
LeaseLabelEndedValue string = "true"
LeaseLabelEnded LeaseLabel = "jumpstarter.dev/lease-ended"
LeaseLabelEndedValue string = "true"
LeaseTagMetadataPrefix string = "metadata.jumpstarter.dev/"
)

// +kubebuilder:object:root=true
Expand Down
17 changes: 12 additions & 5 deletions controller/api/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion controller/cmd/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ func main() {
os.Exit(1)
}

authenticator, prefix, router, option, provisioning, err := config.LoadConfiguration(
authenticator, prefix, router, option, provisioning, leasePolicy, err := config.LoadConfiguration(
context.Background(),
mgr.GetAPIReader(),
mgr.GetScheme(),
Expand Down Expand Up @@ -283,6 +283,7 @@ func main() {
}),
Router: router,
ServerOptions: option,
LeasePolicy: leasePolicy,
}).SetupWithManager(mgr); err != nil {
setupLog.Error(err, "unable to create service", "service", "Controller")
os.Exit(1)
Expand Down
2 changes: 1 addition & 1 deletion ...elm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_clients.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
annotations:
controller-gen.kubebuilder.io/version: v0.16.3
controller-gen.kubebuilder.io/version: v0.18.0
name: clients.jumpstarter.dev
spec:
group: jumpstarter.dev
Expand Down
2 changes: 1 addition & 1 deletion .../charts/jumpstarter-controller/templates/crds/jumpstarter.dev_exporteraccesspolicies.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
annotations:
controller-gen.kubebuilder.io/version: v0.16.3
controller-gen.kubebuilder.io/version: v0.18.0
name: exporteraccesspolicies.jumpstarter.dev
spec:
group: jumpstarter.dev
Expand Down
2 changes: 1 addition & 1 deletion ...m/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_exporters.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
annotations:
controller-gen.kubebuilder.io/version: v0.16.3
controller-gen.kubebuilder.io/version: v0.18.0
name: exporters.jumpstarter.dev
spec:
group: jumpstarter.dev
Expand Down
Loading
Loading