Skip to content

Inconsistency in credentials specification for CPS Publisher and Subscriber #1959

Description

@kamalaboulhosn

Both the Publisher and the Subscriber class state "If no credentials are provided, the [Publisher|Subscriber] will use application default creentials through GoogleCredentials#getApplicationDefault. However, while Subscriber.Builder has a setCredentials method, Publisher.Builder does not. Moreover, from what I can see, Subscriber does not actually use the credentials set via Subscriber.Builder.setCredentials. If we are going to allow one to set credentials this way, we should be consistent between the Publisher and Subscriber and ensure we use the provided credentials. Otherwise, we should remove the method from the Subscriber.Builder.

Activity

  1. was2 commented on Apr 22, 2017

    @was2

    I am seeing the same behavior with Subscriber.Builder.setCrecentials - even when valid creds are provided, I get an error complaining that app default credentials are not available. This appears to have started in 0.13.0.

  2. garrettjonesgoogle commented on Apr 23, 2017

    @garrettjonesgoogle
    Contributor

    gRPC has a way to provide credentials through CallOptions, which is currently marked with @ExperimentalApi, but that will be removed soon. This enables us to change how credentials are provided to GAPIC clients: we could move CredentialsProvider fromChannelProvider up to ClientSettings (so that it is a sibling of ChannelProvider). This would make the code to provide custom credentials a bit simpler. Then, this same design could be used in Publisher/Subscriber, where credentials are provided separately from the channel.

    Current code:

    CredentialsProvider credentialsProvider =
        FixedCredentialsProvider
            .create(ServiceAccountCredentials.fromStream(new FileInputStream("credentials.json")));
    ChannelProvider channelProvider =
        TopicAdminSettings.defaultChannelProviderBuilder()
            .setCredentialsProvider(credentialsProvider).build();
    TopicName topicName = TopicName.create("my-project-id", "my-topic-id");
    Publisher publisher =
        Publisher.defaultBuilder(topicName).setChannelProvider(channelProvider).build();
    

    Proposed:

    CredentialsProvider credentialsProvider =
        FixedCredentialsProvider
            .create(ServiceAccountCredentials.fromStream(new FileInputStream("credentials.json")));
    TopicName topicName = TopicName.create("my-project-id", "my-topic-id");
    Publisher publisher =
        Publisher.defaultBuilder(topicName).setCredentialsProvider(credentialsProvider).build();
    

    @pongad what are your thoughts?

  3. self-assigned this
    on Apr 24, 2017
  4. pongad commented on Apr 24, 2017

    @pongad
    Contributor

    This was missed when we migrate from ChannelBuilder to ChannelProvider. The method setCredentials weren't removed but calling it is essentially a noop.

    @garrettjonesgoogle My understanding of credentials is that we put an interceptor into the channel, and the interceptor insert credentials into calls. If the CredentialsProvider is a sibling of the ChannelProvider, how does the ChannelProvider create a channel without knowing what the credentials are? In general, I think Subscriber/Publisher should use the same "credential inserter" as gapic clients. Or do we plan to change those too?

    In either case, I think we can all agree that the doc and method are misleading and should be removed. I'll make a PR for this.

  5. garrettjonesgoogle commented on Apr 24, 2017

    @garrettjonesgoogle
    Contributor

    @pongad we would no longer use an interceptor; we would add a new UnaryCallable in the stack which adds the credentials to CallOptions before calling the next UnaryCallable. In that way, the credentials and the channel are decoupled.

    I agree that Subscriber/Publisher should use the same mechanism as GAPIC clients.

  6. pongad commented on Apr 24, 2017

    @pongad
    Contributor

    Ah OK. I assume this new layer hasn't been written yet? I'll make a PR to remove this old code. When the layer is ready, we can migrate en masse.

  7. garrettjonesgoogle commented on Apr 24, 2017

    @garrettjonesgoogle
    Contributor

    @pongad correct, support for this is not written yet into GAX or toolkit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

api: pubsubIssues related to the Pub/Sub API.

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions