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

TT-13680 - feat: add option to get one OpenTelemetry header from a K8s secret #364

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sym-stiller
Copy link

@sym-stiller sym-stiller commented Nov 27, 2024

Description

This change allows you to add one Authorization header to the OpenTelemetry traffic in a secure way. A Bearer token is typically secret and should not be exposed in the Helm values file or in the K8s manifest of the Tyk Gateway deployment.

I'm sure this is not the most optimal way to implement this; for example, with my solution you can only use either the headers or headerSecret value, but not both at the same time. This would limit you to exactly one header, if you're using the headerSecret. I just want to start a discussion about a good way to solve the linked issue.

Related Issue

#363

https://tyktech.atlassian.net/browse/TT-13680

Motivation and Context

This change improves the protection of sensitive authorization secrets.

Test Coverage For This Change

I have templated the charts locally and inspected the result manually.

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring or add test (improvements in base code or adds test coverage to functionality)
  • Documentation updates or improvements.

Checklist

  • Make sure you are requesting to pull a topic/feature/bugfix branch (right side). If PRing from your fork, don't come from your master!
  • Make sure you are making a pull request against our master branch (left side). Also, it would be best if you started your change off our latest master.
  • My change requires a change to the documentation.
    • I have manually updated the README(s)/documentation accordingly.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

This is useful to add Authorization headers, which are typically secret and should not be exposed in the Helm values file or in the K8s manifest of the Tyk Gateway deployment.
@sym-stiller sym-stiller requested a review from a team as a code owner November 27, 2024 15:21
@sym-stiller sym-stiller requested review from buraksekili and removed request for a team November 27, 2024 15:21
@buraksekili buraksekili changed the title feat: add option to get one OpenTelemetry header from a K8s secret TT-13680 - feat: add option to get one OpenTelemetry header from a K8s secret Dec 9, 2024
Copy link
Member

@buraksekili buraksekili left a comment

Choose a reason for hiding this comment

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

Thank you @sym-stiller for this great PR 🙏 I just left a few comments.

Comment on lines +530 to +532
headerSecret:
name: otel-auth-secret
key: Authorization
Copy link
Member

Choose a reason for hiding this comment

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

Let's set headerSecret to a null object by default

Suggested change
headerSecret:
name: otel-auth-secret
key: Authorization
headerSecret: {}

This prevents any failures during chart upgrades. If headers is being in use, upgrading to the new chart where headerSecret is set by default, can trigger the fail call defined in the values.yaml file.

Additionally, the default value assumes that users already have a Kubernetes secret named otel-auth-secret containing an Authorization key, which may not be the case for everyone.

Please apply this change to all values.yaml files (in tyk-stack, tyk-oss etc)

{{- if .Values.gateway.opentelemetry.headers }}
- name: TYK_GW_OPENTELEMETRY_HEADERS
value: "{{ include "otel-headers" . }}"
{{- else if and .Values.gateway.opentelemetry.headerSecret.name .Values.gateway.opentelemetry.headerSecret.key }}
- name: TYK_GW_OPENTELEMETRY_HEADERS
Copy link
Member

Choose a reason for hiding this comment

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

TYK_GW_OPENTELEMETRY_HEADERS environment variable needs to be declared as a comma-separated list of key-value pairs, where each pair is separated by a colon. For instance:

TYK_GW_OPENTELEMETRY_HEADERS: "header_key1:header_value1,header_key2:header_value2"

So, we need to construct the value of the environment variable in this key-value pair format.

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.

2 participants