Run ironic-api as WSGI when standalone with TLS capability by namnx228 · Pull Request #230 · metal3-io/ironic-image

namnx228 · 2020-12-01T10:46:43Z

This PR runs Ironic-api as a WSGI application on top of a fully-capable web server (Apache) and lets this Apache server takes care of TLS. In details, Ironic-api runs as a WSGI application on top of an Apache server. This web server handles the HTTPS connections between ironic-api and other components.
Related PRs:
metal3-io/baremetal-operator#728
metal3-io/ironic-inspector-image#70

namnx228 · 2020-12-01T11:37:11Z

/assign @dtantsur
/cc @dhellmann
/cc @maelk
/cc @kashifest

namnx228 · 2020-12-01T11:42:35Z

/test-integration

namnx228 · 2020-12-01T11:46:27Z

/cc @zaneb
/cc @derekhiggins

dtantsur

Missed an important bit (may explain why we need to keep the old approach around).

dtantsur · 2020-12-01T17:09:41Z

+else
+  python3 -c 'import os; import sys; import jinja2; sys.stdout.write(jinja2.Template(sys.stdin.read()).render(env=os.environ))' < /etc/httpd-ironic-api.conf.j2 > /etc/httpd/conf.d/ironic.conf
+  sed -i "/Listen 80/c\#Listen 80" /etc/httpd/conf/httpd.conf
+  exec /usr/sbin/httpd -DFOREGROUND


There is already an httpd container here, let's not run several more instances of httpd

Currently, the PR adds 2 more httpd instance to the picture. If we avoid to add extra httpd instances and make use of the existing one, IMO there will be two problems:

Web server and WSGI application need to be on the same container. if we only have one httpd instance, we have to run Ironic-api on the existing httpd container.

In the PR, a dedicated instance of Apache server acts as a reverse proxy offloading TLS and forward http requests to the conductor. If the existing httpd container also becomes a reverse proxy, that means we are forcing the ironic-api and ironic-conductor to live on the same pod. This may reduce the flexibility of design we can choose for our system.

Actually, I think we can split conductor and httpd into different containers, and also make use of the existing httpd instance (used by IPA) to acts as a reverse proxy for the conductor. However, a httpd instance is needed to be in the ironic-api container since we run it as a WSGI application. Therefore, we need at least two httpd instance in the picture, and one instance needs to be in the same container as the ironic-api. WDYT @dtantsur ?

First, I'm not convinced that we should use httpd for the conductor. This is a setup nobody has ever used, and I'm not sure what the implications are. I'd leave it alone. I don't see a problem with having ironic-api in the httpd container, after all, this is how non-containerized deployments work. Having 2 httpd containers will raise memory requirements quite severely.

@dtantsur The reason why httpd is used for the conductor is that it also takes care of offloading the encrypted traffic and forward the unencrypted traffic to the conductor. Since we put httpd and the conduct into one K8S pod, I think the unencrypted traffic doesn't hurt too much. The aim is to make Ironic get rid of handling TLS.

namnx228 · 2021-02-22T13:25:19Z

@zaneb @stbenjam Could you please take a look at this PR?

stbenjam · 2021-02-22T13:25:19Z

+sed -i 's/^Listen .*$/Listen [::]:'"$HTTP_PORT"'/' /etc/httpd/conf/httpd.conf
+sed -i -e 's|\(^[[:space:]]*\)\(DocumentRoot\)\(.*\)|\1\2 "/shared/html"|' \
+    -e 's|<Directory "/var/www/html">|<Directory "/shared/html">|' \
+    -e 's|<Directory "/var/www">|<Directory "/shared">|' /etc/httpd/conf/httpd.conf
+
+# Log to std out/err
+sed -i -e 's%^ \+CustomLog.*%    CustomLog /dev/stderr combined%g' /etc/httpd/conf/httpd.conf
+sed -i -e 's%^ErrorLog.*%ErrorLog /dev/stderr%g' /etc/httpd/conf/httpd.conf


I'm not sure I like managing config files via sed, what if the config file changes just enough that the regex no longer matches?

Can we just ship a minimal /etc/httpd/conf/httpd.conf for this, possibly a jinja template?

Ok, I can do that, but should we do this in a separate PR? These sed commands already existed before, and I just move it from one place to another.

Sure that's fine, I didn't realize this was pre-existing.

namnx228 · 2021-02-22T15:09:27Z

/test-integration
test-centos-integration

namnx228 · 2021-02-22T15:09:41Z

/test-centos-integration

namnx228 · 2021-02-22T15:37:19Z

/test-centos-integration

namnx228 · 2021-02-23T07:39:56Z

@stbenjam Thank you for your comments. I have addressed all of them. Please take a look again.

stbenjam · 2021-02-23T13:03:00Z

+
+Listen 6385
+
+<VirtualHost {{ env.IRONIC_IP }}:6385>


I think we should obey the LISTEN_ALL_INTERFACES env variable here. In the Ironic config, the logic is:

host_ip = {% if env.LISTEN_ALL_INTERFACES | lower == "true" %}::{% else %}{{ env.IRONIC_IP }}{% endif %}

Oh! I failed to get your point in the comment above. Now it is fixed. Thanks.

stbenjam · 2021-02-23T13:04:08Z

Just one comment, please fix that but I think otherwise this good.

namnx228 · 2021-02-23T13:50:54Z

/test-integration
/test-centos-integration

namnx228 · 2021-02-23T16:06:28Z

/test-integration
/test-centos-integration

namnx228 · 2021-02-24T07:48:58Z

/test-centos-integration

namnx228 · 2021-02-24T09:52:45Z

@stbenjam Thanks, the last comment has been fixed. PTAL

stbenjam · 2021-03-01T14:57:17Z

+{% if env.LISTEN_ALL_INTERFACES | lower == "true" %}
+ <VirtualHost *:6385>
+{% else %}
+ <VirtualHost {{ env.IRONIC_IP }}:6385>


Thanks! This is better. One small nit, if IRONIC_IP is IPv6, this won't get wrapped correctly, e.g it'd need to be [fd00::2]:6385. I think you can use the env.IRONIC_URL_HOST instead, which is provided by the ironic common.sh include.

Do you mean change env.IRONIC_IP to env.IRONIC_URL_HOST in line 18?

namnx228 · 2021-03-02T09:49:48Z

/test-integration
/test-centos-integration

stbenjam · 2021-03-02T11:45:08Z

/approve

stbenjam · 2021-03-02T12:41:28Z

/lgtm

@elfosardo Would you mind taking a look again?

elfosardo · 2021-03-08T10:27:09Z

/assign @dtantsur

metal3-io-bot · 2021-03-08T11:33:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dtantsur, elfosardo, namnx228, stbenjam

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [dtantsur]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

namnx228 · 2021-03-09T12:35:01Z

Thanks very much @dtantsur, @stbenjam, @elfosardo for your reviews and approval.

Merge metal3 20211102

metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 1, 2020

namnx228 force-pushed the ironic-api-behind-wsgi-nam branch from 9a02de5 to 6e4ced8 Compare December 1, 2020 10:50

This was referenced Dec 1, 2020

Apache reverse proxy metal3-io/ironic-inspector-image#70

Merged

WIP: Apache webserver handles TLS on Ironic and Ironic-inspector sides metal3-io/metal3-dev-env#562

Closed

Apache webserver handles TLS on Ironic metal3-io/baremetal-operator#728

Merged

metal3-io-bot assigned dtantsur Dec 1, 2020

metal3-io-bot requested review from dhellmann, kashifest and maelk December 1, 2020 11:37

namnx228 force-pushed the ironic-api-behind-wsgi-nam branch 2 times, most recently from d66a4ae to 03d3fc3 Compare December 1, 2020 11:41

metal3-io-bot requested review from derekhiggins and zaneb December 1, 2020 11:46

namnx228 force-pushed the ironic-api-behind-wsgi-nam branch 3 times, most recently from 3cecfab to 758a1c7 Compare December 1, 2020 16:18

dtantsur reviewed Dec 1, 2020

View reviewed changes

Comment thread README.md Outdated

dtantsur requested changes Dec 1, 2020

View reviewed changes

namnx228 force-pushed the ironic-api-behind-wsgi-nam branch 2 times, most recently from 4c61356 to e0f6659 Compare December 11, 2020 14:23

namnx228 changed the title ~~Run ironic-api as WSGI when standalone with TLS capability~~ WIP: Run ironic-api as WSGI when standalone with TLS capability Dec 11, 2020

metal3-io-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 11, 2020

namnx228 force-pushed the ironic-api-behind-wsgi-nam branch from e0f6659 to 8360d7d Compare December 16, 2020 16:35

stbenjam reviewed Feb 22, 2021

View reviewed changes

namnx228 force-pushed the ironic-api-behind-wsgi-nam branch from e456ac3 to 0679edd Compare February 22, 2021 15:09

stbenjam reviewed Feb 23, 2021

View reviewed changes

namnx228 force-pushed the ironic-api-behind-wsgi-nam branch from 0679edd to 2ca3197 Compare February 23, 2021 13:47

namnx228 force-pushed the ironic-api-behind-wsgi-nam branch from 2ca3197 to 63e6163 Compare February 23, 2021 16:06

stbenjam reviewed Mar 1, 2021

View reviewed changes

Run ironic-api as WSGI when standalone with TLS capability

d0db1d5

namnx228 force-pushed the ironic-api-behind-wsgi-nam branch from 63e6163 to d0db1d5 Compare March 2, 2021 09:43

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 2, 2021

elfosardo approved these changes Mar 4, 2021

View reviewed changes

dtantsur approved these changes Mar 8, 2021

View reviewed changes

metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 8, 2021

metal3-io-bot merged commit 216be3e into metal3-io:master Mar 8, 2021

elfosardo pushed a commit to elfosardo/ironic-image that referenced this pull request Nov 17, 2021

Merge pull request metal3-io#230 from elfosardo/merge-metal3-20211102

3659ee9

Merge metal3 20211102

Conversation

namnx228 commented Dec 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

namnx228 commented Dec 1, 2020

Uh oh!

namnx228 commented Dec 1, 2020

Uh oh!

namnx228 commented Dec 1, 2020

Uh oh!

Uh oh!

dtantsur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

namnx228 Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

namnx228 commented Feb 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

namnx228 Feb 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

namnx228 commented Feb 22, 2021

Uh oh!

namnx228 commented Feb 22, 2021

Uh oh!

namnx228 commented Feb 22, 2021

Uh oh!

namnx228 commented Feb 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stbenjam commented Feb 23, 2021

Uh oh!

namnx228 commented Feb 23, 2021

Uh oh!

namnx228 commented Feb 23, 2021

Uh oh!

namnx228 commented Feb 24, 2021

Uh oh!

namnx228 commented Feb 24, 2021

Uh oh!

stbenjam Mar 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

namnx228 commented Mar 2, 2021

Uh oh!

stbenjam commented Mar 2, 2021

Uh oh!

stbenjam commented Mar 2, 2021

Uh oh!

elfosardo commented Mar 8, 2021

Uh oh!

metal3-io-bot commented Mar 8, 2021

Uh oh!

namnx228 commented Mar 9, 2021

Uh oh!

namnx228 commented Dec 1, 2020 •

edited

Loading

namnx228 Dec 2, 2020 •

edited

Loading

namnx228 Feb 22, 2021 •

edited

Loading

stbenjam Mar 1, 2021 •

edited

Loading