Use kubectl in EjsonSecretProvisioner #91
Conversation
| msg = secret_exists?(secret_name) ? "Updating secret #{secret_name}" : "Creating secret #{secret_name}" | ||
| KubernetesDeploy.logger.info(msg) | ||
|
|
||
| file = Tempfile.new(secret_name) |
There was a problem hiding this comment.
Alternatively, we could use create and construct a really long command with --from-literal=k=v --from-literal=k=v .... I'm not sure if/how that works with updates though.
|
|
||
| file = Tempfile.new(secret_name) | ||
| file.write(secret_yaml) | ||
| file.close |
There was a problem hiding this comment.
Maybe do this close on an ensure block ? otherwise you might end up on the situation where the GC takes the file away from you
There was a problem hiding this comment.
To address both this and @mkobetic comment below, kubectl won't be able to read the file unless we close it. Tempfiles are automatically GC'd when the process exits, if not sooner. Tempfile.open { |f| ... } doesn't return the file. I don't think it would be possible for the file to get GC'd when we need it in this case, since we only need it for the duration of this method, which retrains a reference to it. However, I can switch to a regular file and manually unlink it in an ensure to be extra super safe.
There was a problem hiding this comment.
I would expect that if the file was flushed after write the shell command should be able to read it then, but I haven't tried.
There was a problem hiding this comment.
TBH I think this is the simplest solution. The GC should take care of unlinking at the appropriate time, but I'll add file.unlink if file in an ensure just in case.
| end | ||
|
|
||
| def run_kubectl(*args) | ||
| raise FatalDeploymentError, "Namespace missing for namespaced command" if @namespace.blank? |
There was a problem hiding this comment.
Because this is copied from another place where it wouldn't make sense in the constructor. :P
Really, it's a super-paranoid check to make sure we don't run commands without a context/namespace. In reality, it should be impossible to hit in production (several other things would have raised before we get here). But I don't feel strongly about where it goes and can move it.
|
|
||
| def run_kubectl(*args) | ||
| raise FatalDeploymentError, "Namespace missing for namespaced command" if @namespace.blank? | ||
| raise KubectlError, "Explicit context is required to run this command" if @context.blank? |
| file.write(secret_yaml) | ||
| file.close | ||
|
|
||
| out, err, st = run_kubectl("apply", "--filename=#{file.path}") |
There was a problem hiding this comment.
We should make sure the file is deleted too, would Tempfile.open work for this?
|
Updated |
| file = Tempfile.new(secret_name) | ||
| file.write(secret_yaml) | ||
| file.close | ||
|
|
||
| out, err, st = run_kubectl("apply", "--filename=#{file.path}") | ||
| KubernetesDeploy.logger.debug(out) | ||
| raise EjsonSecretError, err unless st.success? | ||
| ensure | ||
| file.unlink if file |
There was a problem hiding this comment.
I wonder if this can actually error out in some circumstances, I'd be fine to rely on Tempfile to take care of this. But I'm fine either way
We're having auth problems with kubeclient on some clusters, and deploys did not depend on that gem until the provisioner was introduced. We still would prefer to use kubeclient for this in the long run.
tl;dr
kubeclientisn't working for some of our production clusters, so this switchesEjsonSecretProvisionerover tokubectl--which is what the rest of the deploy process uses--for the time being.When I rolled out #81 in shipit, I discovered that kubeclient does not seem to be working with our GCP clusters. It is failing with an SSL error on
client.discover(see this deploy). This may be a further piece to the work @xldenis already did in #88; he is investigating.The most meaningful change is that we need to write a tempfile to use
kubectl applyfor secret creation/update. Note that although the unit tests required surgery, the integration tests all passed as-is.Why not wait for the kubeclient fix?
The current consensus is still that we want to use a gem rather than kubectl for everything we can (which doesn't include "apply" operations) in the long run, but I didn't realize that none of the tasks that already use kubeclient had been tested in our production Shipit environment. This feature is high-priority, and switching to kubeclient is far from it. With the problems we've had, we may even want to reconsider this particular gem choice.
cc @Shopify/cloudplatform