Skip to content
This repository was archived by the owner on Jan 24, 2018. It is now read-only.

Conversation

@lasley
Copy link
Contributor

@lasley lasley commented Dec 14, 2016

This is a WIP - I still need to implement an interface to the CLI. I'm having a real helluva a time getting this to work the way I want though, so maybe you can help.

CFSSL isn't a service like everything else is, it's a CLI app that is going to be hosted in its own container. See the following console:

$ docker run -i -t lasley/cfssl-exec
Usage:
Available commands:
	serve
	ocspsign
	ocspserve
	selfsign
	bundle
	genkey
	gencert
	certinfo
	scan
	info
	print-defaults
	revoke
	version
	gencrl
	ocspdump
	ocsprefresh
	sign
Top-level flags:
  -allow_verification_with_non_compliant_keys
    	Allow a SignatureVerifier to use keys which are technically non-compliant with RFC6962.

I tried setting clouder.application.link.service to false, but that didn't seem to have any affect. Is what I'm looking to do supported? My stupid container just keeps restarting.

Every 2.0s: docker ps | grep cfssl                                                                                                                                                                                        Tue Dec 13 23:03:30 2016

28cac556cf41        dev-ssl-exec-20161214.070106                "cfssl --help"           2 minutes ago       Restarting (2) 33 seconds ago   0.0.0.0:31003->8888/tcp
               dev-ssl-exec

@codecov-io
Copy link

Current coverage is 31.08% (diff: 100%)

Merging #180 into master will not change coverage

@@             master       #180   diff @@
==========================================
  Files            73         73          
  Lines          5666       5666          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           1761       1761          
  Misses         3905       3905          
  Partials          0          0          

Powered by Codecov. Last update e735b00...56974b3

@lasley lasley force-pushed the release/master/clouder_certificate_authority branch 2 times, most recently from 8e8bb6d to 14ca562 Compare December 14, 2016 09:10

# Enter
ENTRYPOINT ["cfssl"]
CMD ["--help"]
Copy link
Owner

Choose a reason for hiding this comment

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

This means the command of your Dockerfile is cfssl --help

This command is not permanent, it immediately exit, thus killing your container.

If you just want to execute command inside this container and not provide any services with it, while keeping it running, just don't define any ENTRYPOINT nor CMD.
Like any data container in Clouder, it'll use the CMD defined in the base container, aka tail -f /dev/null which keep the container running.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, so the tail keeps the container available so that I can then call it with clouder.model.execute with the args that I would want to send to my shell script?

Copy link
Owner

Choose a reason for hiding this comment

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

yep exactly

Copy link
Contributor Author

@lasley lasley Dec 14, 2016

Choose a reason for hiding this comment

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

Sweet this makes sense, thanks.

Side note- I wonder what the performance impact of leaving the tail running is vs using it as a console app is. Have you measured this at all?

Copy link
Owner

Choose a reason for hiding this comment

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

Well, at least it cost less than the yes command I tried at some point...

I'm pretty sure that a tail cost almost nothing, now if anyone has another opinion and better/standard solution I'm all open to it :3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@t3ddftw - thoughts? We're obviously counting pennies, but good to think about given the goals of this project are centered around scaling.

Choose a reason for hiding this comment

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

What about running cat without an argument? That will have the same result but without taking up a file handle AND it is seven less syscall's than tail -f /dev/null.

tail:

execve("/bin/tailf", ["tailf", "/dev/null"], [/* 17 vars */]) = 0
brk(0)                                  = 0x151b000
access("/etc/ld.so.nohwcap", F_OK)      = -1 ENOENT (No such file or directory)
mmap(NULL, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f30715dc000
access("/etc/ld.so.preload", R_OK)      = -1 ENOENT (No such file or directory)
open("/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 3
fstat(3, {st_mode=S_IFREG|0644, st_size=24043, ...}) = 0
mmap(NULL, 24043, PROT_READ, MAP_PRIVATE, 3, 0) = 0x7f30715d6000
close(3)                                = 0
access("/etc/ld.so.nohwcap", F_OK)      = -1 ENOENT (No such file or directory)
open("/lib/x86_64-linux-gnu/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
read(3, "\177ELF\2\1\1\0\0\0\0\0\0\0\0\0\3\0>\0\1\0\0\0P \2\0\0\0\0\0"..., 832) = 832
fstat(3, {st_mode=S_IFREG|0755, st_size=1840928, ...}) = 0
mmap(NULL, 3949248, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = 0x7f3070ff7000
mprotect(0x7f30711b1000, 2097152, PROT_NONE) = 0
mmap(0x7f30713b1000, 24576, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x1ba000) = 0x7f30713b1000
mmap(0x7f30713b7000, 17088, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x7f30713b7000
close(3)                                = 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f30715d5000
mmap(NULL, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f30715d3000
arch_prctl(ARCH_SET_FS, 0x7f30715d3740) = 0
mprotect(0x7f30713b1000, 16384, PROT_READ) = 0
mprotect(0x603000, 4096, PROT_READ)     = 0
mprotect(0x7f30715de000, 4096, PROT_READ) = 0
munmap(0x7f30715d6000, 24043)           = 0
brk(0)                                  = 0x151b000
brk(0x153c000)                          = 0x153c000
open("/usr/lib/locale/locale-archive", O_RDONLY|O_CLOEXEC) = 3
fstat(3, {st_mode=S_IFREG|0644, st_size=2919792, ...}) = 0
mmap(NULL, 2919792, PROT_READ, MAP_PRIVATE, 3, 0) = 0x7f3070d2e000
close(3)                                = 0
stat("/dev/null", {st_mode=S_IFCHR|0666, st_rdev=makedev(1, 3), ...}) = 0
open("/dev/null", O_RDONLY)             = 3
fstat(3, {st_mode=S_IFCHR|0666, st_rdev=makedev(1, 3), ...}) = 0
ioctl(3, SNDCTL_TMR_TIMEBASE or SNDRV_TIMER_IOCTL_NEXT_DEVICE or TCGETS, 0x7fff83f8afb0) = -1 ENOTTY (Inappropriate ioctl for device)
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f30715db000
read(3, "", 4096)                       = 0
close(3)                                = 0
munmap(0x7f30715db000, 4096)            = 0
inotify_init()                          = 3
inotify_add_watch(3, "/dev/null", IN_MODIFY|IN_DELETE_SELF|IN_MOVE_SELF|IN_UNMOUNT) = 1
read(3,

cat:

execve("/bin/cat", ["cat"], [/* 17 vars */]) = 0
brk(0)                                  = 0x1c31000
access("/etc/ld.so.nohwcap", F_OK)      = -1 ENOENT (No such file or directory)
mmap(NULL, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f29ec71d000
access("/etc/ld.so.preload", R_OK)      = -1 ENOENT (No such file or directory)
open("/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 3
fstat(3, {st_mode=S_IFREG|0644, st_size=24043, ...}) = 0
mmap(NULL, 24043, PROT_READ, MAP_PRIVATE, 3, 0) = 0x7f29ec717000
close(3)                                = 0
access("/etc/ld.so.nohwcap", F_OK)      = -1 ENOENT (No such file or directory)
open("/lib/x86_64-linux-gnu/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
read(3, "\177ELF\2\1\1\0\0\0\0\0\0\0\0\0\3\0>\0\1\0\0\0P \2\0\0\0\0\0"..., 832) = 832
fstat(3, {st_mode=S_IFREG|0755, st_size=1840928, ...}) = 0
mmap(NULL, 3949248, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = 0x7f29ec138000
mprotect(0x7f29ec2f2000, 2097152, PROT_NONE) = 0
mmap(0x7f29ec4f2000, 24576, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x1ba000) = 0x7f29ec4f2000
mmap(0x7f29ec4f8000, 17088, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x7f29ec4f8000
close(3)                                = 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f29ec716000
mmap(NULL, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f29ec714000
arch_prctl(ARCH_SET_FS, 0x7f29ec714740) = 0
mprotect(0x7f29ec4f2000, 16384, PROT_READ) = 0
mprotect(0x60a000, 4096, PROT_READ)     = 0
mprotect(0x7f29ec71f000, 4096, PROT_READ) = 0
munmap(0x7f29ec717000, 24043)           = 0
brk(0)                                  = 0x1c31000
brk(0x1c52000)                          = 0x1c52000
open("/usr/lib/locale/locale-archive", O_RDONLY|O_CLOEXEC) = 3
fstat(3, {st_mode=S_IFREG|0644, st_size=2919792, ...}) = 0
mmap(NULL, 2919792, PROT_READ, MAP_PRIVATE, 3, 0) = 0x7f29ebe6f000
close(3)                                = 0
fstat(1, {st_mode=S_IFCHR|0620, st_rdev=makedev(136, 1), ...}) = 0
fstat(0, {st_mode=S_IFCHR|0620, st_rdev=makedev(136, 1), ...}) = 0
fadvise64(0, 0, 0, POSIX_FADV_SEQUENTIAL) = 0
read(0, 

yes endlessly writes to stdout, so I can see how that ended up being resource intensive :)

Copy link
Owner

Choose a reason for hiding this comment

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

Well why not, cat looks good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, I'm going to give this a shot sometime today.

@@ -0,0 +1,38 @@
FROM yannickburon/clouder:base
Copy link
Owner

Choose a reason for hiding this comment

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

I forgot to make the change on most Clouder images, but you can use the official repo now

FROM clouder/base:3.4

<field name="required" eval="True" />
<field name="auto" eval="True" />
<field name="make_link" eval="True" />
</record>
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think you need this link.

The link feature is used when you want to execute action depending on other component of the infrastructure.
Example : a link between Odoo container to postgres container so odoo user are created on postgres container, the user is deleted and recreated each time we redeploy this link.

In your case, I believe for example if you need a certificate for the elactic container, you'll want a link in elastic container to the cfssl, which will generate the certificate for elastic on deploy. But I don't see the point of a link cfssl to cfssl :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I'm understanding correctly then, we would create one of these links in applications using features from this certificate authority? Verify cert, Sign CSR, etc?

Copy link
Owner

Choose a reason for hiding this comment

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

Yep, exactly

<field name="name">listen</field>
<field name="type">service</field>
<field name="default">*</field>
</record>
Copy link
Owner

Choose a reason for hiding this comment

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

I feel like you copied the options from the postgres container, I don't believe you'll need theses options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I totally did 😆

from openerp import api, fields, models


class ClouderCertificateName(models.Model):
Copy link
Owner

Choose a reason for hiding this comment

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

Creating a new model, I'll keep an eye on what you're doing to make sure this model is really needed.

I think it may be, and thus will replace the cert_key and cert_public fields in clouder.base, clouder.domain by many2one fields.

Waiting to see views to be sure.

Copy link
Contributor Author

@lasley lasley Dec 14, 2016

Choose a reason for hiding this comment

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

Yeah so that is my plan, and I'm actually planning some sort of RFC or email or something. We've got a problem here that I'm not sure can be solved in Odoo. Or maybe just not in one Odoo.

Copy link
Owner

Choose a reason for hiding this comment

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

ok, waiting for more information

@lasley lasley force-pushed the release/master/clouder_certificate_authority branch from 14ca562 to d7b4e4d Compare December 14, 2016 09:52
@lasley lasley mentioned this pull request Dec 14, 2016
@lasley lasley force-pushed the release/master/clouder_certificate_authority branch from d7b4e4d to 02d3338 Compare December 15, 2016 01:28
@lasley lasley force-pushed the release/master/clouder_certificate_authority branch 2 times, most recently from 17265a7 to 464e7ab Compare December 15, 2016 21:31
@lasley lasley force-pushed the release/master/clouder_certificate_authority branch 5 times, most recently from b8a860e to 1c2942e Compare December 16, 2016 04:21
@lasley lasley force-pushed the release/master/clouder_certificate_authority branch from 1c2942e to be5534a Compare December 16, 2016 04:58
@@ -0,0 +1,381 @@
# -*- coding: utf-8 -*-
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lasley lasley force-pushed the release/master/clouder_certificate_authority branch from 96d6c1e to 0bb79e3 Compare December 17, 2016 00:25
@lasley lasley force-pushed the release/master/clouder_certificate_authority branch 3 times, most recently from f6a3bc6 to cedcce4 Compare December 23, 2016 01:17
<field name="required" eval="True" />
<field name="update_strategy">auto</field>
<field name="child_ids"
eval="[(4, ref('application_openssl_exec'))]"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I nest an exec in an exec like this, will the deploy recurse correctly?

Copy link
Owner

@YannickB YannickB Dec 26, 2016

Choose a reason for hiding this comment

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

Hum, you're creating a tree like this :
cfssl
-cfssl data
-cfssl exec
--openssl exec

I suspect we'd want to have this tree :
cfssl
-openssl exec (sequence 1)
-cfssl data (sequence 2)
-cfssl exec (sequence 3)
And make a link between openssl exec and cfssl exec

More information incoming when I'll make a proper review of the PR (this day I hope)

to.
"""
try:
# @TODO: Figure out how the hell to get this host from the base
Copy link
Contributor Author

@lasley lasley Dec 23, 2016

Choose a reason for hiding this comment

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

I need the IP of the container and exposed port here in order to access the web API. Or maybe I need the IP/port of the proxy instead? Any samples to point me to?

For ease of reference, ClouderCertificateAuthority is inheriting clouder.application by delegation, so we have all fields for the CFSSL application available.

Copy link
Owner

Choose a reason for hiding this comment

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

See my comment on clouder.service, it shall resolve the problem.

@lasley lasley force-pushed the release/master/clouder_certificate_authority branch 2 times, most recently from b1d2a06 to b6a5e3f Compare December 23, 2016 02:21
@lasley lasley force-pushed the release/master/clouder_certificate_authority branch from b6a5e3f to e3b40c6 Compare December 23, 2016 02:34
Copy link
Owner

@YannickB YannickB left a comment

Choose a reason for hiding this comment

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

Ok finished the review. The main points are :

  • Structure of children service shall be at the same level
  • You shall base inheritance on clouder.service, not clouder.application.

That said, the design looks good to me. A lot of new models, I can say I really like that because I want to avoid them as much as possible outside of the core.
But I don't really any other simple way so I'm for optimistic merge on this point, and see what happen.

/>
</record>

<record id="application_type_openssl"
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think we really need the openssl application.type, we shall remove it and use cfssl.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm but they serve different purposes. What does an application type actually grant us anyways?

Copy link
Owner

Choose a reason for hiding this comment

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

The application type is usually used in the .py files to know which function shall be executed.

Ex: https://github.com/clouder-community/clouder/blob/0.9.0/clouder_template_odoo/template.py#L51

Until you have a template.py file in your module, you'll not really know if you need an openssl application type, but I'm willing to bet you'll not need it.

FROM clouder/base:3.4
MAINTAINER Dave Lasley <dave@laslabs.com>

CMD tail -f /dev/null
Copy link
Owner

Choose a reason for hiding this comment

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

Remove CMD, inherit CMD of base

FROM clouder/base:3.4
MAINTAINER Dave Lasley <dave@laslabs.com>

CMD tail -f /dev/null
Copy link
Owner

Choose a reason for hiding this comment

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

Remove CMD, inherit CMD of base

WORKDIR /var/pki

# Enter
ENTRYPOINT ["cfssl"]
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think ENTRYPOINT is needed


# Enter
ENTRYPOINT ["cfssl"]
CMD ["cat"]
Copy link
Owner

Choose a reason for hiding this comment

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

We shall remove it, instead replace the CMD in base image by cat

'clouder_certificate_authority.tag_cert_authority',
),
)
api_port_id = fields.Many2one(
Copy link
Owner

Choose a reason for hiding this comment

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

We shall not need this field if we use clouder.service. If you want easy access to the value, use computed field or property (https://github.com/clouder-community/clouder/blob/0.9.0/clouder/container.py#L620)

Copy link
Owner

Choose a reason for hiding this comment

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

My bad, it was already a computed field. We shall decide if we prefer computed fields or property in such case.

comodel_name='clouder.image.port',
compute='_compute_api_port_id',
)
host_id = fields.Many2one(
Copy link
Owner

Choose a reason for hiding this comment

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

We shall not need this field, information shall be access through node_id field

}""",
help='Certificate Signing Request.',
)
exec_service_id = fields.Many2one(
Copy link
Owner

Choose a reason for hiding this comment

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

We shall not need this field if we use clouder.service, we'll find it through child_ids field. If you want easy access to the value, use computed field or property (https://github.com/clouder-community/clouder/blob/0.9.0/clouder/container.py#L620)

string='Initialized',
help='Has this CA server been initialized yet?',
)
openssl_service_id = fields.Many2one(
Copy link
Owner

Choose a reason for hiding this comment

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

We shall not need this field if we use clouder.service, we'll find it through child_ids field. If you want easy access to the value, use computed field or property (https://github.com/clouder-community/clouder/blob/0.9.0/clouder/container.py#L620)

required=True,
)
port = fields.Integer()
api_object = fields.Binary(
Copy link
Owner

Choose a reason for hiding this comment

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

I don't know if we shall keep using a computed field or use a property like anywhere else in Clouder (like https://github.com/clouder-community/clouder/blob/0.9.0/clouder/container.py#L620).
Whatever we chose, we shall use only one of them in all Clouder sourcecode, since the purpose seems to be exactly the same.

Copy link
Contributor Author

@lasley lasley Dec 26, 2016

Choose a reason for hiding this comment

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

Computed fields > properties because they are RecordSet aware. Properties only work at the model level (or singleton record), and should only be used if describing attributes of a model or collection of data as a whole vs. actually being data.

Copy link
Owner

Choose a reason for hiding this comment

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

Ok I agree, then I'll support all use of computed fields in PR in the future. We'll also think about replacing properties with computed fields later.

That said, I wish computed fields would be easier to read in the code, I'd like we keep function near the field definition. That's the main added value of properties IMO

Copy link
Contributor Author

@lasley lasley Dec 26, 2016

Choose a reason for hiding this comment

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

I wish computed fields would be easier to read in the code

I agree. I've found that the OCA guidelines provide a healthy balance for this though - the computation methods should just be _compute_my_field_name, and should be declared in field declaration order. It at least makes it all predictable, even when in downstream modules (although I just alphabetize them in a group of computation methods if the field definition isn't in the current class in order to not open more code /lazy).

@lasley lasley force-pushed the release/master/clouder_certificate_authority branch from 328b7a0 to 4bbd304 Compare January 16, 2017 02:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants