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.

Sign up for GitHub

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

Fix annotations propagation #1505

Closed
wants to merge 3 commits into from
Closed

Fix annotations propagation #1505

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b42583f
Fix annotations propagation
yuriolisa Feb 23, 2023
c626e67
Fix annotations propagation
yuriolisa Feb 23, 2023
0710b0b
Adding new chlog
yuriolisa Feb 23, 2023
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
16 changes: 16 additions & 0 deletions .chloggen/fix_propagate-metadata-annotations.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: 'bug_fix'

# The name of the component, or a single word describing the area of concern, (e.g. operator, target allocator, github action)
component: operator

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: "Propagate annotations to Deployment's PodTemplate"

# One or more tracking issues related to the change
issues: [900]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext:
16 changes: 0 additions & 16 deletions pkg/collector/annotations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,22 +43,6 @@ func Annotations(instance v1alpha1.OpenTelemetryCollector) map[string]string {
return annotations
}

// PodAnnotations return the spec annotations for OpenTelemetryCollector pod.
func PodAnnotations(instance v1alpha1.OpenTelemetryCollector) map[string]string {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder why the pod annotations were separated from the others. Did you try to look it up in the history?

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 took a look at the history, but it seems this behavior was a try to fix a bug related to reconciling the collector once the configmap changes. However, I don't know why it wasn't simply added the sha256 onto the Deployment annotations and replicate them to the podAnnotations.

Copy link
Member

Choose a reason for hiding this comment

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

#670

LGTM to get rid of it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think we could have a situation in which pods should have a specific annotation not present on Deployment's annotations?

// new map every time, so that we don't touch the instance's annotations
podAnnotations := map[string]string{}

// allow override of pod annotations
for k, v := range instance.Spec.PodAnnotations {
podAnnotations[k] = v
}

// make sure sha256 for configMap is always calculated
podAnnotations["opentelemetry-operator-config/sha256"] = getConfigMapSHA(instance.Spec.Config)

return podAnnotations
}

func getConfigMapSHA(config string) string {
h := sha256.Sum256([]byte(config))
return fmt.Sprintf("%x", h)
Expand Down
6 changes: 0 additions & 6 deletions pkg/collector/annotations_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,12 @@ func TestDefaultAnnotations(t *testing.T) {

// test
annotations := Annotations(otelcol)
podAnnotations := PodAnnotations(otelcol)

//verify
assert.Equal(t, "true", annotations["prometheus.io/scrape"])
assert.Equal(t, "8888", annotations["prometheus.io/port"])
assert.Equal(t, "/metrics", annotations["prometheus.io/path"])
assert.Equal(t, "9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08", annotations["opentelemetry-operator-config/sha256"])
assert.Equal(t, "9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08", podAnnotations["opentelemetry-operator-config/sha256"])
}

func TestUserAnnotations(t *testing.T) {
Expand All @@ -66,14 +64,12 @@ func TestUserAnnotations(t *testing.T) {

// test
annotations := Annotations(otelcol)
podAnnotations := PodAnnotations(otelcol)

//verify
assert.Equal(t, "false", annotations["prometheus.io/scrape"])
assert.Equal(t, "1234", annotations["prometheus.io/port"])
assert.Equal(t, "/test", annotations["prometheus.io/path"])
assert.Equal(t, "9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08", annotations["opentelemetry-operator-config/sha256"])
assert.Equal(t, "9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08", podAnnotations["opentelemetry-operator-config/sha256"])
}

func TestAnnotationsPropagateDown(t *testing.T) {
Expand All @@ -89,10 +85,8 @@ func TestAnnotationsPropagateDown(t *testing.T) {

// test
annotations := Annotations(otelcol)
podAnnotations := PodAnnotations(otelcol)

// verify
assert.Len(t, annotations, 5)
assert.Equal(t, "mycomponent", annotations["myapp"])
assert.Equal(t, "pod_annotation_value", podAnnotations["pod_annotation"])
}
3 changes: 1 addition & 2 deletions pkg/collector/daemonset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ func DaemonSet(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTelem
labels["app.kubernetes.io/name"] = naming.Collector(otelcol)

annotations := Annotations(otelcol)
podAnnotations := PodAnnotations(otelcol)
return appsv1.DaemonSet{
ObjectMeta: metav1.ObjectMeta{
Name: naming.Collector(otelcol),
Expand All @@ -46,7 +45,7 @@ func DaemonSet(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTelem
Template: corev1.PodTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
Labels: labels,
Annotations: podAnnotations,
Annotations: annotations,
},
Spec: corev1.PodSpec{
ServiceAccountName: ServiceAccountName(otelcol),
Expand Down
16 changes: 12 additions & 4 deletions pkg/collector/daemonset_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@ func TestDaemonSetNewDefault(t *testing.T) {
// verify sha256 podAnnotation
expectedAnnotations := map[string]string{
"opentelemetry-operator-config/sha256": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
"prometheus.io/path": "/metrics",
"prometheus.io/port": "8888",
"prometheus.io/scrape": "true",
}
assert.Equal(t, expectedAnnotations, d.Spec.Template.Annotations)

Expand Down Expand Up @@ -103,9 +106,17 @@ func TestDaemonsetHostNetwork(t *testing.T) {
func TestDaemonsetPodAnnotations(t *testing.T) {
// prepare
testPodAnnotationValues := map[string]string{"annotation-key": "annotation-value"}

// Add sha256 podAnnotation
testPodAnnotationValues["opentelemetry-operator-config/sha256"] = "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"
Copy link
Member

Choose a reason for hiding this comment

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

this can be moved up to the declaration of testPodAnnotationValues

testPodAnnotationValues["prometheus.io/path"] = "/metrics"
testPodAnnotationValues["prometheus.io/port"] = "8888"
testPodAnnotationValues["prometheus.io/scrape"] = "true"

otelcol := v1alpha1.OpenTelemetryCollector{
ObjectMeta: metav1.ObjectMeta{
Name: "my-instance",
Name: "my-instance",
Annotations: testPodAnnotationValues,
},
Spec: v1alpha1.OpenTelemetryCollectorSpec{
PodAnnotations: testPodAnnotationValues,
Expand All @@ -116,9 +127,6 @@ func TestDaemonsetPodAnnotations(t *testing.T) {
// test
ds := DaemonSet(cfg, logger, otelcol)

// Add sha256 podAnnotation
testPodAnnotationValues["opentelemetry-operator-config/sha256"] = "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"

// verify
assert.Equal(t, "my-instance-collector", ds.Name)
assert.Equal(t, testPodAnnotationValues, ds.Spec.Template.Annotations)
Expand Down
3 changes: 1 addition & 2 deletions pkg/collector/deployment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ func Deployment(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTele
labels["app.kubernetes.io/name"] = naming.Collector(otelcol)

annotations := Annotations(otelcol)
podAnnotations := PodAnnotations(otelcol)

return appsv1.Deployment{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -48,7 +47,7 @@ func Deployment(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTele
Template: corev1.PodTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
Labels: labels,
Annotations: podAnnotations,
Annotations: annotations,
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this introduce a regression where you can no longer set specific pod annotations? I rely on exactly this currently in a few places.

Copy link
Member

Choose a reason for hiding this comment

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

correct , I missed this 👍🏼

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, so, to avoid this kind of regression, I would propagate the annotations into podAnnotations. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be fine to append the annotations in to the pod annotations. How would you handle conflicts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WDYT to take the Deployment's annotations as precedence?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pavolloffay , @jaronoff97 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@yuriolisa i think the pod annotations should win, otherwise there would be no way to override something on the deployment.

},
Spec: corev1.PodSpec{
ServiceAccountName: ServiceAccountName(otelcol),
Expand Down
9 changes: 8 additions & 1 deletion pkg/collector/deployment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,9 @@ func TestDeploymentNewDefault(t *testing.T) {
// verify sha256 podAnnotation
expectedAnnotations := map[string]string{
"opentelemetry-operator-config/sha256": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
"prometheus.io/path": "/metrics",
"prometheus.io/port": "8888",
"prometheus.io/scrape": "true",
}
assert.Equal(t, expectedAnnotations, d.Spec.Template.Annotations)

Expand Down Expand Up @@ -113,7 +116,8 @@ func TestDeploymentPodAnnotations(t *testing.T) {
testPodAnnotationValues := map[string]string{"annotation-key": "annotation-value"}
otelcol := v1alpha1.OpenTelemetryCollector{
ObjectMeta: metav1.ObjectMeta{
Name: "my-instance",
Name: "my-instance",
Annotations: testPodAnnotationValues,
},
Spec: v1alpha1.OpenTelemetryCollectorSpec{
PodAnnotations: testPodAnnotationValues,
Expand All @@ -126,6 +130,9 @@ func TestDeploymentPodAnnotations(t *testing.T) {

// Add sha256 podAnnotation
testPodAnnotationValues["opentelemetry-operator-config/sha256"] = "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"
testPodAnnotationValues["prometheus.io/path"] = "/metrics"
testPodAnnotationValues["prometheus.io/port"] = "8888"
testPodAnnotationValues["prometheus.io/scrape"] = "true"

// verify
assert.Equal(t, "my-instance-collector", d.Name)
Expand Down
3 changes: 1 addition & 2 deletions pkg/collector/statefulset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ func StatefulSet(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTel
labels["app.kubernetes.io/name"] = naming.Collector(otelcol)

annotations := Annotations(otelcol)
podAnnotations := PodAnnotations(otelcol)

return appsv1.StatefulSet{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -48,7 +47,7 @@ func StatefulSet(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTel
Template: corev1.PodTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
Labels: labels,
Annotations: podAnnotations,
Annotations: annotations,
},
Spec: corev1.PodSpec{
ServiceAccountName: ServiceAccountName(otelcol),
Expand Down
9 changes: 8 additions & 1 deletion pkg/collector/statefulset_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ func TestStatefulSetNewDefault(t *testing.T) {
// verify sha256 podAnnotation
expectedAnnotations := map[string]string{
"opentelemetry-operator-config/sha256": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
"prometheus.io/path": "/metrics",
"prometheus.io/port": "8888",
"prometheus.io/scrape": "true",
}
assert.Equal(t, expectedAnnotations, ss.Spec.Template.Annotations)

Expand Down Expand Up @@ -154,7 +157,8 @@ func TestStatefulSetPodAnnotations(t *testing.T) {
testPodAnnotationValues := map[string]string{"annotation-key": "annotation-value"}
otelcol := v1alpha1.OpenTelemetryCollector{
ObjectMeta: metav1.ObjectMeta{
Name: "my-instance",
Name: "my-instance",
Annotations: testPodAnnotationValues,
},
Spec: v1alpha1.OpenTelemetryCollectorSpec{
PodAnnotations: testPodAnnotationValues,
Expand All @@ -167,6 +171,9 @@ func TestStatefulSetPodAnnotations(t *testing.T) {

// Add sha256 podAnnotation
testPodAnnotationValues["opentelemetry-operator-config/sha256"] = "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"
testPodAnnotationValues["prometheus.io/path"] = "/metrics"
testPodAnnotationValues["prometheus.io/port"] = "8888"
testPodAnnotationValues["prometheus.io/scrape"] = "true"

// verify
assert.Equal(t, "my-instance-collector", ss.Name)
Expand Down
9 changes: 9 additions & 0 deletions tests/e2e/smoke-pod-annotations/00-assert.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,20 @@ metadata:
name: pa-collector
annotations:
regular-annotation: regular-value
opentelemetry-operator-config/sha256: dad2b0c2baaeed3a4d75d654900786da3721e6976b3a37eb25a9eaaba87d7b1e
prometheus.io/path: /metrics
prometheus.io/port: "8888"
prometheus.io/scrape: "true"
spec:
template:
metadata:
annotations:
pod-annotation1: value1
pod-annotation2: value2
opentelemetry-operator-config/sha256: dad2b0c2baaeed3a4d75d654900786da3721e6976b3a37eb25a9eaaba87d7b1e
prometheus.io/path: /metrics
prometheus.io/port: "8888"
prometheus.io/scrape: "true"
regular-annotation: regular-value
status:
readyReplicas: 1