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.

Sign up for GitHub

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

Jump to bottom

Add cluster spec to enable/disable ssl #64

Closed
wants to merge 4 commits into from
Closed

Add cluster spec to enable/disable ssl #64

wants to merge 4 commits into from

Conversation

gianrubio
Copy link
Contributor

@gianrubio gianrubio commented Jul 4, 2017 •
edited
Loading

@gianrubio gianrubio force-pushed the disable-ssl branch 3 times, most recently from 0c68c7e to c395d0d Compare July 4, 2017 10:58
@stevesloka
Copy link
Member

Hey @gianrubio I merged your change and pushed new image tag: 5.3.1_3

@gianrubio gianrubio force-pushed the disable-ssl branch 2 times, most recently from 9a173d4 to 03ce723 Compare July 7, 2017 09:34
gianrubio
gianrubio commented Jul 7, 2017
View reviewed changes
pkg/k8sutil/k8sutil.go Outdated
@@ -752,6 +761,12 @@ func (k *K8sutil) CreateClientMasterDeployment(deploymentType, baseImage string,
},
}

// disable secrets volume mount when ssl is not enabled
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stevesloka I'm not really happy with this part of the code. Any thoughts are welcome.

Copy link
Member

Choose a reason for hiding this comment

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

I think the idea is you won't need the volume when TLS is disabled. You could add the volumes to the object after it's created if needed. Meaning, if TLS enabled, then add the volume, but by default it won't be there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So your recommendation is to remove this code or keep it? Do you have more suggestions for this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stevesloka friendly ping

Copy link
Member

Choose a reason for hiding this comment

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

@gianrubio sorry for the delay on this. I think the cleaner way is to not bring in the secret volume if we don't need unless you think that muddy's the code. So when the statefulsets / deployments are created, we can decide to add the secret volume or not.

We'll need to add this check to the piece where certs are auto-generated since that's been recently added.

@stevesloka stevesloka mentioned this pull request Sep 15, 2017
Open
@gianrubio gianrubio force-pushed the disable-ssl branch 2 times, most recently from cec8def to 818e476 Compare September 29, 2017 10:13
@gianrubio
Copy link
Contributor Author

@stevesloka sorry fot the delay, could you review again?

@gianrubio gianrubio changed the title WIP: add cluster spec to enable/disable ssl Add cluster spec to enable/disable ssl Sep 29, 2017
@stevesloka
Copy link
Member

Hey @gianrubio, I should have merged this before I did the CRD refactor. It looks like there are conflicts now. Would you mind fixing those conflicts? I can then test it all out and we can get this merged.

@gianrubio gianrubio force-pushed the disable-ssl branch 3 times, most recently from 7848d9d to eb5db0f Compare October 23, 2017 06:56
@gianrubio
Copy link
Contributor Author

@stevesloka please review again

@stevesloka
Copy link
Member

Do you think this is still a good feature given we have auto gen'd certs now? If so I'm fine, just curious your thoughts.

Would you mind one last rebase, the error PR I merged has caused a bunch. Thanks!

@gianrubio
Copy link
Contributor Author

Rebased, ready to merge!

@stevesloka
Copy link
Member

I was just testing through this, wanted to see if you got the same. I'm getting an error when it's generating certs:

INFO[0075] Using [upmcenterprises/docker-elasticsearch-kubernetes:5.3.1_3] as image for es cluster 
INFO[0075] Creating new certs!                          
INFO[0075] Creating ca cert...                          
ERRO[0075] exit status 1                                
ERRO[0075] exit status 1

@gianrubio
Copy link
Contributor Author

Are you running the controller in your machine or in a container?
Did you create the directories /tmp/certs/config and /tmp/certs/certs?

@stevesloka
Copy link
Member

I was running locally on my mac. I did have the directories, when I switch back to master branch it all worked. I'll try again on my linux box and see if I can come up with what's different.

@adarshaj
Copy link
Contributor

What's the status on this? I'd like to have an option to disable ssl as I terminate ssl on my ingress and don't need to re-encrypt for kubernetes service?

@sreeram-dev
Copy link

I would like to disable ssl and use official elastic search image. Is it possible?

@pieterlange
Copy link
Contributor

The official ES images won't have the required kubernetes plugin, so no.

@gianrubio
Copy link
Contributor Author

replaced by #161

@gianrubio gianrubio closed this Apr 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevesloka stevesloka stevesloka left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gianrubio @stevesloka @adarshaj @sreeram-dev @pieterlange