[WIP] Use the registries operator to handle custom certificates for registries #702

ereslibre · 2018-12-21T14:34:40Z

TODO:

Use the proper registries operator image (not from the docker hub)

MalloZup

i got this clarified

ereslibre · 2019-01-08T14:42:22Z

The point behind this work is to work on operators for the new stack, but at the same time, being able to "backport" these operators to the old CaaSP; this would mean treat operators as we treat other addons (such as tiller or kubedns).

This would help us in that the operator is the one actually managing the logic of handling these resources, while salt would just submit the resources (the YAML documents themselves). This would allow us for an easier integration with the new pieces on the old world, without having to introduce many changes on the way we manage everything. It would also allow us to incrementally reduce the salt code as soon as those operators are ready and we deploy the operators on the cluster (old caasp), so they can manage the resources created by salt (that come from the UI).

The neat part IMO is that salt would be a simple mapping from database -> YAML documents [external API] submitted to the cluster, and then operators handling these documents and performing changes to the cluster as we do in Kubic today. I see this is a good balance between the old and new worlds, and how we can start "backporting" operators to the old world, increasingly removing logic from salt.

MalloZup · 2019-01-08T18:08:07Z

@ereslibre thx for info 💌 i get anyways inof today, thx 🐉

salt/addons/registries-operator/apply.rb.jinja

salt/addons/registries-operator/init.sls

As discussed

inercia · 2019-01-11T13:35:41Z

salt/_modules/caasp_kubernetes_secrets.py

+        secret_name = secret_name_prefix + secret_name
+    secret_file_path = temp_file_with_contents(secret_contents)
+    kubectl('create secret generic %s --from-file=%s=%s' % (secret_name, secret_key, secret_file_path), namespace=namespace)
+    os.remove(secret_file_path)


Instead of using this pattern for creating a temporary file, I think it would be better to have a kubectl_apply function that could accept maybe two different parameters, file= and string=. Then, when using a string, we could use the better Python idiom:

with tempfile.TemporaryFile() as fp: fp.write("the manifest contents") # run the kubectl apply now

Oh, do you suggest to use kubectl apply to create the secrets too? We can do it that way, yes. In that case I could reuse the caasp_kubernetes_kubectl.kubectl logic that doesn't use a temporary file but stdin instead.

I think you could both use stdin (preferred) or a temporary file, but in case you use a temporary file it would be better to use a with context, as it does a cleanup of the file (that can contain sensistive data) when leaving the context.

inercia · 2019-01-11T13:38:22Z

salt/_modules/caasp_kubernetes_crds.py

+    return deserialize(current_resources)
+
+
+def reconcile_desired_resources(custom_resource_name, namespace, desired_resources=[]):


I think this function tries to be too smart. I would mimic many other Salt modules and have a caasp_kubernetes_crd.absent (equivalent to file.missing) and caasp_kubernetes_crd.managed (equivalent to file.managed). Then we could skip all this code for detecting the current CRD and if we want to delete it or not...

I agree it tries to be too smart, however I don't see a way around it. When a user modifies the settings on the UI and we run an orchestration we know what settings we want, and for removing the extra settings we need to reconcile somehow with the real state of the cluster and that needs to happen somewhere. Am I missing an alternative approach?

I think we have two cases:

the cleanup of registries not present in the pillar: for each registry obtained with kubectl get registries, if it is not managed by us, do a caasp_kubernetes.crd.absent (a kubectl delete under the hood). This could be done with a Jinja loop.

the add/modify registry. It would be handled by kubectl applys (I think it doesn't really matter if we are creating the CR for the first time or if we are modifying the resource: the k8s API will deal with updating it if that is the case). We could iterate for all the certificates in get_certs() and caasp_kubernetes.crd.manage each one. This could be done with a Jinja loop.

Okay, so moving a bit of the logic to the rendering. I'm not sure I'm very happy with that, because with the current solution the SLS look simpler and for future operators we will have less replicated logic in the templates and will be rendering less clutter.

If this was a state/module for others to consume I'd agree present/absent is the way to go, but since this is for us and for making it easier to integrate this pattern in future operators for this mixed world I think I prefer to have simpler SLS and the big chunk of the logic in the python module.

I don't oppose to what you propose either.

I agree it would mean more code on the SLSs, but (and this is a completely personal view) I think it would be better because

explicit is better than implicit, and it would be clearer what the code does.

I think it would follow more Salt's philosophy, where things are declared as present, missing and so on instead of using deus ex machina states that reconcile desired things.

But, as I said, this is my personal take on this...

i think personally that the saltstack way is to have managed, absent etc, logic.

I think yop, we we will have more rendundant code in SLS but if we gan simplicity is better to have it 🚀

I'm not sure in this case, because we would be trading the same "reconcile" logic from having it in one single place (inside the module), where all SLS's deploying and managing operators CR's would use it to duplicate the reconcile logic on every single SLS's managing operators resources, so I'm not sure at all we would ve gaining simplicity...

inercia · 2019-01-11T13:39:33Z

salt/_states/caasp_kubernetes_crds.py

+from __future__ import absolute_import
+
+
+def reconcile(name, namespace='kube-system', desired_resources=[]):


I think this tries to be too smart too. A caasp_kubernetes_secret.absent and caasp_kubernetes_secret.managed would be more explicit and clear in my view.

flavio · 2019-01-11T17:30:44Z

salt/_modules/caasp_kubernetes_crds.py

+
+
+def kubectl(command, **kwargs):
+    return __salt__['caasp_kubernetes_kubectl.kubectl'](command, **kwargs)


I don't know if caasp_kubernetes_kubectl.kubectl is already doing that, but maybe it would be a good idea to log eventual errors

salt/_modules/caasp_docker.py

MalloZup · 2019-03-18T18:52:46Z

This GitHub PR is unactive since 30. Is this GitHub PR still needed? Please close or update it accordingly.
This reminder is autogenerated by https://github.com/MalloZup/blacktango

MalloZup previously requested changes Jan 8, 2019

View reviewed changes

inercia reviewed Jan 10, 2019

View reviewed changes

salt/addons/registries-operator/apply.rb.jinja Outdated Show resolved Hide resolved

salt/addons/registries-operator/init.sls Outdated Show resolved Hide resolved

inercia suggested changes Jan 11, 2019

View reviewed changes

flavio reviewed Jan 11, 2019

View reviewed changes

Use the registries operator to handle custom certificates for registries

3aeb79a

MalloZup reviewed Jan 12, 2019

View reviewed changes

salt/_modules/caasp_docker.py Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Use the registries operator to handle custom certificates for registries #702

[WIP] Use the registries operator to handle custom certificates for registries #702

ereslibre commented Dec 21, 2018 •

edited

Loading

MalloZup left a comment •

edited

Loading

ereslibre commented Jan 8, 2019

MalloZup commented Jan 8, 2019

inercia Jan 11, 2019

ereslibre Jan 11, 2019

inercia Jan 11, 2019

inercia Jan 11, 2019

ereslibre Jan 11, 2019

inercia Jan 11, 2019

ereslibre Jan 11, 2019

inercia Jan 11, 2019

MalloZup Jan 14, 2019

ereslibre Jan 14, 2019

inercia Jan 11, 2019

flavio Jan 11, 2019

MalloZup commented Mar 18, 2019

		return deserialize(current_resources)


		def reconcile_desired_resources(custom_resource_name, namespace, desired_resources=[]):

		from __future__ import absolute_import


		def reconcile(name, namespace='kube-system', desired_resources=[]):



		def kubectl(command, **kwargs):
		return __salt__['caasp_kubernetes_kubectl.kubectl'](command, **kwargs)

[WIP] Use the registries operator to handle custom certificates for registries #702

Are you sure you want to change the base?

[WIP] Use the registries operator to handle custom certificates for registries #702

Conversation

ereslibre commented Dec 21, 2018 • edited Loading

MalloZup left a comment • edited Loading

Choose a reason for hiding this comment

ereslibre commented Jan 8, 2019

MalloZup commented Jan 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MalloZup commented Mar 18, 2019

ereslibre commented Dec 21, 2018 •

edited

Loading

MalloZup left a comment •

edited

Loading