-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: add BPDM to umbrella chart #106
chore: add BPDM to umbrella chart #106
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @gomezbc thank you so much for the contribution!
charts/umbrella/Chart.yaml
Outdated
@@ -36,7 +36,7 @@ dependencies: | |||
- condition: portal.enabled | |||
name: portal | |||
repository: https://eclipse-tractusx.github.io/charts/dev | |||
version: 1.8.1 | |||
version: 2.0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please restrict your changes to bpdm?
I know it won't work as a whole as long as portal isn't upgrade as well (that's why we work on a separate upgrade branch until all components are ready). I'd upgrade portal, centraldip and sharedidp all together in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood. I'll revert portal version upgrade. I'm ready to help with the portal, centraldip, and sharedidp upgrades if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome that you offer to support with portal, centralidp and sharedidp! That would be very welcome.
Essentially it would be a take over from the state of the localdev chart here (I already upgraded that). Maybe after this pull request has been merged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! In that case, should I create a separate pull request for each component, or do I upgrade them in one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think one pull request with the upgrade of all 3 components would be also good :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you pleas also raise the chart version to 0.17.0? (the helm test workflow requires the increment to run https://github.com/eclipse-tractusx/tractus-x-umbrella/actions/runs/9844462507/job/27182944379?pr=106)
@evegufy Done 👍🏼 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @gomezbc the helm test workflow did its job correctly now :) and uncovered some linting issues, could you please fix them?
@evegufy I have resolved the linting issues. I noticed that other components disable PostgreSQL persistence by default, so I have done the same for the BPDM database. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one linting issue still:
charts/umbrella/values.yaml
Error: 748:19 [trailing-spaces] trailing spaces
Hi @evegufy, the action failed due to a timeout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @gomezbc sorry for the delay, I finally came around to reviewing again.
When install I'm facing 401 errors for pool, gate and dummy:
logs-from-bpdm-pool-in-umbrella-bpdm-pool-f5bb96f6d-d8xtn.log
logs-from-bpdm-cleaning-service-dummy-in-umbrella-bpdm-cleaning-service-dummy-678cc46bb9-hps8t.log
logs-from-bpdm-gate-in-umbrella-bpdm-gate-6d6cc7fdff-8mnkp.log
This might be due to missing the bdpm config discussed here eclipse-tractusx/portal-backend#819 (comment)
As @evegufy already said, you can use the newest hotfix version for BPDM 5.0.3 with the configuration provided in #108. The files also contain README instructions and a workflow to test the BPDM chart dependency setup. The new version has the advantage that the postgres does not need to have a fullnameOverride anymore. The ingress can be changed for BPDM but I experienced compatibility problems with the Portal. At least for the Partner Network page, I discovered that that Portal version has outdated endpoint URLs when trying to access the Pool. I don't see a way to fix that except upgrading the Portal so the correct endpoint paths are used. I did not try the registration workflow with the Portal, so I don't know which path to the BPDM Gate the Portal expects there. |
Hi @nicoprow, thanks for the comments. I have review your configuration and is awesome. As you have a better understanding of BPDM, I think the best approach would be to use your configuration. Regarding the compatibility with the portal, in my local setup, I had to upgrade to version 2.0.0 to make it compatible with BPDM. Using that version, the registration process works correctly. As I discussed with @evegufy (#106 (comment)), I will provide an upgrade to the portal, centralidp, and sharedidp. I will try to provide it as soon as I can. |
Hi @nicoprow After creating the PR for the portal version update #110, I switched my configuration to yours to connect the portal with BPDM. However, I encountered some permission issues. Upon comparing the roles required by BPDM with those defined in CentralIdp, I resolved these issues. The roles you defined for the Gate, Pool, and Orchestrator do not match those used by the CentralIdp sa-cl7-cx-5 client. For example, here are the roles you set for Gate (Cl16-CX-BPDMGate): permissions:
readInputPartner: view_company_data
writeInputPartner: update_company_data
readOutputPartner: view_shared_data
readInputChangelog: view_company_data
readOutputChangelog: view_shared_data
readSharingState: view_company_data
writeSharingState: update_company_data
read_stats: view_company_data And here are the roles provided by CentralIdp: To fix the Gate and Pool permissions, I deleted those settings, as the default configuration matches Cl16-CX-BPDMGate and Cl17-CX-BPDM. For the Orchestrator, I modified the values you set in permissions to these: permissions:
createTask: "write_partner"
readTask: "write_partner"
reservation:
clean: "write_partner"
cleanAndSync: "write_partner"
poolSync: "write_partner"
result:
clean: "write_partner"
cleanAndSync: "write_partner"
poolSync: "write_partner" Warning I have successfully completed the BPN creation process from the portal using the updated portal version here. It will not be possible to complete the BPN creation process with the portal version defined in this PR. |
Just quickly clarifying on what you mentioned regarding outdated endpoints in the portal: I assume you tested with the version of the portal which is currently still in the main branch of the umbrella repo, which is the portal version 1.8.0 for the release 24.03, that one is expected to be not compatible with the BPDM version for 24.05, the portal version for the 24.05 release is the 2.0.0. |
Yea, your permission configuration is exactly right to do for the Central-IDP 24.05 release. Great that you made it work! If you want you can remove the base-URL overwrite for the BPDM clients as they are able to find each other by service name. I don't think is necessary to change it but I thought I should mention it (see the comment below) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments below as an option to further simplify the configuration.
Since there is now also the values-test-bpdm.yaml inside I would also expect the BPDM chart test to be part of this pull request.
@evegufy will check out the chart testing |
1f75ae1
to
fd12616
Compare
a5afa79
to
cfd8477
Compare
Description
This PR adds BPDM chart to the umbrella repository. Due to incompatible Portal version, I had to bump portal chart to 2.0.0. As mentioned in this issue, eclipse-tractusx/portal#248, in version 1.8.1 of Portal, it calls this enpoint
/companies/test-company/v6/
of the gate, which is incompatible with the added gate.This BPDM is prepared to work with centralidp, but as it is planned for future release eclipse-tractusx/sig-release#751, currently it will return 403 error in BPN step.
eclipse-tractusx/sig-release#710
Pre-review checks
Please ensure to do as many of the following checks as possible, before asking for committer review: