Skip to content
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

DocDB instance user hook improvements #67

Merged
merged 42 commits into from
Apr 26, 2024
Merged

DocDB instance user hook improvements #67

merged 42 commits into from
Apr 26, 2024

Conversation

tomklapiscak
Copy link
Contributor

@tomklapiscak tomklapiscak commented Apr 25, 2024

https://jsw.ibm.com/browse/MASCORE-2333

This PR fixes and tidies up the aws-docdb sync hooks that used to run in the ibm-sls chart:

  • instance user will now actually be deleted when the MAS instance is deprovisioned (using a PostDelete hook)
  • the sync jobs are no longer run with cluster-admin priviledges
  • they live in their own chart (rather than being bundled in with ibm-sls as before which was a bit confusing)
  • the job to create a new user / update their password in DocDB will now only be rerun when it's actually necessary (i.e. when docdb config changes)

It was necessary to add 2 new (very minimal) argo applications per MAS instance:

  • 90-ibm-sync-resources contains a namespace (mas-xxx-syncres) and resources needed to run jobs (secrets/rbac/etc)
  • 91-ibm-sync-jobs is where the jobs will actually be run. Because this is in later syncwave than 90-ibm-sync-resources , it means the resources will persist long enough for any PostDelete hooks in 91-ibm-sync-jobs to complete successfully.

image

Testing performed

  • Minimal setup for driving the jobs setup in fvtsaas.
  • Pushed config necessary to initialize the instance.
  • Made sure the docdb-add-user job ran as expected:

image

  • Verified the instance mongo secret was updated as expected

  • Verified that I could login as the newly-created user

  • Deleted the instance config

  • Made sure the PostDelete hook ran as expected
    image
    image

  • Verified the instance mongo secret was deleted as expected

  • Verified that I could no longer login as the newly-created user

@tomklapiscak tomklapiscak marked this pull request as draft April 25, 2024 15:08
@tomklapiscak tomklapiscak marked this pull request as ready for review April 25, 2024 16:48
@tomklapiscak tomklapiscak requested a review from whitfiea April 25, 2024 16:48
Copy link
Contributor

@whitfiea whitfiea left a comment

Choose a reason for hiding this comment

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

I think we need to update the sync waves table with these new apps https://github.com/ibm-mas/gitops/tree/poc?tab=readme-ov-file#sync-waves

@tomklapiscak
Copy link
Contributor Author

I think we need to update the sync waves table with these new apps https://github.com/ibm-mas/gitops/tree/poc?tab=readme-ov-file#sync-waves

I think that documentation was written before we realised that sync waves are local to each application (which is why we thought that having separate sync wave ranges per chart was necessary). I've updated the docs to reflect this.

Now that we no longer need to reserve ranges for each application (and it's really easy to see which wave each app belongs to by looking at the filenames), I don't think there's much to be gained by repeating that information in the table, so I've removed it.

@whitfiea
Copy link
Contributor

@tomklapiscak yeah I agree, it was just there to help show the picture of syncwaves, but the names of the files should do that. Could you update the new ibm-sync- apps to be 090 rather than 90 as that helps with the vision ordering when looking in github.

@whitfiea whitfiea self-requested a review April 26, 2024 12:10
@tomklapiscak tomklapiscak merged commit 05d83b8 into poc Apr 26, 2024
1 check failed
@tomklapiscak tomklapiscak deleted the mascore2333 branch April 26, 2024 12:12
@tomklapiscak
Copy link
Contributor Author

@tomklapiscak yeah I agree, it was just there to help show the picture of syncwaves, but the names of the files should do that. Could you update the new ibm-sync- apps to be 090 rather than 90 as that helps with the vision ordering when looking in github.

For the benefit of anyone reading this, we agreed to not make (and test) this change now as we're planning to reorganise our application structure soon anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants