Skip to content

feat(flagd): Add features to customize auth to Sync API server (authorityOverride and clientInterceptors) #1260

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

Merged
merged 3 commits into from
Mar 5, 2025

Conversation

cupofcat
Copy link
Contributor

@cupofcat cupofcat commented Mar 4, 2025

This PR

  • adds authorityOverride option
  • adds clientInterceptors option
  • add relevant tests and documentation

@beeme1mr
Copy link
Member

beeme1mr commented Mar 4, 2025

Relates to open-feature/flagd#1554

@toddbaert
Copy link
Member

I think this is a good idea, but I believe it's been implemented slightly differently in JS: https://github.com/search?q=repo%3Aopen-feature%2Fjs-sdk-contrib%20authority&type=code

This isn't yet an option specified in the flagd provider spec but it would be great if we could be consistent. Do you think we could match the param name there? And also add a matching environment variable? If we implement it twice, it might be worth adding to the flagd spec.

@cupofcat
Copy link
Contributor Author

cupofcat commented Mar 5, 2025

Hey @toddbaert, this is interesting. So the JS uses default because the underlying gRPC library uses default. I use override here in Java because the underlying gRPC library for Java has overrideAuthority method. In Go, it's not even necessary because one can use the WithDialOptionsOverride(... grpc.WithAuthority(...)... ). I don't think other providers expose this yet.

So, should we try to be consistent across providers, and inconsistent with the given language idioms? Or, should the language take precedence?

@toddbaert
Copy link
Member

toddbaert commented Mar 5, 2025

Hey @toddbaert, this is interesting. So the JS uses default because the underlying gRPC library uses default. I use override here in Java because the underlying gRPC library for Java has overrideAuthority method. In Go, it's not even necessary because one can use the WithDialOptionsOverride(... grpc.WithAuthority(...)... ). I don't think other providers expose this yet.

So, should we try to be consistent across providers, and inconsistent with the given language idioms? Or, should the language take precedence?

@cupofcat In most cases, I would prefer to stick with language idioms. However, one of the reasons we standardize these options is so they can be easily configured in "cloud-native" workloads via env-vars, regardless of language (the OpenFeature Operator does this, for example). Having a different env var per language would make this tricky for admins, and if we are going to be consistent with the env vars, we should also make sure the option name and env var match.

So in the end, I would prefer we pick a single option and env var name for all implementations. Perhaps the one implemented in JS isn't ideal, so we could change it since we are still sub 1.0 and pick something ideal if we need. What is your opinion?

@cupofcat
Copy link
Contributor Author

cupofcat commented Mar 5, 2025

@toddbaert I see your point. In this case I am OK with going with default authority. It looks like Python gRPC also uses default and it looks that this is, in fact, how the impl names this - https://github.com/grpc/grpc/blob/b73dbd94df4bd9f9362d16b76f34e4c7c2358409/include/grpc/impl/channel_arg_names.h#L151

So, I will go ahead and change this to defaultAuthority and add FLAGD_GRPC_DEFAULT_AUTHORITY env var.

Edit: Actually, perhaps the field should also be called gRPCDefaultAuthority?

Edit2: After more thinking, almost every option already is about gRPC so I think gRPC prefix is not needed. So
defaultAuthority and FLAGD_DEFAULT_AUTHORITY.

@toddbaert
Copy link
Member

Thanks @cupofcat

@toddbaert toddbaert merged commit 0c2803a into open-feature:main Mar 5, 2025
5 checks passed
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.

5 participants