| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-06-12 | |||
| 13:36:51 | mriedem | bauzas: looks like you and i were reviewing https://review.openstack.org/#/c/479949/ at the same time | |
| 13:36:57 | bauzas | mriedem: correct | |
| 13:37:03 | mriedem | the answer to some of your questions is, it's later in the series, | |
| 13:37:10 | bauzas | I had this tab open for a while | |
| 13:37:13 | mriedem | but i don't exactly know how enable_certificate_validation is meant to be used | |
| 13:37:34 | bauzas | well, tbh, I think we need to understand how to upgrade | |
| 13:37:56 | mriedem | brianna is sometimes in irc, but don't see her now | |
| 13:37:59 | bauzas | if folks want to stop using the opt, fine with me, but then, you at least need a nova-status check and a upgrade note | |
| 13:38:22 | bauzas | AFAICU, the opt was introduced as a temporary way | |
| 13:38:33 | bauzas | hence it being deprecated from the inception | |
| 13:38:46 | bauzas | so I understand why they want to raise an exception | |
| 13:39:06 | bauzas | but we at least need to think about operators already using it | |
| 13:39:11 | mriedem | i guess i need to understand https://review.openstack.org/#/c/457678/ | |
| 13:39:27 | bauzas | right, I was on https://github.com/openstack/nova/commit/05bf7269 | |
| 13:39:53 | bauzas | (I'm glad I'm done with other things so I can be back reviewing and participating \o/ ) | |
| 13:40:31 | mriedem | what i don't understand is this part from the config option help: | |
| 13:40:32 | mriedem | with the instance." | |
| 13:40:32 | mriedem | image's signing certificate using the set of trusted certificates associated | |
| 13:40:32 | mriedem | "During image signature verification nova will first verify the validity of the | |
| 13:40:43 | mriedem | at this point in the series, the instance doesn't have trusted certs | |
| 13:40:46 | mriedem | that comes later in the api change | |
| 13:41:19 | bauzas | yeah I saw | |
| 13:41:45 | bauzas | mriedem: in https://github.com/openstack/nova/commit/05bf7269 L113, they explain what they want | |
| 13:42:07 | mriedem | right that's what i asked about here https://review.openstack.org/#/c/479949/76/nova/image/glance.py@385 | |
| 13:42:42 | mriedem | this is the API change https://review.openstack.org/#/c/486204/109/nova/compute/api.py@970 | |
| 13:42:54 | mriedem | ids=CONF.glance.default_trusted_certificate_ids) | |
| 13:42:54 | mriedem | certs_to_return = objects.TrustedCerts( | |
| 13:42:54 | mriedem | CONF.glance.default_trusted_certificate_ids): | |
| 13:42:54 | mriedem | CONF.glance.enable_certificate_validation and | |
| 13:42:54 | mriedem | elif (CONF.glance.verify_glance_signatures and | |
| 13:43:15 | mriedem | ^ is essentially this glance download() logic except it doesn't use default_trusted_certificate_ids | |
| 13:45:01 | bauzas | mriedem: mmm, I don't like the API magically guessing which certs IDs to use | |
| 13:46:09 | bauzas | mriedem: so, IIUC, we store the certs in the instance ? | |
| 13:46:37 | bauzas | mriedem: and then, later on, the virt driver passes the certs to the glance API | |
| 13:47:03 | mriedem | if the user provides trusted cert IDs, we use those, | |
| 13:47:31 | mriedem | if the user does not provide trusted cert IDs, but image sig verification is enabled, and default_trusted_certificate_ids is set, we use those | |
| 13:47:34 | bauzas | oh, and if it doesn't but nova is configured, we store the default ones | |
| 13:47:37 | mriedem | yes they are persisted with the instance | |
| 13:47:45 | mriedem | by default, there are no default ones, but yes | |
| 13:47:59 | bauzas | ok, so, follow my logic about why I don't like us persisting the defaults | |
| 13:48:19 | bauzas | say I'm an operator and I did set some default certs | |
| 13:48:36 | bauzas | some users went by, and some instances now persisted my default certs | |
| 13:48:40 | bauzas | that's awesome | |
| 13:49:02 | bauzas | but then, I need to remove those default certs because of any security concern | |
| 13:49:10 | bauzas | revoke it, if you prefer | |
| 13:49:14 | bauzas | but then I'm stuck | |
| 13:49:27 | mriedem | you can rebuild those instances | |
| 13:49:41 | bauzas | because I don't know whether the cert is in the instance was persisted because the user asked for it, or because we defaulted it | |
| 13:50:04 | mriedem | well, they are IDs | |
| 13:50:06 | bauzas | of course, yeah you can rebuild | |
| 13:50:20 | mriedem | so if you were looking at the db, then you know which cert IDs were your defaults that you're revoking | |
| 13:50:41 | bauzas | but from a guessing logic, I have no idea whether my IDs that are in the instance are those given by the user or by a very old configuration | |
| 13:51:24 | mriedem | i'm not sure how much this matters, | |
| 13:51:49 | mriedem | if the instance is storing cert IDs for defaults that you've revoked, those cert IDs aren't getting used - they were used when spawning the instance | |
| 13:52:10 | mriedem | they would fail if you tried to rebuild the instance w/o providing new trusted certs | |
| 13:52:18 | sahid | dansmith, jaypipes, you might have already think about that but... what about to add a 'version' attribute for our objects fields | |
| 13:52:27 | mriedem | or rebuilding and specifying trusted_certs=None to get the new defaults i guess | |
| 13:52:33 | sahid | something like fields.IntegerField(version=2.0, nullable=True) | |
| 13:52:55 | sahid | so we could remove that obj_make_compatible method from all of our objects | |
| 13:52:58 | mriedem | sahid: there is already a version on the object | |
| 13:53:00 | sahid | to have it only in the base class | |
| 13:53:08 | mriedem | oh version per field.... | |
| 13:53:12 | sahid | yes | |
| 13:53:33 | dansmith | sahid: removing the field isn't the only action we need to take during a backport, | |
| 13:53:40 | dansmith | it's just a common one | |
| 13:54:08 | sahid | but that seems a bit hacky to have that obj_make_compatible() method everywhere, no? | |
| 13:54:29 | sahid | we could probably provide something smart to avoid that change each time we add new field | |
| 13:55:05 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Remove network info stubbing in functional test https://review.openstack.org/570018 | |
| 13:55:06 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Add request_spec.RequestGroup versioned object https://review.openstack.org/568840 | |
| 13:55:07 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Add requested_resources field to RequestSpec https://review.openstack.org/567267 | |
| 13:55:08 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Add bandwidth related standard resource classes https://review.openstack.org/570847 | |
| 13:55:09 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Transfer port.resource_request to the scheduler https://review.openstack.org/567268 | |
| 13:55:10 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Send resource allocations in the port binding https://review.openstack.org/569459 | |
| 13:55:11 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Test boot with more ports with bandwidth request https://review.openstack.org/573317 | |
| 13:55:33 | dansmith | I dunno, it's two lines of code per thing we add, and it makes people think about what needs to happen to backport the thing | |
| 13:56:18 | dansmith | we used to have a mapping for backporting nested objects, and maintaining that was not free either | |
| 13:56:22 | sahid | yes i don't know eother, it's just a suggestion | |
| 13:57:03 | dansmith | version per field kinda adds noise to the definition too, which isn't great | |
| 13:58:08 | sahid | yep true, but beside of that we could avoid to have the obj_make_compatible() method + unit test related | |
| 13:58:32 | dansmith | sahid: you could add to our base object an implementation of make compatible that, if field in self.obj_field_history: delete if < self.obj_field_history[field] | |
| 13:58:45 | dansmith | it'd still be one line per change though | |
| 14:00:35 | sahid | what this obj_field_history is supposed to carry? | |
| 14:00:44 | dansmith | sahid: hang on I'll show you what I mean | |
| 14:01:09 | mriedem | bauzas: http://lists.openstack.org/pipermail/openstack-dev/2018-June/131410.html | |
| 14:01:40 | mriedem | guess i can go back and read the giant spec on this | |
| 14:02:10 | openstackgerrit | Simon Dodsley proposed openstack/nova master: Add enhanced KVM storage QoS quotas https://review.openstack.org/558530 | |
| 14:02:13 | dansmith | sahid: something like this: https://pastebin.com/waWNUM3U | |
| 14:02:27 | dansmith | sahid: (that is in base.NovaObject btw) | |
| 14:03:10 | sahid | dansmith: oh i see | |
| 14:03:38 | sahid | not a big fan :) but yes it's a way to achieve the same thing | |
| 14:04:44 | mriedem | bauzas: relevant parts of the spec https://specs.openstack.org/openstack/nova-specs/specs/rocky/approved/nova-validate-certificates.html#proposed-change | |
| 14:04:51 | mriedem | https://specs.openstack.org/openstack/nova-specs/specs/rocky/approved/nova-validate-certificates.html#other-deployer-impact | |
| 14:05:30 | mriedem | i think it just means, enable_certificate_validation=True means enforce cert validation and default_trusted_certificate_ids is if you enforce and don't want to kill your users if they don't provide trusted certs in the server create/rebuild requests | |
| 14:06:04 | mriedem | both get used in the API changes, and you wouldn't want to set enable_certificate_validation=True UNTIL the api changes are in place to use default_trusted_certificate_ids | |
| 14:06:09 | mriedem | at least as things are written today | |
| 14:07:03 | mriedem | since enable_certificate_validation is deprecated, i'm not sure at what point it is removed, | |
| 14:07:09 | mriedem | nor do i understand how it's a transition to anything | |
| 14:07:41 | mriedem | i guess both options together are meant to enable the transition to cert validation of images | |
| 14:07:48 | mriedem | before forcing it on users | |
| 14:11:38 | bauzas | mriedem: sorry was afk | |