Skip to content

Conversation

@zymap
Copy link
Member

@zymap zymap commented Nov 7, 2019


Master Issue: #127

Motivation

Pulsarctl needs to support bookie client API, and will use the
same HTTP client to request a different server. So we need to
separate the HTTP client to make the different admin client request
to different servers.

Modifications

  • separate the HTTP client from admin client

zymap added 2 commits November 7, 2019 12:24
---

Master Issue: streamnative#127

*Motivation*

Pulsarctl needs to support bookie client API, and will use the
same HTTP client to request a different server. So we need to
separate the HTTP client to make the different admin client request
to different servers.

*Modifications*

- separate the HTTP client from admin client
@zymap zymap self-assigned this Nov 7, 2019
@zymap zymap changed the title Separate HTTP client and admin client [BK-SUPPORT-PART-1] Separate HTTP client and admin client Nov 7, 2019
@zymap zymap added this to the 0.2.0 milestone Nov 7, 2019
@zymap zymap mentioned this pull request Nov 7, 2019
5 tasks
@zymap zymap requested review from sijie and wolfstudy November 7, 2019 10:40
@zymap
Copy link
Member Author

zymap commented Nov 7, 2019

@sijie @wolfstudy PTAL. Thanks.

@zymap
Copy link
Member Author

zymap commented Nov 7, 2019

@sijie @wolfstudy PTAL. Thanks

1 similar comment
@zymap
Copy link
Member Author

zymap commented Nov 11, 2019

@sijie @wolfstudy PTAL. Thanks

@zymap
Copy link
Member Author

zymap commented Nov 11, 2019

@sijie @wolfstudy PTAL. Thanks

pkg/auth/tls.go Outdated
return &cert, err
}

func (p *TLSAuthProvider) AddAuthParams(client *http.Client, req *http.Request) {
Copy link
Member

Choose a reason for hiding this comment

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

The logic here is not adding auth params. You are initializing a tls transport. I don't think we should call it AddAuthParams.

Also req is not used in this method.

Copy link
Member

Choose a reason for hiding this comment

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

Java client implementation already provides a good abstraction for different type of authentication implementation. Please check them out.

I don't think the abstraction here is a good abstraction.

  1. initializing a transport should be done during initializing auth provider. we should not do it in a method that will be called for each request. You can try avoid initializing transport each time this method is called, by adding a check logic. It is a hack. but the abstraction is wrong.

  2. for a method that will be called for each request, http.client is not needed. The java client already provides a good abstraction.

/**
     * For mutual authentication, This method use passed in `data` to evaluate and challenge,
     * then returns null if authentication has completed;
     * returns authenticated data back to server side, if authentication has not completed.
     *
     * <p>Mainly used for mutual authentication like sasl.
     */
    default AuthData authenticate(AuthData data) throws AuthenticationException {
        byte[] bytes = (hasDataFromCommand() ? this.getCommandData() : "").getBytes(UTF_8);
        return AuthData.of(bytes);
    }

you can just add a method authenticate(req *http.Request) (*http.Request) in the auth provider. It is a clear abstraction.

@zymap
Copy link
Member Author

zymap commented Nov 12, 2019

@sijie I update the auth implementation.

The TLS already init when the HTTP client created. But when users using the token to authenticate, we need to add the header to the HTTP requests. So I provide a new method GetHTTPHeaders to add the headers to the HTTP requests as java did.

Please take a look when you have time. Thanks.

@sijie sijie merged commit 551d6d6 into streamnative:master Nov 13, 2019
tisonkun pushed a commit to tisonkun/pulsar-client-go that referenced this pull request Aug 15, 2023
…ve/pulsarctl#132)

Master Issue: streamnative/pulsarctl#127

*Motivation*

Pulsarctl needs to support bookie client API, and will use the
same HTTP client to request a different server. So we need to
separate the HTTP client to make the different admin client request
to different servers.

*Modifications*

- separate the HTTP client from admin client
tisonkun pushed a commit to apache/pulsar-client-go that referenced this pull request Aug 16, 2023
…ve/pulsarctl#132)

Master Issue: streamnative/pulsarctl#127

*Motivation*

Pulsarctl needs to support bookie client API, and will use the
same HTTP client to request a different server. So we need to
separate the HTTP client to make the different admin client request
to different servers.

*Modifications*

- separate the HTTP client from admin client
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants