`gcp_storage_object` does not work

[nkakouros]

SUMMARY

When trying to upload an object, the module gives a python traceback and no object is uploaded.

ISSUE TYPE

• Bug Report

COMPONENT NAME

gcp_storage_object

ANSIBLE VERSION

Just checked out the google.cloud collection.

ACTUAL RESULTS

The full traceback is:
Traceback (most recent call last):
  File "<stdin>", line 102, in <module>
  File "<stdin>", line 94, in _ansiballz_main
  File "<stdin>", line 40, in invoke_module
  File "/usr/lib/python3.8/runpy.py", line 207, in run_module
    return _run_module_code(code, init_globals, run_name, mod_spec)
  File "/usr/lib/python3.8/runpy.py", line 97, in _run_module_code
    _run_code(code, mod_globals, init_globals,
  File "/usr/lib/python3.8/runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "/tmp/ansible_gcp_storage_object_payload_gqp67u8p/ansible_gcp_storage_object_payload.zip/ansible_collections/google/cloud/plugins/modules/gcp_storage_object.py", line 301, in <module>
  File "/tmp/ansible_gcp_storage_object_payload_gqp67u8p/ansible_gcp_storage_object_payload.zip/ansible_collections/google/cloud/plugins/modules/gcp_storage_object.py", line 203, in main
  File "/usr/lib/python3.8/site-packages/google/cloud/storage/blob.py", line 180, in __init__
    name = _bytes_to_unicode(name)
  File "/usr/lib/python3.8/site-packages/google/cloud/_helpers.py", line 389, in _bytes_to_unicode
    raise ValueError("%r could not be converted to unicode" % (value,))
ValueError: None could not be converted to unicode
failed: [viktor-cloud-hopper -> localhost] (item=/home/nikos/Projects/ethical-hacking/EN2720/data/.tmp/roles/twmn.mamma-nubes/function.spec) => {
    "ansible_loop_var": "item",
    "changed": false,
    "item": "/home/nikos/Projects/ethical-hacking/EN2720/data/.tmp/roles/twmn.mamma-nubes/function.spec",
    "module_stderr": "Traceback (most recent call last):\n  File \"<stdin>\", line 102, in <module>\n  File \"<stdin>\", line 94, in _ansiballz_main\n  File \"<stdin>\", line 40, in invoke_module\n  File \"/usr/lib/python3.8/runpy.py\", line 207, in run_module\n    return _run_module_code(code, init_globals, run_name, mod_spec)\n  File \"/usr/lib/python3.8/runpy.py\", line 97, in _run_module_code\n    _run_code(code, mod_globals, init_globals,\n  File \"/usr/lib/python3.8/runpy.py\", line 87, in _run_code\n    exec(code, run_globals)\n  File \"/tmp/ansible_gcp_storage_object_payload_gqp67u8p/ansible_gcp_storage_object_payload.zip/ansible_collections/google/cloud/plugins/modules/gcp_storage_object.py\", line 301, in <module>\n  File \"/tmp/ansible_gcp_storage_object_payload_gqp67u8p/ansible_gcp_storage_object_payload.zip/ansible_collections/google/cloud/plugins/modules/gcp_storage_object.py\", line 203, in main\n  File \"/usr/lib/python3.8/site-packages/google/cloud/storage/blob.py\", line 180, in __init__\n    name = _bytes_to_unicode(name)\n  File \"/usr/lib/python3.8/site-packages/google/cloud/_helpers.py\", line 389, in _bytes_to_unicode\n    raise ValueError(\"%r could not be converted to unicode\" % (value,))\nValueError: None could not be converted to unicode\n",
    "module_stdout": "",
    "msg": "MODULE FAILURE\nSee stdout/stderr for the exact error",
    "rc": 1
}

[Tobrek]

Any news at this issue? Today i run into the same problem. I had a rpm file stored in a bucket and try to download it using gcp_storage_object. The size of the file doubled and the file type changed from "RPM bin" to "data".

At the target instance the downloaded file looks like: [root@target-instance]# ls -l my_package.rpm -rw-r--r--. 1 root root 25784644 18. Aug 15:03 my_package.rpm [root@target-instance]# file my_package.rpm my_package.rpm: data

At my local system as reference it looks like local$ ls -l my_package.rpm -rw-rw-r-- 1 me me 12697788 Aug 18 17:24 my_package.rpm local$ file my_package.rpm my_package.rpm: RPM v3.0 bin i386/x86_64

[nkakouros]

@toumorokoshi This bug is still present. Could you take a look? Thank you for reviving this collection.

[toumorokoshi]

@nkakouros can you paste an example? Looking at the code for this issue, that error is raised when it can't resolve the remote file path.

Wondering what parameters would trigger that.

@Tobrek your issues sounds different. Can you file a different issue? I'll update the title to pertain specifically to the original issue.

[nkakouros]

I am using this:

- name: Create storage bucket
  google.cloud.gcp_storage_bucket:
    name: 'blueprint-1f99b38bb9d43dc28961f671f957067b24ccbefg'
    project: "{{ my_project_here }}"
    storage_class: REGIONAL
    location: "{{ my_region_here }}"

- name: Upload object
   google.cloud.gcp_storage_object:
    bucket: 'blueprint-1f99b38bb9d43dc28961f671f957067b24ccbefg'
    src: "{{ local_path }}"
    action: upload

Experimenting, I found out that if I set the dest parameter in the second task, the module works ok. But the parameter is not required and I assume above that it just uploads the file to the root of the bucket with the same file name as the local source. This would be on par with the interface of the gsutil cp command.

Another issue that I encounter is that the first task returns always as changed.

[toumorokoshi]

Experimenting, I found out that if I set the dest parameter in the second task, the module works ok. But the parameter is not required and I assume above that it just uploads the file to the root of the bucket with the same file name as the local source. This would be on par with the interface of the gsutil cp command.

I think that's almost a working expectation, but what if the local path is an absolute path? Would you want to include the whole directory structure in the path?

It might be better to just be explicit: since the lack of a dest seems broken anyway. WDYT?

[toumorokoshi]

I guess it's conditionally required because it's only required for certain actions:

def local_file_path(module):
    if module.params["action"] == "download":
        return module.params["dest"]
    else:
        return module.params["src"]


def remote_file_path(module):
    if module.params["action"] == "download":
        return module.params["src"]
    elif module.params["action"] == "delete":
        return module.params["src"]
    else:
        return module.params["dest"]

Which I guess based on this usage is because "dest" and "src" mean different things depending on the action:

  src:
    description:
    - Source location of file (may be local machine or cloud depending on action). Cloud locations need to be urlencoded including slashes.
    required: false
    type: path
  dest:
    description:
    - Destination location of file (may be local machine or cloud depending on action). Cloud location need to be urlencoded including slashes.
    required: false
    type: path
  bucket:

Which, frankly, I think is a poor user experience: the meaning of a variable shouldn't change depending on context.

I think the true fix here is new variable names that have a consistent meaning (gcs_path and local_path?), but to address this specific issue, I think we can just fix the behavior by requiring dest (since upload is broken without it anyway).

Again I hesitate with defaulting to src because it could be some complex absolute path.

[nkakouros]

I think that even if src is absolute, the dest could be just the filename from src. This is what cp and gsutil cp do actually. The following example is taken from gsutil help cp:

gsutil cp *.txt gs://my-bucket

The destination it the above command is missing.

The above would be on par with the copy module, for example doing:

---

- hosts: localhost
  tasks:
    - copy:
        src: /tmp/file
        dest: /tmp/folder

Copies the file /tmp/file under the folder /tmp/folder, so that the path /tmp/folder/file now hosts the copied file.

[toumorokoshi]

Copies the file /tmp/file under the folder /tmp/folder, so that the path /tmp/folder/file now hosts the copied file.

Although it feels intuitive, I think the detection of a folder to add something to feels like a complex heuristic to me. Also today that's not what "dest" does: it's always a file path, and changing that to detect a folder would be change in behavioral expectations. If there's a PR with appropriately documented behavior I'd be open to merging it, but I personally disagree with the approach at the moment that I'd rather not code it that way.

I'd also rather not set expectation that ansible matches gsutil in all behavior: that's a game of catch-up that we'll never win as gsutil adds new functionality. So I don't want to advertise the gcp_storage_object with such a guarantee.

However, if dest is not present, stripping to the file name and copying it seems ok. How strongly do you feel about that as a requirement vs just asserting for dest?

[nkakouros]

However, if dest is not present, stripping to the file name and copying it seems ok. How strongly do you feel about that as a requirement vs just asserting for dest?

Personally I would do it this way, but as long as the behavior is documented and the errors handled gracefully it doesn't matter that much.

[toumorokoshi]

Thanks! I already have a PR written up to assert for dest, so I'll get that merged in at least.

DO you want me to re-open this in case you want the more complex matching logic to match gsutil?

[nkakouros]

Thank you for fixing this in some way! No need to reopen.