integrate konflux-ui with namespace-lister #5297

filariow · 2025-01-16T13:28:49Z

Signed-off-by: Francesco Ilario [email protected]

openshift-ci · 2025-01-16T13:28:54Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

filariow · 2025-01-16T14:15:39Z

/hold

gbenhaim · 2025-01-16T14:28:09Z

components/konflux-ui/staging/base/proxy/nginx.conf

+            rewrite ^/(.*)/$ /$1 permanent;
+            proxy_pass https://kubernetes.default.svc;
+            if ($request_method = GET) {
+                proxy_pass http://namespace-lister.namespace-lister.svc.cluster.local:8080;


please use https. certificates for namespace-lister can be generated using openshift https://docs.openshift.com/container-platform/4.15/security/certificates/service-serving-certificate.html

components/konflux-ui/staging/base/proxy/nginx.conf

gbenhaim · 2025-01-16T14:36:40Z

components/konflux-ui/staging/base/proxy/nginx.conf

+            # namespace-lister endpoint
+            rewrite ^/(.*)/$ /$1 permanent;
+            proxy_pass https://kubernetes.default.svc;
+            if ($request_method = GET) {


it's discouraged to use "if" in location blocks. can we use map instead?

gbenhaim · 2025-01-16T14:39:00Z

components/konflux-ui/staging/base/proxy/nginx.conf

@@ -161,6 +161,24 @@ http {
            include /mnt/nginx-generated-config/bearer.conf;
        }

+        location ~* /api/v1/namespaces(/?)$ {


the ui prefixes requests to the k8s api with /api/k8s/

gbenhaim · 2025-01-16T15:02:07Z

components/konflux-ui/staging/base/proxy/nginx.conf

+            include /mnt/nginx-generated-config/bearer.conf;
+
+            # namespace-lister endpoint
+            rewrite ^/(.*)/$ /$1 permanent;


we should use break instead of permanent. we want to reverse proxy not redirect.

sadlerap

lgtm, please fix merge conflicts

filariow · 2025-01-16T17:08:19Z

@sadlerap thanks for merging the conflicts

openshift-ci · 2025-01-16T17:08:48Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: filariow, sadlerap
Once this PR has been reviewed and has the lgtm label, please assign ksprinkl for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

sadlerap · 2025-01-16T17:28:17Z

/retest

Signed-off-by: Francesco Ilario <[email protected]>

components/konflux-ui/staging/base/proxy/nginx.conf

gbenhaim · 2025-01-16T17:51:19Z

components/namespace-lister/base/service.yaml

@@ -3,11 +3,13 @@ kind: Service
 metadata:
  name: namespace-lister
  namespace: namespace-lister
+  annotations:
+    service.beta.openshift.io/serving-cert-secret-name: namespace-lister-tls


I would expect to see a change to namespace-lister deployment as well for mounting the tls certificates.

namespace-lister is not supporting TLS at the moment. let's move this to a future PR.

I've filed konflux-ci/namespace-lister#29 to enable it in namespace-lister.

gbenhaim · 2025-01-16T17:53:04Z

components/konflux-ui/staging/base/proxy/nginx.conf

@@ -161,6 +161,25 @@ http {
            include /mnt/nginx-generated-config/bearer.conf;
        }

+        location ~* /api/k8s/api/v1/namespaces(/?)$ {


I think this can be simplified by dropping the regex expression. wdyt?

IIUC, If we remove this regex this will become the path every namespaced request will take, e.g. api/v1/namespaces/my-ns/pods will be processed by this rule

Correct. We only want to match on requests for namespace resources, i.e. when the path is /api/k8s/api/v1/namespaces or /api/k8s/api/v1/namespaces/.

If I'm understanding nginx's docs correctly, we could set it up like this and have it work:

Suggested change

location ~* /api/k8s/api/v1/namespaces(/?)$ {

location = /api/k8s/api/v1/namespaces/ {

This would match the path we want, both with and without the trailing slash. However, this has slightly different behavior than what we currently have: nginx will issue a 301 permanent redirect from /api/k8s/api/v1/namespaces to /api/k8s/api/v1/namespaces/, which I'm not sure I'm a fan of. To me, the regex is clearer in its intent.

Also, if we do decide to use regex matching here, we should probably do case-sensitive matching:

Suggested change

location ~* /api/k8s/api/v1/namespaces(/?)$ {

location ~ /api/k8s/api/v1/namespaces(/?)$ {

Signed-off-by: Francesco Ilario <[email protected]>

sadlerap

Looks okay, but there's a merge conflict that can't be resolved from github's web interface.

openshift-ci bot added the do-not-merge/work-in-progress label Jan 16, 2025

filariow force-pushed the integrate-namespace-lister-ui branch from b9960bb to 79c08ab Compare January 16, 2025 14:09

openshift-ci bot added the do-not-merge/hold label Jan 16, 2025

filariow marked this pull request as ready for review January 16, 2025 14:15

openshift-ci bot removed the do-not-merge/work-in-progress label Jan 16, 2025

openshift-ci bot requested review from dperaza4dustbit and sadlerap January 16, 2025 14:15

filariow requested a review from gbenhaim January 16, 2025 14:16

gbenhaim requested changes Jan 16, 2025

View reviewed changes

filariow requested a review from gbenhaim January 16, 2025 15:35

sadlerap reviewed Jan 16, 2025

View reviewed changes

sadlerap approved these changes Jan 16, 2025

View reviewed changes

filariow added 3 commits January 16, 2025 18:31

integrate konflux-ui with namespace-lister

1a73296

Signed-off-by: Francesco Ilario <[email protected]>

address review comments

1368647

Signed-off-by: Francesco Ilario <[email protected]>

fix rewrite rule

539b099

Signed-off-by: Francesco Ilario <[email protected]>

filariow force-pushed the integrate-namespace-lister-ui branch from 7c062c9 to 539b099 Compare January 16, 2025 17:31

filariow commented Jan 16, 2025

View reviewed changes

components/konflux-ui/staging/base/proxy/nginx.conf Outdated Show resolved Hide resolved

Update components/konflux-ui/staging/base/proxy/nginx.conf

0151cf2

gbenhaim reviewed Jan 16, 2025

View reviewed changes

fix nginx.conf

38ff1ef

Signed-off-by: Francesco Ilario <[email protected]>

filariow force-pushed the integrate-namespace-lister-ui branch from a062b59 to 38ff1ef Compare January 16, 2025 18:38

filariow added 2 commits January 16, 2025 20:00

rollback tls changes

b78750b

Signed-off-by: Francesco Ilario <[email protected]>

fix nginx.conf

f72882d

Signed-off-by: Francesco Ilario <[email protected]>

sadlerap reviewed Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

integrate konflux-ui with namespace-lister #5297

integrate konflux-ui with namespace-lister #5297

filariow commented Jan 16, 2025

openshift-ci bot commented Jan 16, 2025

filariow commented Jan 16, 2025

gbenhaim Jan 16, 2025

gbenhaim Jan 16, 2025

gbenhaim Jan 16, 2025

gbenhaim Jan 16, 2025

sadlerap left a comment

filariow commented Jan 16, 2025

openshift-ci bot commented Jan 16, 2025

sadlerap commented Jan 16, 2025

gbenhaim Jan 16, 2025

filariow Jan 16, 2025

sadlerap Jan 16, 2025

gbenhaim Jan 16, 2025

filariow Jan 16, 2025

sadlerap Jan 16, 2025

sadlerap left a comment

	location ~* /api/k8s/api/v1/namespaces(/?)$ {
	location = /api/k8s/api/v1/namespaces/ {

	location ~* /api/k8s/api/v1/namespaces(/?)$ {
	location ~ /api/k8s/api/v1/namespaces(/?)$ {

integrate konflux-ui with namespace-lister #5297

Are you sure you want to change the base?

integrate konflux-ui with namespace-lister #5297

Conversation

filariow commented Jan 16, 2025

openshift-ci bot commented Jan 16, 2025

filariow commented Jan 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadlerap left a comment

Choose a reason for hiding this comment

filariow commented Jan 16, 2025

openshift-ci bot commented Jan 16, 2025

sadlerap commented Jan 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadlerap left a comment

Choose a reason for hiding this comment