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

HTTP Proxy customer cases update #14513

Merged
merged 1 commit into from
May 16, 2024
Merged

HTTP Proxy customer cases update #14513

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions pytest_fixtures/component/http_proxy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,4 @@ def setup_http_proxy(request, module_manifest_org, target_sat):
yield http_proxy, request.param
target_sat.update_setting('content_default_http_proxy', content_proxy_value)
target_sat.update_setting('http_proxy', general_proxy_value)
http_proxy.delete()
79 changes: 39 additions & 40 deletions tests/foreman/cli/test_http_proxy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,33 +85,55 @@ def test_positive_create_update_delete(module_org, module_location, target_sat):


@pytest.mark.tier3
@pytest.mark.run_in_one_thread
@pytest.mark.stubbed
def test_insights_client_registration_with_http_proxy():
@pytest.mark.no_containers
@pytest.mark.rhel_ver_match(r'^(?!6$)\d+$')
@pytest.mark.parametrize(
'setup_http_proxy',
[True, False],
indirect=True,
ids=['auth_http_proxy', 'unauth_http_proxy'],
)
def test_insights_client_registration_with_http_proxy(
module_target_sat,
setup_http_proxy,
rhel_contenthost,
rhcloud_activation_key,
rhcloud_manifest_org,
):
"""Verify that insights-client registration work with http proxy.

:id: 5158d5c1-2b88-4c05-914b-f1c53656ffc2

:customerscenario: true
:parametrized: yes

:steps:
1. Create HTTP Proxy.
2. Set created proxy as "Default HTTP Proxy" in settings.
3. Edit /etc/resolv.conf and comment out all entries so that
satellite can not directly communicate outside. Ensure that
NetworkManger won't change it.
Comment on lines -98 to -102
Copy link
Collaborator

Choose a reason for hiding this comment

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

As we're still following steps as part of setup with fixture, so can we keep this steps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, yes and no. Right now we don't do this kind of setup for any HTTP proxy testing, so this would be an exception. If we decide to hack the Satellite like this, I would rather do it for all tests through some fixture, not just for this particular case. It will require more testing for potential side-effects, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Gauravtalreja1 PoC for the above #14754

4. Register a host with satellite.
5. Register host with insights.
6. Try insights-client register/unregister/test-connection/status
:setup:
1. Satellite with Default HTTP Proxy set.

:BZ: 1959932
:steps:
1. Register a host with satellite.
2. Register host with insights.
3. Try insights-client register/unregister/test-connection/status

:expectedresults:
1. insights-client register/unregister/test-connection/status
works with http proxy set.
1. insights-client register/unregister/test-connection/status works with http proxy set.

:CaseAutomation: NotAutomated
:BZ: 1959932

:customerscenario: true
"""
rhel_contenthost.configure_rex(
satellite=module_target_sat, org=rhcloud_manifest_org, register=False
)
rhel_contenthost.configure_rhai_client(
satellite=module_target_sat,
Gauravtalreja1 marked this conversation as resolved.
Show resolved Hide resolved
activation_key=rhcloud_activation_key.name,
org=rhcloud_manifest_org.label,
rhel_distro=f"rhel{rhel_contenthost.os_version.major}",
)
assert rhel_contenthost.execute('insights-client --register').status == 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

configure_rhai_client() will register this client to satellite, so here are we checking if it could be registered again to same satelllite? if not, can we remove this assertion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is correct, I think testing re-register won't harm.

assert rhel_contenthost.execute('insights-client --test-connection').status == 0
assert rhel_contenthost.execute('insights-client --status').status == 0
assert rhel_contenthost.execute('insights-client --unregister').status == 0


@pytest.mark.tier2
Expand Down Expand Up @@ -163,29 +185,6 @@ def test_positive_set_content_default_http_proxy(block_fake_repo_access, target_
assert rpm_repo.read().content_counts['rpm'] >= 1


@pytest.mark.stubbed
@pytest.mark.tier3
def test_positive_environment_variable_unset_set():
"""Verify that satellite installer unsets and then sets back the environment variables

:id: 596d753b-660b-49cb-b663-ff3cec439564

:BZ: 1886040

:customerscenario: true

Comment on lines -167 to -176
Copy link
Collaborator

Choose a reason for hiding this comment

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

I remember this test, its related to old feature added in installer, which identifies and fails, if proxy is set on system, with this mentioned environment variable. So, I understand that this test is valid, but can I ask why are you considering removing it instead of automating it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered removing because the BZ says

3. Why does the customer need this? (List the business requirements here)

Since satellite maintain takes care of end to end upgrade, it should also have an additional check to unset an environment variable along with alerting the user.
The addition of this check will ensure that we can avoid the installer execution failure and fixing a potential issue at the pre-upgrade stage.

This coverage already exists here, so I considered it redundant.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see it as redundant as the test you linked is being verified by foreman-maintain check which verifies it it exists in the environment, and here we use the installer to test, where behaviour here is different, installers unsets the variable while running and then sets it back when installer run completes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, my assumptions were following:

  1. The foreman-maintain health check (covered by the test linked above) checks if HTTP_PROXY env var is set and fails if it is.
  2. Satellite upgrade leverages the foremain-maintain health check as pre-upgrade check.
  3. If the pre-upgrade check fails (e.g. because of the HTTP_PROXY env var is set or otherwise), no upgrade nor satellite-installer is run. So the satellite-installer unset/set functionality is unreachable and thus appears redundant to me.

Is any of the assumptions wrong?

:steps:
1. Export any environment variable from
[http_proxy, https_proxy, ssl_cert_file, HTTP_PROXY, HTTPS_PROXY, SSL_CERT_FILE]
2. satellite-installer

:expectedresults: satellite-installer unsets system proxy and SSL environment variables
only for the duration of install and sets back those in the end.

:CaseAutomation: NotAutomated
"""


@pytest.mark.e2e
@pytest.mark.tier2
@pytest.mark.skipif((not settings.robottelo.REPOS_HOSTING_URL), reason='Missing repos_hosting_url')
Expand Down
Loading