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

BC-6420 - unsynced users deletion #4868

Merged
merged 77 commits into from
Apr 11, 2024
Merged
Show file tree
Hide file tree
Changes from 74 commits
Commits
Show all changes
77 commits
Select commit Hold shift + click to select a range
42cf9e1
crate entity, repo, do, mapper for synchronization and deletion-recon…
WojciechGrancow Feb 8, 2024
bd982fb
Merge branch 'main' into BC-6223-implementation-moinSchule-interface-…
WojciechGrancow Feb 9, 2024
a7f6cfc
add synchronization service and tests
WojciechGrancow Feb 9, 2024
438a0a2
changes in reconcilation UC
grancow Feb 12, 2024
a243c10
Merge branch 'main' into BC-6223-implementation-moinSchule-interface-…
WojciechGrancow Feb 14, 2024
b289240
add lastSyncedAt to userEntity and change create in synchronization s…
WojciechGrancow Feb 14, 2024
f5efadd
create synchronization module and move files from deletion module int…
WojciechGrancow Feb 14, 2024
bdffe26
add new interface, builders, method in userrepo and new methos in use…
WojciechGrancow Feb 15, 2024
27f053e
some changes
WojciechGrancow Feb 16, 2024
16f8958
Merge branch 'main' into BC-6223-implementation-moinSchule-interface-…
WojciechGrancow Feb 19, 2024
b0c9472
mofdify acount, user service and synchronization UC
WojciechGrancow Feb 19, 2024
f278c46
add some tests, remove userIdAndExternalId interface, builder
WojciechGrancow Mar 18, 2024
5eaeaa2
Merge branch 'main' into BC-6223-implementation-moinSchule-interface-…
WojciechGrancow Mar 19, 2024
f9ecc80
changes in synchronizationEntity
WojciechGrancow Mar 19, 2024
c0e4302
add staus to synchronizationEntity
WojciechGrancow Mar 19, 2024
f34eafb
add update method in repo in synchronization module
WojciechGrancow Mar 20, 2024
161dc2a
Merge branch 'main' into BC-6223-implementation-moinSchule-interface-…
WojciechGrancow Mar 20, 2024
bc23aa8
fix imports, add update method and test in service
WojciechGrancow Mar 20, 2024
d3e9050
add synchromization.module, fix import, modify uc
WojciechGrancow Mar 20, 2024
667c40d
fix in acount service test
WojciechGrancow Mar 20, 2024
ad33efb
change folder structure in synchronization module, fix test in repo
WojciechGrancow Mar 20, 2024
d8ed8fb
coments about chunk
WojciechGrancow Mar 20, 2024
a198901
add loggable, and extension of UC
WojciechGrancow Mar 21, 2024
99dd7d0
Merge branch 'main' into BC-6223-implementation-moinSchule-interface-…
WojciechGrancow Mar 21, 2024
98cd070
Merge branch 'main' into BC-6223-implementation-moinSchule-interface-…
WojciechGrancow Mar 21, 2024
9d49585
add test cases to loggable and to userService
WojciechGrancow Mar 21, 2024
32c9652
add tests to useCase in synchronizationModule
WojciechGrancow Mar 21, 2024
54a5f27
add test to UC
WojciechGrancow Mar 21, 2024
05942b2
BC-6223-add chunks
Mar 21, 2024
dc7ac4b
add test cases in UC
WojciechGrancow Mar 21, 2024
9c5d26a
add tests
Mar 21, 2024
fe5aabc
Merge branch 'BC-6223-implementation-moinSchule-interface-in-KNL' of …
WojciechGrancow Mar 21, 2024
219ac19
remove private methon
WojciechGrancow Mar 21, 2024
63ec575
refactor
Mar 21, 2024
e6a8cf1
Merge branch 'BC-6223-implementation-moinSchule-interface-in-KNL' of …
Mar 21, 2024
b20766e
add tests and change loop
WojciechGrancow Mar 22, 2024
e484758
refactor
WojciechGrancow Mar 22, 2024
804ab65
add test to UC
WojciechGrancow Mar 22, 2024
2f03180
add config
WojciechGrancow Mar 22, 2024
f4c62d5
Merge branch 'main' into BC-6223-implementation-moinSchule-interface-…
WojciechGrancow Mar 22, 2024
84d0dd9
sum with promise.all
Mar 22, 2024
99ee885
fixes typo, changes test in UC
WojciechGrancow Mar 22, 2024
e8bab90
Merge branch 'main' into BC-6223-implementation-moinSchule-interface-…
WojciechGrancow Mar 22, 2024
87e5c90
rename testConfig file
WojciechGrancow Mar 22, 2024
f8c50ab
add new command (for queueing unsynchronized entities for deletion)
bn-pass Mar 22, 2024
740c772
add new Ansible role for moin.schule sync with users deletion queuein…
bn-pass Mar 22, 2024
4399232
Update apps/server/src/shared/repo/user/user.repo.integration.spec.ts
WojciechGrancow Mar 22, 2024
981f177
Merge branch 'main' into BC-6420-unsynced-users-del
bn-pass Mar 22, 2024
b5a82a7
fix some issues after review
WojciechGrancow Mar 22, 2024
4f90380
Merge branch 'main' into BC-6223-implementation-moinSchule-interface-…
WojciechGrancow Mar 22, 2024
7359efd
fiz error cases in uc
WojciechGrancow Mar 22, 2024
257d7e7
add test in UC
WojciechGrancow Mar 22, 2024
3dc6a80
modify SynchronizationErrorLoggableException
WojciechGrancow Mar 23, 2024
9aa696f
Merge remote-tracking branch 'origin/BC-6223-implementation-moinSchul…
sszafGCA Mar 24, 2024
046a930
main logic impl
sszafGCA Mar 25, 2024
b3df782
Merge branch 'main' into BC-6420-unsynced-users-del
sszafGCA Mar 25, 2024
24bb4c5
fix tests
sszafGCA Mar 26, 2024
205412d
Merge remote-tracking branch 'origin/main' into BC-6420-unsynced-user…
sszafGCA Mar 26, 2024
a25a0db
remove files that were not resolved automatically after merge
sszafGCA Mar 26, 2024
58704c6
Merge branch 'main' into BC-6420-unsynced-users-del
sszafGCA Mar 26, 2024
dcbba58
Merge branch 'main' into BC-6420-unsynced-users-del
sszafGCA Mar 27, 2024
6de977e
Pr fix and test implementation part1
sszafGCA Mar 27, 2024
b473b77
test impl part2
sszafGCA Mar 27, 2024
c806112
test impl part 3
sszafGCA Mar 27, 2024
49d3ede
test implementation part 3
sszafGCA Mar 28, 2024
585230e
Merge branch 'main' into BC-6420-unsynced-users-del
sszafGCA Mar 28, 2024
4a6fb24
add needed modules and providers
sszafGCA Mar 28, 2024
61f7746
add mongoDb flag
sszafGCA Mar 28, 2024
efa9ca1
fix import order
sszafGCA Mar 28, 2024
1b2dcc7
import fix
sszafGCA Mar 29, 2024
7331efb
Merge branch 'main' into BC-6420-unsynced-users-del
sszafGCA Mar 29, 2024
7b67223
import chnages
sszafGCA Apr 2, 2024
2c7a8ac
Merge branch 'main' into BC-6420-unsynced-users-del
sszafGCA Apr 2, 2024
3b7e2a0
Merge branch 'main' into BC-6420-unsynced-users-del
sszafGCA Apr 3, 2024
5cb2e3a
Merge branch 'main' into BC-6420-unsynced-users-del
bn-pass Apr 10, 2024
074568b
refactor dependencies
bn-pass Apr 10, 2024
3bf0bc2
Merge branch 'main' into BC-6420-unsynced-users-del
bn-pass Apr 11, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions ansible/roles/moin-schule-sync/meta/main.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
galaxy_info:
role_name: moin-schule-sync
author: Schul-Cloud Verbund
description: moin-schule-sync role for the moin.schule synchronization purposes
company: Schul-Cloud Verbund
license: license (AGPLv3)
min_ansible_version: 2.8
galaxy_tags: []
dependencies: []
13 changes: 13 additions & 0 deletions ansible/roles/moin-schule-sync/tasks/main.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
- name: moin-schule-sync secret provisioned by 1Password
when: WITH_MOIN_SCHULE is defined and WITH_MOIN_SCHULE|bool == true
kubernetes.core.k8s:
kubeconfig: ~/.kube/config
namespace: "{{ NAMESPACE }}"
template: moin-schule-sync-onepassword.yml.j2

- name: unsynced moin.schule users deletion queueing CronJob
when: WITH_MOIN_SCHULE is defined and WITH_MOIN_SCHULE|bool == true and WITH_UNSYNCED_ENTITIES_DELETION is defined and WITH_UNSYNCED_ENTITIES_DELETION|bool == true
kubernetes.core.k8s:
kubeconfig: ~/.kube/config
namespace: "{{ NAMESPACE }}"
template: moin-schule-users-deletion-queueing-cronjob.yml.j2
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
apiVersion: onepassword.com/v1
kind: OnePasswordItem
metadata:
name: moin-schule-sync-secret
namespace: {{ NAMESPACE }}
labels:
app: moin-schule-sync
spec:
itemPath: "vaults/{{ ONEPASSWORD_OPERATOR_VAULT }}/items/moin-schule-sync"
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
apiVersion: batch/v1
kind: CronJob
metadata:
namespace: {{ NAMESPACE }}
labels:
app: moin-schule-users-deletion-queueing-cronjob
app.kubernetes.io/part-of: schulcloud-verbund
app.kubernetes.io/version: {{ SCHULCLOUD_SERVER_IMAGE_TAG }}
app.kubernetes.io/name: moin-schule-users-deletion-queueing-cronjob
app.kubernetes.io/component: sync
app.kubernetes.io/managed-by: ansible
git.branch: {{ SCHULCLOUD_SERVER_BRANCH_NAME }}
git.repo: {{ SCHULCLOUD_SERVER_REPO_NAME }}
name: moin-schule-users-deletion-queueing-cronjob
spec:
schedule: "{{ MOIN_SCHULE_USERS_DELETION_QUEUEING_CRONJOB_SCHEDULE|default("@hourly", true) }}"
jobTemplate:
spec:
template:
spec:
containers:
- name: moin-schule-users-deletion-queueing-cronjob
image: {{ SCHULCLOUD_SERVER_IMAGE }}:{{ SCHULCLOUD_SERVER_IMAGE_TAG }}
envFrom:
- secretRef:
name: moin-schule-sync-secret
command: ['/bin/sh','-c']
args: ['npm run nest:start:deletion-console -- queue unsynced --systemId $SYSTEM_ID']
resources:
limits:
cpu: {{ API_CPU_LIMITS|default("2000m", true) }}
memory: {{ API_MEMORY_LIMITS|default("2Gi", true) }}
requests:
cpu: {{ API_CPU_REQUESTS|default("100m", true) }}
memory: {{ API_MEMORY_REQUESTS|default("150Mi", true) }}
restartPolicy: OnFailure
{% if AFFINITY_ENABLE is defined and AFFINITY_ENABLE|bool %}
affinity:
podAffinity:
preferredDuringSchedulingIgnoredDuringExecution:
- weight: 100
podAffinityTerm:
labelSelector:
matchExpressions:
- key: app.kubernetes.io/part-of
operator: In
values:
- schulcloud-verbund
topologyKey: "kubernetes.io/hostname"
namespaceSelector: {}
{% endif %}
metadata:
labels:
app: moin-schule-users-deletion-queueing-cronjob
app.kubernetes.io/part-of: schulcloud-verbund
app.kubernetes.io/version: {{ SCHULCLOUD_SERVER_IMAGE_TAG }}
app.kubernetes.io/name: moin-schule-users-deletion-queueing-cronjob
app.kubernetes.io/component: sync
app.kubernetes.io/managed-by: ansible
git.branch: {{ SCHULCLOUD_SERVER_BRANCH_NAME }}
git.repo: {{ SCHULCLOUD_SERVER_REPO_NAME }}
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import { ObjectId } from 'bson';
import { UnsyncedEntitiesOptions } from '../interface';
import { UnsyncedEntitiesOptionsBuilder } from './unsynced-entities-options.builder';

describe(UnsyncedEntitiesOptionsBuilder.name, () => {
describe(UnsyncedEntitiesOptionsBuilder.build.name, () => {
describe('when called with valid arguments', () => {
const setup = () => {
const systemId = new ObjectId().toHexString();
const unsyncedForMinutes = 3600;
const targetRefDomain = 'school';
const deleteInMinutes = 43200;
const callsDelayMs = 100;

const expectedOutput: UnsyncedEntitiesOptions = {
systemId,
unsyncedForMinutes,
targetRefDomain,
deleteInMinutes,
callsDelayMs,
};

return {
systemId,
unsyncedForMinutes,
targetRefDomain,
deleteInMinutes,
callsDelayMs,
expectedOutput,
};
};

it('should return valid options object with expected values', () => {
const { systemId, unsyncedForMinutes, targetRefDomain, deleteInMinutes, callsDelayMs, expectedOutput } =
setup();

const output = UnsyncedEntitiesOptionsBuilder.build(
systemId,
unsyncedForMinutes,
targetRefDomain,
deleteInMinutes,
callsDelayMs
);

expect(output).toStrictEqual(expectedOutput);
});
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import { EntityId } from '@shared/domain/types';
import { UnsyncedEntitiesOptions } from '../interface';

export class UnsyncedEntitiesOptionsBuilder {
static build(
systemId: EntityId,
unsyncedForMinutes: number,
targetRefDomain?: string,
deleteInMinutes?: number,
callsDelayMs?: number
): UnsyncedEntitiesOptions {
return {
systemId,
unsyncedForMinutes,
targetRefDomain,
deleteInMinutes,
callsDelayMs,
};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,46 @@ import { HttpModule } from '@nestjs/axios';
import { ConfigModule } from '@nestjs/config';
import { ConsoleModule } from 'nestjs-console';
import { ConsoleWriterModule } from '@infra/console';
import { createConfigModuleOptions } from '@src/config';
import { DB_PASSWORD, DB_URL, DB_USERNAME, createConfigModuleOptions } from '@src/config';
import { CqrsModule } from '@nestjs/cqrs';
import { MikroOrmModule } from '@mikro-orm/nestjs';
import { ALL_ENTITIES } from '@shared/domain/entity';
import { RoleRepo, UserRepo } from '@shared/repo';
import { UserDORepo } from '@shared/repo/user/user-do.repo';
import { LoggerModule } from '@src/core/logger';
import { UserService } from '@modules/user';
import { AccountModule } from '@modules/account';
import { RoleService } from '@modules/role';
import { RegistrationPinService } from '@modules/registration-pin';
import { DeletionClient } from './deletion-client';
import { getDeletionClientConfig } from './deletion-client/deletion-client.config';
import { DeletionQueueConsole } from './deletion-queue.console';
import { DeletionExecutionConsole } from './deletion-execution.console';
import { BatchDeletionService } from './services';
import { BatchDeletionUc, DeletionExecutionUc } from './uc';
import { defaultMikroOrmOptions } from '../server';
import { FileEntity } from '../files/entity';
import { RegistrationPinRepo } from '../registration-pin/repo';

@Module({
imports: [
ConsoleModule,
ConsoleWriterModule,
HttpModule,
ConfigModule.forRoot(createConfigModuleOptions(getDeletionClientConfig)),
AccountModule,
MikroOrmModule.forRoot({
...defaultMikroOrmOptions,
type: 'mongo',
clientUrl: DB_URL,
password: DB_PASSWORD,
user: DB_USERNAME,
allowGlobalContext: true,
entities: [...ALL_ENTITIES, FileEntity],
debug: true,
}),
LoggerModule,
CqrsModule,
],
providers: [
DeletionClient,
Expand All @@ -25,6 +51,13 @@ import { BatchDeletionUc, DeletionExecutionUc } from './uc';
DeletionExecutionUc,
DeletionQueueConsole,
DeletionExecutionConsole,
UserService,
Copy link
Contributor

Choose a reason for hiding this comment

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

these services need to go, if you need them import the modules instead.
Some of the Repos are an exception since they are not part of a module, but you should avoid using the repos at all directly, and instead introduce service methods for things you need to do from the outside.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 074568b - all the non-local dependencies were moved to imports, in providers there are just the local dependencies now.

UserRepo,
UserDORepo,
RoleService,
RegistrationPinService,
RoleRepo,
RegistrationPinRepo,
],
})
export class DeletionConsoleModule {}
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import { ObjectId } from 'bson';
import { Test, TestingModule } from '@nestjs/testing';
import { ConsoleWriterService } from '@infra/console';
import { createMock } from '@golevelup/ts-jest';
import { DeletionQueueConsole } from './deletion-queue.console';
import { PushDeleteRequestsOptionsBuilder } from './builder';
import { BatchDeletionUc } from './uc';
import { UnsyncedEntitiesOptionsBuilder } from './builder/unsynced-entities-options.builder';

describe(DeletionQueueConsole.name, () => {
let module: TestingModule;
Expand Down Expand Up @@ -76,4 +78,34 @@ describe(DeletionQueueConsole.name, () => {
});
});
});

describe('unsyncedEntities', () => {
describe('when called with an invalid "unsyncedForMinutes" option', () => {
const setup = () => {
const options = UnsyncedEntitiesOptionsBuilder.build(new ObjectId().toHexString(), 15);

return { options };
};

it('should throw an exception', async () => {
const { options } = setup();

await expect(console.unsyncedEntities(options)).rejects.toThrow();
});
});

describe('when called with valid options', () => {
const setup = () => {
const options = UnsyncedEntitiesOptionsBuilder.build(new ObjectId().toHexString(), 3600);

return { options };
};

it('should not throw any exception indicating invalid options', async () => {
const { options } = setup();

await expect(console.unsyncedEntities(options)).resolves.not.toThrow();
});
});
});
});
81 changes: 64 additions & 17 deletions apps/server/src/modules/deletion-console/deletion-queue.console.ts
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sszafGCA Please remember to add proper imports and providers to the DeletionConsoleModule here to make it work after all these changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

done but lot of circular dependecies were present so added some services, repos to providers

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding Services as Providers directly is NOT the solution to circular dependencies. It does not solve them, it only makes them more narrow and harder to detect. They seem to be solved, but they are not, they will come back to bite us later.
It is absolutely forbidden to provide things that are already provided by another module.

Why do these circular dependencies appear? there should be no other module with any dependency on deletion console right?

Possibly they are caused by other circular dependencies that are already in the system, but hidden because other people tried to trick them by hiding them in explicit imports. Thats exactly the reason why you cant provide the UserService in your module, it will cause an dependency cycle in some harmless change later down the line in a different PR that has nothing to do with this one.

We need to figure out where the circular dependencies come from, what modules actually form the circle (modules, not files), and then either untangling or avoid using them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Metauriel I think you referred to the DeletionConsoleModule here right? If so, please take a look at this comment: #4868 (comment) .

Original file line number Diff line number Diff line change
@@ -1,9 +1,29 @@
/* eslint-disable @typescript-eslint/no-unsafe-assignment */
import { Console, Command } from 'nestjs-console';
import { Console, Command, CommandOption } from 'nestjs-console';
import { ConsoleWriterService } from '@infra/console';
import { PushDeletionRequestsOptions } from './interface';
import { PushDeletionRequestsOptions, UnsyncedEntitiesOptions } from './interface';
import { BatchDeletionUc } from './uc';

const sharedCommandOptions: CommandOption[] = [
{
flags: '-trd, --targetRefDomain <value>',
description: 'Name of the target ref domain.',
required: false,
defaultValue: 'user',
},
{
flags: '-dim, --deleteInMinutes <value>',
description: 'Number of minutes after which the data deletion process should begin.',
required: false,
defaultValue: 43200, // 43200 minutes = 30 days
},
{
flags: '-cdm, --callsDelayMs <value>',
description: 'Delay between all the performed client calls, in milliseconds.',
required: false,
},
];

@Console({ command: 'queue', description: 'Console providing an access to the deletion queue.' })
export class DeletionQueueConsole {
constructor(private consoleWriter: ConsoleWriterService, private batchDeletionUc: BatchDeletionUc) {}
Expand All @@ -17,21 +37,7 @@ export class DeletionQueueConsole {
description: 'Path of the file containing all the references to the data that should be deleted.',
required: true,
},
{
flags: '-trd, --targetRefDomain <value>',
description: 'Name of the target ref domain.',
required: false,
},
{
flags: '-dim, --deleteInMinutes <value>',
description: 'Number of minutes after which the data deletion process should begin.',
required: false,
},
{
flags: '-cdm, --callsDelayMs <value>',
description: 'Delay between all the performed client calls, in milliseconds.',
required: false,
},
...sharedCommandOptions,
],
})
async pushDeletionRequests(options: PushDeletionRequestsOptions): Promise<void> {
Expand All @@ -44,4 +50,45 @@ export class DeletionQueueConsole {

this.consoleWriter.info(JSON.stringify(summary));
}

@Command({
command: 'unsynced',
description: 'Finds unsynchronized users and queue them for deletion.',
options: [
{
flags: '-si, --systemId <value>',
description: 'ID of a synchronized system.',
required: true,
},
{
flags: '-ufm, --unsyncedForMinutes <value>',
description:
'Number of minutes that must have passed before entity can be considered unsynchronized. Minimum value: 60.',
required: false,
defaultValue: 10080, // 10080 minutes = 7 days
},
...sharedCommandOptions,
],
})
async unsyncedEntities(options: UnsyncedEntitiesOptions): Promise<void> {
if (options.unsyncedForMinutes < 60) {
throw new Error(`invalid "unsyncedForMinutes" option value - minimum value is 60`);
}

this.consoleWriter.info(
JSON.stringify({ message: 'starting queueing unsynchronized entities for deletion', options })
);

const summary = await this.batchDeletionUc.deleteUnsynchronizedRefs(
options.systemId,
options.unsyncedForMinutes,
options.targetRefDomain,
options.deleteInMinutes,
options.callsDelayMs
);

this.consoleWriter.info(
JSON.stringify({ message: 'successfully finished queueing unsynchronized entities for deletion', summary })
);
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
export * from './push-delete-requests-options.interface';
export * from './trigger-deletion-execution-options.interface';
export * from './unsynced-entities-options.interface';
export * from './deletion-execution-trigger-status.enum';
export * from './deletion-execution-trigger-result';
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import { EntityId } from '@shared/domain/types';

export interface UnsyncedEntitiesOptions {
systemId: EntityId;
unsyncedForMinutes: number;
targetRefDomain?: string;
deleteInMinutes?: number;
callsDelayMs?: number;
}
Loading
Loading