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

Unify use of pulp_content_bind and pulp_api_bind #322

Merged
merged 1 commit into from Jun 22, 2020

Conversation

Spredzy
Copy link
Contributor

@Spredzy Spredzy commented Jun 8, 2020

In its current state, the installer relies on pulp_content_bind and
pulp_api_bind for the following roles: pulp and pulp_content.
Yet, for pulp_webserver it relies on a new set of parameters for the
exact same purposes (note: those parameters are only used in this role);
namely: pulp_content_host, pulp_content_port, pulp_api_host and
pulp_api_port.

While it would be less error prone to follow the 'define it once use
it everywhere pattern', the following PR also allows has the benefit
of:

  • Enabling the use of Unix Domain Socket (UDS) to improve scale
    (ie. /var/run/pulpcore-api/pulpcore-api.sock) and apply best
    practice (if you only deal with localhost you don't need to get
    the network stack involved)

  • Pave the way for a better IPv6 integration. 127.0.0.1 doesn't
    exist in IPv6 only servers. By using UDS as previously mentionned
    this limitation becomes void for those two (api and content)
    components

fixes #6921

@pulpbot
Copy link
Member

pulpbot commented Jun 8, 2020

Attached issue: https://pulp.plan.io/issues/6921

@Spredzy Spredzy changed the title Unify use of pulp_content_bind and pulp_api_binD [WIP] Unify use of pulp_content_bind and pulp_api_binD Jun 8, 2020
@Spredzy
Copy link
Contributor Author

Spredzy commented Jun 8, 2020

This PR has been successfully tested with the following two scenarios. The design make it so it should be transparent for current user/deployments while allowing one to specify UDS.

Scenario 1: Using Unix Domain Soocket

- hosts: localhost
  vars:
    pulp_settings:
      secret_key: secret
      content_origin: "http://{{ ansible_fqdn }}"
      x_pulp_api_host: 127.0.0.1
      x_pulp_api_port: 80
      x_pulp_api_user: "admin"
      x_pulp_api_password: "{{ pulp_default_admin_password }}"
      x_pulp_api_prefix: "pulp_ansible/galaxy/automation-hub/api"
      galaxy_require_content_approval: "False"
    pulp_default_admin_password: password
    pulp_install_plugins:
      pulp-ansible: {}
      galaxy-ng: {}
      pulp-container: {}
    pulp_api_workers: 4
    pulp_api_bind: 'unix:/var/run/pulpcore-api/pulpcore-api.sock'
    pulp_content_bind: 'unix:/var/run/pulpcore-content/pulpcore-content.sock'
  roles:
    - pulp_database
    - pulp_workers
    - pulp_resource_manager
    - pulp_webserver
    - pulp_content
  environment:
    DJANGO_SETTINGS_MODULE: pulpcore.app.settings

- hosts: localhost
  roles:
    - role: chouseknecht.ansible_galaxy_config
      vars:
        pulp_api_port: 80

Scenario 2: Current implementation binding to 127.0.0.1

- hosts: localhost
  vars:
    pulp_settings:
      secret_key: secret
      content_origin: "http://{{ ansible_fqdn }}"
      x_pulp_api_host: "{{ pulp_api_host }}"
      x_pulp_api_port: "{{ pulp_api_port }}"
      x_pulp_api_user: "admin"
      x_pulp_api_password: "{{ pulp_default_admin_password }}"
      x_pulp_api_prefix: "pulp_ansible/galaxy/automation-hub/api"
      galaxy_require_content_approval: "False"
    pulp_default_admin_password: password
    pulp_install_plugins:
      pulp-ansible: {}
      galaxy-ng: {}
      pulp-container: {}
    pulp_api_workers: 4
  roles:
    - pulp_database
    - pulp_workers
    - pulp_resource_manager
    - pulp_webserver
    - pulp_content
    - chouseknecht.ansible_galaxy_config
  environment:
    DJANGO_SETTINGS_MODULE: pulpcore.app.settings

Note: I am not aware of the upgrade (if any) and the variable deprecation (if any) process. My change hence might not be compatible with those said processes. Please let me know if there is any doc I can check to update my PR accordingly.

@Spredzy Spredzy changed the title [WIP] Unify use of pulp_content_bind and pulp_api_binD Unify use of pulp_content_bind and pulp_api_binD Jun 8, 2020
@Spredzy Spredzy changed the title Unify use of pulp_content_bind and pulp_api_binD Unify use of pulp_content_bind and pulp_api_bind Jun 8, 2020
Copy link
Member

@fao89 fao89 left a comment

Choose a reason for hiding this comment

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

LGTM!
Thank you for your explanation and tests, your PRs are awesome!

@fao89 fao89 requested a review from mikedep333 June 8, 2020 14:49
Spredzy added a commit to Spredzy/pulp_installer that referenced this pull request Jun 9, 2020
This commit ensures one can configure redis so it listens to unix domain
socket (if wanted) rather than forcing it to listen on TCP socket.

Benefits of this commit has been higlighted in previous PR[1].

Also, this commit introduces `pulp_redis_bind` to offer an expected
experience for people using `pulp/pulp_installer` when it comes to
configure network services (ie. `pulp_api_bind`, `pulp_content_bind`).

[1] pulp#322
Spredzy added a commit to Spredzy/pulp_installer that referenced this pull request Jun 9, 2020
This commit ensures one can configure redis so it listens to unix domain
socket (if wanted) rather than forcing it to listen on TCP socket.

Benefits of this commit has been higlighted in previous PR[1].

Also, this commit introduces `pulp_redis_bind` to offer an expected
experience for people using `pulp/pulp_installer` when it comes to
configure network services (ie. `pulp_api_bind`, `pulp_content_bind`).

[1] pulp#322
Spredzy added a commit to Spredzy/pulp_installer that referenced this pull request Jun 9, 2020
This commit ensures one can configure redis so it listens to unix domain
socket (if wanted) rather than forcing it to listen on TCP socket.

Benefits of this commit has been higlighted in previous PR[1].

Also, this commit introduces `pulp_redis_bind` to offer an expected
experience for people using `pulp/pulp_installer` when it comes to
configure network services (ie. `pulp_api_bind`, `pulp_content_bind`).

[1] pulp#322
Spredzy added a commit to Spredzy/pulp_installer that referenced this pull request Jun 9, 2020
This commit ensures one can configure redis so it listens to unix domain
socket (if wanted) rather than forcing it to listen on TCP socket.

Benefits of this commit has been higlighted in previous PR[1].

Also, this commit introduces `pulp_redis_bind` to offer an expected
experience for people using `pulp/pulp_installer` when it comes to
configure network services (ie. `pulp_api_bind`, `pulp_content_bind`).

[1] pulp#322
Spredzy added a commit to Spredzy/pulp_installer that referenced this pull request Jun 9, 2020
This commit ensures one can configure redis so it listens to unix domain
socket (if wanted) rather than forcing it to listen on TCP socket.

Benefits of this commit has been higlighted in previous PR[1].

Also, this commit introduces `pulp_redis_bind` to offer an expected
experience for people using `pulp/pulp_installer` when it comes to
configure network services (ie. `pulp_api_bind`, `pulp_content_bind`).

fixes #6931

[1] pulp#322
Spredzy added a commit to Spredzy/pulp_installer that referenced this pull request Jun 9, 2020
This commit ensures one can configure redis so it listens to unix domain
socket (if wanted) rather than forcing it to listen on TCP socket.

Benefits of this commit has been higlighted in previous PR[1].

Also, this commit introduces `pulp_redis_bind` to offer an expected
experience for people using `pulp/pulp_installer` when it comes to
configure network services (ie. `pulp_api_bind`, `pulp_content_bind`).

fixes #6931

[1] pulp#322
Spredzy added a commit to Spredzy/pulp_installer that referenced this pull request Jun 9, 2020
This commit ensures one can configure redis so it listens to unix domain
socket (if wanted) rather than forcing it to listen on TCP socket.

Benefits of this commit has been higlighted in previous PR[1].

Also, this commit introduces `pulp_redis_bind` to offer an expected
experience for people using `pulp/pulp_installer` when it comes to
configure network services (ie. `pulp_api_bind`, `pulp_content_bind`).

fixes #6931

[1] pulp#322
@bmbouter
Copy link
Member

bmbouter commented Jun 9, 2020

Before merging I want to finalize the discussion on the ticket.

Spredzy added a commit to Spredzy/pulp_installer that referenced this pull request Jun 10, 2020
This commit ensures one can configure redis so it listens to unix domain
socket (if wanted) rather than forcing it to listen on TCP socket.

Benefits of this commit has been higlighted in previous PR[1].

Also, this commit introduces `pulp_redis_bind` to offer an expected
experience for people using `pulp/pulp_installer` when it comes to
configure network services (ie. `pulp_api_bind`, `pulp_content_bind`).

fixes #6931

[1] pulp#322
Spredzy added a commit to Spredzy/pulp_installer that referenced this pull request Jun 10, 2020
This commit ensures one can configure redis so it listens to unix domain
socket (if wanted) rather than forcing it to listen on TCP socket.

Benefits of this commit has been higlighted in previous PR[1].

Also, this commit introduces `pulp_redis_bind` to offer an expected
experience for people using `pulp/pulp_installer` when it comes to
configure network services (ie. `pulp_api_bind`, `pulp_content_bind`).

fixes #6931

[1] pulp#322
Copy link
Member

@bmbouter bmbouter left a comment

Choose a reason for hiding this comment

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

We do not need to carry the [deprecated] fields. Can those please be removed?

Also for any vars that are removed, can you add a 6921.removal changelog (separate from the feature changelog). This will clearly call that out as a breaking change for users.

Also can changelogs be line wrapped to 100 chars. Sorry the CI doesn't let you know this. It's part of the Pulp community style guide, but it should also be more clear for contributors on individual repos.

Spredzy added a commit to Spredzy/pulp_installer that referenced this pull request Jun 18, 2020
In its current state pulp3 installs fine with SELInux enforced but fails
to execute, because nginx is trying to connect to another network
service (api and content - on port 24817 and port 24816).

While this is easily solved by the introduction of Unix Domain Socket in
pulp#322 one may still want to
use network services (if api is on another host) and hence the proper
SELinux flag must be set.

Flag to enable: httpd_can_network_connect

fixes #6998
Spredzy added a commit to Spredzy/pulp_installer that referenced this pull request Jun 18, 2020
In its current state pulp3 installs fine with SELInux enforced but fails
to execute, because nginx is trying to connect to another network
service (api and content - on port 24817 and port 24816).

While this is easily solved by the introduction of Unix Domain Socket in
pulp#322 one may still want to
use network services (if api is on another host) and hence the proper
SELinux flag must be set.

Flag to enable: httpd_can_network_connect

fixes #6998
Spredzy added a commit to Spredzy/pulp_installer that referenced this pull request Jun 18, 2020
In its current state pulp3 installs fine with SELInux enforced but fails
to execute, because nginx is trying to connect to another network
service (api and content - on port 24817 and port 24816).

While this is easily solved by the introduction of Unix Domain Socket in
pulp#322 one may still want to
use network services (if api is on another host) and hence the proper
SELinux flag must be set.

Flag to enable: httpd_can_network_connect

fixes #6998
CHANGES/6921.feature Outdated Show resolved Hide resolved
Copy link
Member

@bmbouter bmbouter left a comment

Choose a reason for hiding this comment

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

Can you also add a CHANGES/6921.removal file that has a changelog entry indicating which variables are removed and what users should use instead?

CHANGES/6921.removal Outdated Show resolved Hide resolved
In its current state, the installer relies on `pulp_content_bind` and
`pulp_api_bind` for the following roles: `pulp` and `pulp_content`.
Yet, for `pulp_webserver` it relies on a new set of parameters for the
exact same purposes (note: those parameters are only used in this role);
namely: `pulp_content_host`, `pulp_content_port`, `pulp_api_host` and
`pulp_api_port`

While it would be less error prone to follow the 'define it once use
it everywhere pattern', the following PR also allows has the benefit
of:

  * Enabling the use of Unix Domain Socket (UDS) to improve scale
    (ie. `/var/run/pulpcore-api/pulpcore-api.sock`) and apply best
    practice (if you only deal with localhost you don't need to get
    the network stack involved)

  * Pave the way for a better IPv6 integration. 127.0.0.1 doesn't
    exist in IPv6 only servers. By using UDS as previously mentionned
    this limitation becomes void for those two (api and content)
    components

fixes #6921
Copy link
Member

@bmbouter bmbouter left a comment

Choose a reason for hiding this comment

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

This looks great. Thanks @Spredzy !

@mikedep333 mikedep333 merged commit bcc1285 into pulp:master Jun 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants