New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle missing source file in ssh_auth. #29693

Merged
merged 1 commit into from Jan 6, 2016

Conversation

Projects
None yet
4 participants
@abednarik
Contributor

abednarik commented Dec 15, 2015

Added one more condition to check if source file exists. In case source is set but source is not a file data and err variables are set to return a proper response.

Issue #29654

Hope doesn't hurt that my editor follow pep8 standards regarding indentation. In any case, just let me know.

@jfindlay

This comment has been minimized.

Show comment
Hide comment
@jfindlay

jfindlay Dec 15, 2015

Contributor

@abednarik, this is great, thanks. There is a test that needs to be updated.

Contributor

jfindlay commented Dec 15, 2015

@abednarik, this is great, thanks. There is a test that needs to be updated.

@abednarik

This comment has been minimized.

Show comment
Hide comment
@abednarik

abednarik Dec 15, 2015

Contributor

Thanks for advice @jfindlay. Will review this tonight if I get some time.

I will run setup test.py before pushing :(

Just a note, when using travis, you can run all test directly in the fork, without making a PR like this one and bother project owner's. Did you ever think about using Travis?

Thanks.

Contributor

abednarik commented Dec 15, 2015

Thanks for advice @jfindlay. Will review this tonight if I get some time.

I will run setup test.py before pushing :(

Just a note, when using travis, you can run all test directly in the fork, without making a PR like this one and bother project owner's. Did you ever think about using Travis?

Thanks.

@abednarik

This comment has been minimized.

Show comment
Hide comment
@abednarik

abednarik Dec 15, 2015

Contributor

My guest is his lne the one that make this test fail since source contains a query string.
I need to remove that query string before checking.

Would be great if someone can confirm this, since I still have pending undertanding this tests. My bad :(

Contributor

abednarik commented Dec 15, 2015

My guest is his lne the one that make this test fail since source contains a query string.
I need to remove that query string before checking.

Would be great if someone can confirm this, since I still have pending undertanding this tests. My bad :(

@jfindlay

This comment has been minimized.

Show comment
Hide comment
@jfindlay

jfindlay Dec 15, 2015

Contributor

@abednarik, if you are having difficulty updating the test, you are welcome to ping me and I'll look at it. We've looked into using travis multiple times and have even used it before, but chose jenkins instead as travis is underpowered/underfeatured for the features we need. Actually, most of the automatic testing work that is done against pull requests is handled by salt and salt-cloud and jenkins is basically a dispatcher and aggregator.

Contributor

jfindlay commented Dec 15, 2015

@abednarik, if you are having difficulty updating the test, you are welcome to ping me and I'll look at it. We've looked into using travis multiple times and have even used it before, but chose jenkins instead as travis is underpowered/underfeatured for the features we need. Actually, most of the automatic testing work that is done against pull requests is handled by salt and salt-cloud and jenkins is basically a dispatcher and aggregator.

@abednarik

This comment has been minimized.

Show comment
Hide comment
@abednarik

abednarik Dec 16, 2015

Contributor

I'm still trying to figure out this one. I will not leave this one for good or bad.

Contributor

abednarik commented Dec 16, 2015

I'm still trying to figure out this one. I will not leave this one for good or bad.

@abednarik

This comment has been minimized.

Show comment
Hide comment
@abednarik

abednarik Dec 17, 2015

Contributor

Hi @jfindlay

I need help :( Do I ping you in IRC? or do you want to review it and give me some feedback/help?

Thanks.

Contributor

abednarik commented Dec 17, 2015

Hi @jfindlay

I need help :( Do I ping you in IRC? or do you want to review it and give me some feedback/help?

Thanks.

@jfindlay

This comment has been minimized.

Show comment
Hide comment
@jfindlay

jfindlay Dec 17, 2015

Contributor

@abednarik, anywhere is fine.

Contributor

jfindlay commented Dec 17, 2015

@abednarik, anywhere is fine.

@jfindlay jfindlay self-assigned this Dec 17, 2015

@jfindlay

This comment has been minimized.

Show comment
Hide comment
@jfindlay

jfindlay Dec 18, 2015

Contributor

@abednarik, abednarik#1. The problem was that you were running os.path.isfile against the salt:// URL, but what we really need to do is check that salt://keyfile resolves to something accessible, whether a file or a remote resource of some kind for which there are already salt utilities.

Contributor

jfindlay commented Dec 18, 2015

@abednarik, abednarik#1. The problem was that you were running os.path.isfile against the salt:// URL, but what we really need to do is check that salt://keyfile resolves to something accessible, whether a file or a remote resource of some kind for which there are already salt utilities.

@abednarik

This comment has been minimized.

Show comment
Hide comment
@abednarik

abednarik Dec 18, 2015

Contributor

Thanks :)

Contributor

abednarik commented Dec 18, 2015

Thanks :)

@cachedout

This comment has been minimized.

Show comment
Hide comment
@cachedout

cachedout Dec 18, 2015

Contributor

Doesn't look like we're quite there. Still some lint errors and that test isn't passing. @jfindlay and @abednarik could you please have another look?

Contributor

cachedout commented Dec 18, 2015

Doesn't look like we're quite there. Still some lint errors and that test isn't passing. @jfindlay and @abednarik could you please have another look?

@abednarik

This comment has been minimized.

Show comment
Hide comment
@abednarik

abednarik Dec 18, 2015

Contributor

Sure thing @cachedout

Contributor

abednarik commented Dec 18, 2015

Sure thing @cachedout

@abednarik

This comment has been minimized.

Show comment
Hide comment
@abednarik

abednarik Dec 18, 2015

Contributor

Lint issue should be fixed now.

Contributor

abednarik commented Dec 18, 2015

Lint issue should be fixed now.

@jfindlay

This comment has been minimized.

Show comment
Hide comment
@jfindlay

jfindlay Dec 18, 2015

Contributor

@abednarik, it looks like you may have accidentally wiped out some commits.

diff --git a/salt/states/ssh_auth.py b/salt/states/ssh_auth.py
index aad85bc..e04cb5d 100644
--- a/salt/states/ssh_auth.py
+++ b/salt/states/ssh_auth.py
@@ -286,14 +286,12 @@ def present(
                 )
         return ret

-    # Split source into path and env since we need to check if path exists
-    # env is not relevant here that's why I use _
-    source_path, _ = salt.utils.url.split_env(source)
-    if source != '' and not os.path.isfile(source_path):
+    source_content = __salt__['cp.get_url'](source, None)
+    if source != '' and not source_content:
         data = 'fail'
         err = ('Failed to add the ssh key. Source file {0} is missing'
                .format(source))
-    elif source != '' and os.path.isfile(source_path):
+    elif source != '' and source_path:
         key = __salt__['cp.get_file_str'](
                 source,
                 saltenv=__env__)

and

diff --git a/salt/states/ssh_auth.py b/salt/states/ssh_auth.py
index e04cb5d..eb73c54 100644
--- a/salt/states/ssh_auth.py
+++ b/salt/states/ssh_auth.py
@@ -288,9 +288,7 @@ def present(

     source_path = __salt__['cp.get_url'](source, None)
     if source != '' and not source_path:
-        data = 'fail'
-        err = ('Failed to add the ssh key. Source file {0} is missing'
-               .format(source))
+        data = 'no key'
     elif source != '' and source_path:
         key = __salt__['cp.get_file_str'](
                 source,
@@ -341,6 +339,10 @@ def present(
         ret['changes'][name] = 'New'
         ret['comment'] = ('The authorized host key {0} for user {1} was added'
                           .format(name, user))
+    elif data == 'no key':
+        ret['result'] = False
+        ret['comment'] = ('Failed to add the ssh key. Source file {0} is '
+                          'missing'.format(source))
     elif data == 'fail':
         ret['result'] = False
         err = sys.modules[
Contributor

jfindlay commented Dec 18, 2015

@abednarik, it looks like you may have accidentally wiped out some commits.

diff --git a/salt/states/ssh_auth.py b/salt/states/ssh_auth.py
index aad85bc..e04cb5d 100644
--- a/salt/states/ssh_auth.py
+++ b/salt/states/ssh_auth.py
@@ -286,14 +286,12 @@ def present(
                 )
         return ret

-    # Split source into path and env since we need to check if path exists
-    # env is not relevant here that's why I use _
-    source_path, _ = salt.utils.url.split_env(source)
-    if source != '' and not os.path.isfile(source_path):
+    source_content = __salt__['cp.get_url'](source, None)
+    if source != '' and not source_content:
         data = 'fail'
         err = ('Failed to add the ssh key. Source file {0} is missing'
                .format(source))
-    elif source != '' and os.path.isfile(source_path):
+    elif source != '' and source_path:
         key = __salt__['cp.get_file_str'](
                 source,
                 saltenv=__env__)

and

diff --git a/salt/states/ssh_auth.py b/salt/states/ssh_auth.py
index e04cb5d..eb73c54 100644
--- a/salt/states/ssh_auth.py
+++ b/salt/states/ssh_auth.py
@@ -288,9 +288,7 @@ def present(

     source_path = __salt__['cp.get_url'](source, None)
     if source != '' and not source_path:
-        data = 'fail'
-        err = ('Failed to add the ssh key. Source file {0} is missing'
-               .format(source))
+        data = 'no key'
     elif source != '' and source_path:
         key = __salt__['cp.get_file_str'](
                 source,
@@ -341,6 +339,10 @@ def present(
         ret['changes'][name] = 'New'
         ret['comment'] = ('The authorized host key {0} for user {1} was added'
                           .format(name, user))
+    elif data == 'no key':
+        ret['result'] = False
+        ret['comment'] = ('Failed to add the ssh key. Source file {0} is '
+                          'missing'.format(source))
     elif data == 'fail':
         ret['result'] = False
         err = sys.modules[
@abednarik

This comment has been minimized.

Show comment
Hide comment
@abednarik

abednarik Dec 18, 2015

Contributor

Hhmm. Let me check this and fix it please.

Thanks.

Contributor

abednarik commented Dec 18, 2015

Hhmm. Let me check this and fix it please.

Thanks.

@abednarik

This comment has been minimized.

Show comment
Hide comment
@abednarik

abednarik Dec 21, 2015

Contributor

Sorry @jfindlay , I will need to review this. Hope I can do it tonight. Sorry for any inconvenience.

Contributor

abednarik commented Dec 21, 2015

Sorry @jfindlay , I will need to review this. Hope I can do it tonight. Sorry for any inconvenience.

@jfindlay jfindlay removed their assignment Dec 22, 2015

Handle missing source file in ssh_auth.
Added one more condition to check if source file exsists. In case source is set
but source is not a file data and err variables are set to return a proper response.

Fixes #29654
@abednarik

This comment has been minimized.

Show comment
Hide comment
@abednarik

abednarik Dec 28, 2015

Contributor

Hi @jfindlay hope you are doing well.

I made some tests and change looks good

----------
          ID: common_packages
    Function: pkg.installed
      Result: True
     Comment: All specified packages are already installed
     Started: 17:07:46.118792
    Duration: 263.864 ms
     Changes:   
----------
          ID: testuser
    Function: ssh_auth.present
      Result: False
     Comment: Failed to add the ssh key. Source file salt://ssh_keys/key_is_missing.pub is missing
     Started: 17:07:46.383898
    Duration: 2.577 ms
     Changes:   

Summary for vagrant-ubuntu-trusty-64
------------
Succeeded: 1
Failed:    1
------------
Total states run:     2

My guess is that tests is failing Jenkins test output because we only need to run this function if source is not empty. I made a small modification.

        source_path = __salt__['cp.get_url'](source, None)

I still need to build my own environment for destructive tests, that's why I'm pushing this.

Contributor

abednarik commented Dec 28, 2015

Hi @jfindlay hope you are doing well.

I made some tests and change looks good

----------
          ID: common_packages
    Function: pkg.installed
      Result: True
     Comment: All specified packages are already installed
     Started: 17:07:46.118792
    Duration: 263.864 ms
     Changes:   
----------
          ID: testuser
    Function: ssh_auth.present
      Result: False
     Comment: Failed to add the ssh key. Source file salt://ssh_keys/key_is_missing.pub is missing
     Started: 17:07:46.383898
    Duration: 2.577 ms
     Changes:   

Summary for vagrant-ubuntu-trusty-64
------------
Succeeded: 1
Failed:    1
------------
Total states run:     2

My guess is that tests is failing Jenkins test output because we only need to run this function if source is not empty. I made a small modification.

        source_path = __salt__['cp.get_url'](source, None)

I still need to build my own environment for destructive tests, that's why I'm pushing this.

@abednarik

This comment has been minimized.

Show comment
Hide comment
@abednarik

abednarik Dec 28, 2015

Contributor

Looks like test is no longer falling :)

Contributor

abednarik commented Dec 28, 2015

Looks like test is no longer falling :)

@jfindlay

This comment has been minimized.

Show comment
Hide comment
@jfindlay

jfindlay Dec 28, 2015

Contributor

Thanks, @abednarik.

Contributor

jfindlay commented Dec 28, 2015

Thanks, @abednarik.

basepi added a commit that referenced this pull request Jan 6, 2016

Merge pull request #29693 from abednarik/handle_missing_source_in_ssh…
…_auth

Handle missing source file in ssh_auth.

@basepi basepi merged commit 27df727 into saltstack:2015.8 Jan 6, 2016

2 of 5 checks passed

default Merged build finished.
Details
jenkins/salt-pr-linode-ubuntu14.04-n Salt PR - Linode Ubuntu 14.04 #3456 — FAILURE
Details
jenkins/salt-pr-rs-cent7-n Salt PR - RS CentOS 7 #10959 — FAILURE
Details
jenkins/salt-pr-clone Salt PR - Clone Repository #12375 — SUCCESS
Details
jenkins/salt-pr-lint-n Salt PR - Code Lint #12077 — SUCCESS
Details
@basepi

This comment has been minimized.

Show comment
Hide comment
@basepi

basepi Jan 6, 2016

Collaborator

Thanks @abednarik ! This looks great.

Collaborator

basepi commented Jan 6, 2016

Thanks @abednarik ! This looks great.

@abednarik abednarik deleted the abednarik:handle_missing_source_in_ssh_auth branch Jan 6, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment