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

[fix](broker-load) Correction of kerberos authentication time determination rule #11793

Merged
merged 5 commits into from
Sep 18, 2022

Conversation

whutpencil
Copy link
Contributor

Proposed changes

Issue Number: #11792

Problem summary

Every time a new broker load comes in, Doris will update the start time of Kerberos authentication, and this logic is wrong. Because the authentication duration of Kerberos is calculated from the moment when the ticket is obtained.

Describe your changes.

Delete the code that updates the lastAccessTime to get the correct judgment.

Checklist(Required)

  1. Does it affect the original behavior:
    • [ ✅ ] Yes
    • No
    • I don't know
  2. Has unit tests been added:
    • Yes
    • [ ✅ ] No
    • No Need
  3. Has document been added or modified:
    • Yes
    • [ ✅ ] No
    • No Need
  4. Does it need to update dependencies:
    • Yes
    • [ ✅ ] No
  5. Are there any changes that cannot be rolled back:
    • Yes (If Yes, please explain WHY)
    • [ ✅ ] No

Further comments

If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...

@@ -394,7 +394,6 @@ public BrokerFileSystem getS3AFileSystem(String path, Map<String, String> proper
String disableCache = properties.getOrDefault(FS_S3A_IMPL_DISABLE_CACHE, "true");
String s3aUgi = accessKey + "," + secretKey;
FileSystemIdentity fileSystemIdentity = new FileSystemIdentity(host, s3aUgi);
cachedFileSystem.putIfAbsent(fileSystemIdentity, new BrokerFileSystem(fileSystemIdentity));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@morningman Because this step has been done in the updateCachedFileSystem method in the next line of code, this line of code is redundant.

if (brokerFileSystem.isExpiredByCreateTime(BrokerConfig.client_expire_seconds)) {
logger.info("file system " + brokerFileSystem + " is expired, update it.");
try {
Configuration conf = new HdfsConfiguration();
Copy link
Contributor

Choose a reason for hiding this comment

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

Only do this for kerberos

Copy link
Contributor

@morningman morningman left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions github-actions bot added the approved Indicates a PR has been approved by one committer. label Sep 15, 2022
@github-actions
Copy link
Contributor

PR approved by at least one committer and no changes requested.

@github-actions
Copy link
Contributor

PR approved by anyone and no changes requested.

@morningman morningman merged commit 00dda79 into apache:master Sep 18, 2022
FreeOnePlus pushed a commit to FreeOnePlus/doris that referenced this pull request Oct 8, 2022
…nation rule (apache#11793)

Every time a new broker load comes in, Doris will update the start time of Kerberos authentication,
but this logic is wrong.
Because the authentication duration of Kerberos is calculated from the moment when the ticket is obtained.

This PR change the logic:
1. If it is kerberos, check fs expiration by create time.
2.Otherwise, check fs expiration by access time
FreeOnePlus pushed a commit to FreeOnePlus/doris that referenced this pull request Oct 8, 2022
…nation rule (apache#11793)

Every time a new broker load comes in, Doris will update the start time of Kerberos authentication,
but this logic is wrong.
Because the authentication duration of Kerberos is calculated from the moment when the ticket is obtained.

This PR change the logic:
1. If it is kerberos, check fs expiration by create time.
2.Otherwise, check fs expiration by access time
FreeOnePlus pushed a commit to FreeOnePlus/doris that referenced this pull request Oct 8, 2022
…nation rule (apache#11793)

Every time a new broker load comes in, Doris will update the start time of Kerberos authentication,
but this logic is wrong.
Because the authentication duration of Kerberos is calculated from the moment when the ticket is obtained.

This PR change the logic:
1. If it is kerberos, check fs expiration by create time.
2.Otherwise, check fs expiration by access time
FreeOnePlus pushed a commit to FreeOnePlus/doris that referenced this pull request Oct 8, 2022
…nation rule (apache#11793)

Every time a new broker load comes in, Doris will update the start time of Kerberos authentication,
but this logic is wrong.
Because the authentication duration of Kerberos is calculated from the moment when the ticket is obtained.

This PR change the logic:
1. If it is kerberos, check fs expiration by create time.
2.Otherwise, check fs expiration by access time
FreeOnePlus pushed a commit to FreeOnePlus/doris that referenced this pull request Oct 8, 2022
…nation rule (apache#11793)

Every time a new broker load comes in, Doris will update the start time of Kerberos authentication,
but this logic is wrong.
Because the authentication duration of Kerberos is calculated from the moment when the ticket is obtained.

This PR change the logic:
1. If it is kerberos, check fs expiration by create time.
2.Otherwise, check fs expiration by access time
FreeOnePlus pushed a commit to FreeOnePlus/doris that referenced this pull request Oct 8, 2022
…nation rule (apache#11793)

Every time a new broker load comes in, Doris will update the start time of Kerberos authentication,
but this logic is wrong.
Because the authentication duration of Kerberos is calculated from the moment when the ticket is obtained.

This PR change the logic:
1. If it is kerberos, check fs expiration by create time.
2.Otherwise, check fs expiration by access time
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by one committer. area/broker Issues or PRs related to broker kind/improvement reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants