Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add pyroscope to LGTM stack #124

Merged
merged 4 commits into from
Nov 10, 2024
Merged

feat: add pyroscope to LGTM stack #124

merged 4 commits into from
Nov 10, 2024

Conversation

saidsef
Copy link
Owner

@saidsef saidsef commented Nov 10, 2024

No description provided.

@saidsef saidsef self-assigned this Nov 10, 2024
@saidsef saidsef added the enhancement New feature or request label Nov 10, 2024
@saidsef
Copy link
Owner Author

saidsef commented Nov 10, 2024

/code pr

Copy link
Owner Author

@saidsef saidsef left a comment

Choose a reason for hiding this comment

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

🤖
I'll review the key changes in this PR:

  1. Updates to existing components:
- image: grafana/mimir:2.14.0
+ image: grafana/mimir:2.14.1

- image: docker.io/prom/prometheus:v2.55.0
+ image: docker.io/prom/prometheus:v2.55.1

Minor version bumps for Mimir and Prometheus which look reasonable.

  1. New Pyroscope deployment:
  • Configuration looks well-structured with appropriate security settings:
    securityContext:
      runAsNonRoot: true
      runAsGroup: 65534
      runAsUser: 65534
  • Good resource limits set:
    resources:
      requests:
        cpu: 100m
        memory: "512Mi"
      limits:
        cpu: 200m
        memory: "1Gi"

Suggestions:

  1. deployment/pyroscope/rbac.yml: Consider adding namespace to the ClusterRoleBinding subject section
  2. deployment/pyroscope/deployment.yml: Add liveness/readiness probes
  3. Consider adding network policies to restrict traffic

Overall, this is a solid addition of Pyroscope to the monitoring stack with good security practices and resource management in place.

> claude-3-5-sonnet-20241022 [pr | 2]

Comment on lines +32 to +80
- name: pyroscope
image: docker.io/grafana/pyroscope:1.10.0
imagePullPolicy: IfNotPresent
args:
- "-target=all"
- "-self-profiling.disable-push=true"
- "-server.http-listen-port=4040"
- "-memberlist.cluster-label=pyroscope"
- "-memberlist.join=dns+pyroscope:7946"
- "-config.file=/etc/pyroscope/config.yaml"
- "-runtime-config.file=/etc/pyroscope/overrides/overrides.yaml"
- "-log.level=info"
ports:
- name: http
containerPort: 4040
protocol: TCP
- name: memberlist
containerPort: 7946
protocol: TCP
resources:
requests:
cpu: 100m
memory: "512Mi"
limits:
cpu: 200m
memory: "1Gi"
volumeMounts:
- name: config
mountPath: /etc/pyroscope/config.yaml
subPath: config.yaml
- name: config
mountPath: /etc/pyroscope/overrides/
- name: storage
mountPath: /data
- name: storage
mountPath: /data-compactor
subPath: data-compactor
- name: storage
mountPath: /data-shared
subPath: data-shared
securityContext:
allowPrivilegeEscalation: false
privileged: false
readOnlyRootFilesystem: true
seccompProfile:
type: RuntimeDefault
capabilities:
drop:
- ALL

Check notice

Code scanning / Trivy

Runs with UID <= 10000 Low

Artifact: deployment/pyroscope/deployment.yml
Type: kubernetes
Vulnerability KSV020
Severity: LOW
Message: Container 'pyroscope' of Deployment 'pyroscope' should set 'securityContext.runAsUser' > 10000
Link: KSV020
Comment on lines +32 to +80
- name: pyroscope
image: docker.io/grafana/pyroscope:1.10.0
imagePullPolicy: IfNotPresent
args:
- "-target=all"
- "-self-profiling.disable-push=true"
- "-server.http-listen-port=4040"
- "-memberlist.cluster-label=pyroscope"
- "-memberlist.join=dns+pyroscope:7946"
- "-config.file=/etc/pyroscope/config.yaml"
- "-runtime-config.file=/etc/pyroscope/overrides/overrides.yaml"
- "-log.level=info"
ports:
- name: http
containerPort: 4040
protocol: TCP
- name: memberlist
containerPort: 7946
protocol: TCP
resources:
requests:
cpu: 100m
memory: "512Mi"
limits:
cpu: 200m
memory: "1Gi"
volumeMounts:
- name: config
mountPath: /etc/pyroscope/config.yaml
subPath: config.yaml
- name: config
mountPath: /etc/pyroscope/overrides/
- name: storage
mountPath: /data
- name: storage
mountPath: /data-compactor
subPath: data-compactor
- name: storage
mountPath: /data-shared
subPath: data-shared
securityContext:
allowPrivilegeEscalation: false
privileged: false
readOnlyRootFilesystem: true
seccompProfile:
type: RuntimeDefault
capabilities:
drop:
- ALL

Check notice

Code scanning / Trivy

Runs with GID <= 10000 Low

Artifact: deployment/pyroscope/deployment.yml
Type: kubernetes
Vulnerability KSV021
Severity: LOW
Message: Container 'pyroscope' of Deployment 'pyroscope' should set 'securityContext.runAsGroup' > 10000
Link: KSV021
github-actions[bot]
github-actions bot previously approved these changes Nov 10, 2024
github-actions[bot]
github-actions bot previously approved these changes Nov 10, 2024
github-actions[bot]
github-actions bot previously approved these changes Nov 10, 2024
@saidsef
Copy link
Owner Author

saidsef commented Nov 10, 2024

/code pr

Copy link
Owner Author

@saidsef saidsef left a comment

Choose a reason for hiding this comment

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

🤖

Summary of New Suggestions for Enhancements

  1. Pyroscope Configuration:

    • The config.river section in deployment/pyroscope/cm.yml provides detailed logging and discovery configurations for Pyroscope. This includes setting up logging levels, formats, and Kubernetes discovery rules. The configuration also handles profiling configurations for memory, CPU, goroutine, block, mutex, and fgprof profiles.
    data:
      config.river: |
        logging {
          level  = "info"
          format = "logfmt"
        }
    
        discovery.kubernetes "pyroscope_kubernetes" {
          role = "pod"
        }
    
        // Example annotations for scraping profiles:
        // profiles.grafana.com/memory.scrape: "true"
        // profiles.grafana.com/memory.port: "8080"
        // profiles.grafana.com/cpu.scrape: "true"
        // profiles.grafaana.com/cpu.port: "8080"
        // profiles.grafana.com/goroutine.scrape: "true"
        // profiles.grafana.com/goroutine.port: "8080"
    
        discovery.relabel "kubernetes_pods_memory_default_name" {
          targets = concat(discovery.kubernetes.pyroscope_kubernetes.targets)
    
          rule {
            action        = "drop"
            source_labels = ["__meta_kubernetes_pod_phase"]
            regex         = "Pending|Succeeded|Failed|Completed"
          }
    
          rule {
            action = "labelmap"
            regex  = "__meta_kubernetes_pod_label_(.+)"
          }
    
          rule {
            action        = "replace"
            source_labels = ["__meta_kubernetes_namespace"]
            target_label  = "namespace"
          }
    
          rule {
            action        = "replace"
            source_labels = ["__meta_kubernetes_pod_name"]
            target_label  = "pod"
          }
    
          rule {
            action        = "replace"
            source_labels = ["__meta_kubernetes_pod_container_name"]
            target_label  = "container"
          }
        }
    
        // More relabeling rules for other profiles...
  2. Deployment Configuration:

    • The deployment/pyroscope/deployment.yml file defines a deployment for Pyroscope with specific configurations such as port mappings, resource requests/limits, and security context settings.
    apiVersion: apps/v1
    kind: Deployment
    metadata:
      name: pyroscope
      labels:
        app.kubernetes.io/name: pyroscope
      annotations:
        prometheus.io/port: http
        prometheus.io/scrape: "true"
    
    spec:
      revisionHistoryLimit: 1
      replicas: 1
      selector:
        matchLabels:
          app.kubernetes.io/name: pyroscope
    
      template:
        metadata:
          labels:
            app.kubernetes.io/name: pyroscope
    
          annotations:
            prometheus.io/scrape: "true"
            prometheus.io/port: http
    
        spec:
          automountServiceAccountToken: false
          serviceAccountName: pyroscope
    
          securityContext:
            runAsNonRoot: true
            runAsGroup: 10001
            runAsUser: 10001
    
          containers:
          - name: pyroscope
            image: docker.io/grafana/pyroscope:1.10.0
    
            imagePullPolicy: IfNotPresent
    
            args:
              - "-target=all"
              - "-self-profiling.disable-push=true"
              - "-server.http-listen-port=4040"
              - "-memberlist.cluster-label=pyroscope"
              - "-memberlist.join=dns+pyroscope:7946"
              - "-config.file=/etc/pyroscope/config.yaml"
              - "-runtime-config.file=/etc/pyroscope/overrides/overrides.yaml"
              - "-log.level=info"
    
            ports:
              - name: http
                containerPort: 4040
                protocol: TCP
    
              - name: memberlist
                containerPort: 7946
                protocol: TCP
    
            resources:
              requests:
                cpu: 100m
                memory: "512Mi"
    
              limits:
                cpu: 200m
                memory: "1Gi"
    
            volumeMounts:
              - name: config
                mountPath: /etc/pyroscope/config.yaml
                subPath: config.yaml
    
              - name: config
                mountPath: /etc/pyroscope/overrides/
    
              - name: storage
                mountPath: /data
    
              - name: storage
                mountPath: /data-compactor
                subPath: data-compactor
    
              - name: storage
                mountPath: /data-shared
                subPath: data-shared
    
            securityContext:
              allowPrivilegeEscalation: false
              privileged: false
              readOnlyRootFilesystem: true
              seccompProfile:
                type: RuntimeDefault
    
              capabilities:
                drop:
                  - ALL
    
      volumes:
        - name: config
          configMap:
            name: config
    
        - name: storage
          emptyDir:
            sizeLimit: "2Gi"
  3. RBAC Configuration:

    • The deployment/pyroscope/rbac.yml file defines a ClusterRole and ClusterRoleBinding for Pyroscope, granting necessary permissions to the service account.
    apiVersion: rbac.authorization.k8s.io/v1
    kind: ClusterRole
    metadata:
      name: pyroscope
      labels:
        app.kubernetes.io/name: pyroscope
    
    rules:
      - apiGroups:
          - ""
        resources:
          - pods
        verbs:
          - list
          - watch
    
      - apiGroups:
          - ""
        resources:
          - nodes
        verbs:
          - get
    
    ---
    apiVersion: rbac.authorization.k8s.io/v1
    kind: ClusterRoleBinding
    metadata:
      name: pyroscope
      labels:
        app.kubernetes.io/name: pyroscope
    
    roleRef:
      apiGroup: rbac.authorization.k8s.io
      kind: ClusterRole
      name: pyroscope
    
    subjects:
      - kind: ServiceAccount
        name: pyroscope
        namespace: monitoring

Issues

  1. Security Context:

    • The runAsUser and runAsGroup settings in deployment/pyroscope/deployment.yml have been updated to values greater than 10000, which might not be necessary or could potentially cause issues if not properly configured. It's important to ensure these values align with your security policies and do not introduce vulnerabilities.
    securityContext:
      runAsNonRoot: true
      runAsGroup: 10001
      runAsUser: 10001
  2. Temp Config Removal:

    • The patch [PATCH 2/4] fix: temp config removed autocomplete_filtering_enabled removes a configuration option from deployment/tempo/cm.yml. This change might be necessary if the feature is no longer required or if it caused issues.
    data:
      tempo.yaml: |
        target: all
        # Removed line:
        # autocomplete_filtering_enabled: true
        multitenancy_enabled: false
        usage_report:
          reporting_enabled: true
  3. Traces Integration:

    • The patch [PATCH 4/4] feat: add traces to profiles to temp config adds integration with traces in deployment/grafana/cm.yml. This enhancement allows tracing data to be integrated with profiles, which could be useful for comprehensive profiling.
    data:
      tracesToLogsV2:
        datasourceUid: Loki
      tracesToProfiles:
        datasourceUid: Pyroscope
      tracesToMetrics:
        datasourceUid: Prometheus
      serviceMap:

Conclusion

The patches introduce significant enhancements to the deployment configuration, including detailed Pyroscope configuration, updated security context settings, and integration with traces. These changes should be reviewed carefully to ensure they align with your project's requirements and security standards.

> llama-3.1-sonar-small-128k-chat [pr | 0]

@saidsef saidsef merged commit 6cabb75 into main Nov 10, 2024
5 checks passed
@saidsef saidsef deleted the pyroscope branch November 10, 2024 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant