Fixes #21449 - Drop host from known hosts on proxies #451

adamruzicka · 2019-11-25T08:27:24Z

No description provided.

app/models/concerns/foreman_remote_execution/smart_proxy_extensions.rb

app/models/concerns/foreman_remote_execution/orchestration/ssh.rb

tux93 · 2019-12-09T12:58:24Z

I tested this by applying it on a foreman-1.22.1 server and it seems currently the keys are only removed from known_hosts upon deletion but kept when only re-building a host.

To completely solve #21449 I believe the keys need to also be removed when re-building a host.

adamruzicka · 2019-12-09T13:10:34Z

@tux93 thank you for testing this. As far as I understood, with these changes the keys should be removed even on rebuild. Looks like I'll have to look into it a bit more

EDIT: Oh, I see. Good catch

adamruzicka · 2019-12-10T12:36:40Z

Tests are green-ish

tux93 · 2019-12-10T14:10:11Z

I tried again with the current state and keys are still only removed upon deletion, not rebuild, though that could also be due to the old foreman version on my test system

app/models/concerns/foreman_remote_execution/orchestration/ssh.rb

adamruzicka · 2019-12-11T13:02:43Z

app/models/concerns/foreman_remote_execution/orchestration/ssh.rb

+    extend ActiveSupport::Concern
+
+    included do
+      register_rebuild(:rebuild_ssh, N_("SSH_#{self.to_s.split('::').first}"))


What is this good for? I wasn't able to trigger anything using just this, I had to trigger things from after_validation callback

tux93 · 2019-12-20T13:42:42Z

I got around to re-test today and it works now for rebuilding too!

ares

Thanks @adamruzicka and @tux93 for testing, merging now!

ares · 2020-01-10T14:43:05Z

oh actually not merging, tests failures are related NameError: undefined local variable or method queue' for #Nic::Base:0x000000000beb5180`

ares

tests

adamruzicka · 2020-02-24T13:09:43Z

[test foreman_remote_execution]

adamruzicka · 2020-02-24T14:22:23Z

@tux93 could you please try this one more time?

tux93 · 2020-02-25T15:22:43Z

Re-tested with the latest version of the patch and it still works for me

adamruzicka · 2020-02-25T15:25:07Z

[test foreman_remote_execution]

adamruzicka · 2020-03-03T09:08:14Z

[test foreman_remote_execution]

houndci-bot · 2020-03-24T13:59:12Z

app/models/concerns/foreman_remote_execution/orchestration/ssh.rb

+
+    def should_drop_from_known_hosts?
+      host, = host_kind_target
+      host && host.build && host.changes.key?('build') && !orchestration_errors?


Style/SafeNavigation: Use safe navigation (&.) instead of checking if an object exists before calling the method.

houndci-bot · 2020-03-24T13:59:12Z

app/models/concerns/foreman_remote_execution/orchestration/ssh.rb

+    end
+
+    def queue_ssh_destroy
+      return unless should_drop_from_known_hosts?


Layout/EmptyLineAfterGuardClause: Add empty line after guard clause.

shouldn't we disable this cop? We've decided to disable it in core.. (theforeman/foreman#7518) this is exactly the case where it doesn't deserve the blank line.

I'm all in for this

houndci-bot · 2020-04-21T13:44:47Z

app/models/concerns/foreman_remote_execution/orchestration/ssh.rb

+
+    def should_drop_from_known_hosts?
+      host, = host_kind_target
+      host && host.build && host.changes.key?('build')


Style/SafeNavigation: Use safe navigation (&.) instead of checking if an object exists before calling the method.

this is still on, even though I'm not sure I like the new style more 🤔

Suggested change

host && host.build && host.changes.key?('build')

host&.build && host&.changes&.key?('build')

ezr-ondrej

Just nitpicks, but 👮‍♂️ is unhappy now.

ezr-ondrej · 2020-04-23T10:49:11Z

app/models/concerns/foreman_remote_execution/orchestration/ssh.rb

+
+    def should_drop_from_known_hosts?
+      host, = host_kind_target
+      host && host.build && host.changes.key?('build')


this is still on, even though I'm not sure I like the new style more 🤔

Suggested change

host && host.build && host.changes.key?('build')

host&.build && host&.changes&.key?('build')

app/models/concerns/foreman_remote_execution/orchestration/ssh.rb

ezr-ondrej · 2020-04-23T10:53:21Z

app/models/concerns/foreman_remote_execution/orchestration/ssh.rb

+    end
+
+    def queue_ssh_destroy
+      return unless should_drop_from_known_hosts?


shouldn't we disable this cop? We've decided to disable it in core.. (theforeman/foreman#7518) this is exactly the case where it doesn't deserve the blank line.

adamruzicka · 2020-05-04T15:34:13Z

We're finally 🍏

ezr-ondrej

works as expected, just don't understand why do we care about Ansible proxies here 🤔

app/models/concerns/foreman_remote_execution/orchestration/ssh.rb

ezr-ondrej

We could simplify, apart of it, I'm good and I can ignore redmine and squash on merge 👍

app/models/concerns/foreman_remote_execution/orchestration/ssh.rb

ezr-ondrej · 2020-05-05T12:04:06Z

WWFJ

ezr-ondrej · 2020-05-05T12:09:08Z

nooo! now we go again! xD I wanted to squash xD

adamruzicka · 2020-05-05T12:12:02Z

I can always dig through reflog and un-squash it if you want it that bad :)

ezr-ondrej · 2020-05-05T12:13:51Z

I can always dig through reflog and un-squash it if you want it that bad :)

Nah xD It was just about Jenkins being almost done xD and now he goes again 🕐

adamruzicka · 2020-05-05T15:17:54Z

🍏

tests are green

ezr-ondrej

Thanks @adamruzicka !

houndci-bot reviewed Nov 25, 2019

View reviewed changes

app/models/concerns/foreman_remote_execution/smart_proxy_extensions.rb Show resolved Hide resolved

app/models/concerns/foreman_remote_execution/orchestration/ssh.rb Outdated Show resolved Hide resolved

theforeman-bot added Needs testing Not yet reviewed labels Nov 25, 2019

adamruzicka force-pushed the known-hosts branch from 0925ad8 to b2c6d9b Compare November 25, 2019 08:28

adamruzicka force-pushed the known-hosts branch from b2c6d9b to cca744d Compare December 9, 2019 14:11

adamruzicka force-pushed the known-hosts branch from e70bddc to f943e8d Compare December 11, 2019 13:06

houndci-bot reviewed Dec 11, 2019

View reviewed changes

app/models/concerns/foreman_remote_execution/orchestration/ssh.rb Outdated Show resolved Hide resolved

adamruzicka force-pushed the known-hosts branch from f943e8d to 56936ff Compare December 11, 2019 13:08

adamruzicka commented Dec 11, 2019

View reviewed changes

ares approved these changes Jan 10, 2020

View reviewed changes

theforeman-bot removed the Not yet reviewed label Jan 10, 2020

ares previously requested changes Jan 10, 2020

View reviewed changes

theforeman-bot added the Waiting on contributor label Jan 10, 2020

adamruzicka force-pushed the known-hosts branch from 56936ff to 35e65ba Compare February 17, 2020 15:06

theforeman-bot added Needs re-review and removed Waiting on contributor labels Feb 17, 2020

adamruzicka force-pushed the known-hosts branch 2 times, most recently from 7446648 to 9e42dad Compare March 24, 2020 13:59

houndci-bot reviewed Mar 24, 2020

View reviewed changes

adamruzicka force-pushed the known-hosts branch from 9e42dad to 71151dc Compare April 21, 2020 13:44

houndci-bot reviewed Apr 21, 2020

View reviewed changes

ezr-ondrej reviewed Apr 23, 2020

View reviewed changes

ezr-ondrej reviewed May 5, 2020

View reviewed changes

app/models/concerns/foreman_remote_execution/orchestration/ssh.rb Outdated Show resolved Hide resolved

theforeman-bot added the Waiting on contributor label May 5, 2020

ezr-ondrej reviewed May 5, 2020

View reviewed changes

app/models/concerns/foreman_remote_execution/orchestration/ssh.rb Show resolved Hide resolved

Fixes #21449 - Drop host from known hosts on proxies

f9a6ba8

adamruzicka force-pushed the known-hosts branch from 9a6cf62 to f9a6ba8 Compare May 5, 2020 12:08

theforeman-bot removed the Waiting on contributor label May 5, 2020

ezr-ondrej approved these changes May 5, 2020

View reviewed changes

theforeman-bot removed the Needs re-review label May 5, 2020

ezr-ondrej merged commit 734acd4 into theforeman:master May 5, 2020

adamruzicka deleted the known-hosts branch May 6, 2020 10:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #21449 - Drop host from known hosts on proxies #451

Fixes #21449 - Drop host from known hosts on proxies #451

adamruzicka commented Nov 25, 2019

tux93 commented Dec 9, 2019

adamruzicka commented Dec 9, 2019 •

edited

adamruzicka commented Dec 10, 2019

tux93 commented Dec 10, 2019

adamruzicka Dec 11, 2019

tux93 commented Dec 20, 2019

ares left a comment

ares commented Jan 10, 2020

ares left a comment

adamruzicka commented Feb 24, 2020

adamruzicka commented Feb 24, 2020

tux93 commented Feb 25, 2020

adamruzicka commented Feb 25, 2020

adamruzicka commented Mar 3, 2020

houndci-bot Mar 24, 2020

houndci-bot Mar 24, 2020

ezr-ondrej Apr 23, 2020

adamruzicka Apr 23, 2020

houndci-bot Apr 21, 2020

ezr-ondrej Apr 23, 2020

ezr-ondrej left a comment

ezr-ondrej Apr 23, 2020

ezr-ondrej Apr 23, 2020

adamruzicka commented May 4, 2020

ezr-ondrej left a comment

ezr-ondrej left a comment

ezr-ondrej commented May 5, 2020

ezr-ondrej commented May 5, 2020

adamruzicka commented May 5, 2020

ezr-ondrej commented May 5, 2020

adamruzicka commented May 5, 2020

ezr-ondrej left a comment

	host && host.build && host.changes.key?('build')
	host&.build && host&.changes&.key?('build')

Fixes #21449 - Drop host from known hosts on proxies #451

Fixes #21449 - Drop host from known hosts on proxies #451

Conversation

adamruzicka commented Nov 25, 2019

tux93 commented Dec 9, 2019

adamruzicka commented Dec 9, 2019 • edited

adamruzicka commented Dec 10, 2019

tux93 commented Dec 10, 2019

Choose a reason for hiding this comment

tux93 commented Dec 20, 2019

ares left a comment

Choose a reason for hiding this comment

ares commented Jan 10, 2020

ares left a comment

Choose a reason for hiding this comment

adamruzicka commented Feb 24, 2020

adamruzicka commented Feb 24, 2020

tux93 commented Feb 25, 2020

adamruzicka commented Feb 25, 2020

adamruzicka commented Mar 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezr-ondrej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamruzicka commented May 4, 2020

ezr-ondrej left a comment

Choose a reason for hiding this comment

ezr-ondrej left a comment

Choose a reason for hiding this comment

ezr-ondrej commented May 5, 2020

ezr-ondrej commented May 5, 2020

adamruzicka commented May 5, 2020

ezr-ondrej commented May 5, 2020

adamruzicka commented May 5, 2020

ezr-ondrej left a comment

Choose a reason for hiding this comment

adamruzicka commented Dec 9, 2019 •

edited