Skip to content

Conversation

maruiz93
Copy link
Contributor

@maruiz93 maruiz93 commented Aug 19, 2025

After applying the changes in #7417 the nginx pod failed to start with the following error:

nginx: [emerg] open() "/mnt/nginx-generated-config/tekton-results.conf" failed (2: No such file or directory) in /mnt/nginx-additional-location-configs/tekton-results-workspaces.conf:7

This refactor was then reverted in #7706

In this PR I am reapplying the initial refactor with the needed fix to prevent the error.
Note that the 7th line in components/konflux-ui/production/base/proxy/tekton-results-workspaces.conf now shows

proxy_pass https://tekton-results-api-service.tekton-results.svc.cluster.local:8080;

instead of

include /mnt/nginx-generated-config/tekton-results.conf;

@openshift-ci openshift-ci bot requested review from sadlerap and sahil143 August 19, 2025 07:33
Copy link
Contributor

Code Review by Gemini

The pull request aims to refactor Nginx configuration, addressing a previous issue where the Nginx pod failed to start due to incorrect file paths. While the PR correctly adjusts file paths, it introduces a new Nginx syntax error due to a change in the content of an included configuration file.

Here are the identified issues and suggested improvements:

Issues and Improvements

  1. Nginx Syntax Error in tekton-results-workspaces.conf

    • Issue: The file components/konflux-ui/production/base/proxy/tekton-results-workspaces.conf attempts to include tekton-results.conf. However, tekton-results.conf (the new static file) now defines a complete location block. Nginx does not allow nesting location blocks using the include directive in this manner. This will result in a configuration parsing error. Previously, tekton-results.conf was dynamically generated to contain only the proxy_pass directive, which was valid for inclusion within another location block.
    • Suggested Change: Instead of including tekton-results.conf, the proxy_pass directive should be directly specified within tekton-results-workspaces.conf, mirroring the proxy_pass from tekton-results.conf.
    --- a/components/konflux-ui/production/base/proxy/tekton-results-workspaces.conf
    +++ b/components/konflux-ui/production/base/proxy/tekton-results-workspaces.conf
    @@ -4,6 +4,6 @@
     
         rewrite /api/k8s/plugins/tekton-results/workspaces/.+?/(.+) /$1 break;
         proxy_read_timeout 30m;
    -    include /mnt/nginx-additional-location-configs/tekton-results.conf;
    +    proxy_pass https://tekton-results-api-service.tekton-results.svc.cluster.local:8080;
         include /mnt/nginx-generated-config/auth.conf;
     }
  2. Functional Change: Impersonation Always Enabled

    • Issue: Previously, the auth.conf content was conditionally generated based on the IMPERSONATE environment variable from proxy-init-config. If IMPERSONATE was false, impersonation headers were not added. With this change, auth.conf is now a static template that always includes proxy_set_header Impersonate-User and proxy_set_header Impersonate-Group, and the proxy-init-config (which provided IMPERSONATE) is removed. This means impersonation will always be enabled.
    • Improvement Suggestion: Confirm if this change in behavior (always enabling impersonation) is intended. If conditional impersonation is still required, the generate-nginx-configs init container's logic for auth.conf needs to be re-evaluated to allow for conditional generation or templating.
  3. Functional Change: Hardcoded Service URLs

    • Issue: The TEKTON_RESULTS_URL and KUBEARCHIVE_URL were previously configurable via environment variables from proxy-init-config. These URLs are now hardcoded in the new static tekton-results.conf and kubearchive.conf files. The proxy-init-config is removed from all kustomization overlays.
    • Improvement Suggestion: Confirm if these URLs are truly static across all environments. If there's a need for environment-specific URLs, this change introduces a regression in configurability. To maintain configurability, these URLs could be templated in the static .conf files (e.g., __TEKTON_RESULTS_URL__) and replaced by the generate-nginx-configs init container, similar to how __BEARER_TOKEN__ is handled.

@maruiz93
Copy link
Contributor Author

/hold

Follow up of 7033 PR

Signed-off-by: Marta Anon <[email protected]>
@maruiz93 maruiz93 force-pushed the refactor-nginx-configs-prod branch from 09bb74b to 5a423d6 Compare August 19, 2025 07:41
@maruiz93
Copy link
Contributor Author

/unhold

Copy link
Contributor

Code Review by Gemini

None

Copy link
Contributor

@hugares hugares left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link

openshift-ci bot commented Aug 19, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hugares, maruiz93

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-merge-bot openshift-merge-bot bot merged commit 9b78b9f into redhat-appstudio:main Aug 19, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants