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!: truncate fullname after 22 characters #3248

Merged
merged 1 commit into from
Sep 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .changelog/3248.breaking.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
feat!: truncate fullname after 22 characters
2 changes: 1 addition & 1 deletion deploy/helm/sumologic/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ The following table lists the configurable parameters of the Sumo Logic chart an
| Parameter | Description | Default |
| ------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
| `nameOverride` | Used to override the Chart name. | `Nil` |
| `fullnameOverride` | Used to override the chart's full name. | `Nil` |
| `fullnameOverride` | Used to override the chart's full name. Names longer than 22 characters will be truncated. | `Nil` |
| `namespaceOverride` | Used to override the chart's default target namepace. | `Nil` |
| `sumologic.setupEnabled` | If enabled, a pre-install hook will create Collector and Sources in Sumo Logic. | `true` |
| `sumologic.cleanupEnabled` | If enabled, a pre-delete hook will destroy Kubernetes secret and Sumo Logic Collector. | `false` |
Expand Down
4 changes: 0 additions & 4 deletions deploy/helm/sumologic/templates/NOTES.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,6 @@ Thank you for installing {{ .Chart.Name }}.
WARNING: You defined sumologic.clusterName with spaces, which is not supported. Spaces have been replaced with dashes.
{{- end }}

{{- if gt (len .Release.Name) 19 }}
WARNING: Your release name is longer than 19 characters. Some features may not work due length limits on Kubernetes labels. This limit will be enforced explicitly in the next major release.
{{- end }}

A Collector with the name {{ .Values.sumologic.collectorName | default (include "sumologic.clusterNameReplaceSpaceWithDash" . ) | quote }} has been created in your Sumo Logic account.

Check the release status by running:
Expand Down
7 changes: 4 additions & 3 deletions deploy/helm/sumologic/templates/_helpers/_common.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,15 @@ Expand the name of the chart.

{{/*
Create a default fully qualified app name.
Copy link

Choose a reason for hiding this comment

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

I think we want to make the same change for sumologic.name above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it used anywhere? I grepped for that name and it seems that it's not used anywhere

Copy link

Choose a reason for hiding this comment

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

Huh, we should delete it then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do it in another pr

We truncate at 63 chars because some Kubernetes name fields are limited to this (by the DNS naming spec).
We truncate at 22 chars because some Kubernetes name fields are limited to 63 characters (by the DNS naming spec).
In particular, some statefulsets will have too long names if the name is longer than 22 characters.
*/}}
{{- define "sumologic.fullname" -}}
{{- if .Values.fullnameOverride }}
{{- .Values.fullnameOverride | trunc 63 | trimSuffix "-" }}
{{- .Values.fullnameOverride | trunc 22 | trimSuffix "-" }}
{{- else }}
{{- $name := default .Chart.Name .Values.nameOverride -}}
{{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" }}
{{- printf "%s-%s" .Release.Name $name | trunc 22 | trimSuffix "-" }}
{{- end -}}
{{- end -}}

Expand Down
8 changes: 7 additions & 1 deletion docs/v4-migration-doc.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@

<!-- TOC -->

- [Kubernetes Collection v4.0.0 - Breaking Changes](#kubernetes-collection-v400---breaking-changes)
- [Kubernetes Collection `v4.0.0` - Breaking Changes](#kubernetes-collection-v400---breaking-changes)
- [Important changes](#important-changes)
- [OpenTelemetry Collector](#opentelemetry-collector)
- [Drop Prometheus recording rule metrics](#drop-prometheus-recording-rule-metrics)
- [How to upgrade](#how-to-upgrade)
- [Requirements](#requirements)
- [Metrics migration](#metrics-migration)
Expand Down Expand Up @@ -156,3 +157,8 @@ require additional action.
- node_network_transmit_bytes_total
- node_filesystem_avail_bytes
- node_filesystem_size_bytes

- Truncating full name to 22 characters

Some Kubernetes objects, for example statefulsets, have a tight (63 characters) limit for their names. Because of that, we truncate the
prefix that is attached to the names. In particular, the value under key `fullnameOverride` will be truncated to 22 characters.
25 changes: 0 additions & 25 deletions tests/helm/common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -172,31 +172,6 @@ func TestNameAndLabelLength(t *testing.T) {
}
}

func TestReleaseNameTooLong(t *testing.T) {
valuesFilePath := path.Join(testDataDirectory, "everything-enabled.yaml")
releaseName := strings.Repeat("a", maxHelmReleaseNameLength+1)
_, err := RenderTemplateE(
t,
&helm.Options{
ValuesFiles: []string{valuesFilePath},
SetStrValues: map[string]string{
"sumologic.accessId": "accessId",
"sumologic.accessKey": "accessKey",
},
Logger: logger.Discard, // the log output is noisy and doesn't help much
},
chartDirectory,
releaseName,
[]string{},
true,
"--namespace",
defaultNamespace,
)
require.NoError(t, err)
// we can't check whether NOTES are rendered correctly due to https://github.com/helm/helm/issues/6901
// TODO: Add an error check here after we start enforcing the limit: https://github.com/SumoLogic/sumologic-kubernetes-collection/issues/3057
}

// check the built-in labels added to all K8s objects created by the chart
func checkBuiltinLabels(t *testing.T, object metav1.Object, chartVersion string) {
labels := object.GetLabels()
Expand Down
4 changes: 2 additions & 2 deletions tests/helm/const.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@ const (
yamlDirectory = "static"
chartDirectory = "../../deploy/helm/sumologic"
chartName = "sumologic"
releaseName = "collection-test"
releaseName = "col-test"
Copy link

Choose a reason for hiding this comment

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

Why is this change necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Else we get such failures: https://github.com/SumoLogic/sumologic-kubernetes-collection/actions/runs/6109112931/job/16579492782
and I found it easier to modify the constant instead of modifying the tests

Copy link

Choose a reason for hiding this comment

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

But why does this change cause these failures. collection-test is less than 22 characters, so it shouldn't make any difference, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But -sumologic is added to that. And as a result we truncate collection-test-sumologic, which is more than 22 chars.

defaultNamespace = "sumologic"
defaultK8sVersion = "1.26.0"
testDataDirectory = "./testdata"
otelConfigFileName = "config.yaml"
otelImageFIPSSuffix = "-fips"
otelContainerName = "otelcol"
maxHelmReleaseNameLength = 19 // Helm allows up to 53, this is our own limit
maxHelmReleaseNameLength = 22 // Helm allows up to 53, but for a name longer than 22 some statefulset names will be too long
k8sMaxNameLength = 253 // see https://kubernetes.io/docs/concepts/overview/working-with-objects/names/
k8sMaxLabelLength = 63 // see https://kubernetes.io/docs/concepts/overview/working-with-objects/names/
)
Expand Down
7 changes: 7 additions & 0 deletions tests/helm/testdata/everything-enabled.yaml
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
sumologic:
logs:
collector:
allowSideBySide: true
otelcloudwatch:
enabled: true

metadata:
logs:
autoscaling:
Expand Down
6 changes: 5 additions & 1 deletion tests/integration/internal/strings/strings.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ import (
"time"
)

const (
maxReleaseNameLength = 12
)

func NameFromT(t *testing.T) string {
return strings.ReplaceAll(strings.ToLower(t.Name()), "_", "-")
}
Expand All @@ -29,5 +33,5 @@ func ValueFileFromT(t *testing.T) string {
func ReleaseNameFromT(t *testing.T) string {
h := fnv.New32a()
h.Write([]byte(t.Name()))
return fmt.Sprintf("rel-%d", h.Sum32())
return fmt.Sprintf("rel-%d", h.Sum32())[:maxReleaseNameLength]
}