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.

Sign up for GitHub

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

PIN-5931: added SH feature flag and whitelist #1369

Merged
merged 12 commits into from
Feb 3, 2025
+139 −5
Merged

PIN-5931: added SH feature flag and whitelist #1369

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
cd3f1c9
chore: added SH feature flag
borgesis95 Jan 14, 2025
dff7352
test: update test
borgesis95 Jan 14, 2025
cdf2742
fix: test updateEservice test
borgesis95 Jan 14, 2025
426c72c
chore: put sh_whitelist as optional field
borgesis95 Jan 14, 2025
b097b53
fix: change name of feature flag in FEATURE_FLAG_SIGNALHUB_WHITELIST
borgesis95 Jan 15, 2025
946d8ab
fix: update config.ts
borgesis95 Jan 20, 2025
c60a5c2
test: fix test
borgesis95 Jan 20, 2025
ac0830e
Merge branch 'main' into PIN-5931_added_whitelist_sh
borgesis95 Jan 29, 2025
4ec2d1c
fix: update typo
borgesis95 Jan 29, 2025
f19371a
fix: replace coerce with transform (FEATURE_FLAG_SIGNALHUB_WHITELIST)
borgesis95 Jan 29, 2025
b0072f5
chore: moved featureFlag on featureFlagsConfig file
borgesis95 Jan 29, 2025
a16b07f
chore: moved SIGNALHUB_WHITELIST on featureFlagsConfig
borgesis95 Jan 29, 2025
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
4 changes: 3 additions & 1 deletion packages/catalog-process/.env
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ READMODEL_DB_NAME=readmodel
READMODEL_DB_USERNAME=root
READMODEL_DB_PASSWORD=example
READMODEL_DB_PORT=27017

WELL_KNOWN_URLS="http://127.0.0.1:4500/jwks.json"
ACCEPTED_AUDIENCES="dev.interop.pagopa.it/ui,refactor.dev.interop.pagopa.it/ui,refactor.dev.interop.pagopa.it/internal,dev.interop.pagopa.it/m2m,refactor.dev.interop.pagopa.it/m2m"

Expand All @@ -27,3 +26,6 @@ S3_SERVER_PORT=9000
ESERVICE_DOCUMENTS_PATH="interop-eservice-documents"

PRODUCER_ALLOWED_ORIGINS="IPA"

FEATURE_FLAG_SIGNALHUB_WHITELIST=true
SIGNALHUB_WHITELIST=cec6866f-1561-4c83-a230-c91b98c2a890
10 changes: 10 additions & 0 deletions packages/catalog-process/src/config/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,18 @@ const CatalogProcessConfig = CommonHTTPServiceConfig.and(ReadModelDbConfig)
.object({
ESERVICE_DOCUMENTS_PATH: z.string(),
PRODUCER_ALLOWED_ORIGINS: z.string(),
FEATURE_FLAG_SIGNALHUB_WHITELIST: z
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to create a new config in common for feature flags, so every service can use the same flag.

Plus, feature flags could be optional and default to false. In my mind the idea is to enable only the ones needed, when needed.
No strong opinions, let me know your thoughts

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm afraid to make the feature flag an optional value (with a default false) but I don't have an example of a situation where this behavior could cause problems.
I had to think about it

.enum(["true", "false"])
.transform((value) => value === "true"),
borgesis95 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
FEATURE_FLAG_SIGNALHUB_WHITELIST: z
.enum(["true", "false"])
.transform((value) => value === "true"),
FEATURE_FLAG_SIGNALHUB_WHITELIST: z
.enum(["true", "false"])
.default("false")
.transform((value) => value === "true"),

As suggested by @galales we should set a default in case the env variable is not set and I also agree to define dedicated featureFlags configs, you could do it like this maybe by creating a new file featureFlags that we will use to define all the flags' configs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved FEATURE_FLAG_SIGNALHUB_WHITELIST on dedicated file, however I let SIGNALHUB_WHITELIST var on catalog-process config file. I think that whitelist is closely related to catalog-process. Let me know what you think about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Moved FEATURE_FLAG_SIGNALHUB_WHITELIST on dedicated file, however I let SIGNALHUB_WHITELIST var on catalog-process config file. I think that whitelist is closely related to catalog-process. Let me know what you think about it.

I think that it should be included inside the dedicated config since that variable is strictly related to FEATURE_FLAG_SIGNALHUB_WHITELIST, so these should be together IMHO.

Even if, for now, this config is used only in the catalog-process having all the featureFlags configs in commons would be easier for future development

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you've convinced me! moved var into feature flag config file 👍

SIGNALHUB_WHITELIST: z
.string()
.uuid()
borgesis95 marked this conversation as resolved.
Show resolved Hide resolved
.transform((value) => value.split(","))
.optional(),
})
.transform((c) => ({
featureFlagSignalhubWhitelist: c.FEATURE_FLAG_SIGNALHUB_WHITELIST,
signalhubWhitelist: c.SIGNALHUB_WHITELIST,
eserviceDocumentsPath: c.ESERVICE_DOCUMENTS_PATH,
producerAllowedOrigins: c.PRODUCER_ALLOWED_ORIGINS.split(",")
.map((origin) => origin.trim())
Expand Down
23 changes: 21 additions & 2 deletions packages/catalog-process/src/services/catalogService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,15 @@ async function parseAndCheckAttributes(
};
}

function isOrganizationIdIncludesOnWhitelist(
borgesis95 marked this conversation as resolved.
Show resolved Hide resolved
organizationId: TenantId,
isSignalubEnabled: boolean | undefined
): boolean | undefined {
borgesis95 marked this conversation as resolved.
Show resolved Hide resolved
return config.signalhubWhitelist?.includes(organizationId)
? isSignalubEnabled
: false;
AsterITA marked this conversation as resolved.
Show resolved Hide resolved
}

// eslint-disable-next-line @typescript-eslint/explicit-function-return-type
export function catalogServiceBuilder(
dbInstance: DB,
Expand Down Expand Up @@ -443,7 +452,12 @@ export function catalogServiceBuilder(
descriptors: [],
createdAt: creationDate,
riskAnalysis: [],
isSignalHubEnabled: seed.isSignalHubEnabled,
isSignalHubEnabled: config.featureFlagSignalhubWhitelist
? isOrganizationIdIncludesOnWhitelist(
authData.organizationId,
seed.isSignalHubEnabled
)
: seed.isSignalHubEnabled,
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the fallback be false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if FEATURE_FLAG_SIGNALHUB_WHITELIST = false it means that hasn't to check WHITELIST so that value will be what has been set from UI (seed.isSignalHubEnabled). Maybe could result a bit tricky due to name of isSignalhubEnabled variable that hasn't not to do with FEATURE_FLAG_SIGNALHUB

};

const eserviceCreationEvent = toCreateEventEServiceAdded(
Expand Down Expand Up @@ -564,7 +578,12 @@ export function catalogServiceBuilder(
serverUrls: [],
}))
: eservice.data.descriptors,
isSignalHubEnabled: eserviceSeed.isSignalHubEnabled,
isSignalHubEnabled: config.featureFlagSignalhubWhitelist
? isOrganizationIdIncludesOnWhitelist(
authData.organizationId,
eservice.data.isSignalHubEnabled
)
: eservice.data.isSignalHubEnabled,
};

const event = toCreateEventEServiceUpdated(
Expand Down
79 changes: 79 additions & 0 deletions packages/catalog-process/test/createEService.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
inconsistentDailyCalls,
originNotCompliant,
} from "../src/model/domain/errors.js";
import { config } from "../src/config/config.js";
import {
addOneEService,
buildDescriptorSeedForEserviceCreation,
Expand All @@ -40,6 +41,9 @@ describe("create eservice", () => {
vi.useRealTimers();
});
it("should write on event-store for the creation of an eservice", async () => {
config.featureFlagSignalhubWhitelist = true;
config.signalhubWhitelist = [mockEService.producerId];

const isSignalHubEnabled = randomArrayItem([false, true, undefined]);
const eservice = await catalogService.createEService(
{
Expand Down Expand Up @@ -119,6 +123,81 @@ describe("create eservice", () => {
toEServiceV2(expectedEserviceWithDescriptor)
);
});
it("should assign value inherit from request to isSignalhubEnabled field if signalhub whitelist feature flag is not enabled", async () => {
config.featureFlagSignalhubWhitelist = false;
const isSignalHubEnabled = randomArrayItem([false, true, undefined]);
const eservice = await catalogService.createEService(
{
name: mockEService.name,
description: mockEService.description,
technology: "REST",
mode: "DELIVER",
descriptor: buildDescriptorSeedForEserviceCreation(mockDescriptor),
isSignalHubEnabled,
},
{
authData: getMockAuthData(mockEService.producerId),
correlationId: generateId(),
serviceName: "",
logger: genericLogger,
}
);

expect(eservice).toBeDefined();
expect(eservice.isSignalHubEnabled).toBe(isSignalHubEnabled);
});

it("should assign false to isSignalhubEnabled field if signalhub whitelist feature flag is enabled but the organization is not in whitelist", async () => {
config.featureFlagSignalhubWhitelist = true;
config.signalhubWhitelist = [generateId()];
const isSignalHubEnabled = true;

const eservice = await catalogService.createEService(
{
name: mockEService.name,
description: mockEService.description,
technology: "REST",
mode: "DELIVER",
descriptor: buildDescriptorSeedForEserviceCreation(mockDescriptor),
isSignalHubEnabled,
},
{
authData: getMockAuthData(mockEService.producerId),
correlationId: generateId(),
serviceName: "",
logger: genericLogger,
}
);

expect(eservice).toBeDefined();
expect(eservice.isSignalHubEnabled).toBe(false);
});

it("should assign value inherit from request to isSignalhubEnabled field if signalhub whitelist feature flag is enabled and the organization is in whitelist", async () => {
config.featureFlagSignalhubWhitelist = true;
config.signalhubWhitelist = [mockEService.producerId];
const isSignalHubEnabled = randomArrayItem([false, true, undefined]);

const eservice = await catalogService.createEService(
{
name: mockEService.name,
description: mockEService.description,
technology: "REST",
mode: "DELIVER",
descriptor: buildDescriptorSeedForEserviceCreation(mockDescriptor),
isSignalHubEnabled,
},
{
authData: getMockAuthData(mockEService.producerId),
correlationId: generateId(),
serviceName: "",
logger: genericLogger,
}
);

expect(eservice).toBeDefined();
expect(eservice.isSignalHubEnabled).toBe(isSignalHubEnabled);
});

it("should throw eServiceDuplicate if an eservice with the same name already exists", async () => {
await addOneEService(mockEService);
Expand Down
13 changes: 12 additions & 1 deletion packages/catalog-process/test/updateEservice.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ describe("update eService", () => {
const mockDocument = getMockDocument();
it("should write on event-store for the update of an eService (no technology change)", async () => {
vi.spyOn(fileManager, "delete");

config.featureFlagSignalhubWhitelist = true;
config.signalhubWhitelist = [mockEService.producerId];
const isSignalHubEnabled = randomArrayItem([false, true, undefined]);
const descriptor: Descriptor = {
...getMockDescriptor(),
Expand Down Expand Up @@ -92,6 +93,9 @@ describe("update eService", () => {
it("should write on event-store for the update of an eService (technology change: interface has to be deleted)", async () => {
vi.spyOn(fileManager, "delete");

config.featureFlagSignalhubWhitelist = true;
config.signalhubWhitelist = [mockEService.producerId];

const interfaceDocument = {
...mockDocument,
name: `${mockDocument.name}`,
Expand Down Expand Up @@ -217,6 +221,10 @@ describe("update eService", () => {
});
it("should write on event-store for the update of an eService (update description only)", async () => {
const updatedDescription = "eservice new description";

config.featureFlagSignalhubWhitelist = true;
config.signalhubWhitelist = [mockEService.producerId];

await addOneEService(mockEService);
const returnedEService = await catalogService.updateEService(
mockEService.id,
Expand Down Expand Up @@ -256,6 +264,7 @@ describe("update eService", () => {
});

it("should write on event-store for the update of an eService (update mode to DELIVER so risk analysis has to be deleted)", async () => {
config.featureFlagSignalhubWhitelist = true;
const riskAnalysis = getMockValidRiskAnalysis("PA");
const eservice: EService = {
...mockEService,
Expand All @@ -265,6 +274,8 @@ describe("update eService", () => {
};
await addOneEService(eservice);

config.signalhubWhitelist = [eservice.producerId];

const returnedEService = await catalogService.updateEService(
eservice.id,
{
Expand Down
Loading