add namespace-lister deployment to staging #5163
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sadlerap The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
I think it may not be a bad idea to hold merging this until after the holidays, since I'd rather avoid situations where we accidentally cause an outage while many people are out on vacation. |
sadlerap
force-pushed
the
namespace-lister
branch
2 times, most recently
from
27f18d1 to
aa01551
Compare
sadlerap
force-pushed
the
namespace-lister
branch
from
aa01551 to
b2771ca
Compare
This is largely for testing purposes, to see if this deployment configuration is functional. As such, we're only going to deploy this to one of the member clusters for the time being, and we're not going to integrate this with the UI's nginx config until we have a stable deployment. Signed-off-by: Andy Sadler <[email protected]>
sadlerap
force-pushed
the
namespace-lister
branch
from
b2771ca to
de8725d
Compare
There was a problem hiding this comment.
I suggest that it would be included as a side car in the proxy deployment https://github.com/redhat-appstudio/infra-deployments/tree/main/components/konflux-ui/production/base/proxy it would be much simpler (you can see how it was done for workspace-manager). I don't see a benefit of having it in its own deployment. In addition, since we used impersonation today, token review can't be turned on in namespace-lister, so it means anyone would be able to list namespaces.
There was a problem hiding this comment.
please see my comment above.
In addition namespace-lister should be incorporated in Konflux upstream as well - https://github.com/konflux-ci/konflux-ci/tree/main/konflux-ci/ui/core/proxy
|
I'll start with adding it into the staging deployment of the proxy, then move on to production and upstream if things look stable. |
|
@gbenhaim I'm mostly fine with any setup we chose, but I'd prefer to have separate deployments for the following reasons.
If we chose separate deployments we'll have better isolation and scaling. Isolation: A bug on Scaling: we can scale the
I'm not sure I got it correctly, but if it is a concern on external users (1) or other deployments (2) invoking the |
|
I think I'll leave it as a separate deployment for now. To me, the biggest reason to do so is a separation of concerns: in particular, we don't need nearly as many permissions on the namespace-lister deployment. The list of permissions that the proxy's service account would grant is far beyond what namespace-lister needs. Why should we grant namespace-lister the extra permissions that the proxy's service account would grant? Also, I'm not certain it should be moved under the konflux-ui component. Unless that component is also going to be deployed on the internal clusters, we'd be duplicating the resources between the @gbenhaim thoughts? |
| --- | ||
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: ClusterRoleBinding | ||
| metadata: | ||
| name: namespace-lister-auth-delegator | ||
| subjects: | ||
| - apiGroup: "" | ||
| kind: ServiceAccount | ||
| name: namespace-lister | ||
| namespace: namespace-lister | ||
| roleRef: | ||
| apiGroup: rbac.authorization.k8s.io | ||
| kind: ClusterRole | ||
| name: system:auth-delegator |
There was a problem hiding this comment.
in the near future we won't rely on the TokenReview API. I'd suggest to move this ClusterRoleBinding to separate file under staging/stone-stg-rh01
There was a problem hiding this comment.
If I'm getting it correctly, this is exposing the namespace-lister directly and not the proxy.
If we decide not to deploy the Proxy, this can be removed too
There was a problem hiding this comment.
We may simplify by not deploying a Proxy at all and by using port-forward to test namespace-lister is working properly.
ok, you convinced me with this argument.
The konflux-ui component is going to be deployed on all clusters (external and internal).
You would still need to do the following changes:
|
| labels: | ||
| apps: namespace-lister | ||
| spec: | ||
| # securityContext: |
There was a problem hiding this comment.
can you please remove the comments?
This is largely for testing purposes, to see if this deployment configuration is functional. As such, we're only going to deploy this to one of the member clusters for the time being, and we're not going to integrate this with the UI's nginx config until we have a stable deployment.