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.

Sign up for GitHub

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

helm_pull: Silence false no_log warning #796

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

helm_pull: Silence false no_log warning #796

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8bfaa5d
helm_pull: Silence false no_log warning
colshine1 Nov 22, 2024
61d1f46
docs: add changelog fragment
colshine1 Nov 23, 2024
d67ab45
test: update helm_pull intergration to test to confirm no_log message…
colshine1 Nov 24, 2024
7ecb8d6
test: module helm_pull returns warnings into stderr
colshine1 Dec 5, 2024
ecf31d8
Change no_log to false
gravesm Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelogs/fragments/796-false-positive-helmull.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
bugfixes:
- helm_pull - Apply no_log=True to pass_credentials to silence false positive warning.. (https://github.com/ansible-collections/kubernetes.core/pull/796).
2 changes: 1 addition & 1 deletion plugins/modules/helm_pull.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ def main():
repo_password=dict(
type="str", no_log=True, aliases=["password", "chart_repo_password"]
),
pass_credentials=dict(type="bool", default=False),
pass_credentials=dict(type="bool", default=False, no_log=True),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pass_credentials=dict(type="bool", default=False, no_log=True),
pass_credentials=dict(type="bool", default=False, no_log=False),

This should be False since this is a false positive.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be False since this is a false positive.

False is the default one and it will behave in the same way as now, it throws a warning Module did not set no_log for pass_credentials. So, it should be no_log=True

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, explicitly setting no_log=False is the correct way to deal with a false positive.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, explicitly setting no_log=False is the correct way to deal with a false positive.

Hm... I will check how it works

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I setup a sample test playbook:

- hosts: localhost
  tasks:
    - name: Download chart to controlhost
      kubernetes.core.helm_pull:
        chart_ref: "https://domain.com/helm_chart-0.0.1.tgz"
        destination: "/tmp/"
        untar_chart: false
        repo_username: user
        repo_password: password
        pass_credentials: false

I run the playbook with -vvv to get the module parameters in the output, the output fails as I set fake credentials for this test.

Without any changes to the module the output is:

[WARNING]: Module did not set no_log for pass_credentials
fatal: [localhost]: FAILED! => {
    "changed": false,
    "command": "helm pull https://domain.com/helm_chart-0.0.1.tgz --username user --******** ******** --destination /tmp/",
    "invocation": {
        "module_args": {
            "binary_path": null,
            "chart_ca_cert": null,
            "chart_devel": null,
            "chart_ref": "https://domain.com/helm_chart-0.0.1.tgz",
            "chart_ssl_cert_file": null,
            "chart_ssl_key_file": null,
            "chart_version": null,
            "destination": "/tmp/",
            "pass_credentials": false,
            "provenance": false,
            "repo_password": "VALUE_SPECIFIED_IN_NO_LOG_PARAMETER",
            "repo_url": null,
            "repo_username": "user",
            "skip_tls_certs_check": false,
            "untar_chart": false,
            "verify_chart": false,
            "verify_chart_keyring": null
        }
    },
    "msg": "Failure when executing Helm command.",
    "rc": 1,
    "stderr": "Error: failed to fetch https://domain.com/helm_chart-0.0.1.tgz : 401 Unauthorized\n",
    "stderr_lines": [
        "Error: failed to fetch https://domain.com/helm_chart-0.0.1.tgz : 401 Unauthorized"
    ],
    "stdout": "",
    "stdout_lines": []
}

setting no_log to False on line 192:

fatal: [localhost]: FAILED! => {
    "changed": false,
    "command": "helm pull https://domain.com/helm_chart-0.0.1.tgz --username user --******** ******** --destination /tmp/",
    "invocation": {
        "module_args": {
            "binary_path": null,
            "chart_ca_cert": null,
            "chart_devel": null,
            "chart_ref": "https://domain.com/helm_chart-0.0.1.tgz",
            "chart_ssl_cert_file": null,
            "chart_ssl_key_file": null,
            "chart_version": null,
            "destination": "/tmp/",
            "pass_credentials": false,
            "provenance": false,
            "repo_password": "VALUE_SPECIFIED_IN_NO_LOG_PARAMETER",
            "repo_url": null,
            "repo_username": "user",
            "skip_tls_certs_check": false,
            "untar_chart": false,
            "verify_chart": false,
            "verify_chart_keyring": null
        }
    },
    "msg": "Failure when executing Helm command.",
    "rc": 1,
    "stderr": "Error: failed to fetch https://domain.com/helm_chart-0.0.1.tgz : 401 Unauthorized\n",
    "stderr_lines": [
        "Error: failed to fetch https://domain.com/helm_chart-0.0.1.tgz : 401 Unauthorized"
    ],
    "stdout": "",
    "stdout_lines": []
}

setting no_log to False on line 192:

fatal: [localhost]: FAILED! => {
    "changed": false,
    "command": "helm pull https://domain.com/helm_chart-0.0.1.tgz --username user --******** ******** --destination /tmp/",
    "invocation": {
        "module_args": {
            "binary_path": null,
            "chart_ca_cert": null,
            "chart_devel": null,
            "chart_ref": "https://domain.com/helm_chart-0.0.1.tgz",
            "chart_ssl_cert_file": null,
            "chart_ssl_key_file": null,
            "chart_version": null,
            "destination": "/tmp/",
            "pass_credentials": false,
            "provenance": false,
            "repo_password": "VALUE_SPECIFIED_IN_NO_LOG_PARAMETER",
            "repo_url": null,
            "repo_username": "user",
            "skip_tls_certs_check": false,
            "untar_chart": false,
            "verify_chart": false,
            "verify_chart_keyring": null
        }
    },
    "msg": "Failure when executing Helm command.",
    "rc": 1,
    "stderr": "Error: failed to fetch https://domain.com/helm_chart-0.0.1.tgz : 401 Unauthorized\n",
    "stderr_lines": [
        "Error: failed to fetch https://domain.com/helm_chart-0.0.1.tgz : 401 Unauthorized"
    ],
    "stdout": "",
    "stdout_lines": []
}

I added no_log=True to be consistent with this: https://github.com/Akasurde/kubernetes.core/blob/22013686e7f2f735d5de5850b612dce4daa04b1a/plugins/modules/helm_repository.py#L231. However, it looks like this no longer exists in the module so I'm happy to update based on whatever is the correct solution.

skip_tls_certs_check=dict(type="bool", default=False),
chart_devel=dict(type="bool"),
untar_chart=dict(type="bool", default=False),
Expand Down