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

Sanitize names that must follow DNS naming rules #483

Merged
merged 2 commits into from
Jun 28, 2019

Conversation

jpkrohling
Copy link
Contributor

Closes #466 by making the sanitizing names for services and volumes, to be DNS-compliant. Apart from this change, a name such as jaegerqe.test is now possible and yields no errors.

The bigger topic of "validation" in general isn't being touched by this PR.

cc @jkandasa

Signed-off-by: Juraci Paixão Kröhling juraci@kroehling.de

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
@codecov
Copy link

codecov bot commented Jun 27, 2019

Codecov Report

Merging #483 into master will decrease coverage by 0.01%.
The diff coverage is 92.59%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #483      +/-   ##
==========================================
- Coverage    91.7%   91.68%   -0.02%     
==========================================
  Files          64       65       +1     
  Lines        3181     3200      +19     
==========================================
+ Hits         2917     2934      +17     
- Misses        184      185       +1     
- Partials       80       81       +1
Impacted Files Coverage Δ
pkg/config/ui/ui.go 96.96% <100%> (+0.09%) ⬆️
pkg/service/collector.go 100% <100%> (ø) ⬆️
pkg/service/agent.go 100% <100%> (ø) ⬆️
pkg/config/sampling/sampling.go 97.01% <100%> (+0.09%) ⬆️
pkg/service/query.go 100% <100%> (ø) ⬆️
pkg/util/dns_name.go 86.66% <86.66%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd76c19...a5d9140. Read the comment docs.

@codecov
Copy link

codecov bot commented Jun 27, 2019

Codecov Report

Merging #483 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #483      +/-   ##
==========================================
+ Coverage    91.7%   91.74%   +0.04%     
==========================================
  Files          64       65       +1     
  Lines        3181     3199      +18     
==========================================
+ Hits         2917     2935      +18     
  Misses        184      184              
  Partials       80       80
Impacted Files Coverage Δ
pkg/config/ui/ui.go 96.96% <100%> (+0.09%) ⬆️
pkg/service/collector.go 100% <100%> (ø) ⬆️
pkg/service/agent.go 100% <100%> (ø) ⬆️
pkg/config/sampling/sampling.go 97.01% <100%> (+0.09%) ⬆️
pkg/service/query.go 100% <100%> (ø) ⬆️
pkg/util/dns_name.go 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd76c19...8f96898. Read the comment docs.

@jkandasa
Copy link
Member

@jpkrohling We should document this approach on a notable area. Otherwise, the end user will be waiting for a given name, but Jaeger-operator creates a different name (if the name contains invalid chars).

@jpkrohling
Copy link
Contributor Author

+1 , but note that we only change service names and volume names, both are what I'd consider implementation details.

@jpkrohling
Copy link
Contributor Author

By the way: our readme is already somewhat crowded. I'm actually not sure this belongs to the main documentation.

@JStickler, any ideas?

Copy link
Contributor

@objectiser objectiser left a comment

Choose a reason for hiding this comment

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

Lgtm subject to thd suggested changes.

pkg/util/dns_name.go Outdated Show resolved Hide resolved
pkg/util/dns_name_test.go Show resolved Hide resolved
@JStickler
Copy link

@jpkrohling Yes, the operator README is really long (14 pages if I was going to print it). But it's not part of this PR, and I'm not sure what you were getting at in your question to me? Could you clarify what you wanted my opinion on?

@jpkrohling
Copy link
Contributor Author

@JStickler sorry, I kinda forgot to add the context :-) The change from this PR introduces a change that needs to be documented: if the Jaeger CustomResource contains chars not compatible with the specification for DNS names, the invalid chars are replaced by "-" on object names that need to follow this rule (such as "services" and "volumes").

IMO, this is too specific to be useful in the readme, but I can imagine that users might be confused when they create a Jaeger instance named jaeger.testqe and their service names are all like jaeger-testqe.

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
@jpkrohling jpkrohling merged commit c802a91 into jaegertracing:master Jun 28, 2019
@jpkrohling
Copy link
Contributor Author

I'm merging, but will work on the documentation based on what @JStickler suggests.

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.

CR "metadata.name" invalid format should throw exception on the user console
4 participants