| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-03-19 | |||
| 08:56:34 | gibi | lyarwood: give me 20 mins and I can make a proper conversation... | |
| 08:57:18 | lyarwood | gibi: yeah np I'll reply in the review | |
| 09:06:07 | bauzas | brinzhang0: sure, will review https://review.opendev.org/c/openstack/nova/+/778440 | |
| 09:06:17 | bauzas | as it's an important bugfix for Wallaby | |
| 09:06:42 | brinzhang0 | bauzas: thanks | |
| 09:06:53 | bauzas | brinzhang0: actually, can you please rebase it above https://review.opendev.org/c/openstack/nova/+/761452 ? | |
| 09:08:13 | brinzhang0 | It seems not impact for your patch, right? | |
| 09:09:35 | bauzas | brinzhang0: nevermind, I'll +W your patch and then I'll rebase my one | |
| 09:10:17 | brinzhang0 | bauzas: thanks, there are not order ^ | |
| 09:10:27 | bauzas | brinzhang0: unfortunately, yes | |
| 09:10:34 | bauzas | see the merge conflicts | |
| 09:10:44 | bauzas | but I'll rebase my patch | |
| 09:11:23 | brinzhang0 | bauzas: thanks, may conflict the test case in compute_mgr | |
| 09:22:19 | lucasagomes | hi, could somebody take a look at https://review.opendev.org/c/openstack/nova/+/776419, https://review.opendev.org/c/openstack/nova/+/776944 and https://review.opendev.org/c/openstack/nova/+/776934 ? | |
| 09:22:46 | lucasagomes | they r small patches towards enabling OVN to be the default backend in DevStack at the beggining of the next release cycle | |
| 09:22:59 | gibi | lyarwood: thanks for the reply. Is there anything else that I need to think about in the detach series? | |
| 09:23:02 | lucasagomes | it makes sure the nova gate won't break once we flip it | |
| 09:28:43 | nautik | gibi: Hi! Following our chat the other day, I pushed the blueprint https://blueprints.launchpad.net/nova/+spec/allow-special-characters-in-keypair-name along with the merge request https://review.opendev.org/c/openstack/nova/+/781076 | |
| 09:29:55 | nautik | gibi: reviews and comments are welcome, first time doing this and not yet so-much familiar with nova code; don't hesitate to tell me if you see missing tests for example | |
| 09:31:10 | gibi | nautik: thanks | |
| 09:38:09 | gibi | gmann, stephenfin, bauzas: can we extend accepted charachter set of keypair names without microversion? | |
| 09:38:25 | gibi | see the bp above | |
| 09:39:08 | gibi | the API ref and the json validation code in nova does not restrict a charset for the name but we have some custom code that rejectes @ for example | |
| 09:40:07 | gibi | the custom check is You specified more local devices than the limit allows | |
| 09:40:10 | gibi | sorry | |
| 09:40:20 | gibi | the custom check is https://opendev.org/openstack/nova/commit/c8b0a9a3be7ca276d91d470a629fdd0209812993 | |
| 09:43:49 | gibi | strictly speaking there everyting that worked before will work after we extend the charset so we are not breaking existing clients | |
| 09:43:50 | bauzas | gibi: looking | |
| 09:44:18 | gibi | except for those clients that relied on getting http 400 with keypair names containing @ for example | |
| 09:44:26 | bauzas | gibi: well, in between clouds, this would change, right? | |
| 09:44:45 | bauzas | as a user, how could I know I could use specific chars ? | |
| 09:44:56 | gibi | bauzas: yes, so for discoverability it would need a new microversion | |
| 09:45:10 | bauzas | for interop, yes | |
| 09:45:30 | bauzas | because when I'm creating my keypair, I want to make sure this works | |
| 09:45:40 | bauzas | i could have scripts for creating such keypairs | |
| 09:46:02 | gibi | today you have scripts to create keypairs without @ as @ is not allowed | |
| 09:46:07 | bauzas | also, does the DB accept this, btw ? | |
| 09:46:08 | gibi | this script will work in the future too | |
| 09:46:15 | gibi | bauzas: good question about the db | |
| 09:46:17 | gibi | looking | |
| 09:46:36 | gibi | name = Column(String(255), nullable=False) | |
| 09:46:40 | gibi | db will accept it | |
| 09:47:05 | bauzas | so it's purely an API restriction ? | |
| 09:47:20 | gibi | the thing that will not work is a script that relies on this proposed change using @ in the names, towards a cloud that does not have this change | |
| 09:47:30 | bauzas | what this blueprint is trying to fix ? | |
| 09:47:39 | bauzas | ah I see | |
| 09:47:43 | bauzas | the arobase case | |
| 09:47:46 | bauzas | and the dot one | |
| 09:48:12 | gibi | it is common to have an email address or a domain name in the keypair name | |
| 09:48:16 | bauzas | I see people wanting to use email adresses as keypair names :p | |
| 09:48:40 | bauzas | that, after the instance name... | |
| 09:48:44 | Corwin | bauzas: usually a public key will have a comment in the form of user@hostname | |
| 09:49:00 | Corwin | and hostname can be a fqdn, with dots in it | |
| 09:49:08 | bauzas | Corwin: you're working on clouds | |
| 09:49:15 | bauzas | so users don't know the hosts | |
| 09:49:24 | bauzas | this is irrelevant from this perspective | |
| 09:49:27 | bauzas | hence the string | |
| 09:50:03 | bauzas | and my ssh keynames don't have an arobase :) | |
| 09:50:25 | Corwin | what I'm saying is that a lot of users will generate keypairs and name those after the comment in the public key | |
| 09:50:31 | Corwin | or at least would want to | |
| 09:51:14 | gibi | like when I upload my key to gerrit it also names after the comment | |
| 09:51:28 | Corwin | github does this too | |
| 09:51:33 | bauzas | for knowing where you created the key ? | |
| 09:51:40 | bauzas | and from which user ? | |
| 09:52:01 | Corwin | yes, it's the default behavior of a lot of services to pre-fill the name from the comment | |
| 09:52:38 | bauzas | this comment comes from the fact ssh-keygen stupidely writes who created the key and where | |
| 09:52:51 | bauzas | but that's stale information | |
| 09:53:02 | bauzas | as you can create the key elsewhere and just use it for other things | |
| 09:53:12 | Corwin | I know, but it doesn't really matter | |
| 09:53:42 | bauzas | Corwin: you recognize that identifying a public key doesn't rely on matching the comment ? | |
| 09:53:54 | Corwin | there is no reason to disable those characters though | |
| 09:53:57 | bauzas | but rather on matching the key itself | |
| 09:54:06 | Corwin | bauzas: I know how ssh works thanks | |
| 09:54:09 | bauzas | as I could have N keys created with the same comment | |
| 09:54:15 | gibi | bauzas: the machine protocol use the key itself but humans are bad at matching long strings by eye | |
| 09:54:31 | bauzas | Corwin: sure, I'm just pointing that you could end up having multiple keys in Nova that would share the same name | |
| 09:54:35 | bauzas | which doesn't help | |
| 09:55:21 | Corwin | that would happen also without authorizing those 2 characters, it's still user input | |
| 09:55:36 | bauzas | either way, we're digressing, I agree | |
| 09:55:41 | bauzas | the question is not about the use case | |
| 09:55:52 | bauzas | but whether this sounds interoperable | |
| 09:56:02 | bauzas | and my guts tell me it's not so we need a microversion | |
| 09:56:20 | Corwin | yep I think it would be better with a new microversion | |
| 09:57:04 | gibi | OK I can accept the reasoning that the end user should know if the cloud support @ in the name and the way to publis that informatin is via the /version endpoint telling the max supported microversion | |
| 09:57:18 | Corwin | a script should always work on one microversion regardless of the service provider | |
| 09:58:06 | bauzas | gibi: I guess we unique index the keynames ? | |
| 09:58:21 | bauzas | (I'm lazy and you already opened the code :p ) | |
| 09:58:31 | gibi | __table_args__ = ( | |
| 09:58:31 | gibi | schema.UniqueConstraint("user_id", "name", "deleted", | |
| 09:58:31 | gibi | name="uniq_key_pairs0user_id0name0deleted"), | |
| 09:58:34 | gibi | ) | |
| 09:58:46 | bauzas | that will be fun | |
| 09:58:47 | gibi | so yes | |
| 09:58:48 | gibi | per user | |
| 09:59:04 | bauzas | "I don't understand why Nova isn't accepting my keyname, boo" | |
| 09:59:13 | gibi | bauzas: it is already like that | |
| 09:59:17 | bauzas | I know | |
| 09:59:23 | gibi | we dont change the uniqueness constraint | |
| 09:59:26 | bauzas | but users had to name it explicitely | |
| 09:59:37 | Corwin | that doesn't change | |