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 mTLS support for TracePipeline OTLP output #347

Merged
merged 9 commits into from
Aug 23, 2023

Conversation

dennis-ge
Copy link
Contributor

@dennis-ge dennis-ge commented Aug 21, 2023

Description

Changes proposed in this pull request:

  • add support for TLS certificates for the TracePipeline and MetricPipeline OTLP Output
  • The E2E test is included in a separate PR.

Related Issues and Documents

Closes:

Related issues: kyma-project/kyma#17995

Traceability

  • The PR is linked to a GitHub Issue.
  • New features have a milestone label set.
  • New features have defined acceptance criteria in a corresponding GitHub Issue, and all criteria are satisfied with this PR.
  • The corresponding GitHub Issue has a respective area label.
  • The follow-up issues (if any) are linked in the Related Issues section.

Testability

The feature is unit-tested:

  • Yes.
  • No, because unit tests are not needed.
  • No, because of ...

The feature is e2e-tested:

  • Yes.
  • No, because e2e-tests are not needed.
  • No, because the e2e-test will be introduced in a later PR

Tests conducted for the PR:

  • Created multiple TracePipelines with different tls configurations to see how the otel collector reacts

Codebase

  • My code follows the Effective Go style guidelines.
  • The code was planned and designed following the defined architecture and the separation of concerns.
  • The code has sufficient comments, particularly for all hard-to-understand areas.
  • This PR adds value and shows no feature creep.
  • I have augmented the test suite that proves my fix is effective or that my feature works. (unit tests only)
  • Adjusted the documentation if the change is user-facing.

@kyma-bot kyma-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 21, 2023
@kyma-bot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@kyma-bot kyma-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 21, 2023
@dennis-ge
Copy link
Contributor Author

/test all

@dennis-ge
Copy link
Contributor Author

/test all

return cfg
}

if !cfg.Insecure {
Copy link
Contributor Author

@dennis-ge dennis-ge Aug 22, 2023

Choose a reason for hiding this comment

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

I restored the isInsecureOutput function that checks if the endpoint scheme is http. It is necessary that this is set before line 82 (output.TLS == nil) to have the same functionality as before.

This line checks if the output is not insecure, i.e., line 80 (cfg.Insecure = isInsecureOutput(otlpEndpointValue)) returned false. If this is the case, we allow to overwrite the TLS setting.

It is not a nice solution but it should be the same as the behavior before

@dennis-ge dennis-ge marked this pull request as ready for review August 22, 2023 14:47
@dennis-ge dennis-ge requested review from a team as code owners August 22, 2023 14:47
@kyma-bot kyma-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 22, 2023
return fmt.Sprintf("HEADER_%s_%s", envvar.MakeEnvVarCompliant(pipelineName), envvar.MakeEnvVarCompliant(header.Name))
}

func makeTLSVariable(prefix, pipelineName string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is more generic than it's name. Let's either make 3 specific function for each purpose or use a more generic name.

@dennis-ge
Copy link
Contributor Author

/retest-required

@chrkl chrkl changed the title add mTLS support for TracePipeline OTLP outout feat: Add mTLS support for TracePipeline OTLP output Aug 23, 2023
chrkl
chrkl previously approved these changes Aug 23, 2023
@kyma-bot kyma-bot added the lgtm Looks good to me! label Aug 23, 2023
@kyma-bot kyma-bot removed the lgtm Looks good to me! label Aug 23, 2023
@kyma-bot kyma-bot added the lgtm Looks good to me! label Aug 23, 2023
@chrkl
Copy link
Contributor

chrkl commented Aug 23, 2023

/retest

@kyma-bot
Copy link
Contributor

kyma-bot commented Aug 23, 2023

@dennis-ge: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-telemetry-manager-governance 98a75d2 link false /test pull-telemetry-manager-governance

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/traces TracePipeline kind/feature Categorizes issue or PR as related to a new feature. lgtm Looks good to me! size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants