Skip to content

Add more tests for column masking#10437

Closed
guyco33 wants to merge 3 commits intotrinodb:masterfrom
guyco33:issue_10370
Closed

Add more tests for column masking#10437
guyco33 wants to merge 3 commits intotrinodb:masterfrom
guyco33:issue_10370

Conversation

@guyco33
Copy link
Copy Markdown
Member

@guyco33 guyco33 commented Dec 30, 2021

Tests are failing due to #10370

Copy link
Copy Markdown
Contributor

@findinpath findinpath Jan 3, 2022

Choose a reason for hiding this comment

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

it would be worth for the readability of the test scenario to perform the query without any column masking

Copy link
Copy Markdown
Contributor

@findinpath findinpath Jan 3, 2022

Choose a reason for hiding this comment

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

I don't quite get the purpose of this masking.
The column comment has the value nstructions sleep furiously among .
Why do you check for password in this text?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I want to mask some values in comment column (in this case the password value) and also to use comment column as a reference for the decision about masking other columns (we called it "conditional masking")

In a real example there is a json payload column with some attributes that should be masked and a country attribute that used as a reference for masking other columns.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The column comment has the value nstructions sleep furiously among .

This value doesn't contain a password substring. So, I guess the masking is pointless in this case. Or am I missing something?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I found that issue happens in InlineProjections#inlineProjections
Maybe it relates on how column masks are added

Copy link
Copy Markdown
Member Author

@guyco33 guyco33 Jan 3, 2022

Choose a reason for hiding this comment

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

The column comment has the value nstructions sleep furiously among .

This value doesn't contain a password substring. So, I guess the masking is pointless in this case. Or am I missing something?

You are right. That's why I added the last test (L293:L337)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I had a look again on the test and I am sorry to say that I still don't quite understand why isn't there any masking in the result of the queries.
Shouldn't the test demonstrate that the * is masking the content of the returned columns on which the masking should apply?

Copy link
Copy Markdown
Member Author

@guyco33 guyco33 Jan 3, 2022

Choose a reason for hiding this comment

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

Shouldn't the test demonstrate that the * is masking the content of the returned columns on which the masking should apply?

You are right. I fixed it now. Take a look on the tests with orderkey = 39 (L293:L337)

Copy link
Copy Markdown
Member Author

@guyco33 guyco33 Jan 3, 2022

Choose a reason for hiding this comment

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

The column comment has the value nstructions sleep furiously among .

This value doesn't contain a password substring. So, I guess the masking is pointless in this case. Or am I missing something?

That's right. masking is pointless for this specific row, but I added it to demonstrate the issue when adding the masking on orderstatus while refering clerk (L291)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see now.

The purpose of the test is currently just to demonstrate that the masking doesn't work properly. Thank you for the patience in trying to explain the purpose of the tests.

@guyco33 guyco33 force-pushed the issue_10370 branch 2 times, most recently from e7f93e4 to 13b91b4 Compare January 3, 2022 16:54
Comment on lines 330 to 338
Copy link
Copy Markdown
Member Author

@guyco33 guyco33 Jan 3, 2022

Choose a reason for hiding this comment

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

Fails with Symbol clerk already has assignment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fails with Symbol clerk already has assignment

}

@Test
public void testMultipleMasksOnSameColumn()
Copy link
Copy Markdown
Member Author

@guyco33 guyco33 Jan 4, 2022

Choose a reason for hiding this comment

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

Removing this test, since AC doesn't allow to have more than one masking per column.

this.columnConstraints = Maps.uniqueIndex(requireNonNull(columns, "columns is null").orElse(ImmutableList.of()), ColumnConstraint::getName);

@findepi findepi requested a review from kokosing January 4, 2022 13:00
Copy link
Copy Markdown
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

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

The change is good. Please squash the commits and change the PR title and description as well as commit message to focus on a bug fix (not adding tests).

@guyco33
Copy link
Copy Markdown
Member Author

guyco33 commented Jan 9, 2022

@kokosing Although the fix solves the issue, it creates another issue that can be seem in the last commit

@guyco33
Copy link
Copy Markdown
Member Author

guyco33 commented May 10, 2022

Fixed by #12262

@guyco33 guyco33 closed this May 10, 2022
@guyco33 guyco33 deleted the issue_10370 branch May 10, 2022 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants