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

pass skip_credential_subscoping_indirection param to TaskFileIOSupplier #400

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

alessandro-nori
Copy link
Contributor

@alessandro-nori alessandro-nori commented Oct 23, 2024

Description

Fixes #379

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Checklist:

Please delete options that are not relevant.

  • I have performed a self-review of my code
  • My changes generate no new warnings

@alessandro-nori alessandro-nori force-pushed the fix_task_fileio_supplier_wo_creds_subscoping branch 5 times, most recently from 102e771 to 09d3b09 Compare October 23, 2024 12:18
// Typically this setting is used in single-tenant server deployments that don't rely on
// "credential-vending" and can use server-default environment variables or credential config
// files for all storage access, or in test/dev scenarios.
public static final Boolean SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION_DEFAULT = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather not have a separate public variable here; you can always get the default with SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION.defaultValue

Copy link
Contributor

@eric-maynard eric-maynard left a comment

Choose a reason for hiding this comment

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

This looks really close to me; if you can add some details on how you tested this I think we can merge it almost as-is. Thanks for working on this!

@@ -103,6 +103,22 @@ public static <T> Builder<T> builder() {
.defaultValue(false)
.build();

// Config key for whether to skip credential-subscoping indirection entirely whenever trying
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this into the description? The description should ideally be detailed enough to understand the config without an additional comment explaining it.

@alessandro-nori
Copy link
Contributor Author

This looks really close to me; if you can add some details on how you tested this I think we can merge it almost as-is. Thanks for working on this!

Thanks for your input on this @eric-maynard !
Do you have any suggestion for a unit test? I didn't find a way to check the properties of the loaded FileIO to see if it contains credentials or not.
For now I've only tested that it's working on my setup.

@eric-maynard
Copy link
Contributor

I see, but how did you test it's working? i.e. how did you confirm that the credentials vended were not subscoped?

@alessandro-nori
Copy link
Contributor Author

I see, but how did you test it's working? i.e. how did you confirm that the credentials vended were not subscoped?

I did some manual tests and verified it's working (it was not working in my setup when Polaris was subscoping the credentials because I cannot reach aws endpoints directly).
I also observed the traces and there are no calls to aws.AssumeRole anymore

Copy link
Contributor

@collado-mike collado-mike left a comment

Choose a reason for hiding this comment

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

This seems a reasonable change - rather than saddling the PolarisApplication with the details of the TaskFileIOSupplier's configuration keys, we can push that lookup down into the thing that actually cares about it.

Comment on lines 211 to 219
Boolean skipCredentialSubscopingIndirection =
configurationStore.getConfiguration(
null, PolarisConfiguration.SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION.key);
TaskHandlerConfiguration taskConfig = configuration.getTaskHandler();
TaskExecutorImpl taskExecutor =
new TaskExecutorImpl(taskConfig.executorService(), metaStoreManagerFactory);
TaskFileIOSupplier fileIOSupplier =
new TaskFileIOSupplier(metaStoreManagerFactory, fileIOFactory);
new TaskFileIOSupplier(
metaStoreManagerFactory, fileIOFactory, skipCredentialSubscopingIndirection);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd probably just pass the PolarisConfigurationStore directly to the TaskFileIOSupplier so that it can look up its own configuration keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your feedback

@alessandro-nori alessandro-nori force-pushed the fix_task_fileio_supplier_wo_creds_subscoping branch from 6288de6 to b5a4142 Compare November 27, 2024 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION is ignored in TaskFileIOSupplier
3 participants