Automate TLS secrets generation for Kubernetes family infrastructures #220

mmorhun · 2020-04-15T10:02:26Z

Signed-off-by: Mykola Morhun mmorhun@redhat.com

What this PR does

This PR adds checks into Che Operator reconcile loop for required for Che deployment TLS secrets on Kubernetes infrastructure, and if they are absent, it starts job to generate them.

Which issues this PR fixes or references

eclipse-che/che#16546

How to test

Via chectl:
- Build operator image, push it
- Disable TLS secrets check and then run:

checlt server:start --platform=minikube --installer=operator --self-signed-cert --tls --che-operator-image=<operator-image>

Via local-debug.sh:
Copy default CR, and change following fields:

spec.server.tlsSupport: true
spec.server.selfSignedCert: true
spec.k8s.ingressDomain: <cluster-domain>

deploy/operator-local.yaml

tolusha · 2020-04-15T13:19:52Z

pkg/controller/che/che_controller.go

@@ -265,6 +265,14 @@ func (r *ReconcileChe) Reconcile(request reconcile.Request) (reconcile.Result, e
 		}
 	}

+	// Handle Che TLS certificates on Kubernetes like infrastructures
+	if instance.Spec.Server.TlsSupport && instance.Spec.Server.SelfSignedCert && !isOpenShift {
+		shouldReturn, reconsileResult, err := HandleCheTLSSecrets(instance, r)


Can we return something like status structure?
Something like this
https://github.com/eclipse/che-operator/blob/master/pkg/deploy/deployment.go#L42

Theoretically we can, but I am against it. We need to return 3 values from the method and we can do it using embedded in the golang mechanism. Why do we need to introduce new abstraction then?

We should be consistent in the implementations.

+1 About consistent implementation

tolusha · 2020-04-15T13:21:38Z

pkg/controller/che/create.go

@@ -132,6 +133,29 @@ func (r *ReconcileChe) CreateNewRoleBinding(instance *orgv1.CheCluster, roleBind
 	return nil
 }

+// CreateNewJob deploys new instance of given job
+func (r *ReconcileChe) CreateNewJob(instance *orgv1.CheCluster, job *batchv1.Job) error {


Create a syncJobToCluster function with the same logic as for pvc, ingress, route ...

I think it is better to keep ability to create an object (in our case job) configuration and then separately flush it to cluster. Because we want to have universal methods, but we never know for sure which aspects of it we may want to change in the future. Changing the sync* method signature is not good as it will affect other consumers. Updating object in runtime is worse or even impossible.
I may move this method into job.go and rename it to match sync* pattern, but I don't want to loose the flexibility here.

tolusha · 2020-04-15T13:22:51Z

pkg/controller/che/tls-secrets.go

+var shouldReturn = true
+
+// HandleCheTLSSecrets handles TLS secrets required for Che deployment
+func HandleCheTLSSecrets(checluster *orgv1.CheCluster, r *ReconcileChe) (bool, reconcile.Result, error) {


Let's return status like this
https://github.com/eclipse/che-operator/blob/master/pkg/deploy/deployment.go#L42

No, please see my answer above. Moreover, inside internal function we behave the same as in reconcile loop.

pkg/controller/che/tls-secrets.go

AndrienkoAleksandr · 2020-04-16T08:57:01Z

Interesting, some day maybe we will have tls generation from the box https://github.com/operator-framework/operator-sdk/blob/master/doc/proposals/tls-utilities.md

mmorhun · 2020-04-16T09:00:01Z

@AndrienkoAleksandr I've already seen that. Unfortunately current functionality they provide has only basic stuff which doesn't suite our needs. But thanks for commenting.

pkg/controller/che/tls-secrets.go

pkg/deploy/job.go

pkg/controller/che/tls-secrets.go

tolusha · 2020-04-16T14:16:33Z

pkg/deploy/job.go

+// NewJob creates new job configuration by giben parameters.
+func NewJob(checluster *orgv1.CheCluster, name string, namespace string, image string, serviceAccountName string, env map[string]string, backoffLimit int32) *batchv1.Job {
+	labels := GetLabels(checluster, util.GetValue(checluster.Spec.Server.CheFlavor, DefaultCheFlavor))
+	labels["component"] = "che-create-tls-secret-job"


I would keep `che component

It is not possible due to conflicts with Che master. All Che related pods have lebel app=che including this job.

tolusha · 2020-04-16T14:17:08Z

pkg/deploy/job.go

+}
+
+// NewJob creates new job configuration by giben parameters.
+func NewJob(checluster *orgv1.CheCluster, name string, namespace string, image string, serviceAccountName string, env map[string]string, backoffLimit int32) *batchv1.Job {


we can get namespace from checluster, not needed to pass it

we don't configure ttlSecondsAfterFinished,
backoffLimit, why do we need to configure it? let's use some default value for instance 5.

Just for case when we need to start a job in different than Che namespace.

We indeed don't configure ttlSecondsAfterFinished so I may leave it with default value. As for backoffLimit, to me it may vary for different jobs, so I left it configurable. I would like to use some default values, but golang doesn't provide such ability.

AndrienkoAleksandr

Tested, works Good.

tolusha · 2020-04-23T05:27:41Z

time="2020-04-23T08:25:54+03:00" level=info msg="Creating a new object: Job, name: che-tls-job"
time="2020-04-23T08:25:54+03:00" level=info msg="Creating a new object: Job, name: che-tls-job"
time="2020-04-23T08:25:54+03:00" level=error msg="Failed to create che-tls-job Job: jobs.batch \"che-tls-job\" already exists"
time="2020-04-23T08:25:54+03:00" level=error msg="Error creating Che TLS job \"che-tls-job\": jobs.batch \"che-tls-job\" already exists"
time="2020-04-23T08:25:55+03:00" level=info msg="Creating a new object: Job, name: che-tls-job"
time="2020-04-23T08:25:55+03:00" level=error msg="Failed to create che-tls-job Job: jobs.batch \"che-tls-job\" already exists"
time="2020-04-23T08:25:55+03:00" level=error msg="Error creating Che TLS job \"che-tls-job\": jobs.batch \"che-tls-job\" already exists"
time="2020-04-23T08:25:56+03:00" level=info msg="Creating a new object: Job, name: che-tls-job"
time="2020-04-23T08:25:56+03:00" level=error msg="Failed to create che-tls-job Job: jobs.batch \"che-tls-job\" already exists"
time="2020-04-23T08:25:56+03:00" level=error msg="Error creating Che TLS job \"che-tls-job\": jobs.batch \"che-tls-job\" already exists"
time="2020-04-23T08:25:57+03:00" level=info msg="Creating a new object: Job, name: che-tls-job"
time="2020-04-23T08:25:57+03:00" level=error msg="Failed to create che-tls-job Job: jobs.batch \"che-tls-job\" already exists"
time="2020-04-23T08:25:57+03:00" level=error msg="Error creating Che TLS job \"che-tls-job\": jobs.batch \"che-tls-job\" already exists"

mmorhun · 2020-04-23T07:15:18Z

@tolusha thanks. It was hard to reproduce the issue. So actually the only way I can see it is to:

Stop Che operator
Delete Che secrets and all deployments, but leave namespace with the job
Start operator again

This looks like artificial for me, but better to fix it. I can do it, when you let me know your thought about proposed approach.

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

…oCluster. Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

Signed-off-by: Anatoliy Bazko <abazko@redhat.com>

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

mmorhun requested review from davidfestal, tolusha and AndrienkoAleksandr April 15, 2020 10:02

mmorhun requested review from l0rd, nickboldt, rhopp and vparfonov as code owners April 15, 2020 10:02

mmorhun self-assigned this Apr 15, 2020

tolusha mentioned this pull request Apr 15, 2020

Che Deploy Sprint #182 eclipse-che/che#16455

Closed

43 tasks

mmorhun mentioned this pull request Apr 15, 2020

Add Che CR validator #221

Merged

tolusha reviewed Apr 15, 2020

View reviewed changes

deploy/operator-local.yaml Outdated Show resolved Hide resolved

tolusha reviewed Apr 15, 2020

View reviewed changes

pkg/controller/che/tls-secrets.go Show resolved Hide resolved

tolusha mentioned this pull request Apr 16, 2020

Che Deploy Sprint #183 eclipse-che/che#16623

Closed

50 tasks

AndrienkoAleksandr reviewed Apr 16, 2020

View reviewed changes