[scalar-manager] Add Headlamp integration support#325
Conversation
Summary of ChangesHello @thongdk8, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances scalar-manager by introducing robust integration capabilities with Headlamp, a Kubernetes UI. The changes involve adding new Helm chart configurations to control Headlamp's enablement and behavior, establishing a dedicated ServiceAccount with cluster-admin permissions for secure token generation, and injecting necessary environment variables into the scalar-manager API and Web containers to facilitate Headlamp service discovery and proxying. This allows users to seamlessly connect scalar-manager with a deployed Headlamp instance for improved Kubernetes cluster management. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Headlamp integration support for the scalar-manager Helm chart. The changes are comprehensive, adding new values, documentation, and Kubernetes resources like a ServiceAccount, ClusterRole, and ClusterRoleBinding to facilitate this integration. While the implementation is largely correct, there is a critical security concern regarding the granting of cluster-admin privileges to the Headlamp service account. This should be carefully reconsidered to adhere to the principle of least privilege. A minor documentation improvement is also suggested.
| namespace: {{ include "scalar-manager.headlampServiceAccountNamespace" . }} | ||
| roleRef: | ||
| kind: ClusterRole | ||
| name: cluster-admin |
There was a problem hiding this comment.
Granting cluster-admin privileges to the Headlamp service account introduces a significant security risk. This provides unrestricted access to the entire cluster, and a compromise of this service account's token could lead to a full cluster compromise. It is highly recommended to follow the principle of least privilege and define a more restrictive ClusterRole with only the permissions that Headlamp requires.
There was a problem hiding this comment.
To be able to fully utilize the Headlamp features, the service account that is used for Headlamp should be bound to the cluster-admin role, so we did that on purpose.
There was a problem hiding this comment.
Regarding this point, I have three questions:
1. The number of Service Accounts used for access to Headlamp
Please let me confirm the design of the access pattern for Headlamp through Scalar Manager from the user role perspective.
Which of the following is the designed behavior in Scalar Manager?
-
Pattern 1 (Share one service account in all Scalar Manager users)
+---[Scalar Manager]------------------+ | | [User A (ADMINISTRATOR)]---+ | | | | | [User B (WRITER)]----------+--->+---[Headlamp Integration Feature]--->+---(Share one SA with `cluster-admin`)--->[Headlamp]---(Monitor Kubernetes Resources)--->[Kubernetes] | | | [User C (READER)]----------+ | | | | +-------------------------------------+ -
Pattern 2 (Generate dedicated service account for each Scalar Manager user)
+---[Scalar Manager]------------------+ | | [User A (ADMINISTRATOR)]------->+---[Headlamp Integration Feature]--->+---(Dedicated SA `user-a` with `cluster-admin`)---+ | | | [User B (WRITER)]-------------->+---[Headlamp Integration Feature]--->+---(Dedicated SA `user-b` with `edit`)------------+--->[Headlamp]---(Monitor Kubernetes Resources)--->[Kubernetes] | | | [User C (READER)]-------------->+---[Headlamp Integration Feature]--->+---(Dedicated SA `user-c` with `view`)------------+ | | +-------------------------------------+ -
Pattern 3 (Generate shared service account for each Scalar Manager role)
+---[Scalar Manager]------------------+ | | [User A (ADMINISTRATOR)]------->+---[Headlamp Integration Feature]--->+---(Shared SA `scalar-manager-admin` with `cluster-admin`)----+ | | | [User B (WRITER)]-------------->+---[Headlamp Integration Feature]--->+---(Shared SA `scalar-manager-writer` with `edit`)------------+ | | | [User C (WRITER)]-------------->+---[Headlamp Integration Feature]--->+---(Shared SA `scalar-manager-writer` with `edit`)------------+--->[Headlamp]---(Monitor Kubernetes Resources)--->[Kubernetes] | | | [User D (READER)]-------------->+---[Headlamp Integration Feature]--->+---(Shared SA `scalar-manager-reader` with `view`)------------+ | | | [User E (READER)]-------------->+---[Headlamp Integration Feature]--->+---(Shared SA `scalar-manager-reader` with `view`)------------+ | | +-------------------------------------+
2. The default role for access to Headlamp
It seems that the service account for Headlamp has the cluster-admin role. However, I think it's too strong as a default value.
Especially, if the designed access pattern in Q1 is Pattern 1 (Share one service account in all Scalar Manager users), normal users, even though READER, have full access, including any update operations of the Kubernetes cluster through Headlamp.
I think it's not good from the perspective of the principle of least privilege. It seems that similar discussions exist on the Headlamp side as follows:
- Insecure default: Helm chart grants cluster-admin to the Pod automatically kubernetes-sigs/headlamp#4435
- charts: improve security and naming of default ClusterRoleBinding kubernetes-sigs/headlamp#4437
- charts: Remove ClusterRoleBinding default kubernetes-sigs/headlamp#4551
Therefore, I think it would be better to:
- Set the
viewrole as a default, and make it configurable in thevalues.yamlfile.- I think we should also discuss whether a configurable role is really necessary or not.
- Or, define fine-grained custom roles for Scalar Manager users based on Scalar Manager roles,
ADMINISTRATOR,WRITER, andREADER.
Also, users can use the generated token on the Scalar Manager page to access the Kubernetes cluster through other tools like the kubectl command. Therefore, we need to treat the generated token very carefully. Sorry, I overlooked this point when I talked about the rough design before, but I think we should reconsider this point again.
3. The purpose of Headlamp integration in Scalar Manager
This is related to the 2. The default role for access to Headlamp, but I'd like to confirm the purpose of the Headlamp integration.
I think Scalar Manager is almost a monitoring tool, for now. And, I think we decided to integrate Headlamp as an alternative to the CPU or Memory panel at the top page of Scalar Manager that existed in the prior implementation. In such a case, I think it's enough to provide integration with Headlamp only with read operations. In other words, I think we don't need to provide the way to create, update, and delete operations for Kubernetes resources through Scalar Manager.
I'd like to clarify the goal of the Headlamp integration and consider what role we need carefully. What is the purpose of this Headlamp integration?
There was a problem hiding this comment.
@feeblefakie
As I shared in the meeting, I think we need additional discussions about design based on this thread.
Please let me discuss these points with you and the middleware team later. 🙏
thongdk8
force-pushed
the
feat/sm-add-headlamp
branch
from
49245e9 to
55343e7
Compare
Add configuration options for integrating with Headlamp Kubernetes UI: - Service discovery configuration for locating Headlamp services - Dedicated ServiceAccount with cluster-admin for token generation - Environment variables for API and Web containers - Schema and documentation updates
thongdk8
force-pushed
the
feat/sm-add-headlamp
branch
from
55343e7 to
6dfc067
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds Headlamp integration support to the Scalar Manager Helm chart, allowing Scalar Manager to embed Headlamp (a Kubernetes web UI) for cluster visualization and management capabilities. The integration is disabled by default and requires explicit enablement.
Changes:
- Added comprehensive Headlamp configuration section in
values.yamlcovering service discovery, service account settings, and proxy configuration - Created dedicated ServiceAccount and ClusterRoleBinding for Headlamp with cluster-admin privileges
- Added environment variables to API and Web containers for service discovery and proxy configuration
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| charts/scalar-manager/values.yaml | Added headlamp configuration section with kubernetes discovery, serviceAccount, and web proxy settings |
| charts/scalar-manager/values.schema.json | Added JSON schema validation for headlamp configuration structure |
| charts/scalar-manager/templates/_helpers.tpl | Added helper templates for Headlamp ServiceAccount name and namespace resolution |
| charts/scalar-manager/templates/scalar-manager/serviceaccount.yaml | Added conditional ServiceAccount creation for Headlamp |
| charts/scalar-manager/templates/scalar-manager/clusterrolebinding.yaml | Added ClusterRoleBinding granting cluster-admin role to Headlamp ServiceAccount |
| charts/scalar-manager/templates/scalar-manager/clusterrole.yaml | Added permission to create tokens for Headlamp ServiceAccount |
| charts/scalar-manager/templates/scalar-manager/deployment.yaml | Added environment variables for Headlamp configuration to API and Web containers |
| charts/scalar-manager/README.md | Auto-generated documentation updates for new Headlamp configuration options |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| subjects: | ||
| - kind: ServiceAccount | ||
| name: {{ include "scalar-manager.headlampServiceAccountName" . }} | ||
| namespace: {{ include "scalar-manager.headlampServiceAccountNamespace" . }} |
There was a problem hiding this comment.
The ServiceAccount subject is missing the apiGroup field. For consistency with the existing ClusterRoleBinding in this file (line 11), add 'apiGroup: ""' to the subject specification. While this field is optional for ServiceAccounts and defaults to an empty string, it's better to be explicit for consistency and clarity.
| namespace: {{ include "scalar-manager.headlampServiceAccountNamespace" . }} | |
| namespace: {{ include "scalar-manager.headlampServiceAccountNamespace" . }} | |
| apiGroup: "" |
There was a problem hiding this comment.
We have specified in the cluster roles already
There was a problem hiding this comment.
To keep the consistency in this chart, could you please add apiGroup: "" for now?
If we omit apiGroup: "" here, I think we should apply the same thing in other places in this chart.
| {{- if .Values.scalarManager.headlamp.enabled }} | ||
| # Permission to create tokens for Headlamp ServiceAccount | ||
| - apiGroups: [""] | ||
| resources: ["serviceaccounts/token"] | ||
| verbs: ["create"] | ||
| resourceNames: ["{{ include "scalar-manager.headlampServiceAccountName" . }}"] | ||
| {{- end }} |
There was a problem hiding this comment.
The ClusterRole adds token creation permission for the Headlamp ServiceAccount whenever headlamp is enabled, but doesn't verify that the ServiceAccount is actually being created. If headlamp.enabled is true but serviceAccount.create is false and no custom serviceAccount.name is provided, this could result in referencing a non-existent ServiceAccount in the resourceNames field.
Consider updating the condition to check both enabled and create flags, or add validation to ensure a ServiceAccount name is provided when create is false.
There was a problem hiding this comment.
We can refer to an existing service account as well. This is made on purpose
There was a problem hiding this comment.
It seems that we can set the following configuration and deploy Scalar Manager without any errors (i.e., we can run the helm install command without errors).
scalarManager:
headlamp:
enabled: true
serviceAccount:
create: false
name: ""However, in the above deployment, it seems that the Generate Token button shows an error when I click it on the UI.
Could you please confirm whether this behavior is expected or not?
(I will share more details in internal Slack later.)
kota2and3kan
left a comment
kota2and3kan
left a comment
There was a problem hiding this comment.
Thank you for the updates!
I left several comments, and I think we need additional discussions about the design of Scalar Manager itself.
Could you please take a look when you have time? 🙇
| {{- if .Values.scalarManager.headlamp.enabled }} | ||
| # Permission to create tokens for Headlamp ServiceAccount | ||
| - apiGroups: [""] | ||
| resources: ["serviceaccounts/token"] | ||
| verbs: ["create"] | ||
| resourceNames: ["{{ include "scalar-manager.headlampServiceAccountName" . }}"] | ||
| {{- end }} |
There was a problem hiding this comment.
It seems that we can set the following configuration and deploy Scalar Manager without any errors (i.e., we can run the helm install command without errors).
scalarManager:
headlamp:
enabled: true
serviceAccount:
create: false
name: ""However, in the above deployment, it seems that the Generate Token button shows an error when I click it on the UI.
Could you please confirm whether this behavior is expected or not?
(I will share more details in internal Slack later.)
| subjects: | ||
| - kind: ServiceAccount | ||
| name: {{ include "scalar-manager.headlampServiceAccountName" . }} | ||
| namespace: {{ include "scalar-manager.headlampServiceAccountNamespace" . }} |
There was a problem hiding this comment.
To keep the consistency in this chart, could you please add apiGroup: "" for now?
If we omit apiGroup: "" here, I think we should apply the same thing in other places in this chart.
| namespace: {{ include "scalar-manager.headlampServiceAccountNamespace" . }} | ||
| roleRef: | ||
| kind: ClusterRole | ||
| name: cluster-admin |
There was a problem hiding this comment.
Regarding this point, I have three questions:
1. The number of Service Accounts used for access to Headlamp
Please let me confirm the design of the access pattern for Headlamp through Scalar Manager from the user role perspective.
Which of the following is the designed behavior in Scalar Manager?
-
Pattern 1 (Share one service account in all Scalar Manager users)
+---[Scalar Manager]------------------+ | | [User A (ADMINISTRATOR)]---+ | | | | | [User B (WRITER)]----------+--->+---[Headlamp Integration Feature]--->+---(Share one SA with `cluster-admin`)--->[Headlamp]---(Monitor Kubernetes Resources)--->[Kubernetes] | | | [User C (READER)]----------+ | | | | +-------------------------------------+ -
Pattern 2 (Generate dedicated service account for each Scalar Manager user)
+---[Scalar Manager]------------------+ | | [User A (ADMINISTRATOR)]------->+---[Headlamp Integration Feature]--->+---(Dedicated SA `user-a` with `cluster-admin`)---+ | | | [User B (WRITER)]-------------->+---[Headlamp Integration Feature]--->+---(Dedicated SA `user-b` with `edit`)------------+--->[Headlamp]---(Monitor Kubernetes Resources)--->[Kubernetes] | | | [User C (READER)]-------------->+---[Headlamp Integration Feature]--->+---(Dedicated SA `user-c` with `view`)------------+ | | +-------------------------------------+ -
Pattern 3 (Generate shared service account for each Scalar Manager role)
+---[Scalar Manager]------------------+ | | [User A (ADMINISTRATOR)]------->+---[Headlamp Integration Feature]--->+---(Shared SA `scalar-manager-admin` with `cluster-admin`)----+ | | | [User B (WRITER)]-------------->+---[Headlamp Integration Feature]--->+---(Shared SA `scalar-manager-writer` with `edit`)------------+ | | | [User C (WRITER)]-------------->+---[Headlamp Integration Feature]--->+---(Shared SA `scalar-manager-writer` with `edit`)------------+--->[Headlamp]---(Monitor Kubernetes Resources)--->[Kubernetes] | | | [User D (READER)]-------------->+---[Headlamp Integration Feature]--->+---(Shared SA `scalar-manager-reader` with `view`)------------+ | | | [User E (READER)]-------------->+---[Headlamp Integration Feature]--->+---(Shared SA `scalar-manager-reader` with `view`)------------+ | | +-------------------------------------+
2. The default role for access to Headlamp
It seems that the service account for Headlamp has the cluster-admin role. However, I think it's too strong as a default value.
Especially, if the designed access pattern in Q1 is Pattern 1 (Share one service account in all Scalar Manager users), normal users, even though READER, have full access, including any update operations of the Kubernetes cluster through Headlamp.
I think it's not good from the perspective of the principle of least privilege. It seems that similar discussions exist on the Headlamp side as follows:
- Insecure default: Helm chart grants cluster-admin to the Pod automatically kubernetes-sigs/headlamp#4435
- charts: improve security and naming of default ClusterRoleBinding kubernetes-sigs/headlamp#4437
- charts: Remove ClusterRoleBinding default kubernetes-sigs/headlamp#4551
Therefore, I think it would be better to:
- Set the
viewrole as a default, and make it configurable in thevalues.yamlfile.- I think we should also discuss whether a configurable role is really necessary or not.
- Or, define fine-grained custom roles for Scalar Manager users based on Scalar Manager roles,
ADMINISTRATOR,WRITER, andREADER.
Also, users can use the generated token on the Scalar Manager page to access the Kubernetes cluster through other tools like the kubectl command. Therefore, we need to treat the generated token very carefully. Sorry, I overlooked this point when I talked about the rough design before, but I think we should reconsider this point again.
3. The purpose of Headlamp integration in Scalar Manager
This is related to the 2. The default role for access to Headlamp, but I'd like to confirm the purpose of the Headlamp integration.
I think Scalar Manager is almost a monitoring tool, for now. And, I think we decided to integrate Headlamp as an alternative to the CPU or Memory panel at the top page of Scalar Manager that existed in the prior implementation. In such a case, I think it's enough to provide integration with Headlamp only with read operations. In other words, I think we don't need to provide the way to create, update, and delete operations for Kubernetes resources through Scalar Manager.
I'd like to clarify the goal of the Headlamp integration and consider what role we need carefully. What is the purpose of this Headlamp integration?
| name: {{ include "scalar-manager.fullname" . }}-headlamp-cluster-admin | ||
| labels: | ||
| {{- include "scalar-manager.labels" . | nindent 4 }} | ||
| app.kubernetes.io/component: headlamp |
There was a problem hiding this comment.
Just a question. What is the purpose of this label? When and where do we use this label?
Please let me confirm the purpose of this label, since I couldn't find a clear reason to set this label here from this PR.
| # -- Kubernetes service discovery configuration (used by API) | ||
| kubernetes: | ||
| # -- Label name to identify Headlamp service | ||
| serviceLabelName: "app.kubernetes.io/name" | ||
| # -- Label value to identify Headlamp service | ||
| serviceLabelValue: "headlamp" | ||
| # -- Port name of the Headlamp service | ||
| servicePortName: "http" |
There was a problem hiding this comment.
I have two questions about the Headlamp discovery:
1. In case of multiple Headlamp deployments
I think we assume that the Headlamp deployment is only one per Kubernetes cluster, but I think there is a possibility that multiple Headlamp deployments exist in one Kubernetes cluster.
What will happen in the Scalar Manager's Headlamp discovery feature if there are two or more deployments of Headlamp in one Kubernetes cluster?
2. Method of Headlamp detection
It's a bit related to Q1, but I think it might be better to set the Headlamp information explicitly instead of the dynamic discovery based on the labels, because we assume that we use only one Headlamp deployment.
If we can detect the Headlamp deployment without any configurations like label name and label value, it is worth providing such a feature because customers don't need to care about the Headlamp-related configurations.
However, users need to set the label name and label value in the current implementations. In such a case, I think it might be better to set a more explicit configuration, for example, namespace and service name, instead of dynamic detection.
In such implementations, I think we can also avoid unexpected issues when multiple Headlamp deployments exist in one Kubernetes cluster.
What do you think?
3 participants
Description
This PR adds Headlamp integration support to the Scalar Manager Helm chart. Headlamp is a Kubernetes web UI that can be embedded within Scalar Manager to provide cluster visualization and management capabilities.
Related issues and/or PRs
N/A
Changes made
scalarManager.headlampconfiguration section invalues.yamlwith:enabledflag to toggle Headlamp integrationkubernetessettings for service discovery (label name/value, port name)serviceAccountsettings for token generationwebsettings for proxy configuration (base path, namespace, cache TTL)_helpers.tplvalues.schema.jsonwith Headlamp configuration schemaChecklist
Additional notes (optional)
NA
Release notes
Added Headlamp integration support to Scalar Manager for embedded Kubernetes cluster visualization.