feat(config): configurable bootstrap policies file #8812

sgomezvillamor · 2023-09-08T12:59:22Z

Checklist

The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
Links to related issues (if applicable)
Tests for the changes have been added/updated (if applicable)
Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

sgomezvillamor · 2023-09-08T14:36:46Z

hi @david-leifker! just noted you self-assigned this PR. Some questions before opening for the review:

Do you think some tests should be included for this?
Tested locally with defaut values as well as with env var BOOTSTRAP_POLICIES_FILE=file:///test/my-policies.json, all fine! Just not sure how to cover this second scenario in a test 🤔

Also, should this be documented somewhere?

david-leifker · 2023-09-09T00:05:11Z

I self-assigned because I think that the default policy is loaded once and then never loaded again from the jar. If I recall correctly the state of whether the bootstrap step has run is stored in the SQL database. Overriding this file after the very first run isn't going to work as expected. I wanted to validate that it will behave like that and to ask whether that is your intent (to override this policy once for a new instance)?

sgomezvillamor · 2023-09-11T10:06:55Z

default policy is loaded once and then never loaded again from the jar.

datahub/metadata-service/factories/src/main/java/com/linkedin/metadata/boot/steps/IngestPoliciesStep.java

Lines 80 to 105 in 6c55eb8

    
           for (final JsonNode policyObj : policiesObj) { 
        
             final Urn urn = Urn.createFromString(policyObj.get("urn").asText()); 
        
             // If the info is not there, it means that the policy was there before, but must now be removed 
        
             if (!policyObj.has("info")) { 
        
               _entityService.deleteUrn(urn); 
        
               continue; 
        
             } 
        
             final DataHubPolicyInfo info = 
        
                 RecordUtils.toRecordTemplate(DataHubPolicyInfo.class, policyObj.get("info").toString()); 
        
             if (!info.isEditable()) { 
        
               // If the Policy is not editable, always re-ingest. 
        
               log.info(String.format("Ingesting default policy with urn %s", urn)); 
        
               ingestPolicy(urn, info); 
        
             } else { 
        
               // If the Policy is editable (ie. an example policy), only ingest on a clean boot up. 
        
               if (!hasPolicy(urn)) { 
        
                 log.info(String.format("Ingesting default policy with urn %s", urn)); 
        
                 ingestPolicy(urn, info); 
        
               } else { 
        
                 log.info(String.format("Skipping ingestion of editable policy with urn %s", urn)); 
        
               } 
        
             } 
        
           }

For every policy in the bootstrap file, the following criteria determines whether policy is loaded again or not:

if is not editable --> reload
else:
- if policy not exists in backend --> reload
- else --> skip

I recall correctly the state of whether the bootstrap step has run is stored in the SQL database.

Haven't found evidence of this. From what I see in the code and logs, IngestionPoliciesStep is processed in every boot and the criteria above is followed to determine reload or not.

Overriding this file after the very first run isn't going to work as expected.

I think it will. If we make the IngestionPoliciesStep to process a different file, as soon as we keep stable ids, the criteria above will still be valid.

The goal is to use a different file from the one in the jar while keeping all other functionality. There are some policies there we want to disable (even removing) while keeping all policies managed as code. So using a custom bootstrap policies file looks perfect match here.

Please, let me know your thoughts here

jjoyce0510 · 2023-09-20T15:01:19Z

metadata-service/configuration/src/main/resources/application.yml

@@ -276,6 +276,8 @@ bootstrap:
    enabled: ${UPGRADE_DEFAULT_BROWSE_PATHS_ENABLED:false} # enable to run the upgrade to migrate legacy default browse paths to new ones
  backfillBrowsePathsV2:
    enabled: ${BACKFILL_BROWSE_PATHS_V2:false} # Enables running the backfill of browsePathsV2 upgrade step. There are concerns about the load of this step so hiding it behind a flag. Deprecating in favor of running through SystemUpdate
+  policies:
+    file: ${BOOTSTRAP_POLICIES_FILE:classpath:boot/policies.json}


we've validated that this still works, right? :)

yes, I tested with default values (so loading from classpath) and with ENV BOOTSTRAP_POLICIES_FILE=file:///datahub/datahub-gms/resources/custom-policies.json

It works fine in both cases

jjoyce0510 · 2023-09-20T15:01:50Z

...ata-service/factories/src/main/java/com/linkedin/metadata/boot/steps/IngestPoliciesStep.java

@@ -66,10 +70,10 @@ public void execute() throws IOException, URISyntaxException {
        .maxStringLength(maxSize).build());

    // 0. Execute preflight check to see whether we need to ingest policies
-    log.info("Ingesting default access policies...");
+    log.info("Ingesting default access policies from: {}...", _policiesResource);


does this correctly print the path of the resource?

or the contents?

The path

This is the output with default values

2023-09-22 10:05:22 2023-09-22 08:05:22,700 [main] INFO c.l.m.boot.steps.IngestPoliciesStep:73 - Ingesting default access policies from: class path resource [boot/policies.json]... 2023-09-22 10:05:22 2023-09-22 08:05:22,710 [main] INFO c.l.m.boot.steps.IngestPoliciesStep:98 - Ingesting default policy with urn urn:li:dataHubPolicy:0 2023-09-22 10:05:22 2023-09-22 08:05:22,725 [main] INFO c.l.m.entity.EntityServiceImpl:1835 - Ingesting aspect with name dataHubPolicyKey, urn urn:li:dataHubPolicy:0 2023-09-22 10:05:22 2023-09-22 08:05:22,727 [main] INFO c.l.m.entity.EntityServiceImpl:1835 - Ingesting aspect with name dataHubPolicyInfo, urn urn:li:dataHubPolicy:0 2023-09-22 10:05:22 2023-09-22 08:05:22,746 [main] INFO c.l.m.boot.steps.IngestPoliciesStep:98 - Ingesting default policy with urn urn:li:dataHubPolicy:1 2023-09-22 10:05:22 2023-09-22 08:05:22,762 [main] INFO c.l.m.entity.EntityServiceImpl:1835 - Ingesting aspect with name dataHubPolicyKey, urn urn:li:dataHubPolicy:1 2023-09-22 10:05:22 2023-09-22 08:05:22,762 [main] INFO c.l.m.entity.EntityServiceImpl:1835 - Ingesting aspect with name dataHubPolicyInfo, urn urn:li:dataHubPolicy:1 2023-09-22 10:05:22 2023-09-22 08:05:22,810 [main] INFO c.l.m.boot.steps.IngestPoliciesStep:106 - Skipping ingestion of editable policy with urn urn:li:dataHubPolicy:7 2023-09-22 10:05:22 2023-09-22 08:05:22,831 [main] INFO c.l.m.boot.steps.IngestPoliciesStep:106 - Skipping ingestion of editable policy with urn urn:li:dataHubPolicy:view-entity-page-all 2023-09-22 10:05:22 2023-09-22 08:05:22,841 [main] INFO c.l.m.boot.steps.IngestPoliciesStep:106 - Skipping ingestion of editable policy with urn urn:li:dataHubPolicy:view-dataset-sensitive 2023-09-22 10:05:22 2023-09-22 08:05:22,847 [main] INFO c.l.m.boot.steps.IngestPoliciesStep:98 - Ingesting default policy with urn urn:li:dataHubPolicy:admin-platform-policy 2023-09-22 10:05:22 2023-09-22 08:05:22,863 [main] INFO c.l.m.entity.EntityServiceImpl:1835 - Ingesting aspect with name dataHubPolicyKey, urn urn:li:dataHubPolicy:admin-platform-policy ...

And this is the output with ENV BOOTSTRAP_POLICIES_FILE=file:///datahub/datahub-gms/resources/custom-policies.json

2023-09-22 10:45:27 2023-09-22 08:45:27,027 [main] INFO c.l.m.boot.steps.IngestPoliciesStep:73 - Ingesting default access policies from: URL [file:/datahub/datahub-gms/resources/custom-policies.json]... 2023-09-22 10:45:27 2023-09-22 08:45:27,032 [main] INFO c.l.m.boot.steps.IngestPoliciesStep:98 - Ingesting default policy with urn urn:li:dataHubPolicy:XXX 2023-09-22 10:45:27 2023-09-22 08:45:27,132 [main] WARN org.apache.velocity.deprecation:62 - configuration key 'resource.loader' has been deprecated in favor of 'resource.loaders' 2023-09-22 10:45:27 2023-09-22 08:45:27,133 [main] WARN org.apache.velocity.deprecation:62 - configuration key 'class.resource.loader.class' has been deprecated in favor of 'resource.loader.class.class' 2023-09-22 10:45:27 2023-09-22 08:45:27,133 [main] WARN org.apache.velocity.deprecation:62 - configuration key 'file.resource.loader.class' has been deprecated in favor of 'resource.loader.file.class' 2023-09-22 10:45:27 2023-09-22 08:45:27,133 [main] WARN org.apache.velocity.deprecation:62 - configuration key 'file.resource.loader.path' has been deprecated in favor of 'resource.loader.file.path' 2023-09-22 10:45:27 2023-09-22 08:45:27,133 [main] WARN org.apache.velocity.deprecation:62 - configuration key 'runtime.references.strict' has been deprecated in favor of 'runtime.strict_mode.enable' 2023-09-22 10:45:27 2023-09-22 08:45:27,627 [main] INFO c.l.m.boot.steps.IngestPoliciesStep:115 - Successfully ingested default access policies.

jjoyce0510 · 2023-09-20T15:02:45Z

...ata-service/factories/src/main/java/com/linkedin/metadata/boot/steps/IngestPoliciesStep.java


    // 1. Read from the file into JSON.
-    final JsonNode policiesObj = mapper.readTree(new ClassPathResource("./boot/policies.json").getFile());
+    final JsonNode policiesObj = mapper.readTree(_policiesResource.getFile());


definitely need to ensure that we've confirmed that this works still

It does, see comment https://github.com/datahub-project/datahub/pull/8812/files#r1334091670

jjoyce0510 · 2023-09-20T15:06:33Z

Overall I think this looks good. I have not used the dynamic Resource injection via spring but looks like all should work as expected!

If you provide a file, do you need to prefix the file path similar to how is done for classpath?

e.g. file:path/to/file?

If yes, I'd recommend adding a comment in application.yml to exemplify the correct format!

sgomezvillamor · 2023-09-22T08:56:37Z

Overall I think this looks good. I have not used the dynamic Resource injection via spring but looks like all should work as expected!

I also learnt that in this contrib 😅 https://www.baeldung.com/spring-load-resource-as-string#using-spel

Overall I think this looks good. I have not used the dynamic Resource injection via spring but looks like all should work as expected!

If you provide a file, do you need to prefix the file path similar to how is done for classpath?

e.g. file:path/to/file?

If yes, I'd recommend adding a comment in application.yml to exemplify the correct format!

With file:... definitely works. I tested with ENV BOOTSTRAP_POLICIES_FILE=file:///datahub/datahub-gms/resources/custom-policies.json in my local deployment. Not sure if there are other variants that could work also.
I'm adding the comment/example in application.yml 👍 9cfae00

vercel · 2023-10-03T11:59:37Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
docs-website	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 3, 2023 0:33am

jjoyce0510 · 2023-10-11T00:05:52Z

LGTM- Once we are green we merge!

feat(config): configurable bootstrap policies file

e6596cd

github-actions bot added the devops PR or Issue related to DataHub backend & deployment label Sep 8, 2023

vercel bot deployed to Preview September 8, 2023 13:18 View deployment

david-leifker self-assigned this Sep 8, 2023

fix checkstyle

822f395

vercel bot deployed to Preview September 8, 2023 18:31 View deployment

sgomezvillamor marked this pull request as ready for review September 12, 2023 07:08

Merge branch 'master' into feat-configurable-bootstrap-policies

4e2bc5c

vercel bot deployed to Preview September 12, 2023 07:22 View deployment

jjoyce0510 reviewed Sep 20, 2023

View reviewed changes

sgomezvillamor added 2 commits September 22, 2023 10:58

example as doc

9cfae00

Merge branch 'master' into feat-configurable-bootstrap-policies

0c8b575

sgomezvillamor requested a review from jjoyce0510 September 22, 2023 09:25

vercel bot deployed to Preview September 22, 2023 09:37 View deployment

Merge branch 'master' into feat-configurable-bootstrap-policies

11058d0

vercel bot deployed to Preview September 26, 2023 10:01 View deployment

Merge branch 'master' into feat-configurable-bootstrap-policies

d2524ff

vercel bot deployed to Preview October 2, 2023 07:57 View deployment

Merge branch 'master' into feat-configurable-bootstrap-policies

af3ca90

vercel bot deployed to Preview October 3, 2023 12:33 View deployment

Merge branch 'master' into feat-configurable-bootstrap-policies

4ece833

vercel bot deployed to Preview October 4, 2023 15:37 View deployment

Merge branch 'master' into feat-configurable-bootstrap-policies

2347a85

jjoyce0510 approved these changes Oct 11, 2023

View reviewed changes

jjoyce0510 enabled auto-merge (squash) October 11, 2023 00:10

vercel bot deployed to Preview October 11, 2023 00:37 View deployment

jjoyce0510 merged commit dfcea24 into datahub-project:master Oct 11, 2023

skrydal mentioned this pull request Nov 27, 2023

feat(config): Configurable bootstrap of ownership types #9308

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(config): configurable bootstrap policies file #8812

feat(config): configurable bootstrap policies file #8812

sgomezvillamor commented Sep 8, 2023

sgomezvillamor commented Sep 8, 2023

david-leifker commented Sep 9, 2023 •

edited

Loading

sgomezvillamor commented Sep 11, 2023

jjoyce0510 Sep 20, 2023

sgomezvillamor Sep 22, 2023

jjoyce0510 Sep 20, 2023

sgomezvillamor Sep 22, 2023

jjoyce0510 Sep 20, 2023

sgomezvillamor Sep 22, 2023

jjoyce0510 commented Sep 20, 2023

sgomezvillamor commented Sep 22, 2023 •

edited

Loading

vercel bot commented Oct 3, 2023 •

edited

Loading

jjoyce0510 commented Oct 11, 2023

feat(config): configurable bootstrap policies file #8812

feat(config): configurable bootstrap policies file #8812

Conversation

sgomezvillamor commented Sep 8, 2023

Checklist

sgomezvillamor commented Sep 8, 2023

david-leifker commented Sep 9, 2023 • edited Loading

sgomezvillamor commented Sep 11, 2023

jjoyce0510 Sep 20, 2023

Choose a reason for hiding this comment

sgomezvillamor Sep 22, 2023

Choose a reason for hiding this comment

jjoyce0510 Sep 20, 2023

Choose a reason for hiding this comment

sgomezvillamor Sep 22, 2023

Choose a reason for hiding this comment

jjoyce0510 Sep 20, 2023

Choose a reason for hiding this comment

sgomezvillamor Sep 22, 2023

Choose a reason for hiding this comment

jjoyce0510 commented Sep 20, 2023

sgomezvillamor commented Sep 22, 2023 • edited Loading

vercel bot commented Oct 3, 2023 • edited Loading

jjoyce0510 commented Oct 11, 2023

david-leifker commented Sep 9, 2023 •

edited

Loading

sgomezvillamor commented Sep 22, 2023 •

edited

Loading

vercel bot commented Oct 3, 2023 •

edited

Loading