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

Synch changes in cert generation script with CLO #1008

Merged

Conversation

@pavolloffay pavolloffay requested a review from objectiser April 8, 2020 12:55
@codecov
Copy link

codecov bot commented Apr 8, 2020

Codecov Report

Merging #1008 into master will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1008      +/-   ##
==========================================
+ Coverage   64.55%   64.60%   +0.05%     
==========================================
  Files          82       82              
  Lines        6540     6547       +7     
==========================================
+ Hits         4222     4230       +8     
+ Misses       2177     2176       -1     
  Partials      141      141              
Impacted Files Coverage Δ
pkg/apis/jaegertracing/v1/logger.go 0.00% <0.00%> (ø)
pkg/apis/jaegertracing/v1/options.go 89.65% <0.00%> (+1.65%) ⬆️

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 5151c77...b9d6ce7. Read the comment docs.

@objectiser
Copy link
Contributor

@pavolloffay KevinE is running the internal e2e tests, so will wait for those results. But wanted to find out are there going to be any upgrade issues with this? Is there a procedure that will need to be followed - e.g. stop / restart jaeger instance and ES cluster.

@pavolloffay
Copy link
Member Author

I would say it should not cause problems. Once new operator is deployed existing certs are reconstructed from secrets and stored on operator's filesystem in /tmp.

func (ed *ElasticsearchDeployment) CreateCerts() error {

Then the script checks whether certs exist and are not expired

if [ ! -f ${WORKING_DIR}/ca.crt ] || [ ! -f ${WORKING_DIR}/ca.key ] || ! openssl x509 -checkend 0 -noout -in ${WORKING_DIR}/ca.crt; then
.

@objectiser
Copy link
Contributor

@pavolloffay ok thanks - internal tests passed, so will approve/merge.

@objectiser objectiser merged commit bff60c0 into jaegertracing:master Apr 8, 2020
@ewolinetz
Copy link

Is there a procedure that will need to be followed - e.g. stop / restart jaeger instance and ES cluster.

You will need to restart the ES cluster, but the Elasticsearch Operator handles this for you.
The operator also tries to connect to the ES cluster using a service account token first, so it bypasses any potential issues where the secret has rotated but ES hasn't yet loaded it in.

@pavolloffay
Copy link
Member Author

Thanks for looking into this @ewolinetz.

You will need to restart the ES cluster, but the Elasticsearch Operator handles this for you.

Could you please explain why the ES cluster will have to be restarted? Will the certs change? Could you please point me to the code?

The operator also tries to connect to the ES cluster using a service account token first, so it bypasses any potential issues where the secret has rotated but ES hasn't yet loaded it in.

Which operator? Jaeger/jaeger operator is using only certs for auth. If certs change jaeger collector will not able to connect. I am not sure if it restarts automatically. I am going to test this.

jpkrohling pushed a commit to jpkrohling/jaeger-operator that referenced this pull request Dec 15, 2020
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.

3 participants