Skip to content

Fix not being able to edit Billing members - #535

Draft
ccremer wants to merge 1 commit into
masterfrom
fix-be-members
Draft

Fix not being able to edit Billing members#535
ccremer wants to merge 1 commit into
masterfrom
fix-be-members

Conversation

@ccremer

@ccremer ccremer commented Apr 14, 2023

Copy link
Copy Markdown
Contributor

Summary

v0.24 of control-api has been released, which creates automatically the admin cluster role for billing entities when created. This removes the need (and assumption) that the portal should be managing both ClusterRole and ClusterRoleBinding. Now, only the ClusterRoleBinding is required to be edited.

Also fixes the redirect after saving the members -> goes back to BE details instead of Zones.

Checklist

  • Categorize the PR by setting a good title and adding one of the labels:
    bug, enhancement, documentation, change, breaking,
    as they show up in the changelog
  • Update tests.
  • Link this PR to related issues.

@ccremer ccremer added the bug Something isn't working label Apr 14, 2023
@ccremer
ccremer changed the base branch from master to guide-new-users April 14, 2023 14:36
@ccremer
ccremer requested a review from corvus-ch April 14, 2023 14:36
@ccremer
ccremer marked this pull request as ready for review April 14, 2023 14:42
@ccremer
ccremer requested a review from a team as a code owner April 14, 2023 14:42
@ccremer
ccremer temporarily deployed to preview April 14, 2023 14:44 — with GitHub Actions Inactive
@ccremer
ccremer temporarily deployed to preview April 14, 2023 14:45 — with GitHub Actions Inactive
@github-actions

This comment was marked as outdated.

@corvus-ch
corvus-ch requested a review from bastjan April 17, 2023 09:13

@corvus-ch corvus-ch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I added @bastjan as a reviewer for this. From a priority perspective, this is not urgent. The cause, triggering this, is unlikely to happen within the next days(/weeks).

@ccremer
ccremer temporarily deployed to preview April 17, 2023 09:35 — with GitHub Actions Inactive
@bastjan

bastjan commented Apr 17, 2023

Copy link
Copy Markdown
Member

There will still be cases where a sudoer needs to create those roles from the portal GUI. We still have customers in the Odoo that might request access but can't see their BEs since they are not setup correctly.

We might add some reconciling logic for the roles but would prefer to correct the rules here until we have smth like this.

See for correct rules:

https://github.com/appuio/control-api/blob/c108373328b681637e39f1728425cd9a27046766/apiserver/billing/rbac.go#L48-L60

https://github.com/appuio/control-api/blob/c108373328b681637e39f1728425cd9a27046766/apiserver/billing/rbac.go#L73-L91

@ccremer
ccremer temporarily deployed to preview April 17, 2023 09:36 — with GitHub Actions Inactive
@github-actions

This comment was marked as outdated.

@ccremer
ccremer temporarily deployed to preview April 17, 2023 10:01 — with GitHub Actions Inactive
@ccremer
ccremer temporarily deployed to preview April 17, 2023 10:02 — with GitHub Actions Inactive
@github-actions

This comment was marked as outdated.

Base automatically changed from guide-new-users to master April 18, 2023 09:20
@ccremer
ccremer temporarily deployed to preview April 18, 2023 10:42 — with GitHub Actions Inactive
@ccremer
ccremer temporarily deployed to preview April 18, 2023 10:42 — with GitHub Actions Inactive
@github-actions

This comment was marked as outdated.

@ccremer

ccremer commented Apr 20, 2023

Copy link
Copy Markdown
Contributor Author

@corvus-ch @bastjan I don't quite understand what the holdup is for this fix.
Right now (also in Prod), when users create a billing address and then try to edit members (billingentities/be-xxxx/members), they get presented with an error and the glitchtip reporter dialog. This PR primarily addresses this issue so the reporter doesn't show up anymore.

@bastjan

bastjan commented Apr 20, 2023

Copy link
Copy Markdown
Member

As i said: there is no reconciliation and no guarantee that the admin role is created if an existing, wrongly setup BE is enabled/fixed. We still need the admin role creation for VSHN support team members.

I'd strongly prefer to update the role creation code with the rules i linked.

@ccremer
ccremer temporarily deployed to preview April 20, 2023 11:54 — with GitHub Actions Inactive
@ccremer
ccremer temporarily deployed to preview April 20, 2023 11:55 — with GitHub Actions Inactive
@ccremer

ccremer commented Apr 20, 2023

Copy link
Copy Markdown
Contributor Author

Ok, leaving this for now. Let me know if you prefer to disable this view temporarily, so that users can't click/visit the BE members list view.

@github-actions

This comment was marked as outdated.

@bastjan

bastjan commented Apr 20, 2023

Copy link
Copy Markdown
Member

I might misunderstand the issue.

Why not just create the roles as defined here? #535 (comment)

@corvus-ch

Copy link
Copy Markdown
Contributor

As far as I understand, this issue does not surface when users create their BillingEntity on their own. Only when they get created in the ERP. So, this is not an issue to expect within the next days. Instead of wasting time with some workarounds (which we might have to remove again later) in the frontend, I would prefer to fix this for good in the API.

@HappyTetrahedron

Copy link
Copy Markdown

As far as I have just tested, this issue does in fact surface for user-created BillingEntities as well.

I just created a billingentity through the portal (using a new account created through social login), and subsequently when I click on "edit members", the issue described above pops up (error message + glitchtip popup). So it is not as uncommon as we initially thought.

@ccremer
ccremer temporarily deployed to preview May 8, 2023 09:15 — with GitHub Actions Inactive
@github-actions

github-actions Bot commented May 8, 2023

Copy link
Copy Markdown

🚀 Preview deployment active

App URL https://portal-pr-535-appuio-control-api-preview.apps.cloudscale-lpg-2.appuio.cloud
Revision 60c87ba
Helm release appuio-control-api-preview/portal-pr-535
Cluster https://api.cloudscale-lpg-2.appuio.cloud:6443

To uninstall this deployment, close or merge this PR.

@bastjan
bastjan marked this pull request as draft July 16, 2025 09:07
@github-actions

Copy link
Copy Markdown

✔️ Preview deployment uninstalled

Helm release appuio-control-api-preview/portal-pr-535
Cluster https://api.cloudscale-lpg-2.appuio.cloud:6443

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants