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

Logic to generate default cluster_name if not supplied #80

Merged
merged 6 commits into from
Feb 8, 2024

Conversation

mitali-salvi
Copy link
Contributor

@mitali-salvi mitali-salvi commented Feb 5, 2024

Description of changes:

  1. Implement logic to generate a default cluster-name (generated using the release name) if not supplied in Helm chart. This cluster name will be passed down to the cloudwatch-agent and fluent-bit config
  2. Marking region as a required field. this region will be passed down to both the CWA and fluent-bit config

Testing
cluster_name is not provided, attaching snippets rather than the entire YAML

CWA --
  image: public.ecr.aws/cloudwatch-agent/cloudwatch-agent:1.300031.1b317
  mode: daemonset
  serviceAccount: cloudwatch-agent
  config: "{\"agent\":{\"region\":\"us-east-1\"},\"logs\":{\"metrics_collected\":{\"app_signals\":{\"hosted_in\":\"k8s-cluster-0ba3ba5\"},\"kubernetes\":{\"cluster_name\":\"k8s-cluster-0ba3ba5\",\"enhanced_container_insights\":true}}},\"traces\":{\"traces_collected\":{\"app_signals\":{}}}}"

FluentBit --
        image: public.ecr.aws/aws-observability/aws-for-fluent-bit:2.31.12.20230911
        imagePullPolicy: Always
        env:
        - name: AWS_REGION
          value: us-east-1
        - name: CLUSTER_NAME
          value: k8s-cluster-0ba3ba5

cluster_name is provided via the helm chart, attaching snippets rather than the entire YAML

helm upgrade --install  --debug --namespace amazon-cloudwatch amazon-cloudwatch-operator ./ --create-namespace --set clusterName=cloudwatchagent-operator --set region=us-east-1


CWA --
  image: public.ecr.aws/cloudwatch-agent/cloudwatch-agent:1.300031.1b317
  mode: daemonset
  serviceAccount: cloudwatch-agent
  config: "{\"logs\":{\"metrics_collected\":{\"app_signals\":{\"hosted_in\":\"k8s-cluster-0ba3ba5\"},\"kubernetes\":{\"cluster_name\":\"k8s-cluster-0ba3ba5\",\"enhanced_container_insights\":true}}},\"traces\":{\"traces_collected\":{\"app_signals\":{}}}}"

FluentBit --
        image: public.ecr.aws/aws-observability/aws-for-fluent-bit:2.31.12.20230911
        imagePullPolicy: Always
        env:
        - name: AWS_REGION
          value: us-east-1
        - name: CLUSTER_NAME
          value: "k8s-cluster-0ba3ba5"

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

lisguo
lisguo previously approved these changes Feb 6, 2024
helm/templates/_helpers.tpl Outdated Show resolved Hide resolved
helm/templates/_helpers.tpl Outdated Show resolved Hide resolved
helm/values.yaml Outdated Show resolved Hide resolved
lisguo
lisguo previously approved these changes Feb 7, 2024
@mitali-salvi mitali-salvi merged commit 0bb89bd into main Feb 8, 2024
4 checks passed
Comment on lines +12 to +15
{{- if empty .Values.clusterName }}
{{- default "" (printf "k8s-cluster-%s" (sha256sum .Release.Name | trunc 7)) }}
{{- else }}
{{- default "" .Values.clusterName }}
Copy link
Contributor

@sky333999 sky333999 Feb 9, 2024

Choose a reason for hiding this comment

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

Isnt lines 12-16 the same as

{{- default (printf "k8s-cluster-%s" (sha256sum .Release.Name | trunc 7)) .Values.clusterName }}

?

Comment on lines +1 to +3
{{- if empty .Values.region }}
{{- fail "region is a required field" }}
{{- end }}
Copy link
Contributor

@sky333999 sky333999 Feb 9, 2024

Choose a reason for hiding this comment

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

Using the built in required func, Lines 1-3 can instead be:

{{- $region := .Values.region | required ".Values.region is required." -}}

@@ -28,7 +31,7 @@ spec:
- name: AWS_REGION
value: {{ .Values.region }}
- name: CLUSTER_NAME
value: {{ .Values.clusterName | quote }}
value: {{ include "kubernetes-cluster.name" . }}
Copy link
Contributor

Choose a reason for hiding this comment

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

@movence doesnt this bring back the bug you fixed for numeric only cluster names not being quoted?

{{- end }}

{{/*
Helper function to modify cloudwatch-agent config
Copy link
Contributor

Choose a reason for hiding this comment

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

Whats diff between the two funcs below?
I would have expected a generic helper function that takes a config (either user provided or default) as an input and then just merges region & cluster name on top of that.

.Values.agent.config | injectRegionAndClusterName | toJson | quote
.Values.agent.defaultConfig | injectRegionAndClusterName | toJson | quote


## Provide the Region (optional when installing via EKS add-on)
region: AWS_REGION_NAME
## Provide the Region
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we keep the comments on these saying this is required and the cluster name is optional and we generate one if not provdied

*/}}
{{- define "kubernetes-cluster.name" -}}
{{- if empty .Values.clusterName }}
{{- default "" (printf "k8s-cluster-%s" (sha256sum .Release.Name | trunc 7)) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the release name unique enough for our purposes here? Is a hash good enough or do we need randomness?
If the same release is installed on multiple clusters with the same name, wont all the clusters have the same hash?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good point, and we should add some randomness to it.

@mitali-salvi mitali-salvi deleted the helm-auto-generate branch March 14, 2024 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants