Skip to content

HADOOP-18675: Fix CachedSASToken noisy log errors when SAS token has YYYY-MM-DD expiration #5508

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

Closed
wants to merge 3 commits into from
Closed

HADOOP-18675: Fix CachedSASToken noisy log errors when SAS token has YYYY-MM-DD expiration #5508

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c67ca8a
HADOOP-18675: Fix CachedSASToken noisy log errors when SAS token has …
redcape Mar 23, 2023
77444f6
remove unused imports
redcape Mar 23, 2023
ac12f4b
checkstyle
redcape Mar 23, 2023
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
27 changes: 25 additions & 2 deletions ...-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/utils/CachedSASToken.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,14 @@

import java.io.UnsupportedEncodingException;
import java.net.URLDecoder;
import java.time.LocalDateTime;
import java.time.OffsetDateTime;
import java.time.ZoneOffset;
import java.time.format.DateTimeFormatter;
import java.time.format.DateTimeFormatterBuilder;
import java.time.format.DateTimeParseException;
import java.time.temporal.ChronoField;
import java.time.temporal.TemporalAccessor;

import org.apache.hadoop.classification.VisibleForTesting;
import org.slf4j.Logger;
Expand All @@ -39,6 +43,13 @@
*/
public final class CachedSASToken {
public static final Logger LOG = LoggerFactory.getLogger(CachedSASToken.class);

private static final DateTimeFormatter ISO_DATE_MIDNIGHT = new DateTimeFormatterBuilder()
.append(DateTimeFormatter.ISO_DATE)
.parseDefaulting(ChronoField.HOUR_OF_DAY, 0)
.parseDefaulting(ChronoField.MINUTE_OF_HOUR, 0)
.toFormatter();

private final long minExpirationInSeconds;
private String sasToken;
private OffsetDateTime sasExpiry;
Expand Down Expand Up @@ -114,8 +125,20 @@ private static OffsetDateTime getExpiry(String token) {
OffsetDateTime seDate = OffsetDateTime.MIN;
try {
seDate = OffsetDateTime.parse(seValue, DateTimeFormatter.ISO_DATE_TIME);
} catch (DateTimeParseException ex) {
LOG.error("Error parsing se query parameter ({}) from SAS.", seValue, ex);
} catch (DateTimeParseException dateTimeException) {
try {
TemporalAccessor dt = ISO_DATE_MIDNIGHT.parseBest(seValue, OffsetDateTime::from, LocalDateTime::from);
if (dt instanceof OffsetDateTime) {
seDate = (OffsetDateTime) dt;
} else if (dt instanceof LocalDateTime) {
seDate = ((LocalDateTime) dt).atOffset(ZoneOffset.UTC);
} else {
throw dateTimeException;
}
} catch (DateTimeParseException dateOnlyException) {
Copy link
Contributor

@saxenapranav saxenapranav Mar 24, 2023

Choose a reason for hiding this comment

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

Can we have something like:

private static final DateTimeFormatter[] formatters;

private OffsetDateTime  getParsedDateTime(String dateTime) {
    for(DateTimeFormatter formatter : formatters) {
      try {
        TemporalAccessor temporalAccessor = formatter.parseBest(dateTime);
        if(temporalAccessor instanceof OffsetDateTime) {
          return (OffsetDateTime) temporalAccessor; 
        }
        if(temporalAccessor instanceof LocalDateTime) {
          return ((LocalDateTime) temporalAccessor).atOffset(ZoneOffset.UTC);
        }
      } catch (DateTimeParseException e) {
        
      }
    }
    return null;
  }

And from this method we will just call seDate = getParsedDateTime(seValue);

This way I feel we can prevent nested code.

// log original exception
LOG.error("Error parsing se query parameter ({}) from SAS as ISO_DATE_TIME or ISO_DATE.", seValue, dateTimeException);
Copy link
Contributor

@steveloughran steveloughran Mar 23, 2023

Choose a reason for hiding this comment

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

now, if there's a problem here it is still going to log a lot, isn't it?

might be best to use a LogExactlyOnce log here for the error line, and log at debug the rest of the time

}
}

String signedKeyExpiry = "ske=";
Expand Down
25 changes: 25 additions & 0 deletions ...ls/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/utils/TestCachedSASToken.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,31 @@ public void testUpdateAndGet() throws IOException {
Assert.assertTrue(token3 == cachedToken);
}

@Test
public void testValidExpirationParsing() {
Copy link
Contributor

@steveloughran steveloughran Mar 23, 2023

Choose a reason for hiding this comment

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

is there a test for an invalid date time parsing now?

Copy link
Author

@redcape redcape Mar 23, 2023

Choose a reason for hiding this comment

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

there's an existing one testing invalid se=abc in testUpdateAndGetWithInvalidToken. I'll add another method specifically for invalid expiration.

CachedSASToken cachedSasToken = new CachedSASToken();
String[] values = {
"2123-03-24T00:06:46Z", // sample timestamp from azure portal
"2124-03-30", // sample YYYY-MM-DD date format generated from az cli
"2125-03-30Z", // sample YYYY-MM-DD[offset] date format
};

for (String se : values) {
cachedSasToken.setForTesting(null, null);
String token = "se=" + se;

// set first time and ensure reference equality
cachedSasToken.update(token);
String cachedToken = cachedSasToken.get();
Assert.assertTrue(token == cachedToken);
Copy link
Contributor

@steveloughran steveloughran Mar 23, 2023

Choose a reason for hiding this comment

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

assertEquals(expected, actual) or AssertJ.asserThat(). thanks

Copy link
Author

@redcape redcape Mar 23, 2023

Choose a reason for hiding this comment

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

was following the existing tests which seem to want to assert reference equality a lot, I will change it for the new test.

Copy link
Contributor

@steveloughran steveloughran Mar 27, 2023

Choose a reason for hiding this comment

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

yeah, old code...just because it was done that way doesn't mean we should carry on


// update with same token and ensure reference equality
cachedSasToken.update(token);
cachedToken = cachedSasToken.get();
Assert.assertTrue(token == cachedToken);
}
}

@Test
public void testGetExpiration() throws IOException {
CachedSASToken cachedSasToken = new CachedSASToken();
Expand Down