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

[configtls] mark module as stable #10344

Merged
merged 2 commits into from
Jul 2, 2024
Merged

Conversation

atoulme
Copy link
Contributor

@atoulme atoulme commented Jun 6, 2024

Description

Mark module as stable.

Link to tracking issue

Fixes #9377

@atoulme atoulme requested review from a team and mx-psi June 6, 2024 03:58
@atoulme atoulme mentioned this pull request Jun 6, 2024
8 tasks
Copy link

codecov bot commented Jun 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.36%. Comparing base (72c23aa) to head (addd9c4).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #10344   +/-   ##
=======================================
  Coverage   92.36%   92.36%           
=======================================
  Files         386      386           
  Lines       18402    18402           
=======================================
  Hits        16997    16997           
  Misses       1052     1052           
  Partials      353      353           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@codeboten
Copy link
Contributor

codeboten commented Jun 6, 2024

Reviewing this PR inspired #10358, that removes the dependency on confmap from this configtls

@atoulme
Copy link
Contributor Author

atoulme commented Jun 6, 2024

Thank you @codeboten , indeed the dependency on confmap was only through tests.

Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

I support this:

  • No explicit or implicit dependencies on unstable modules (other than via the fact that this has mapstructure tags)
  • No 1.0 issues remaining in the tracker
  • No experimental or TODO symbols
  • Structs follow correct naming and methods have context.Context on them

cc @open-telemetry/collector-approvers

@codeboten
Copy link
Contributor

@mx-psi @songy23 @atoulme One thing to keep in mind is that this package does expose mapstructure tags for the struct, which would mean that we'd have to keep support for them around for the life of this package. Is this ok? @TylerHelmuth reminded me that this was the reason we'd held on stabilizing other config packages until confmap is stable

@mx-psi
Copy link
Member

mx-psi commented Jun 7, 2024

@codeboten Right, I forgot about this. I don't see us changing mapstructure tags for something else but it makes sense to discuss explicitly. Should we file an issue about this? (We don't have one, do we?)

@codeboten
Copy link
Contributor

@mx-psi i think only issue that comes close is this one around how mapstructure is unmarshaled #9532, i don't know if we want another issue that specific talks about which tags should be supported moving forward?

@atoulme
Copy link
Contributor Author

atoulme commented Jun 7, 2024

Is changing tags on a struct field a breaking change? The go API is unchanged, right?

@codeboten
Copy link
Contributor

@atoulme it's currently listed in versioning.md as breaking change https://github.com/open-telemetry/opentelemetry-collector/blob/main/VERSIONING.md#configuration-structures

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

No plan to move away from mapstructure struct tags at this point 👍🏻

@mx-psi
Copy link
Member

mx-psi commented Jun 24, 2024

We had some discussion about bumping MinTLS to 1.3. I think this is something that is not a breaking change given that the Go team themselves have proposed changes of this sort in the past (see golang/go#45428) and this being considered a security best practice.

I am not sure how we should best clarify this on VERSIONING.md, but I think this can be dealt with independently from this PR and that we can merge this now.

mx-psi added a commit that referenced this pull request Jul 2, 2024
…wed (#10460)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

<!-- Issue number if applicable -->

We have recently discussed bumping the minimum TLS version to follow
security best practices.
Since we are about to stabilize `configtls` (see #10344), I raised the
question of whether this would be a breaking change that should be done
before 1.0.

I argue that we should be allowed to do this after 1.0 because:
- The Go 1 version compatibility doc states
> Security. A security issue in the specification or implementation may
come to light whose resolution requires breaking compatibility. We
reserve the right to address such security issues.

- The Go team has made [similar
changes](golang/go#45428) in the past for Go
as a whole


While this is not a security issue but a security best practice, the
golang/go issue seems to indicate that changes like this would be in the
spirit of the Go 1 version compatibility promise.
@mx-psi mx-psi merged commit 0337ad8 into open-telemetry:main Jul 2, 2024
50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Stabilize module configtls
4 participants