feat(machine-a-tron): k8s controller for mock BMC services#3955
feat(machine-a-tron): k8s controller for mock BMC services#3955akorobkov-nvda wants to merge 11 commits into
Conversation
WalkthroughThis PR adds a Kubernetes controller that polls machine-a-tron status, discovers mock BMC Services, and reconciles per-machine and DPU Services. It includes an HTTP client, Go tooling, a container image, Helm deployment resources, parent-chart integration, and CI image building. ChangesMachine-a-tron Kubernetes controller
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Controller
participant MATServices
participant MachineATron
participant KubernetesAPI
Controller->>MATServices: Discover labeled Services
MATServices-->>Controller: Service URLs and pod names
Controller->>MachineATron: GET /machines/status
MachineATron-->>Controller: Machine and BMC status
Controller->>KubernetesAPI: List managed Services
Controller->>KubernetesAPI: Create, update, recreate, or delete Services
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
akorobkov-nvda
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
dev/k8s/machine-a-tron-controller/pkg/controller/discovery.go (1)
57-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider making the cluster DNS domain configurable.
svc.cluster.localis hardcoded; clusters with a custom--cluster-domainwould break discovery URLs. As per path instructions fordev/**, development tooling should use "safe defaults" while remaining consistent with deployment manifests — exposing this as a constructor parameter (defaulting tocluster.local) would make the tool portable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dev/k8s/machine-a-tron-controller/pkg/controller/discovery.go` at line 57, Make the cluster DNS domain configurable in the discovery component by adding a constructor parameter with a safe default of "cluster.local", then use that value instead of the hardcoded suffix in the URL built by the discovery logic. Update the relevant constructor call sites while preserving existing behavior when no custom domain is provided.Source: Path instructions
dev/k8s/machine-a-tron-controller/pkg/controller/controller.go (2)
156-160: 🩺 Stability & Availability | 🔵 TrivialReserve the BMC IP range from dynamic Service allocation.
Setting
ClusterIPdirectly from the BMC's static IP works only if that address is guaranteed free. Unless the BMC IP subrange is excluded from Kubernetes' dynamic Service IP allocator (e.g. via--service-cluster-ip-rangecarve-out or a reserved sub-range), an unrelated auto-assigned Service could already hold that IP, causingCreate/Updatehere to fail with an allocation conflict.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dev/k8s/machine-a-tron-controller/pkg/controller/controller.go` around lines 156 - 160, Reserve the BMC IP range used by the ClusterIP assignment in the Kubernetes Service CIDR configuration, using a dedicated non-overlapping sub-range excluded from dynamic Service allocation. Update the deployment/configuration referenced by the controller’s Service creation path so addresses assigned from machine.BMC.IP cannot be auto-allocated to unrelated Services.
361-367: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse
matclient.Clientinstances across reconcile passes.A new
matclient.Clientis constructed per instance on everyReconcile()call. WhenWithInsecureSkipVerifyis part ofclientOpts(likely, given self-signed BMC certs), each call clones a brand-new*http.Transport, discarding the previous pass's connection pool and forcing a fresh TCP+TLS handshake every cycle instead of reusing keep-alives.♻️ Proposed fix: cache clients per instance URL
type Reconciler struct { discovery *MatPodDiscovery serviceBuilder *ServiceBuilder k8sClient K8sServiceClient clientOpts []matclient.Option logger zerolog.Logger + clients map[string]*matclient.Client } @@ for _, instance := range instances { - client, err := matclient.NewClient(instance.URL, r.clientOpts...) - if err != nil { - result.Errors = append(result.Errors, fmt.Errorf("creating client for %s: %w", instance.URL, err)) - fetchFailed = true - continue + client, ok := r.clients[instance.URL] + if !ok { + var err error + client, err = matclient.NewClient(instance.URL, r.clientOpts...) + if err != nil { + result.Errors = append(result.Errors, fmt.Errorf("creating client for %s: %w", instance.URL, err)) + fetchFailed = true + continue + } + if r.clients == nil { + r.clients = make(map[string]*matclient.Client) + } + r.clients[instance.URL] = client }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dev/k8s/machine-a-tron-controller/pkg/controller/controller.go` around lines 361 - 367, Cache matclient.Client instances by instance.URL across Reconcile passes instead of constructing them on every iteration in Reconcile. Add or reuse controller-level client storage, return the cached client when available, and only call matclient.NewClient for unseen URLs; preserve the existing error aggregation and fetchFailed behavior for client-creation failures.dev/k8s/machine-a-tron-controller/pkg/controller/controller_test.go (1)
280-389: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a recreate-on-ClusterIP-change test case.
The
TestComputeServiceDifftable covers create/update/delete but omits theRecreatepath (immutable ClusterIP change), which is an explicitly called-out behavior for this controller.✅ Proposed additional test case
+ { + name: "recreate on ClusterIP change", + desired: []*corev1.Service{ + func() *corev1.Service { + svc := makeTestService("svc-1", "mat-id-1") + svc.Spec.ClusterIP = "10.0.0.5" + return svc + }(), + }, + existing: []*corev1.Service{ + func() *corev1.Service { + svc := makeTestService("svc-1", "mat-id-1") + svc.Spec.ClusterIP = "10.0.0.6" + return svc + }(), + }, + wantRecreateCount: 1, + },(Requires adding a
wantRecreateCountfield to the test struct and assertingdiff.Recreate.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dev/k8s/machine-a-tron-controller/pkg/controller/controller_test.go` around lines 280 - 389, Add a wantRecreateCount field to the TestComputeServiceDiff table, add a case with matching service identity but different ClusterIP values expecting one recreate, and assert diff.Recreate alongside Create, Update, and Delete counts. Keep the existing cases’ expected recreate count at zero.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yaml:
- Line 1863: Update the core-ci-pass job’s needs list to include
build-mat-k8s-controller, matching its inclusion in build-summary and
notify-build-status. Preserve the existing dependency entries and ensure a
failed controller build prevents core-ci-pass from passing.
- Around line 819-843: Add build-mat-k8s-controller to the core-ci-pass
aggregator’s required job dependencies so its failure makes the merge-gating
check fail. Preserve the existing job configuration and ensure the dependency
references this exact job identifier.
In `@dev/k8s/machine-a-tron-controller/cmd/mat-k8s-controller/main.go`:
- Line 123: Validate the sync interval before the time.NewTicker call in the
controller startup flow, rejecting zero or negative values from either
SYNC_INTERVAL or --sync-interval. Fail fast with a clear error message, and only
create the ticker after the interval passes validation.
In `@dev/k8s/machine-a-tron-controller/Makefile`:
- Around line 4-10: Add an `all` target to the Makefile and include it in the
`.PHONY` declaration, wiring it to the appropriate default build workflow so
`make all` succeeds and satisfies checkmake.
In `@dev/k8s/machine-a-tron-controller/pkg/matclient/client.go`:
- Around line 109-123: Update applyInsecureTLS to set the cloned transport’s
TLSClientConfig.MinVersion to the required minimum TLS protocol, while
preserving InsecureSkipVerify for dev/test certificates and the existing
transport cloning behavior.
In `@dev/k8s/machine-a-tron-controller/README.md`:
- Around line 68-72: Update the README command example for make run to use the
$HOME-based kubeconfig path instead of ~/.kube/config, ensuring Make and the
shell pass the expanded absolute path to client-go.
---
Nitpick comments:
In `@dev/k8s/machine-a-tron-controller/pkg/controller/controller_test.go`:
- Around line 280-389: Add a wantRecreateCount field to the
TestComputeServiceDiff table, add a case with matching service identity but
different ClusterIP values expecting one recreate, and assert diff.Recreate
alongside Create, Update, and Delete counts. Keep the existing cases’ expected
recreate count at zero.
In `@dev/k8s/machine-a-tron-controller/pkg/controller/controller.go`:
- Around line 156-160: Reserve the BMC IP range used by the ClusterIP assignment
in the Kubernetes Service CIDR configuration, using a dedicated non-overlapping
sub-range excluded from dynamic Service allocation. Update the
deployment/configuration referenced by the controller’s Service creation path so
addresses assigned from machine.BMC.IP cannot be auto-allocated to unrelated
Services.
- Around line 361-367: Cache matclient.Client instances by instance.URL across
Reconcile passes instead of constructing them on every iteration in Reconcile.
Add or reuse controller-level client storage, return the cached client when
available, and only call matclient.NewClient for unseen URLs; preserve the
existing error aggregation and fetchFailed behavior for client-creation
failures.
In `@dev/k8s/machine-a-tron-controller/pkg/controller/discovery.go`:
- Line 57: Make the cluster DNS domain configurable in the discovery component
by adding a constructor parameter with a safe default of "cluster.local", then
use that value instead of the hardcoded suffix in the URL built by the discovery
logic. Update the relevant constructor call sites while preserving existing
behavior when no custom domain is provided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3dd9240a-418e-49b6-916d-35ce5e8b9a28
⛔ Files ignored due to path filters (1)
dev/k8s/machine-a-tron-controller/go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
.github/workflows/ci.yamlcrates/machine-a-tron/README.mddev/k8s/machine-a-tron-controller/Dockerfiledev/k8s/machine-a-tron-controller/Makefiledev/k8s/machine-a-tron-controller/README.mddev/k8s/machine-a-tron-controller/cmd/mat-k8s-controller/main.godev/k8s/machine-a-tron-controller/go.moddev/k8s/machine-a-tron-controller/pkg/controller/controller.godev/k8s/machine-a-tron-controller/pkg/controller/controller_test.godev/k8s/machine-a-tron-controller/pkg/controller/discovery.godev/k8s/machine-a-tron-controller/pkg/controller/k8s_client.godev/k8s/machine-a-tron-controller/pkg/matclient/client.godev/k8s/machine-a-tron-controller/pkg/matclient/client_test.godev/k8s/machine-a-tron-controller/pkg/matclient/types.gohelm/charts/nico-machine-a-tron/Chart.yamlhelm/charts/nico-machine-a-tron/README.mdhelm/charts/nico-machine-a-tron/charts/mat-k8s-controller/Chart.yamlhelm/charts/nico-machine-a-tron/charts/mat-k8s-controller/templates/_helpers.tplhelm/charts/nico-machine-a-tron/charts/mat-k8s-controller/templates/deployment.yamlhelm/charts/nico-machine-a-tron/charts/mat-k8s-controller/templates/rbac.yamlhelm/charts/nico-machine-a-tron/charts/mat-k8s-controller/templates/serviceaccount.yamlhelm/charts/nico-machine-a-tron/charts/mat-k8s-controller/values.yamlhelm/charts/nico-machine-a-tron/templates/bmc-services.yamlhelm/charts/nico-machine-a-tron/templates/deployment.yamlhelm/charts/nico-machine-a-tron/templates/service.yamlhelm/charts/nico-machine-a-tron/values.yaml
| build-mat-k8s-controller: | ||
| if: >- | ||
| ${{ | ||
| always() | ||
| && github.event_name != 'schedule' | ||
| && needs.prepare.result == 'success' | ||
| && needs.prepare.outputs.source_files_changed == 'true' | ||
| }} | ||
| needs: | ||
| - prepare | ||
| uses: ./.github/workflows/docker-build.yml | ||
| with: | ||
| dockerfile_path: dev/k8s/machine-a-tron-controller/Dockerfile | ||
| context_path: dev/k8s/machine-a-tron-controller | ||
| image_name: ${{ needs.prepare.outputs.image_registry }}/mat-k8s-controller | ||
| image_tag: ${{ needs.prepare.outputs.version }} | ||
| additional_tags: ${{ needs.prepare.outputs.image_registry }}/mat-k8s-controller:${{ needs.prepare.outputs.major_minor_version }}-latest | ||
| platforms: linux/amd64,linux/arm64 | ||
| runner: linux-amd64-cpu4 | ||
| push: ${{ needs.prepare.outputs.publish_images == 'true' }} | ||
| load: false | ||
| scan: true | ||
| tag_latest: false | ||
| timeout_minutes: 30 | ||
| secrets: inherit |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
for file in .github/workflows/docker-build.yml .github/workflows/ci.yaml; do
if [[ -f "$file" ]]; then
rg -n -C 8 'workflow_call:|secrets:|permissions:' "$file" || true
fi
doneRepository: NVIDIA/infra-controller
Length of output: 26879
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== .github/workflows/docker-build.yml =="
wc -l .github/workflows/docker-build.yml
sed -n '1,140p' .github/workflows/docker-build.yml
echo
echo "== .github/workflows/ci.yaml aggregator area =="
wc -l .github/workflows/ci.yaml
sed -n '2000,2075p' .github/workflows/ci.yamlRepository: NVIDIA/infra-controller
Length of output: 7388
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== Search for permissions in reusable workflow =="
rg -n '^(\s*)permissions:|^(\s*)- name:|actions/checkout|docker/login|build-push-action|grype|snyk|trivy|cosign|attest' .github/workflows/docker-build.yml
echo
echo "== Exact core-ci-pass membership around the new job =="
rg -n -C 3 'core-ci-pass:|build-mat-k8s-controller|check-rest-core-proto-sync|lint-police|security-secret-scan' .github/workflows/ci.yamlRepository: NVIDIA/infra-controller
Length of output: 5576
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== Token / permission usage in reusable workflow =="
rg -n 'GITHUB_TOKEN|github\.token|permissions:|pull-requests:|contents:|packages:|attest|provenance|cosign|ghcr|github.actor' .github/workflows/docker-build.yml
echo
echo "== build-mat-k8s-controller presence in summary/notification only =="
rg -n -C 2 'build-mat-k8s-controller|core-ci-pass:' .github/workflows/ci.yamlRepository: NVIDIA/infra-controller
Length of output: 1324
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== secrets usage in .github/workflows/docker-build.yml =="
rg -n 'secrets\.' .github/workflows/docker-build.yml
echo
echo "== job permissions in .github/workflows/docker-build.yml =="
rg -n '^(\s*)permissions:' .github/workflows/docker-build.yml || true
echo
echo "== core-ci-pass need list around the build-mat-k8s-controller line =="
sed -n '2018,2048p' .github/workflows/ci.yamlRepository: NVIDIA/infra-controller
Length of output: 1947
Add build-mat-k8s-controller to core-ci-pass.
A controller-image failure can still leave the required branch-protection check green because this job is only wired into summary/notification, not the aggregator that actually gates merges.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 819-843: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[warning] 829-829: secrets unconditionally inherited by called workflow (secrets-inherit): this reusable workflow
(secrets-inherit)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yaml around lines 819 - 843, Add
build-mat-k8s-controller to the core-ci-pass aggregator’s required job
dependencies so its failure makes the merge-gating check fail. Preserve the
existing job configuration and ensure the dependency references this exact job
identifier.
Source: Linters/SAST tools
| - build-release-container-x86_64 | ||
| - build-release-container-aarch64 | ||
| - build-machine-a-tron | ||
| - build-mat-k8s-controller |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Include the controller build in core-ci-pass.
The new job is included in build-summary and notify-build-status, but the required core-ci-pass.needs list around Lines 2023-2047 still omits build-mat-k8s-controller. A failed controller image build can therefore leave the required CI check green.
Proposed fix
core-ci-pass:
needs:
- changes
- prepare
+ - build-mat-k8s-controller📝 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.
| - build-mat-k8s-controller | |
| core-ci-pass: | |
| needs: | |
| - changes | |
| - prepare | |
| - build-mat-k8s-controller |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yaml at line 1863, Update the core-ci-pass job’s needs
list to include build-mat-k8s-controller, matching its inclusion in
build-summary and notify-build-status. Preserve the existing dependency entries
and ensure a failed controller build prevents core-ci-pass from passing.
| }() | ||
|
|
||
| // Run reconciliation loop | ||
| ticker := time.NewTicker(*syncInterval) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='dev/k8s/machine-a-tron-controller/cmd/mat-k8s-controller/main.go'
echo "== File outline =="
ast-grep outline "$FILE" --view expanded || true
echo
echo "== Relevant lines around ticker =="
sed -n '1,220p' "$FILE" | nl -ba | sed -n '1,220p' | awk 'NR>=110 && NR<=140 {print}'
echo
echo "== Search for sync-interval and duration validation =="
rg -n --hidden --no-ignore-vcs 'sync-interval|syncInterval|time.NewTicker|flag.*Duration|Duration\(' dev/k8s/machine-a-tron-controller -SRepository: NVIDIA/infra-controller
Length of output: 767
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='dev/k8s/machine-a-tron-controller/cmd/mat-k8s-controller/main.go'
echo '== main() slice =='
sed -n '31,139p' "$FILE"
echo
echo '== runReconcile() slice =='
sed -n '140,191p' "$FILE"
echo
echo '== parseDurationOrDefault() slice =='
sed -n '192,220p' "$FILE"Repository: NVIDIA/infra-controller
Length of output: 5635
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='dev/k8s/machine-a-tron-controller/cmd/mat-k8s-controller/main.go'
echo '== main() lines around sync interval setup =='
sed -n '31,120p' "$FILE"
echo
echo '== ticker call and surrounding logic =='
sed -n '120,170p' "$FILE"Repository: NVIDIA/infra-controller
Length of output: 4724
Reject non-positive sync intervals before creating the ticker.
time.NewTicker panics on zero or negative durations, and both SYNC_INTERVAL and --sync-interval can supply those values here. Add a guard before this call and fail fast with a clear error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dev/k8s/machine-a-tron-controller/cmd/mat-k8s-controller/main.go` at line
123, Validate the sync interval before the time.NewTicker call in the controller
startup flow, rejecting zero or negative values from either SYNC_INTERVAL or
--sync-interval. Fail fast with a clear error message, and only create the
ticker after the interval passes validation.
| .PHONY: build test test-coverage lint fmt docker-build docker-build-latest clean run verify | ||
|
|
||
| TAG ?= dev | ||
|
|
||
| build: | ||
| @mkdir -p bin | ||
| go build -o bin/mat-k8s-controller ./cmd/mat-k8s-controller |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the required all target.
make all currently has no rule, and checkmake explicitly requires this target.
Proposed fix
-.PHONY: build test test-coverage lint fmt docker-build docker-build-latest clean run verify
+.PHONY: all build test test-coverage lint fmt docker-build docker-build-latest clean run verify
TAG ?= dev
+all: build
+
build:📝 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.
| .PHONY: build test test-coverage lint fmt docker-build docker-build-latest clean run verify | |
| TAG ?= dev | |
| build: | |
| @mkdir -p bin | |
| go build -o bin/mat-k8s-controller ./cmd/mat-k8s-controller | |
| .PHONY: all build test test-coverage lint fmt docker-build docker-build-latest clean run verify | |
| TAG ?= dev | |
| all: build | |
| build: | |
| `@mkdir` -p bin | |
| go build -o bin/mat-k8s-controller ./cmd/mat-k8s-controller |
🧰 Tools
🪛 checkmake (0.3.2)
[warning] 5-5: Required target "all" is missing from the Makefile.
(minphony)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dev/k8s/machine-a-tron-controller/Makefile` around lines 4 - 10, Add an `all`
target to the Makefile and include it in the `.PHONY` declaration, wiring it to
the appropriate default build workflow so `make all` succeeds and satisfies
checkmake.
Source: Linters/SAST tools
| func (c *Client) applyInsecureTLS() { | ||
| transport, ok := c.httpClient.Transport.(*http.Transport) | ||
| if !ok || transport == nil { | ||
| transport = http.DefaultTransport.(*http.Transport).Clone() | ||
| } else { | ||
| transport = transport.Clone() | ||
| } | ||
|
|
||
| if transport.TLSClientConfig == nil { | ||
| transport.TLSClientConfig = &tls.Config{} | ||
| } | ||
| transport.TLSClientConfig.InsecureSkipVerify = true //nolint:gosec // Intentional for dev/test with self-signed certs | ||
|
|
||
| c.httpClient.Transport = transport | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Pin a minimum TLS version on the insecure transport.
The cloned tls.Config sets InsecureSkipVerify but leaves MinVersion unset, so the connection floor defaults to TLS 1.2 as a client. Even with certificate verification disabled for dev/test, pinning a minimum protocol version guards against downgrade to weaker ciphers.
🔒 Proposed fix
if transport.TLSClientConfig == nil {
transport.TLSClientConfig = &tls.Config{}
}
transport.TLSClientConfig.InsecureSkipVerify = true //nolint:gosec // Intentional for dev/test with self-signed certs
+ if transport.TLSClientConfig.MinVersion == 0 {
+ transport.TLSClientConfig.MinVersion = tls.VersionTLS12
+ }📝 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.
| func (c *Client) applyInsecureTLS() { | |
| transport, ok := c.httpClient.Transport.(*http.Transport) | |
| if !ok || transport == nil { | |
| transport = http.DefaultTransport.(*http.Transport).Clone() | |
| } else { | |
| transport = transport.Clone() | |
| } | |
| if transport.TLSClientConfig == nil { | |
| transport.TLSClientConfig = &tls.Config{} | |
| } | |
| transport.TLSClientConfig.InsecureSkipVerify = true //nolint:gosec // Intentional for dev/test with self-signed certs | |
| c.httpClient.Transport = transport | |
| } | |
| func (c *Client) applyInsecureTLS() { | |
| transport, ok := c.httpClient.Transport.(*http.Transport) | |
| if !ok || transport == nil { | |
| transport = http.DefaultTransport.(*http.Transport).Clone() | |
| } else { | |
| transport = transport.Clone() | |
| } | |
| if transport.TLSClientConfig == nil { | |
| transport.TLSClientConfig = &tls.Config{} | |
| } | |
| transport.TLSClientConfig.InsecureSkipVerify = true //nolint:gosec // Intentional for dev/test with self-signed certs | |
| if transport.TLSClientConfig.MinVersion == 0 { | |
| transport.TLSClientConfig.MinVersion = tls.VersionTLS12 | |
| } | |
| c.httpClient.Transport = transport | |
| } |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 117-117: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures
(missing-ssl-minversion-go)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dev/k8s/machine-a-tron-controller/pkg/matclient/client.go` around lines 109 -
123, Update applyInsecureTLS to set the cloned transport’s
TLSClientConfig.MinVersion to the required minimum TLS protocol, while
preserving InsecureSkipVerify for dev/test certificates and the existing
transport cloning behavior.
Source: Linters/SAST tools
| ```bash | ||
| make build | ||
| make test | ||
| make run KUBECONFIG=~/.kube/config | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use $HOME for the kubeconfig path.
After Make expands this variable, the recipe receives --kubeconfig=~/.kube/config; the tilde is not at the start of a shell word, so it is not expanded and client-go receives a literal path. (gnu.org)
Proposed fix
-make run KUBECONFIG=~/.kube/config
+make run KUBECONFIG="$HOME/.kube/config"📝 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.
| ```bash | |
| make build | |
| make test | |
| make run KUBECONFIG=~/.kube/config | |
| ``` |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dev/k8s/machine-a-tron-controller/README.md` around lines 68 - 72, Update the
README command example for make run to use the $HOME-based kubeconfig path
instead of ~/.kube/config, ensuring Make and the shell pass the expanded
absolute path to client-go.
Source: Path instructions
1 participant
Implements a Kubernetes controller that dynamically creates Services for machine-a-tron mock BMC endpoints, enabling multi-pod deployments where each BMC needs stable network exposure.
Why this change?
When machine-a-tron runs in Kubernetes with multiple pods, mock BMCs need stable network exposure so NICo components can reach Redfish endpoints. This controller consumes machine-a-tron's
/machines/statusAPI and reconciles one Service per mock BMC.Machine-a-tron itself stays Kubernetes-agnostic - all K8s logic lives in this separate controller.
What's included:
Controller (
dev/k8s/machine-a-tron-controller/):nvidia-infra-controller/mat-service=truelabel/machines/statusfrom each discovered instanceHelm subchart (
helm/charts/nico-machine-a-tron/charts/mat-k8s-controller/):mat-k8s-controller.enabled=trueCI/CD:
build-mat-k8s-controllerjob to main CI workflowRelated issues
Type of Change
Testing
Manual testing verified:
Additional Notes
ServiceCIDR requirement: BMC IPs must fall within Kubernetes ServiceCIDR range