Fixes metadata format for 'ssh-keys' key

[kgaikwad]

Closes #454

[codecov]

Codecov Report

Merging #455 into master will decrease coverage by 22.06%.
The diff coverage is 20%.

@@             Coverage Diff             @@
##           master     #455       +/-   ##
===========================================
- Coverage   88.86%   66.79%   -22.07%     
===========================================
  Files         339      249       -90     
  Lines        5629     4258     -1371     
===========================================
- Hits         5002     2844     -2158     
- Misses        627     1414      +787

Impacted Files	Coverage Δ
lib/fog/compute/google/models/server.rb	30.84% <20%> (-43.78%)	⬇️
lib/fog/compute/google/models/servers.rb	20.75% <0%> (-75.48%)	⬇️
lib/fog/compute/google/models/addresses.rb	28.57% <0%> (-71.43%)	⬇️
lib/fog/compute/google/models/subnetworks.rb	30.43% <0%> (-69.57%)	⬇️
lib/fog/compute/google/models/forwarding_rules.rb	31.81% <0%> (-68.19%)	⬇️
lib/fog/compute/google/models/disks.rb	32% <0%> (-68%)	⬇️
lib/fog/compute/google/models/images.rb	24% <0%> (-66%)	⬇️
lib/fog/compute/google/models/target_pools.rb	29.16% <0%> (-62.5%)	⬇️
lib/fog/compute/google/models/target_instances.rb	29.16% <0%> (-62.5%)	⬇️
lib/fog/compute/google/requests/insert_server.rb	22.22% <0%> (-60%)	⬇️
... and 241 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 551ebb5...a8a26c1. Read the comment docs.

[kgaikwad]

@Temikus,
I would like to know if any changes required from myside for this PR to get it merge?

[Temikus]

@kgaikwad PTAL at my comments in #454

While this would make the keys work with your key format, it will most likely break it for others, as if someone just reads off a key generated by ssh-keygen it will now look like:

ssh-rsa AAAALONGKEY== user@host user

, which would be invalid.

If you want to proceed with this PR (for example some library/util gives you an ssh key without postfix) - you'll need to make the statement conditional. E.g. if the key doesn't have the username at the end, add it.

Makes sense?

[icco]

As much as I hate adding new dependencies, we could use something like https://github.com/bensie/sshkey to parse incoming keys and add a comment if one was missing.

[Temikus]

@icco Using a full-blown generator lib to parse keys would IMO be overkill. TBH I think just notifying of the correct format would suffice, as the problem is with the API accepting something that the console can’t parse.

Or am I missing something here?

[kgaikwad]

I also agree with @Temikus.
Probably, I will make my statement conditional as you mentioned. I will add username at the end only if comment is absent.

[ezr-ondrej]

I believe it is worth a while to use proper parser for validating correct format of the key, or at least document the supported formats. That being said, we could just easily add the comment part only if missing...

[kgaikwad]

I believe it is worth a while to use proper parser for validating correct format of the key, or at least document the supported formats. That being said, we could just easily add the comment part only if missing...

Thank you @ezr-ondrej for review.
Yeah, this is what we have discussed to add username only if comment is absent.
If everyone agree on adding a parser and raising an error for invalid format then I will be happy to add your suggested changes.

It would be good if we document those formats but in google document, these details are already present.

[icco]

I would prefer to use an external parser, especially one as lightweight as what was suggested. We had it a long time ago but removed it when they dropped 1.8 support before we did, iirc.

…

On Fri, May 31, 2019, 07:23 kgaikwad ***@***.***> wrote: I believe it is worth a while to use proper parser for validating correct format of the key, or at least document the supported formats. That being said, we could just easily add the comment part only if missing... Thank you @ezr-ondrej <https://github.com/ezr-ondrej> for review. Yeah, this is what we have discussed to add username only if comment is absent. If everyone agree on adding a parser and raising an error for invalid format then I will happy to add your suggested changes. It would be good if we document those formats but in google document <https://cloud.google.com/compute/docs/instances/adding-removing-ssh-keys>, these details are already present. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#455?email_source=notifications&email_token=AAAE52JXAQKIVSE2KF4YRWTPYEDEVA5CNFSM4HF7WDZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWU6VIY#issuecomment-497674915>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAE52NRMG5PYPINDMBLOZTPYEDEVANCNFSM4HF7WDZA> .

[codecov]

Codecov Report

Merging #455 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #455      +/-   ##
==========================================
+ Coverage   88.86%   88.86%   +<.01%     
==========================================
  Files         339      339              
  Lines        5629     5633       +4     
==========================================
+ Hits         5002     5006       +4     
  Misses        627      627

Impacted Files	Coverage Δ
lib/fog/compute/google/models/server.rb	75.12% <100%> (+0.5%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 551ebb5...261e959. Read the comment docs.

[kgaikwad]

Updated PR! I will squash changes to single commit once ready for merge.

Though comment field is optional one, in document it clearly states that public-key should match format:ssh-rsa [KEY_VALUE] [USERNAME]
Here, I am still thinking that handling a condition to check comment presence should be sufficient.
Apart from this, I don't see anything else we required here.
Google is already doing a validation part then using parser, what we will achieve differently?

Please let me know if I am missing anything or are there any places where it is needed?

[ezr-ondrej]

I guess it is good enough. As the method is pretty small and straight forward, I guess we do not need to use external library to achieve the same thing.

[icco]

One small change

[kgaikwad]

Thank you @ezr-ondrej and @icco! Updated PR as per suggested changes.

[ezr-ondrej]

👍 thanks @kgaikwad, LGTM

[ezr-ondrej]

What do you think @Temikus ?

[Temikus]

@kgaikwad @ezr-ondrej LGTM overall, thank you! 😃

One small request, is it possible to add a test for this? I'm asking since all the integrations I maintain do not use this so I want to make sure we don't break your use-case down the road.

It should be super-trivial - just add a test method to https://github.com/fog/fog-google/blob/master/test/integration/compute/core_compute/test_servers.rb

Something like:

  def test_key_add
      server = @factory.create
      server.add_key(...)
      # Add the assertion here
  end

If you don't have time for it I understand. Just give me an example of your interaction and how you generate the key so I can add one later.

[kgaikwad]

@Temikus, thank you for merging this change and sorry for delay in response.
For Generating private and public SSH keys, we are using gem sshkey.
While creating instance, I am providing a public key along with user (which varies per image).

I will create pull-request which will add small test as you suggested.