USHIFT-6951: Add metrics-server as optional MicroShift component#6808
Open
copejon wants to merge 5 commits into
Open
USHIFT-6951: Add metrics-server as optional MicroShift component#6808copejon wants to merge 5 commits into
copejon wants to merge 5 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@copejon: This pull request references USHIFT-6951 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Contributor
|
Caution Review failedAn error occurred during the review process. Please try again later. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds metrics-server as an optional MicroShift component: Kubernetes manifests and kustomizations, RBAC and APIService registration, deployment and audit profiles, certificate CSR and runtime provisioning, OpenTelemetry drop-in and systemd wrapper, packaging/subpackages, and auto-rebase tooling to generate image overrides. ChangesMetrics Server Optional Component
Sequence Diagram(s)sequenceDiagram
participant MicroShift
participant Kustomize
participant provisionMetricsServerCerts
participant KubernetesAPI
MicroShift->>Kustomize: apply metrics-server kustomization
Kustomize->>KubernetesAPI: create Namespace/ServiceAccount/RBAC/Service/APIService/Deployment
MicroShift->>provisionMetricsServerCerts: start background provisioning
provisionMetricsServerCerts->>KubernetesAPI: wait for Namespace then Create/Update Secret+ConfigMap
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labelsready-for-human-review Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: copejon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
This was referenced Jun 5, 2026
copejon
force-pushed
the
ushift-6951/metrics-server
branch
from
a185390 to
2f00a69
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
packaging/observability/microshift-observability.service (1)
11-11: ⚡ Quick winUse an argv array instead of a concatenated shell string
Building
ARGSas a string is brittle (word-splitting/path edge cases). Use a bash array and quoted expansion.Proposed change
-ExecStart=/bin/bash -c 'ARGS="--config=file:/etc/microshift/observability/opentelemetry-collector.yaml"; for f in /etc/microshift/observability/otelcol.d/*.yaml; do [ -f "$$f" ] && ARGS="$$ARGS --config=file:$$f"; done; exec /usr/bin/opentelemetry-collector $$ARGS' +ExecStart=/bin/bash -c 'args=(--config=file:/etc/microshift/observability/opentelemetry-collector.yaml); for f in /etc/microshift/observability/otelcol.d/*.yaml; do [ -f "$$f" ] && args+=(--config=file:$$f); done; exec /usr/bin/opentelemetry-collector "$${args[@]}"'🤖 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 `@packaging/observability/microshift-observability.service` at line 11, The ExecStart line builds a brittle space-joined ARGS string; change it to build a bash array and use quoted array expansion when executing the collector: initialize ARGS=() (or ARGS=()), append elements as ARGS+=(--config="file:/etc/microshift/observability/opentelemetry-collector.yaml") and inside the loop for f in /etc/microshift/observability/otelcol.d/*.yaml; do [ -f "$f" ] && ARGS+=(--config="file:$f"); done; finally exec /usr/bin/opentelemetry-collector "${ARGS[@]}" so that each --config is a separate argv element and you avoid word-splitting/path edge cases for the ExecStart/ARGS usage.
🤖 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 `@assets/optional/metrics-server/00-namespace.yaml`:
- Around line 7-9: The namespace-level PodSecurity labels
pod-security.kubernetes.io/enforce, pod-security.kubernetes.io/audit, and
pod-security.kubernetes.io/warn are set to "privileged" which is too permissive;
change their values to a safer profile (e.g., "baseline" or "restricted") unless
you have a documented exception, and ensure the three keys (enforce, audit,
warn) are updated consistently to the chosen profile so namespace-wide policies
default to the tightened posture.
In `@assets/optional/metrics-server/03-deployment.yaml`:
- Around line 67-70: The metrics-server container currently only specifies
resource requests; add a matching limits block under the same resources section
for the metrics-server container to enforce CPU and memory caps (e.g.,
limits.cpu: 100m and limits.memory: 100Mi). Update the Deployment's container
resources block (the metrics-server container in the manifest) to include both
requests and limits so it conforms to the "Resource limits (cpu, memory) on
every container" guideline.
- Around line 71-75: The container securityContext currently sets
allowPrivilegeEscalation, readOnlyRootFilesystem and runAsNonRoot but does not
drop Linux capabilities; update the Pod/Container spec by adding a capabilities
block under securityContext (e.g., in the same container spec that contains
allowPrivilegeEscalation/readOnlyRootFilesystem/runAsNonRoot) with
capabilities.drop set to ["ALL"] so all capabilities are removed by default and
then explicitly add back any minimal capabilities only if absolutely required
elsewhere; ensure the change is applied alongside the existing
terminationMessagePolicy and other securityContext fields.
In `@assets/optional/metrics-server/kustomization.yaml`:
- Around line 3-13: The kustomization.yaml resources list is missing the two
RPM-installed manifests; update the resources array (the same block that
currently lists 00-namespace.yaml, 03-deployment.yaml, 04-service.yaml, etc.) to
include network-policy-downstream.yaml and pod-disruption-budget.yaml so both
NetworkPolicy and PodDisruptionBudget are applied; add those two filenames to
the resources list in assets/optional/metrics-server/kustomization.yaml.
In `@pkg/cmd/metrics.go`:
- Around line 44-50: The polling closure passed to wait.PollUntilContextTimeout
currently treats any Get() error as "not ready" and keeps retrying; change the
logic in the closure used with wait.PollUntilContextTimeout so that after
calling clientset.CoreV1().Namespaces().Get(ctx, ns, metav1.GetOptions{}), if
err == nil return true,nil; if apierrors.IsNotFound(err) log and return
false,nil to keep retrying; for any other error return false, err (so the poll
stops and surfaces auth/TLS/transport failures). Ensure you import/use
apierrors.IsNotFound and update the closure around
clientset.CoreV1().Namespaces().Get accordingly.
- Around line 24-31: The current check uses
util.PathExists(metricsServerManifestPath) so provisioning proceeds whenever the
file exists even if the metrics component is not enabled; change the gating to
verify the component is enabled and that any configured kustomization paths are
present (e.g., read the configured kustomizationPaths or component flag used by
your installer and ensure they are enabled) before attempting cert provisioning.
Update the logic around util.PathExists(metricsServerManifestPath) to first
consult the metrics enablement/config (the same config that controls
installation) and validate each configured kustomization path exists, returning
early if the component is disabled or no configured paths are present.
In `@scripts/auto-rebase/assets_metrics.yaml`:
- Around line 34-37: Update the two YAML asset entries so they match the rest of
the PR naming convention: change the "file" values
"release-metrics-aarch64.json" and "release-metrics-x86_64.json" to
"release-metrics-server-aarch64.json" and "release-metrics-server-x86_64.json"
respectively in the assets_metrics.yaml file (the two "- file:" entries shown),
leaving the corresponding "ignore" values unchanged.
In `@scripts/auto-rebase/assets.yaml`:
- Around line 304-326: The asset list under the optional/metrics-server/ entry
is missing the two manifests that the packaging script expects; update the files
list for the optional/metrics-server/ dir to include entries for
network-policy-downstream.yaml and pod-disruption-budget.yaml so the asset set
matches packaging/rpm/microshift.spec; ensure the new entries use the same
indentation/format as the existing file: ... entries (e.g., add "- file:
network-policy-downstream.yaml" and "- file: pod-disruption-budget.yaml" under
the files: block for the optional/metrics-server/ dir).
In `@scripts/auto-rebase/rebase.sh`:
- Around line 1184-1187: The script currently skips unknown images when
release_tag is empty (checking release_tag, orig_image, component_dir) which
allows silent incomplete kustomization.*.yaml outputs; change the behavior to
fail-fast by replacing the continue path with a non-zero exit (e.g., >&2 echo
descriptive error including orig_image and component_dir, then exit 1) so the
rebase.sh run stops immediately on unmapped images and surfaces the problem to
CI.
---
Nitpick comments:
In `@packaging/observability/microshift-observability.service`:
- Line 11: The ExecStart line builds a brittle space-joined ARGS string; change
it to build a bash array and use quoted array expansion when executing the
collector: initialize ARGS=() (or ARGS=()), append elements as
ARGS+=(--config="file:/etc/microshift/observability/opentelemetry-collector.yaml")
and inside the loop for f in /etc/microshift/observability/otelcol.d/*.yaml; do
[ -f "$f" ] && ARGS+=(--config="file:$f"); done; finally exec
/usr/bin/opentelemetry-collector "${ARGS[@]}" so that each --config is a
separate argv element and you avoid word-splitting/path edge cases for the
ExecStart/ARGS usage.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 25baa466-07b1-4aa1-90d3-a9e96b03c015
📒 Files selected for processing (27)
assets/optional/metrics-server/00-namespace.yamlassets/optional/metrics-server/01-cluster-role-binding-auth-delegator.yamlassets/optional/metrics-server/01-cluster-role-binding.yamlassets/optional/metrics-server/01-cluster-role.yamlassets/optional/metrics-server/01-role-binding-auth-reader.yamlassets/optional/metrics-server/01-service-account.yamlassets/optional/metrics-server/02-configmap-audit-profiles.yamlassets/optional/metrics-server/03-deployment.yamlassets/optional/metrics-server/04-api-service.yamlassets/optional/metrics-server/04-service.yamlassets/optional/metrics-server/kustomization.aarch64.yamlassets/optional/metrics-server/kustomization.x86_64.yamlassets/optional/metrics-server/kustomization.yamlassets/optional/metrics-server/release-metrics-server-aarch64.jsonassets/optional/metrics-server/release-metrics-server-x86_64.jsonpackaging/observability/microshift-observability.servicepackaging/observability/otelcol.d/microshift-metrics-server.yamlpackaging/rpm/microshift.specpkg/cmd/init.gopkg/cmd/metrics.gopkg/cmd/run.gopkg/healthcheck/microshift_optional_workloads.gopkg/util/cryptomaterial/certinfo.goscripts/auto-rebase/assets.yamlscripts/auto-rebase/assets_metrics.yamlscripts/auto-rebase/rebase.shtest/bin/common.sh
Comment on lines
+7
to
+9
| pod-security.kubernetes.io/enforce: privileged | ||
| pod-security.kubernetes.io/audit: privileged | ||
| pod-security.kubernetes.io/warn: privileged |
Contributor
There was a problem hiding this comment.
Namespace-level privileged Pod Security is overly permissive.
Setting enforce/audit/warn to privileged at Line 7-Line 9 weakens namespace isolation for all workloads. Prefer baseline/restricted unless a strict exception is required.
Suggested tightening
- pod-security.kubernetes.io/enforce: privileged
- pod-security.kubernetes.io/audit: privileged
- pod-security.kubernetes.io/warn: privileged
+ pod-security.kubernetes.io/enforce: baseline
+ pod-security.kubernetes.io/audit: restricted
+ pod-security.kubernetes.io/warn: restrictedAs per coding guidelines, "**/*.{yaml,yml,json}: Flag privileged: true ... in container/Kubernetes manifests".
📝 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
| pod-security.kubernetes.io/enforce: privileged | |
| pod-security.kubernetes.io/audit: privileged | |
| pod-security.kubernetes.io/warn: privileged | |
| pod-security.kubernetes.io/enforce: baseline | |
| pod-security.kubernetes.io/audit: restricted | |
| pod-security.kubernetes.io/warn: restricted |
🤖 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 `@assets/optional/metrics-server/00-namespace.yaml` around lines 7 - 9, The
namespace-level PodSecurity labels pod-security.kubernetes.io/enforce,
pod-security.kubernetes.io/audit, and pod-security.kubernetes.io/warn are set to
"privileged" which is too permissive; change their values to a safer profile
(e.g., "baseline" or "restricted") unless you have a documented exception, and
ensure the three keys (enforce, audit, warn) are updated consistently to the
chosen profile so namespace-wide policies default to the tightened posture.
Comment on lines
+67
to
+70
| resources: | ||
| requests: | ||
| cpu: 1m | ||
| memory: 40Mi |
Contributor
There was a problem hiding this comment.
Add container resource limits.
Only requests are set; limits are missing for metrics-server. Add CPU/memory limits to enforce predictable runtime behavior.
As per coding guidelines, "Resource limits (cpu, memory) on every container".
🤖 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 `@assets/optional/metrics-server/03-deployment.yaml` around lines 67 - 70, The
metrics-server container currently only specifies resource requests; add a
matching limits block under the same resources section for the metrics-server
container to enforce CPU and memory caps (e.g., limits.cpu: 100m and
limits.memory: 100Mi). Update the Deployment's container resources block (the
metrics-server container in the manifest) to include both requests and limits so
it conforms to the "Resource limits (cpu, memory) on every container" guideline.
Comment on lines
+24
to
+31
| exists, err := util.PathExists(metricsServerManifestPath) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if !exists { | ||
| klog.V(2).Infof("Metrics-server manifests not found at %s, skipping cert provisioning", metricsServerManifestPath) | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
Gate provisioning on configured kustomization paths, not only file presence.
This runs even when the component is installed but not enabled, causing avoidable polling/timeouts on startup.
Suggested fix
func provisionMetricsServerCerts(ctx context.Context, cfg *config.Config) error {
exists, err := util.PathExists(metricsServerManifestPath)
if err != nil {
return err
}
if !exists {
klog.V(2).Infof("Metrics-server manifests not found at %s, skipping cert provisioning", metricsServerManifestPath)
return nil
}
+
+ configuredPaths, err := cfg.Manifests.GetKustomizationPaths()
+ if err != nil {
+ return fmt.Errorf("reading configured kustomization paths: %w", err)
+ }
+ enabled := false
+ for _, p := range configuredPaths {
+ if p == metricsServerManifestPath {
+ enabled = true
+ break
+ }
+ }
+ if !enabled {
+ klog.V(2).Infof("Metrics-server manifests found but not configured in kustomization paths, skipping cert provisioning")
+ return nil
+ }🤖 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 `@pkg/cmd/metrics.go` around lines 24 - 31, The current check uses
util.PathExists(metricsServerManifestPath) so provisioning proceeds whenever the
file exists even if the metrics component is not enabled; change the gating to
verify the component is enabled and that any configured kustomization paths are
present (e.g., read the configured kustomizationPaths or component flag used by
your installer and ensure they are enabled) before attempting cert provisioning.
Update the logic around util.PathExists(metricsServerManifestPath) to first
consult the metrics enablement/config (the same config that controls
installation) and validate each configured kustomization path exists, returning
early if the component is disabled or no configured paths are present.
2f00a69 to
2552538
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@assets/optional/metrics-server/02-configmap-audit-profiles.yaml`:
- Around line 2-38: The embedded audit policy YAMLs are malformed because keys
and values are written with JSON-style quoted strings; update
metadata-profile.yaml, none-profile.yaml, request-profile.yaml and
requestresponse-profile.yaml inside the ConfigMap data to valid YAML by removing
the unnecessary quotes around keys (apiVersion, kind, metadata, name,
omitStages, rules, level) and unquoting list items (e.g., RequestReceived), and
ensure each rule is a proper mapping (e.g., - level: Metadata) with correct
indentation so each policy is valid Kubernetes audit policy YAML.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 070ff4b2-5dbd-4430-8057-ef1fec980558
⛔ Files ignored due to path filters (3)
etcd/vendor/github.com/openshift/microshift/pkg/config/config.gois excluded by!**/vendor/**etcd/vendor/github.com/openshift/microshift/pkg/config/dns.gois excluded by!**/vendor/**etcd/vendor/github.com/openshift/microshift/pkg/util/cryptomaterial/certinfo.gois excluded by!**/vendor/**
📒 Files selected for processing (27)
assets/optional/metrics-server/00-namespace.yamlassets/optional/metrics-server/01-cluster-role-binding-auth-delegator.yamlassets/optional/metrics-server/01-cluster-role-binding.yamlassets/optional/metrics-server/01-cluster-role.yamlassets/optional/metrics-server/01-role-binding-auth-reader.yamlassets/optional/metrics-server/01-service-account.yamlassets/optional/metrics-server/02-configmap-audit-profiles.yamlassets/optional/metrics-server/03-deployment.yamlassets/optional/metrics-server/04-api-service.yamlassets/optional/metrics-server/04-service.yamlassets/optional/metrics-server/kustomization.aarch64.yamlassets/optional/metrics-server/kustomization.x86_64.yamlassets/optional/metrics-server/kustomization.yamlassets/optional/metrics-server/release-metrics-server-aarch64.jsonassets/optional/metrics-server/release-metrics-server-x86_64.jsonpackaging/observability/microshift-observability.servicepackaging/observability/otelcol.d/microshift-metrics-server.yamlpackaging/rpm/microshift.specpkg/cmd/init.gopkg/cmd/metrics.gopkg/cmd/run.gopkg/healthcheck/microshift_optional_workloads.gopkg/util/cryptomaterial/certinfo.goscripts/auto-rebase/assets.yamlscripts/auto-rebase/assets_metrics.yamlscripts/auto-rebase/rebase.shtest/bin/common.sh
✅ Files skipped from review due to trivial changes (5)
- assets/optional/metrics-server/kustomization.aarch64.yaml
- test/bin/common.sh
- scripts/auto-rebase/assets_metrics.yaml
- assets/optional/metrics-server/release-metrics-server-x86_64.json
- assets/optional/metrics-server/release-metrics-server-aarch64.json
🚧 Files skipped from review as they are similar to previous changes (19)
- pkg/cmd/run.go
- assets/optional/metrics-server/kustomization.x86_64.yaml
- assets/optional/metrics-server/01-cluster-role-binding-auth-delegator.yaml
- assets/optional/metrics-server/00-namespace.yaml
- assets/optional/metrics-server/kustomization.yaml
- assets/optional/metrics-server/01-cluster-role.yaml
- scripts/auto-rebase/assets.yaml
- assets/optional/metrics-server/01-role-binding-auth-reader.yaml
- packaging/observability/microshift-observability.service
- packaging/observability/otelcol.d/microshift-metrics-server.yaml
- assets/optional/metrics-server/04-api-service.yaml
- assets/optional/metrics-server/01-cluster-role-binding.yaml
- pkg/cmd/init.go
- pkg/healthcheck/microshift_optional_workloads.go
- assets/optional/metrics-server/01-service-account.yaml
- pkg/cmd/metrics.go
- assets/optional/metrics-server/03-deployment.yaml
- packaging/rpm/microshift.spec
- scripts/auto-rebase/rebase.sh
Comment on lines
+2
to
+38
| data: | ||
| metadata-profile.yaml: |- | ||
| "apiVersion": "audit.k8s.io/v1" | ||
| "kind": "Policy" | ||
| "metadata": | ||
| "name": "Metadata" | ||
| "omitStages": | ||
| - "RequestReceived" | ||
| "rules": | ||
| - "level": "Metadata" | ||
| none-profile.yaml: |- | ||
| "apiVersion": "audit.k8s.io/v1" | ||
| "kind": "Policy" | ||
| "metadata": | ||
| "name": "None" | ||
| "omitStages": | ||
| - "RequestReceived" | ||
| "rules": | ||
| - "level": "None" | ||
| request-profile.yaml: |- | ||
| "apiVersion": "audit.k8s.io/v1" | ||
| "kind": "Policy" | ||
| "metadata": | ||
| "name": "Request" | ||
| "omitStages": | ||
| - "RequestReceived" | ||
| "rules": | ||
| - "level": "Request" | ||
| requestresponse-profile.yaml: |- | ||
| "apiVersion": "audit.k8s.io/v1" | ||
| "kind": "Policy" | ||
| "metadata": | ||
| "name": "RequestResponse" | ||
| "omitStages": | ||
| - "RequestReceived" | ||
| "rules": | ||
| - "level": "RequestResponse" |
Contributor
There was a problem hiding this comment.
Malformed YAML syntax in embedded audit profiles.
All four audit policy documents use JSON-style quoted keys ("apiVersion", "kind", etc.) instead of proper YAML syntax. Kubernetes audit policies should be written in standard YAML format without quoted keys.
🔧 Proposed fix
data:
metadata-profile.yaml: |-
- "apiVersion": "audit.k8s.io/v1"
- "kind": "Policy"
- "metadata":
- "name": "Metadata"
- "omitStages":
+ apiVersion: audit.k8s.io/v1
+ kind: Policy
+ metadata:
+ name: Metadata
+ omitStages:
- "RequestReceived"
- "rules":
- - "level": "Metadata"
+ rules:
+ - level: Metadata
none-profile.yaml: |-
- "apiVersion": "audit.k8s.io/v1"
- "kind": "Policy"
- "metadata":
- "name": "None"
- "omitStages":
+ apiVersion: audit.k8s.io/v1
+ kind: Policy
+ metadata:
+ name: None
+ omitStages:
- "RequestReceived"
- "rules":
- - "level": "None"
+ rules:
+ - level: None
request-profile.yaml: |-
- "apiVersion": "audit.k8s.io/v1"
- "kind": "Policy"
- "metadata":
- "name": "Request"
- "omitStages":
+ apiVersion: audit.k8s.io/v1
+ kind: Policy
+ metadata:
+ name: Request
+ omitStages:
- "RequestReceived"
- "rules":
- - "level": "Request"
+ rules:
+ - level: Request
requestresponse-profile.yaml: |-
- "apiVersion": "audit.k8s.io/v1"
- "kind": "Policy"
- "metadata":
- "name": "RequestResponse"
- "omitStages":
+ apiVersion: audit.k8s.io/v1
+ kind: Policy
+ metadata:
+ name: RequestResponse
+ omitStages:
- "RequestReceived"
- "rules":
- - "level": "RequestResponse"
+ rules:
+ - level: RequestResponse🤖 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 `@assets/optional/metrics-server/02-configmap-audit-profiles.yaml` around lines
2 - 38, The embedded audit policy YAMLs are malformed because keys and values
are written with JSON-style quoted strings; update metadata-profile.yaml,
none-profile.yaml, request-profile.yaml and requestresponse-profile.yaml inside
the ConfigMap data to valid YAML by removing the unnecessary quotes around keys
(apiVersion, kind, metadata, name, omitStages, rules, level) and unquoting list
items (e.g., RequestReceived), and ensure each rule is a proper mapping (e.g., -
level: Metadata) with correct indentation so each policy is valid Kubernetes
audit policy YAML.
2552538 to
6f8a50b
Compare
coderabbitai
Bot
added
the
ready-for-human-review
copejon
force-pushed
the
ushift-6951/metrics-server
branch
from
6f8a50b to
82550e8
Compare
coderabbitai
Bot
removed
the
ready-for-human-review
copejon
force-pushed
the
ushift-6951/metrics-server
branch
from
82550e8 to
9cc9d0e
Compare
Add metrics-server with Go cert provisioning, kustomize manifests, healthcheck registration, auto-rebase support, RPM packaging, and OpenTelemetry collector drop-in configuration. Establishes shared infrastructure used by all metrics exporters: - OPTIONAL_COMPONENTS in rebase.sh for CMO repo cloning - update_metrics_images() for image digest resolution - otelcol.d drop-in config directory and service wrapper - mergeWorkloads() for shared-namespace healthcheck Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
copejon
force-pushed
the
ushift-6951/metrics-server
branch
from
9cc9d0e to
261c1a0
Compare
…omponent
- Change metrics-server cert provisioning failure from Warning to Error level
so failures are visible without being Fatal for an optional component.
- Split shared release JSON into per-component files. Each metrics component
now ships its own release-{component}-{arch}.json with only its image ref.
- Update rebase.sh to generate per-component release JSONs during rebase.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/auto-rebase/rebase.sh (1)
1191-1193:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not skip unmapped manifest images.
Still valid from the earlier review: warning-and-continue here can emit a partial
kustomization.${arch}.yaml, so the rebase succeeds with broken output.Patch
if [[ -z "${release_tag}" ]]; then - >&2 echo "WARNING: Unknown metrics image '${orig_image}' in ${component_dir}, skipping" - continue + >&2 echo "ERROR: Unknown metrics image '${orig_image}' in ${component_dir}" + return 1 fi🤖 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 `@scripts/auto-rebase/rebase.sh` around lines 1191 - 1193, The current block in rebase.sh that checks if [[ -z "${release_tag}" ]] and then echoes a warning and continues causes partial kustomization.${arch}.yaml output when an image (orig_image) isn't in the mapping; instead, stop skipping: when release_tag is empty, still emit the image into the kustomization entry (use orig_image as the image reference or a sensible fallback) and log the warning but do not use continue; update the code paths that write into kustomization.${arch}.yaml so they handle unmapped images (referencing variables release_tag, orig_image and component_dir) and ensure the resulting kustomization.${arch}.yaml always includes an image entry for every manifest image.
🤖 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 `@scripts/auto-rebase/rebase.sh`:
- Around line 1169-1176: The script assumes jq successfully finds a pullspec;
validate that the jq result is non-empty before writing files: after computing
release_tag and new_image (from the jq command that selects .from.name), check
if new_image is empty (or if new_digest parsed from new_image is empty), and if
so emit a clear error mentioning the release_tag and json_key and exit with a
non-zero status instead of writing component_release_json; apply the same guard
to the other identical block (the one that mirrors lines ~1196-1204) so
malformed release-*.json / kustomization.*.yaml are never created when the
payload tag is missing.
---
Duplicate comments:
In `@scripts/auto-rebase/rebase.sh`:
- Around line 1191-1193: The current block in rebase.sh that checks if [[ -z
"${release_tag}" ]] and then echoes a warning and continues causes partial
kustomization.${arch}.yaml output when an image (orig_image) isn't in the
mapping; instead, stop skipping: when release_tag is empty, still emit the image
into the kustomization entry (use orig_image as the image reference or a
sensible fallback) and log the warning but do not use continue; update the code
paths that write into kustomization.${arch}.yaml so they handle unmapped images
(referencing variables release_tag, orig_image and component_dir) and ensure the
resulting kustomization.${arch}.yaml always includes an image entry for every
manifest image.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 33a078aa-483a-4842-a2dd-2d3d185e3f96
⛔ Files ignored due to path filters (3)
etcd/vendor/github.com/openshift/microshift/pkg/config/config.gois excluded by!**/vendor/**etcd/vendor/github.com/openshift/microshift/pkg/config/dns.gois excluded by!**/vendor/**etcd/vendor/github.com/openshift/microshift/pkg/util/cryptomaterial/certinfo.gois excluded by!**/vendor/**
📒 Files selected for processing (27)
assets/optional/metrics-server/00-namespace.yamlassets/optional/metrics-server/01-cluster-role-binding-auth-delegator.yamlassets/optional/metrics-server/01-cluster-role-binding.yamlassets/optional/metrics-server/01-cluster-role.yamlassets/optional/metrics-server/01-role-binding-auth-reader.yamlassets/optional/metrics-server/01-service-account.yamlassets/optional/metrics-server/02-configmap-audit-profiles.yamlassets/optional/metrics-server/03-deployment.yamlassets/optional/metrics-server/04-api-service.yamlassets/optional/metrics-server/04-service.yamlassets/optional/metrics-server/kustomization.aarch64.yamlassets/optional/metrics-server/kustomization.x86_64.yamlassets/optional/metrics-server/kustomization.yamlassets/optional/metrics-server/release-metrics-server-aarch64.jsonassets/optional/metrics-server/release-metrics-server-x86_64.jsonpackaging/observability/microshift-observability.servicepackaging/observability/otelcol.d/microshift-metrics-server.yamlpackaging/rpm/microshift.specpkg/cmd/init.gopkg/cmd/metrics.gopkg/cmd/run.gopkg/healthcheck/microshift_optional_workloads.gopkg/util/cryptomaterial/certinfo.goscripts/auto-rebase/assets.yamlscripts/auto-rebase/assets_metrics.yamlscripts/auto-rebase/rebase.shtest/bin/common.sh
✅ Files skipped from review due to trivial changes (4)
- packaging/observability/microshift-observability.service
- assets/optional/metrics-server/kustomization.yaml
- assets/optional/metrics-server/kustomization.aarch64.yaml
- test/bin/common.sh
🚧 Files skipped from review as they are similar to previous changes (20)
- assets/optional/metrics-server/release-metrics-server-aarch64.json
- assets/optional/metrics-server/01-service-account.yaml
- pkg/healthcheck/microshift_optional_workloads.go
- assets/optional/metrics-server/01-cluster-role.yaml
- assets/optional/metrics-server/04-api-service.yaml
- assets/optional/metrics-server/00-namespace.yaml
- assets/optional/metrics-server/01-cluster-role-binding.yaml
- assets/optional/metrics-server/02-configmap-audit-profiles.yaml
- assets/optional/metrics-server/04-service.yaml
- pkg/cmd/run.go
- assets/optional/metrics-server/03-deployment.yaml
- packaging/observability/otelcol.d/microshift-metrics-server.yaml
- scripts/auto-rebase/assets_metrics.yaml
- assets/optional/metrics-server/kustomization.x86_64.yaml
- pkg/cmd/init.go
- scripts/auto-rebase/assets.yaml
- packaging/rpm/microshift.spec
- assets/optional/metrics-server/01-role-binding-auth-reader.yaml
- pkg/cmd/metrics.go
- assets/optional/metrics-server/01-cluster-role-binding-auth-delegator.yaml
- Add capabilities.drop ALL to metrics-server container securityContext - Fix release JSON filenames in assets_metrics.yaml to match actual files - Fail-fast on unknown metrics image in rebase.sh instead of silent skip Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fail-fast when jq returns empty or null for release tag or image lookups, preventing garbage data in release JSONs and kustomization files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Only retry when the namespace is not yet created (IsNotFound). Return auth, TLS, and other permanent errors immediately instead of retrying for 5 minutes with a misleading timeout. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Action performedReview finished.
|
Contributor
|
@copejon: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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
2 participants
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.
Add metrics-server as an optional MicroShift component deployed via kustomize manifests and packaged as
microshift-metrics-serverandmicroshift-metrics-server-release-infoRPM sub-packages.This is PR 1/3 splitting #6763 into independently-mergeable patches. The three PRs can merge in any order.
Siblings: #6809 (kube-state-metrics), #6810 (node-exporter)
What's included
assets/optional/metrics-server/) — Deployment, APIService, RBAC, NetworkPolicy, PDB, service-ca TLS, andopenshift-monitoringnamespacepkg/cmd/metrics.go)mergeWorkloads()function and metrics-server map entry in optional workload paths%package metrics-serverand%package metrics-server-release-infowith per-file installsotelcol.d/directory structure andmicroshift-metrics-server.yamlscrape config; service file updated to load drop-in configs via bash wrapperupdate_metrics_images()inrebase.sh+assets_metrics.yamlfor all three exporters (shared infrastructure lives in this PR)test/bin/common.shSummary by CodeRabbit
New Features
Chores / Packaging