Skip to content

Conversation

maruiz93
Copy link
Contributor

Follow up of 7033 PR

After checking the changes in staging work as expected.

@openshift-ci openshift-ci bot requested review from filariow and rrosatti July 31, 2025 19:55
@maruiz93
Copy link
Contributor Author

maruiz93 commented Jul 31, 2025

Follow up of 7033 PR

Signed-off-by: Marta Anon <[email protected]>
@maruiz93 maruiz93 force-pushed the refactor-nginx-configs-prod branch from e74384f to 848a6a1 Compare August 14, 2025 08:23
Copy link
Contributor

Code Review by Gemini

The changes introduce a Nginx configuration syntax error and remove the flexibility to configure backend URLs (Tekton Results, KubeArchive) via environment variables, which is a regression in configurability.

Here are the identified issues and suggested improvements:

1. Nginx Configuration Syntax Error (Nested Location Blocks)

Issue:
The file components/konflux-ui/production/base/proxy/tekton-results-workspaces.conf attempts to include tekton-results.conf. However, tekton-results.conf (as introduced in this PR) is itself a full location block. Nginx does not allow location blocks to be nested or included within other location blocks in this manner. This will result in a Nginx configuration error.

Files to change:

  • components/konflux-ui/production/base/proxy/tekton-results-workspaces.conf
  • components/konflux-ui/production/base/proxy/tekton-results.conf
  • components/konflux-ui/production/base/proxy/kubearchive.conf

Reasoning for change:
The include directive within a location block is typically used to include directives (like proxy_pass, proxy_set_header, etc.), not entire location blocks. The original setup had a dynamically generated file containing only the proxy_pass directive. This pattern should be restored.

Suggested Change:

--- a/components/konflux-ui/production/base/proxy/kubearchive.conf
+++ b/components/konflux-ui/production/base/proxy/kubearchive.conf
@@ -2,5 +2,5 @@
     auth_request /oauth2/auth;
     rewrite /api/k8s/plugins/kubearchive/(.+) /$1 break;
     proxy_read_timeout 30m;
-    proxy_pass https://kubearchive-api-server.product-kubearchive.svc.cluster.local:8081;
+    include /mnt/nginx-generated-config/kubearchive-proxy-pass.conf;
     include /mnt/nginx-generated-config/auth.conf;
 }
--- a/components/konflux-ui/production/base/proxy/tekton-results.conf
+++ b/components/konflux-ui/production/base/proxy/tekton-results.conf
@@ -3,6 +3,6 @@
 
     rewrite /api/k8s/plugins/tekton-results/(.+) /$1 break;
     proxy_read_timeout 30m;
-    proxy_pass https://tekton-results-api-service.tekton-results.svc.cluster.local:8080;
+    include /mnt/nginx-generated-config/tekton-results-proxy-pass.conf;
     include /mnt/nginx-generated-config/auth.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-generated-config/tekton-results.conf;
+    include /mnt/nginx-generated-config/tekton-results-proxy-pass.conf;
     include /mnt/nginx-generated-config/auth.conf;
 }

2. Loss of Configurability (Hardcoded URLs and IMPERSONATE flag)

Issue:
The proxy-init-config ConfigMap, which provided environment-specific URLs for Tekton Results and KubeArchive, has been removed. The generate-nginx-configs init container no longer uses these environment variables to dynamically generate the proxy_pass directives. Instead, the URLs are now hardcoded directly into the Nginx config files (kubearchive.conf, tekton-results.conf). This removes the ability to easily configure these URLs per environment using Kustomize overlays. Additionally, the conditional generation of auth.conf based on the IMPERSONATE flag was removed, making impersonation always enabled.

Files to change:

  • components/konflux-ui/production/base/proxy/kustomization.yaml
  • components/konflux-ui/production/base/proxy/proxy.yaml
  • components/konflux-ui/production/kflux-ocp-p01/kustomization.yaml (and similar files for other environments like kflux-osp-p01, kflux-prd-rh02, etc.)
  • components/konflux-ui/production/kflux-osp-p01/kubearchive.conf (and similar files for other environments)

Reasoning for change:
To restore configurability and maintain the dynamic generation of proxy_pass directives based on environment variables. The IMPERSONATE flag logic should also be restored for auth.conf generation.

Suggested Change:

a) Reintroduce proxy-init-config in base kustomization.yaml:

--- a/components/konflux-ui/production/base/proxy/kustomization.yaml
+++ b/components/konflux-ui/production/base/proxy/kustomization.yaml
@@ -5,9 +5,12 @@
 configMapGenerator:
   - name: proxy
     files:
       - nginx.conf
   - name: proxy-nginx-templates
     files:
       - auth.conf
   - name: proxy-nginx-static
     files:
       - tekton-results.conf
       - tekton-results-workspaces.conf
       - kubearchive.conf
+  - name: proxy-init-config
+    literals:
+      - IMPERSONATE=true
+      - TEKTON_RESULTS_URL=https://tekton-results-api-service.tekton-results.svc.cluster.local:8080
+      - KUBEARCHIVE_URL=https://kubearchive-api-server.product-kubearchive.svc.cluster.local:8081

b) Modify proxy.yaml to use proxy-init-config and generate proxy_pass files:

--- a/components/konflux-ui/production/base/proxy/proxy.yaml
+++ b/components/konflux-ui/production/base/proxy/proxy.yaml
@@ -47,37 +47,39 @@
         resources:
           limits:
             cpu: 50m
             memory: 128Mi
           requests:
             cpu: 10m
             memory: 64Mi
       - name: generate-nginx-configs
         image: registry.access.redhat.com/ubi9/ubi@sha256:66233eebd72bb5baa25190d4f55e1dc3fff3a9b77186c1f91a0abdb274452072
-        envFrom:
-          - configMapRef:
-              name: proxy-init-config
+        envFrom:
+          - configMapRef:
+              name: proxy-init-config
         command:
           - sh
           - -c
           - |
             set -e
 
-            # Generate auth.conf with bearer token replacement
+            # Generate auth.conf with bearer token replacement (conditional on IMPERSONATE)
             token=$(cat /mnt/api-token/token)
-            sed "s/__BEARER_TOKEN__/$token/g" /mnt/nginx-templates/auth.conf > /mnt/nginx-generated-config/auth.conf
+            if [[ "$IMPERSONATE" == "true" ]]; then
+              sed "s/__BEARER_TOKEN__/$token/g" /mnt/nginx-templates/auth.conf > /mnt/nginx-generated-config/auth.conf
+            else
+              echo "# impersonation was disabled by config" > /mnt/nginx-generated-config/auth.conf
+            fi
 
             chmod 640 /mnt/nginx-generated-config/auth.conf
 
-            chmod 640 "$auth_conf"
-
-            echo \
-              "proxy_pass ${TEKTON_RESULTS_URL:?tekton results url must be provided};" \
-              > /mnt/nginx-generated-config/tekton-results.conf
-
-            if [[ "$KUBEARCHIVE_URL" != "" ]]; then
-              echo "location /api/k8s/plugins/kubearchive/ {" > /mnt/nginx-generated-config/kubearchive.conf
-              echo "auth_request /oauth2/auth;" >> /mnt/nginx-generated-config/kubearchive.conf
-              echo "rewrite /api/k8s/plugins/kubearchive/(.+) /\$1 break;" >> /mnt/nginx-generated-config/kubearchive.conf
-              echo "proxy_read_timeout 30m;" >> /mnt/nginx-generated-config/kubearchive.conf
-              echo "proxy_pass ${KUBEARCHIVE_URL};" >> /mnt/nginx-generated-config/kubearchive.conf
-              echo "include /mnt/nginx-generated-config/auth.conf;" >> /mnt/nginx-generated-config/kubearchive.conf
-              echo "}" >> /mnt/nginx-generated-config/kubearchive.conf
+            # Generate tekton-results-proxy-pass.conf
+            echo "proxy_pass ${TEKTON_RESULTS_URL:?tekton results url must be provided};" \
+              > /mnt/nginx-generated-config/tekton-results-proxy-pass.conf
+
+            # Generate kubearchive-proxy-pass.conf
+            if [[ "$KUBEARCHIVE_URL" != "" ]]; then
+              echo "proxy_pass ${KUBEARCHIVE_URL};" \
+                > /mnt/nginx-generated-config/kubearchive-proxy-pass.conf
             else
-              echo "# KubeArchive disabled by config" > /mnt/nginx-generated-config/kubearchive.conf
+              echo "# KubeArchive disabled by config" > /mnt/nginx-generated-config/kubearchive-proxy-pass.conf
             fi
 
         volumeMounts:
         - name: nginx-generated-config
           mountPath: /mnt/nginx-generated-config
         - name: nginx-templates
           mountPath: /mnt/nginx-templates
         - name: api-token
           mountPath: /mnt/api-token
         securityContext:
           readOnlyRootFilesystem: true
           runAsNonRoot: true
           runAsUser: 1001
         resources:
           limits:
             cpu: 50m
             memory: 128Mi

c) Correct Kustomize overlays for environment-specific configurations:

For environments where KubeArchive is disabled (e.g., kflux-osp-p01, kflux-prd-rh02, stone-prd-rh01, stone-prod-p01), the proxy-init-config should be merged to set KUBEARCHIVE_URL to an empty string, rather than removing proxy-init-config and providing a commented kubearchive.conf.

Example for components/konflux-ui/production/kflux-osp-p01/kustomization.yaml:

--- a/components/konflux-ui/production/kflux-osp-p01/kustomization.yaml
+++ b/components/konflux-ui/production/kflux-osp-p01/kustomization.yaml
@@ -1,24 +1,24 @@
 apiVersion: kustomize.config.k8s.io/v1beta1
 kind: Kustomization
 resources:
   - ../base
   - configure-oauth-proxy-secret.yaml
 
 configMapGenerator:
   - name: dex
     files:
       - dex-config.yaml
-  - name: proxy-init-config
-    literals:
-      - IMPERSONATE=true
-      - TEKTON_RESULTS_URL=https://tekton-results-api-service.tekton-results.svc.cluster.local:8080
+  - name: proxy-init-config
+    literals:
+      - KUBEARCHIVE_URL= # Set to empty to disable KubeArchive
+    behavior: merge
   - name: proxy-nginx-static
     files:
       - kubearchive.conf
     behavior: merge
 
 patches:
   - path: add-service-certs-patch.yaml
     target:
       group: ""
       version: v1

Note: The proxy-nginx-static block for kubearchive.conf in these overlays becomes redundant if KUBEARCHIVE_URL is set to empty, as the generate-nginx-configs script will handle disabling it. It can be removed from these overlays for cleaner configuration.

Example for components/konflux-ui/production/kflux-osp-p01/kustomization.yaml (cleaner version):

--- a/components/konflux-ui/production/kflux-osp-p01/kustomization.yaml
+++ b/components/konflux-ui/production/kflux-osp-p01/kustomization.yaml
@@ -1,24 +1,21 @@
 apiVersion: kustomize.config.k8s.io/v1beta1
 kind: Kustomization
 resources:
   - ../base
   - configure-oauth-proxy-secret.yaml
 
 configMapGenerator:
   - name: dex
     files:
       - dex-config.yaml
-  - name: proxy-init-config
-    literals:
-      - IMPERSONATE=true
-      - TEKTON_RESULTS_URL=https://tekton-results-api-service.tekton-results.svc.cluster.local:8080
+  - name: proxy-init-config
+    literals:
+      - KUBEARCHIVE_URL= # Set to empty to disable KubeArchive
+    behavior: merge
 
 patches:
   - path: add-service-certs-patch.yaml
     target:
       group: ""
       version: v1
       kind: Service
       name: proxy

And remove the corresponding kubearchive.conf files from these overlay directories (e.g., components/konflux-ui/production/kflux-osp-p01/kubearchive.conf).

Example for components/konflux-ui/production/kflux-osp-p01/kubearchive.conf:

--- a/components/konflux-ui/production/kflux-osp-p01/kubearchive.conf
+++ /dev/null
@@ -1 +0,0 @@
-# KubeArchive disabled by config

(This file should be deleted)

Apply similar changes to all other affected kustomization.yaml files and their corresponding kubearchive.conf files if they are meant to disable KubeArchive.

@maruiz93
Copy link
Contributor Author

/unhold

@maruiz93
Copy link
Contributor Author

@gbenhaim @sahil143 can I get your reviews here?

Copy link

openshift-ci bot commented Aug 18, 2025

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

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

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 381ffce into redhat-appstudio:main Aug 18, 2025
6 checks passed
JoaoPedroPP added a commit to JoaoPedroPP/infra-deployments that referenced this pull request Aug 18, 2025
openshift-merge-bot bot pushed a commit that referenced this pull request Aug 18, 2025
JoaoPedroPP added a commit to JoaoPedroPP/infra-deployments that referenced this pull request Aug 18, 2025
gcpsoares pushed a commit to gcpsoares/infra-deployments that referenced this pull request Aug 19, 2025
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.

3 participants