BT-711 Refresh SAS token for filesystem on expiry

[kraefrei]

No description provided.

[kraefrei]

Running into issues here. If we can in theory know that the filesystem auth has expired in this method, we could essentially guarantee that we return a valid filesystem by trying to refresh. There are however some errors that I am not sure how we should process (like invalid path).

If we can figure out how to get this method and as a result the below nioPath method below to be the encapsulation of this refreshing behavior, we don't have to reimplement all of the filesystem methods that try to interact with the filesystem, which I think is ideal.

[kraefrei]

So I think I figured out that using a method called checkAccess is how the AzureFileSystem provider approaches identifying if the path has the auth needed to proceed, so I used this to move the onus of the check to the findNioPath method. This meant that we can recover from the thrown exceptions when first building the path object, rather than when trying to access it, however this doesn't really address the use of exceptions for control flow which I am not sure is the best way to go.

[jgainerdewar]

This PR made me realize that we should rename this method. get makes it seem like a regular Java getter. Maybe something like generateAccessToken?

I also think we should change this method signature to return a Try[AzureSasCredential] rather than throwing from this method.

Apologies for the somewhat out-of-scope feedback, but I think the return type change will affect the refresh logic.

[kraefrei]

I made an attempt for this, but am struggling with a situation where I have nested try declarations that I am unsure how to flatten. It seems a little like one way or another this exception needs to feed into the top level try

[jgainerdewar]

Rather than !storageAccount.isEmpty you can use storageAccount.isDefined.

[jgainerdewar]

Can make this even better with storageAccount.exists(parseStorageAccount(uri).contains(_))

[jgainerdewar]

While I'm in here taking credit for IntelliJ's recommendations... can replace .filter(...).headOption with .find(...).

[kraefrei]

Oi, it seems Metals is letting me down at bit, I'll try working with intellij more.

[jgainerdewar]

Does this new way of getting the input stream use blobPath.nioPath under the covers?

[kraefrei]

indeed, I think both ways work now, but I left it using this new way just since it was neater

[jgainerdewar]

Scala-ism: When calling no-arg Java accessors (like toString), we typically leave the parens off. In Scala, empty parens indicate that there are arguments, but we're using all default values instead of supplying them.

[jgainerdewar]

Ideally we'd have all this checking and retrying within retrieveFilesystem. I'm guessing you're not doing that because we don't actually know whether the filesystem doesn't work until we call checkAccess, for which we need to know the Blob file path. Let's discuss!

[kraefrei]

Ay, there's the rub.

[jgainerdewar]

Left a bunch of mostly-minor comments, overall I think the shape of this is good! Looking forward to seeing how well it supports testing.

[jgainerdewar]

This parsing (String => Instant) is a good candidate for unit testing, maybe break it out into its own method?

[jgainerdewar]

Curious why this class ended up in here and not in BlobPathBuilderFactory.

[jgainerdewar]

Eventually we'll want this buffer time to be configurable.

[jgainerdewar]

I would name this something more specific, like BlobFileSystemManager.

[jgainerdewar]

I'm guessing this is just a draft to get it to compile with the right signature rather than the intended final form of this method. I suggest instead wrapping the whole body of the method in a Try, since that will prevent it from throwing.

[jgainerdewar]

We've discussed begrudgingly accepting that this method will throw. We should ensure it throws something useful, though. I think we should throw different informative error messages depending on whether we failed to get the filesystem or failed to create the NIO path.

[jgainerdewar]

Should this method be private?

[jgainerdewar]

Should this method be private?

[jgainerdewar]

We use the term "preemption" for lower-cost cloud computing getting preempted, I'd rather not overload it. Maybe call this expiryBuffer or something?

[jgainerdewar]

We have a LOT of important string identifiers floating around. Not necessarily in scope for this PR, but we should consider replacing them with simple case classes so that the compiler can check that we're passing them around correctly. Otherwise it's too easy to swap their ordering in a method call and introduce a very hard-to-find bug.

case class BlobContainerName(value: String)
case class StorageAccountName(value: String)
case class WorkspaceId(value: String)

...and so on.

[kraefrei]

I think I will give this a try, It will likely help me get the tests right without accidentally swapping strings

[kraefrei]

Use more descriptive exception here

[jgainerdewar]

This is looking great! So many tests!!

[jgainerdewar]

Now is a good time to take another look at this file and see if there's anything we want to move out into other files in this package. There are a lot of top-level classes in here now, the whole group would probably be easier to understand with a little division.

[jgainerdewar]

We've talked a lot about avoiding unprotected .get calls, but in this case it might be clearer to use one (and leave a comment explaining why we're deliberately throwing). That would make this a lot shorter, I think you can do something like:

fsm.retrieveFilesystem.map(_.getPath(path)).get

...though we'd then want to ensure that the error that shows up in the logs makes it obvious what the problem is.

[jgainerdewar]

Thinking out loud, once we have this base config checked into reference.conf it would be preferable to set the default there... but we can't do that yet, so here is good.

[jgainerdewar]

Whitespace nitpick: I think it's standard to have a newline between ( and the first member of the case class, in cases where the members are split over multiple lines.

[jgainerdewar]

What's the use case for passing in an initial expiry time? Feels kind of weird since there's no enforcement that the filesystem exists.

[kraefrei]

This is mostly for testing to be honest, could make it a private arg possibly? I had been considering this or having a setter, but I liked a parameter that could only be set on construction better so that the internal timestamp wasn't something that could be easily modified.

[jgainerdewar]

Optional tweaking: I would find this clearer if the decision-making were pulled out into a boolean shouldReopenFilesystem method.

[jgainerdewar]

Hmmm, do we expect to ever return None from this method? It looks like that would result in us getting a new token and refreshing the filesystem every time we need it. For now we should perhaps throw an error instead. I'd rather it blow up than silently refresh itself constantly.

[kraefrei]

Yeah, good catch. I wrote this this way because it may turn up no expiration token in the general case, but for our purpose that is likely an error.

[jgainerdewar]

Let's not check in this subscription number. :) Does it need to be included in the config?

[kraefrei]

A config is a good idea! I have made this mistake a few times and the default doesn't seem to be working for me.

[jgainerdewar]

So much cleaner! Nice!

[jgainerdewar]

Is there a way to do this without a null?

[kraefrei]

So yes, but it would involve another mock. I was using this way in favor of a mock mostly because we do not actually need the object, but the type signature, but I was mostly trying to use this as an exercise to only include the mocks we need to test against.

[breilly2]

👍 I didn't finish going through line-by-line, but we had a team review of Janet's feedback and that helped me understand some pieces better... enough to have confidence in these changes.

[breilly2]

IntelliJ is telling me that .map(StorageAccountName(_)) can be just .map(StorageAccountName). But I'm also seeing it say that doesn't work in other cases and I don't understand why. 🤷‍♂️

[kraefrei]

Yeah, I was wondering about this shorthand. I may leave this to be consistent

[breilly2]

fold doesn't feel natural to me when dealing with an Option (though I get that it works because it's essentially a container). Is this a common way to handle this in other places in Cromwell?

If not, what do you think of:

    maybeName.toRight(new Exception("Could not parse storage account")).toTry

or

    maybeName.map(Success(_)).getOrElse(Failure(new Exception("Could not parse storage account")))

(.map(Success(_)) is where IntelliJ complains if I try just .map(Success). 🤷‍♂️)
or even just pattern matching on Some/None?

[kraefrei]

I like the map approach! I'll go with that

[breilly2]

It might be helpful to continue pushing this style through the whole function. Managing the Try returned here inside of a Try { } didn't make sense to me at first. I wonder if parseURI is the only other thing in here that could throw an exception... at which point a for-comprehension might be really easy to read.

[breilly2]

I'm interested in understanding if you found BlobPathValidation helped with testability or code clarity. When reading the code, I had assumed it would help with testability, but then this seems like more ceremony than should be necessary to interpret a UnpansableBlobPath as a test failure. It looks like this works for asserting the thing that this test cares about:

    BlobPathBuilder.validateBlobPath(testString, container, endpoint) should equal(BlobPathBuilder.ValidBlobPath(evalPath))

That said, I realize that this line isn't actually changed in this PR. 🤷‍♂️

[jgainerdewar]

Looks like I introduced some merge conflicts, once those are resolved and tests pass this should be good to go!