Add label option for labelling the node - #27
Merged
Merged
Conversation
Collaborator
Author
|
@SourceryAI review |
Reviewer's GuideAdds support for passing arbitrary Kubernetes node labels via the Sequence diagram for node add label propagationsequenceDiagram
actor User
participant AddCmd as newAddCmd
participant Parser as parseLabels
participant Runner as runAdd
participant Cluster as Cluster.Join
participant KubeClientFactory as Cluster.newKubeClient
participant KubeClient as kubeClient
User->>AddCmd: run `node add --label key=value`
AddCmd->>Parser: parseLabels(labelFlags)
Parser-->>AddCmd: labels map[string]string
AddCmd->>Runner: runAdd(ctx, nodeName, controlPlane, nodeImage, role, memory, maxMemory, labels, logger)
Runner->>Cluster: Join(ctx, JoinOptions{Labels: labels, ...})
Cluster->>Cluster: build labels map
alt needs kube client
Cluster->>KubeClientFactory: newKubeClient(ctx, cpSSHClient, containerName)
KubeClientFactory-->>Cluster: kubeClient
Cluster->>KubeClient: LabelNode(ctx, nodeName, labels)
KubeClient-->>Cluster: error or nil
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
Cluster.Join, user-provided labels currently override the built-innode-role.kubernetes.io/workerlabel on key collision; consider enforcing or at least documenting a clear precedence rule so callers cannot accidentally remove the worker role label. - The
runAddsignature change to accept alabelsmap requires callers to construct and pass this argument; if there are non-CLI callers, consider providing a helper or defaulting logic to reduce the impact on existing code paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Cluster.Join`, user-provided labels currently override the built-in `node-role.kubernetes.io/worker` label on key collision; consider enforcing or at least documenting a clear precedence rule so callers cannot accidentally remove the worker role label.
- The `runAdd` signature change to accept a `labels` map requires callers to construct and pass this argument; if there are non-CLI callers, consider providing a helper or defaulting logic to reduce the impact on existing code paths.
## Individual Comments
### Comment 1
<location path="internal/cli/node/add.go" line_range="56-59" />
<code_context>
}
-func runAdd(ctx context.Context, nodeName, controlPlane, nodeImage, role string, memory int, maxMemory int, logger *logrus.Logger) error {
+func parseLabels(labelFlags []string) (map[string]string, error) {
+ labels := make(map[string]string, len(labelFlags))
+ for _, l := range labelFlags {
+ k, v, ok := strings.Cut(l, "=")
+ if !ok || k == "" {
+ return nil, fmt.Errorf("invalid label %q: must be in key=value format", l)
</code_context>
<issue_to_address>
**suggestion:** Clarify behavior for whitespace and duplicate label keys in parseLabels
This will currently treat `--label 'env =prod'` as key `"env "` (with a trailing space) and will silently overwrite earlier values on duplicate keys (e.g. `--label env=prod --label env=staging`). Consider trimming whitespace on keys/values and either rejecting or clearly warning on duplicate keys so users don’t accidentally override labels.
</issue_to_address>
### Comment 2
<location path="internal/cluster/join.go" line_range="96-97" />
<code_context>
if !opts.IsControlPlane {
+ labels["node-role.kubernetes.io/worker"] = "worker"
+ }
+ for k, v := range opts.Labels {
+ labels[k] = v
+ }
+
</code_context>
<issue_to_address>
**question (bug_risk):** Decide whether user labels should be allowed to override system role labels
Because the worker role label is set before merging user labels, a user-supplied `--label node-role.kubernetes.io/worker=...` can override it. If this label is meant to be enforced as a system role, consider either applying user labels first and then setting this label, or explicitly preventing overrides for this key. If overrides are acceptable, this behavior should be clearly documented for users.
</issue_to_address>
### Comment 3
<location path="test/integration/multinode_test.go" line_range="100-103" />
<code_context>
Expect(ready).To(BeTrue(), "Node %s should be Ready", node.Name)
}
+ By("Verifying custom labels on node3")
+ n3, err := kubeClient.CoreV1().Nodes().Get(context.Background(), node3, metav1.GetOptions{})
+ Expect(err).ToNot(HaveOccurred())
+ Expect(n3.Labels).To(HaveKeyWithValue("env", "test2"), "node3 should have label env=test2")
+
By("Verifying DNS entries for all nodes")
</code_context>
<issue_to_address>
**suggestion (testing):** Add a negative/validation test for invalid label syntax at the CLI level
Since `--label` now enforces `key=value` syntax, please add a focused test (ideally unit/CLI-level rather than integration) that covers invalid inputs such as `--label=foo`, `--label==bar`, or an empty key, and verifies the command fails with a clear error. This will explicitly cover the parsing/validation logic and protect against regressions where invalid labels are accepted or mishandled.
Suggested implementation:
```golang
By("Verifying DNS entries for all nodes")
hostsFile = helpers.PodmanExec(helpers.DNSContainerName(clusterName), "cat /var/lib/dnsmasq/cluster-hosts")
Expect(hostsFile).To(ContainSubstring(node1), "cluster-hosts should contain node1")
By("Rejecting invalid label syntax at the CLI level")
for _, label := range []string{"foo", "==bar", "=baz"} {
cmd := exec.Command(
"kube-spawn",
"node",
"create",
"--cluster-name", clusterName,
"--role", "worker",
"--label", label,
)
output, err := cmd.CombinedOutput()
Expect(err).To(HaveOccurred(), "command with invalid label %q should fail", label)
Expect(string(output)).To(ContainSubstring("invalid label"), "error output for %q should explain invalid label syntax", label)
}
```
To fully implement the CLI-level validation test and keep it compiling and consistent with the existing test suite, you should also:
1. **Add imports** at the top of `test/integration/multinode_test.go`:
- Add `os/exec` to the import list:
```go
import (
// existing imports...
"os/exec"
)
```
2. **Adjust the CLI invocation if needed**:
- If `kube-spawn` is not on `PATH` in your test environment, replace `"kube-spawn"` with however you normally locate the binary in tests (for example a helper like `helpers.KubeSpawnBinary()` or using an environment variable).
- If your CLI subcommand for adding nodes differs (e.g. `node add` instead of `node create`), update the `exec.Command` arguments accordingly.
3. **Align the error assertion with the real message**:
- If the actual error text for invalid labels is something like `"invalid label syntax: KEY=VALUE expected"`, update the expectation:
```go
Expect(string(output)).To(ContainSubstring("invalid label syntax"))
```
or to whatever substring is stable and user-facing in your command’s validation error.
4. **(Optional, but closer to your comment) Move this into a more focused/CLI-level test file**:
- For a pure CLI/unit-level test (instead of integration-level), consider:
- Creating a new test file such as `test/cli/label_validation_test.go`.
- Using your existing CLI helpers (e.g. helpers that return a `gexec.Session`) so you can assert on exit codes more explicitly.
- If you do that, you can keep or remove the integration-level check above; the important part is that at least one focused test covers invalid `--label` inputs and asserts the command fails with a clear error.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Closes #23 Assisted-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Assisted-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Assisted-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add support for applying custom Kubernetes labels when adding a node and update node join behavior accordingly.
New Features:
Enhancements:
Tests: