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

Add openshift_node_open_ports to allow arbitrary firewall exposure #5345

Merged
merged 1 commit into from
Sep 16, 2017

Conversation

smarterclayton
Copy link
Contributor

@smarterclayton smarterclayton commented Sep 9, 2017

It should be possible for an admin to define an arbitrary set of ports
to be exposed on each node that will relate to the cluster function.
This adds a new global variable for the node that supports

Array(Object{'service':<name>,'port':<port_spec>,'cond':<boolean>})

which is the same format accepted by the firewall role.

@sdodson as discussed, open to alternatives. I used this from origin-gce with

openshift_node_open_ports:
- service: Router stats
  port: 1936/tcp
- service: Open node ports
  port: 9000-10000/tcp
- service: Open node ports
  port: 9000-10000/udp

Which then allows me to set firewall rules appropriately.

Alternatives considered:

  • Simpler external format (have to parse inputs)
  • Additional parameter to role - felt ugly

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 9, 2017
@smarterclayton
Copy link
Contributor Author

@kwoodson needs to be easier to expose more ports internally for the use of the cluster (for things like node exporter or haproxy metrics stats, for example).

@kwoodson
Copy link
Contributor

@smarterclayton, let's chat about this.

@smarterclayton
Copy link
Contributor Author

Discussed in person - we seem to be in sync now.

@@ -79,6 +79,8 @@ r_openshift_node_os_firewall_allow:
- service: Kubernetes service NodePort UDP
port: "{{ openshift_node_port_range | default('') }}/udp"
cond: "{{ openshift_node_port_range is defined }}"
# Allow multiple port ranges to be added to the role
r_openshift_node_os_firewall_allow: "{{ default_r_openshift_node_os_firewall_allow + (openshift_node_open_ports | default([])) }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the ansible filter for this is union: http://docs.ansible.com/ansible/latest/playbooks_filters.html#set-theory-filters

r_openshift_node_os_firewall_allow: "{{ default_r_openshift_node_os_firewall_allow | union((openshift_node_open_ports | default([]))) }}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Union would remove duplicates if they exist, although that wouldn't actually help for this if the condition values were different. I can't think of any place where we would want duplicates though.

Copy link
Contributor

@kwoodson kwoodson left a comment

Choose a reason for hiding this comment

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

Consider union instead of list + list.

It should be possible for an admin to define an arbitrary set of ports
to be exposed on each node that will relate to the cluster function.
This adds a new global variable for the node that supports

    Array(Object{'service':<name>,'port':<port_spec>,'cond':<boolean>})

which is the same format accepted by the firewall role.
@smarterclayton
Copy link
Contributor Author

Fixed to use union (tested locally)

@smarterclayton
Copy link
Contributor Author

aos-ci-test

@openshift-bot
Copy link

success: "aos-ci-jenkins/OS_3.6_NOT_containerized, aos-ci-jenkins/OS_3.6_NOT_containerized_e2e_tests" for 3f10259 (logs)

@openshift-bot
Copy link

success: "aos-ci-jenkins/OS_3.6_containerized, aos-ci-jenkins/OS_3.6_containerized_e2e_tests" for 3f10259 (logs)

@kwoodson
Copy link
Contributor

[merge]

@smarterclayton
Copy link
Contributor Author

looks like a temporary issue because of an image / code mismatch with master. retrying once [merge]

@smarterclayton
Copy link
Contributor Author

Upstream test is allegedly fixed [merge]

@openshift-bot
Copy link

Evaluated for openshift ansible merge up to 3f10259

@openshift-bot
Copy link

continuous-integration/openshift-jenkins/merge FAILURE (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_openshift_ansible/1035/) (Base Commit: 5183792) (PR Branch Commit: 3f10259)

@smarterclayton smarterclayton added the lgtm Indicates that a PR is ready to be merged. label Sep 15, 2017
@smarterclayton
Copy link
Contributor Author

/retest

1 similar comment
@smarterclayton
Copy link
Contributor Author

/retest

@openshift-merge-robot
Copy link
Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue

@smarterclayton
Copy link
Contributor Author

Documented openshift/origin#21520

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants