Skip to content
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

Unhandled error in vault read_secret for anonymous users #15779

Closed
abretaud opened this issue Mar 13, 2023 · 10 comments
Closed

Unhandled error in vault read_secret for anonymous users #15779

abretaud opened this issue Mar 13, 2023 · 10 comments

Comments

@abretaud
Copy link
Contributor

Describe the bug
We're getting this kind of errors in handlers logs:

galaxy.jobs.runners ERROR 2023-03-10 15:43:02,249 [pN:handler_2,p:79568,tN:SlurmRunner.work_thread-2] (unknown) Unhandled exception calling fail_job
Traceback (most recent call last):
  File "/galaxy/server/lib/galaxy/jobs/runners/__init__.py", line 157, in run_next
    method(arg)
  File "/galaxy/server/lib/galaxy/jobs/runners/__init__.py", line 551, in fail_job
    fail_message, tool_stdout=tool_stdout, tool_stderr=tool_stderr, exception=exception
  File "/galaxy/server/lib/galaxy/jobs/__init__.py", line 1401, in fail
    for dataset_path in self.job_io.get_output_fnames():
  File "/galaxy/server/lib/galaxy/jobs/__init__.py", line 1066, in job_io
    file_sources_dict=self.app.file_sources.to_dict(for_serialization=True, user_context=user_context),
  File "/galaxy/server/lib/galaxy/files/__init__.py", line 182, in to_dict
    "file_sources": self.plugins_to_dict(for_serialization=for_serialization, user_context=user_context),
  File "/galaxy/server/lib/galaxy/files/__init__.py", line 174, in plugins_to_dict
    el = file_source.to_dict(for_serialization=for_serialization, user_context=user_context)
  File "/galaxy/server/lib/galaxy/files/sources/__init__.py", line 126, in to_dict
    rval.update(self._serialization_props(user_context=user_context))
  File "/galaxy/server/lib/galaxy/files/sources/_pyfilesystem2.py", line 86, in _serialization_props
    effective_props[key] = self._evaluate_prop(val, user_context=user_context)
  File "/galaxy/server/lib/galaxy/files/sources/__init__.py", line 187, in _evaluate_prop
    rval = fill_template(prop_val, context=template_context, futurized=True)
  File "/galaxy/server/lib/galaxy/util/template.py", line 131, in fill_template
    raise first_exception or e
  File "/galaxy/server/lib/galaxy/util/template.py", line 83, in fill_template
    return unicodify(t, log_exception=False)
  File "/galaxy/server/lib/galaxy/util/__init__.py", line 1141, in unicodify
    value = str(value)
  File "/galaxy/.venv/lib/python3.7/site-packages/Cheetah/Template.py", line 1053, in __unicode__
    return getattr(self, mainMethName)()
  File "cheetah_DynamicallyCompiledCheetahTemplate_1678459364_9984107_10358.py", line 86, in respond
  File "/galaxy/server/lib/galaxy/security/vault.py", line 201, in read_secret
    return self.vault.read_secret(f"user/{self.user.id}/{key}")
AttributeError: 'NoneType' object has no attribute 'id'
galaxy.jobs.runners.drmaa ERROR 2023-03-10 15:43:02,256 [pN:handler_2,p:79568,tN:SlurmRunner.work_thread-2] (2427346/None) User killed running job, but error encountered removing from DRM queue
Traceback (most recent call last):
  File "/galaxy/server/lib/galaxy/jobs/runners/drmaa.py", line 378, in stop_job
    assert ext_id not in (None, "None"), "External job id is None"
AssertionError: External job id is None

We have some specific file sources configured, with vault storage, and I suspect this error happens on anonymous users (not 100% sure)

Galaxy Version and/or server at which you observed the bug
Galaxy Version: 22.05
Commit: fa21013

To Reproduce
Steps to reproduce the behavior:

  1. not sure what triggered this
@mvdbeek mvdbeek changed the title [22.05] Unhandled exception calling fail_job Unhandled error in vault read_secret for anonymous users Mar 13, 2023
@mvdbeek
Copy link
Member

mvdbeek commented Mar 13, 2023

@nuwang is this what you had in mind ?

diff --git a/lib/galaxy/managers/context.py b/lib/galaxy/managers/context.py
index 5c8839f576..de34668609 100644
--- a/lib/galaxy/managers/context.py
+++ b/lib/galaxy/managers/context.py
@@ -214,7 +214,7 @@ class ProvidesUserContext(ProvidesAppContext):
     @property
     def user_vault(self):
         """Provide access to a user's personal vault."""
-        return UserVaultWrapper(self.app.vault, self.user)
+        return UserVaultWrapper(self.app.vault, self.user) if self.user else defaultdict(lambda: None)
 
     @property
     def anonymous(self) -> bool:

@abretaud
Copy link
Contributor Author

abretaud commented Mar 13, 2023

I've tested your suggestion Marius, but now I get this:

Traceback (most recent call last):
  File "/galaxy/server/lib/galaxy/jobs/runners/__init__.py", line 260, in prepare_job
    job_wrapper.prepare()
  File "/galaxy/server/lib/galaxy/jobs/__init__.py", line 1231, in prepare
    compute_environment = compute_environment or self.default_compute_environment(job)
  File "/galaxy/server/lib/galaxy/jobs/__init__.py", line 1336, in default_compute_environment
    return SharedComputeEnvironment(self.job_io, job)
  File "/galaxy/server/lib/galaxy/jobs/__init__.py", line 1066, in job_io
    file_sources_dict=self.app.file_sources.to_dict(for_serialization=True, user_context=user_context),
  File "/galaxy/server/lib/galaxy/files/__init__.py", line 182, in to_dict
    "file_sources": self.plugins_to_dict(for_serialization=for_serialization, user_context=user_context),
  File "/galaxy/server/lib/galaxy/files/__init__.py", line 174, in plugins_to_dict
    el = file_source.to_dict(for_serialization=for_serialization, user_context=user_context)
  File "/galaxy/server/lib/galaxy/files/sources/__init__.py", line 126, in to_dict
    rval.update(self._serialization_props(user_context=user_context))
  File "/galaxy/server/lib/galaxy/files/sources/_pyfilesystem2.py", line 86, in _serialization_props
    effective_props[key] = self._evaluate_prop(val, user_context=user_context)
  File "/galaxy/server/lib/galaxy/files/sources/__init__.py", line 187, in _evaluate_prop
    rval = fill_template(prop_val, context=template_context, futurized=True)
  File "/galaxy/server/lib/galaxy/util/template.py", line 131, in fill_template
    raise first_exception or e
  File "/galaxy/server/lib/galaxy/util/template.py", line 83, in fill_template
    return unicodify(t, log_exception=False)
  File "/galaxy/server/lib/galaxy/util/__init__.py", line 1141, in unicodify
    value = str(value)
  File "/galaxy/.venv/lib/python3.7/site-packages/Cheetah/Template.py", line 1053, in __unicode__
    return getattr(self, mainMethName)()
  File "cheetah_DynamicallyCompiledCheetahTemplate_1678724123_5518317_31337.py", line 86, in respond
TypeError: 'NoneType' object is not callable

(And found a way to reproduce on .fr: just uploading a file as anonymous user)

@mvdbeek
Copy link
Member

mvdbeek commented Mar 13, 2023

I've updated the diff, but I'll let @nuwang comment on this

@nuwang
Copy link
Member

nuwang commented Mar 13, 2023

I think we should apply the original change you proposed as a safeguard (unless we go for the last fix detailed below). I think a defaultdict will still fail because it will try to call read_secret on it.

@abretaud Can you post a snippet of how your filesources conf is using the vault? Usually it would be something like my_secret: ${user.user_vault.read_secret('my/secret/path')} (ref: #12940), but then, I'd expect the exception at the point of accessing user, so your template must be slightly different.

The fix may be to either conditionally access the secret in your template, or we could introduce a NoOpVault where all operations will return null values just to make it easier to deal with the anonymous user case.

@nuwang
Copy link
Member

nuwang commented Mar 13, 2023

I think something like this in your filesources template might work:

my_secret: |-
   #if user_vault
   ${user_vault.read_secret('my/secret/path')}
   #end if

@abretaud
Copy link
Contributor Author

I have to go, I can test more tomorrow, but at least here's my file_sources_conf.yml:

- type: dropbox
  id: dropbox1
  label: Dropbox files (configure access in user preferences)
  doc: Your Dropbox files - configure an access token via the user preferences
  accessToken: ${user.user_vault.read_secret('preferences/dropbox/access_token')}

- type: webdav
  id: owncloud1
  label: OwnCloud
  doc: External OwnCloud files (configure access in user preferences)
  url: ${user.user_vault.read_secret('preferences/owncloud/url')}
  root: ${user.user_vault.read_secret('preferences/owncloud/root')}
  login: ${user.user_vault.read_secret('preferences/owncloud/username')}
  password: ${user.user_vault.read_secret('preferences/owncloud/password')}

- type: ftp
  id: genouest-ftp
  label: "GenOuest FTP server"
  doc: "GenOuest FTP server"
  host: "ftp.genouest.org"
  user: "${user.user_vault.read_secret('preferences/genouest/username')}"
  passwd: "${user.user_vault.read_secret('preferences/genouest/password')}"
  timeout: 10
  port: 21
  tls: true
  writable: true

@abretaud
Copy link
Contributor Author

I think a defaultdict will still fail because it will try to call read_secret on it

Yep, tested, that's what happens

Tested also with this (adapting your suggestion, I guess we can't access user_vault directly right?)

my_secret: |-
     #if user and user.user_vault
     ${user.user_vault.read_secret('preferences/owncloud/url')}
     #end if

Getting this:

  File "/galaxy/.venv/lib/python3.7/site-packages/Cheetah/Template.py", line 1053, in __unicode__
    return getattr(self, mainMethName)()
  File "cheetah_DynamicallyCompiledCheetahTemplate_1678809697_96869_48343.py", line 86, in respond
NameError: name 'user' is not defined

The NoOpVault seems like a good idea, having to write an if statement in the config file feels a bit awkward no?

@nuwang
Copy link
Member

nuwang commented Mar 15, 2023

Hmm... It's not clear to me why that doesn't work, maybe I've forgotten Cheetah syntax, but I agree, it is very awkward. I'll look into adding a NoOpVault, with some suitable log messages as a replacement.

@abretaud
Copy link
Contributor Author

Hi! Any news on this @nuwang ? It is blocking all our anonymous jobs at the moment on .fr, but a bit too busy to try writing a patch on our own :/

@mvdbeek
Copy link
Member

mvdbeek commented Apr 25, 2023

#15858 should have fixed that. Thanks @abretaud & @tchaussepied!

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

No branches or pull requests

3 participants