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 nginx configurations for the odf-console #305

Merged
merged 2 commits into from Mar 28, 2023

Conversation

SanjalKatiyar
Copy link
Contributor

@SanjalKatiyar SanjalKatiyar commented Mar 21, 2023

Changes required as per BZ: https://bugzilla.redhat.com/show_bug.cgi?id=2139785

Resolves 4 issues:

  1. BZ#2139785 itself: where on disabling ipv6 address on the cluster nodes, odf-console pod was resulting in error state (https://bugzilla.redhat.com/show_bug.cgi?id=2139785#c10)
  2. There was CVE issue raised by IBM team where port 8080 was stated to be an insure port (https://bugzilla.redhat.com/show_bug.cgi?id=2166417). Port 8080 is present by default in the nginx configuration that we use for our pod (under /etc/nginx/nginx.conf: https://pastebin.com/ksX21dsr) and we did not had any control over that config earlier. Now all the nginx configurations (nginx's default and custom config for odf-console) are added via a ConfigMap created by odf-operator, so everything is under our control.
    Also, now removed the port 8080 which was never needed and never used.
  3. Earlier nginx configurations were added during build time, now they are added and can be easily updated during runtime.

  1. There is one more CVE issue as per which we need to add readOnlyRootFilesystem: true spec for the odf-console pod, but on adding it is causing the UI's pod to go into an error state because nginx needs to create some files in pod's filesystem for its functioning but don't have access to do so (if we set this attribute), hence pod is never getting into "Running" status.
    This PR adds the foundation for enabling us to be able to add the changes to fix this CVE (will send a follow-up PR for that).

Possible improvement ?

Currently for BZ#2139785 (if this PR gets merged), once odf-console pod errors due to ipv6 address issue, user will have to perform following operations (we will keep documentation team in loop):

  1. Go to ConfigMap: odf-console-nginx-conf (introduced as part of this PR).
  2. Comment out line listen [::]:9001 ssl; --> # listen [::]:9001 ssl;.
  3. Restart the odf-console pod (just delete it) and Refresh.
    Once done they will be able to see the UI once again as the pod will now be in running state.

We can automate this process so that user don;t have to take manual steps to resolve the issue.

Pro: no manual intervention needed.
Cons:

  1. Not a priority (P0) use-case or something which is generally faced by users (rare).
  2. odf-operator will need to introduce new a controller to keep a watch on resources which has nothing to do with ODF or storage, but are purely infra related.
  3. This BZ is different from IPv4/IPv6 dual-stack networking where we can just watch over network.config.openshift.io cluster CR (https://docs.openshift.com/container-platform/4.12/networking/ovn_kubernetes_network_provider/converting-to-dual-stack.html)...
    here we are disabling ipv6 addressing on the node's kernel level (https://access.redhat.com/solutions/5513111)... not sure if it is good idea to keep a watch for such changes.

odf-console uses nginx for serving the UI assests.
Add its configuration from the operator instead of build time.

Signed-off-by: SanjalKatiyar <sanjaldhir@gmail.com>
generated output of "make bundle" command.

Signed-off-by: SanjalKatiyar <sanjaldhir@gmail.com>
Comment on lines +23 to +53
worker_processes auto;
error_log /var/log/nginx/error.log;
pid /run/nginx.pid;

# Load dynamic modules. See /usr/share/doc/nginx/README.dynamic.
include /usr/share/nginx/modules/*.conf;

events {
worker_connections 1024;
}

http {
log_format main '$remote_addr - $remote_user [$time_local] "$request" '
'$status $body_bytes_sent "$http_referer" '
'"$http_user_agent" "$http_x_forwarded_for"';

access_log /var/log/nginx/access.log main;

sendfile on;
tcp_nopush on;
tcp_nodelay on;
keepalive_timeout 65;
types_hash_max_size 4096;

include /etc/nginx/mime.types;
default_type application/octet-stream;

# Load modular configuration files from the /etc/nginx/conf.d directory.
# See http://nginx.org/en/docs/ngx_core_module.html#include
# for more information.
include /opt/app-root/etc/nginx.d/*.conf;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these are the default nginx configurations, these were used earlier as well (before the changes introduced by this PR): https://pastebin.com/ksX21dsr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

check above link for entire configuration file that was used...

Comment on lines +55 to +76
server {
listen 9001 ssl;
listen [::]:9001 ssl;
ssl_certificate /var/serving-cert/tls.crt;
ssl_certificate_key /var/serving-cert/tls.key;
location / {
root /opt/app-root/src;
}
location /compatibility/ {
root /opt/app-root/src;
}
error_page 500 502 503 504 /50x.html;
location = /50x.html {
root /usr/share/nginx/html;
}
ssi on;
add_header Last-Modified $date_gmt;
add_header Cache-Control 'no-store, no-cache, must-revalidate, proxy-revalidate, max-age=0';
if_modified_since off;
expires off;
etag off;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

odf-console related custom configurations...

Copy link
Contributor

Choose a reason for hiding this comment

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

@SanjalKatiyar I see that this is the same config (server section) that we have in the plugin repo, so we should ensure both remain in sync, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we will eventually remove it from odf-console and add them from here...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no need to read default.conf during build after this PR (we need small change in build Dockerfile as well)...

@SanjalKatiyar
Copy link
Contributor Author

/test odf-operator-e2e-aws

@iamniting
Copy link
Member

/test odf-operator-e2e-aws

I know the problem, I will get it fixed once #304 is merged

@iamniting
Copy link
Member

/test odf-operator-e2e-aws

1 similar comment
@iamniting
Copy link
Member

/test odf-operator-e2e-aws

@SanjalKatiyar
Copy link
Contributor Author

Need to update build Dockerfile once this is merged...

@SanjalKatiyar
Copy link
Contributor Author

@bipuladh PTAL.

@alfonsomthd
Copy link
Contributor

/approve

@alfonsomthd
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 28, 2023
Copy link
Contributor

@bipuladh bipuladh left a comment

Choose a reason for hiding this comment

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

/lgtm

@bipuladh
Copy link
Contributor

/assign @iamniting

@SanjalKatiyar
Copy link
Contributor Author

/cherrypick release-4.13

@openshift-cherrypick-robot

@SanjalKatiyar: once the present PR merges, I will cherry-pick it on top of release-4.13 in a new PR and assign it to you.

In response to this:

/cherrypick release-4.13

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 28, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alfonsomthd, bipuladh, iamniting, SanjalKatiyar

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 28, 2023
@openshift-merge-robot openshift-merge-robot merged commit 6fca0b9 into red-hat-storage:main Mar 28, 2023
33 checks passed
@openshift-cherrypick-robot

@SanjalKatiyar: new pull request created: #306

In response to this:

/cherrypick release-4.13

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants