You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2020/07/08 06:42:56 UTC
[GitHub] [pulsar-client-go] zymap opened a new pull request #313: Support oauth2 authentication for pulsar-client-go
zymap opened a new pull request #313:
URL: https://github.com/apache/pulsar-client-go/pull/313
*Motivation*
Support oauth2 authentication for pulsar-client-go
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] wolfstudy merged pull request #313: Support oauth2 authentication for pulsar-client-go
Posted by GitBox <gi...@apache.org>.
wolfstudy merged pull request #313:
URL: https://github.com/apache/pulsar-client-go/pull/313
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] EronWright commented on a change in pull request #313: Support oauth2 authentication for pulsar-client-go
Posted by GitBox <gi...@apache.org>.
EronWright commented on a change in pull request #313:
URL: https://github.com/apache/pulsar-client-go/pull/313#discussion_r452471003
##########
File path: pulsar/internal/auth/oauth2.go
##########
@@ -0,0 +1,153 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package auth
+
+import (
+ "crypto/tls"
+ "fmt"
+
+ "github.com/apache/pulsar-client-go/oauth2"
+ "github.com/apache/pulsar-client-go/oauth2/cache"
+ "github.com/apache/pulsar-client-go/oauth2/store"
+ "k8s.io/utils/clock"
+)
+
+const (
+ ConfigParamType = "type"
+ ConfigParamTypeClientCredentials = "client_credentials"
+ ConfigParamIssuerURL = "issuerUrl"
+ ConfigParamAudience = "audience"
+ ConfigParamKeyFile = "privateKey"
+ ConfigParamClientID = "clientId"
+)
+
+type oauth2AuthProvider struct {
+ clock clock.Clock
+ issuer oauth2.Issuer
+ store store.Store
+ source cache.CachingTokenSource
+}
+
+// NewAuthenticationOAuth2WithParams return a interface of Provider with string map.
+func NewAuthenticationOAuth2WithParams(params map[string]string) (Provider, error) {
+ issuer := oauth2.Issuer{
+ IssuerEndpoint: params[ConfigParamIssuerURL],
+ ClientID: params[ConfigParamClientID],
+ Audience: params[ConfigParamAudience],
+ }
+
+ // initialize a store of authorization grants
+ st := store.NewMemoryStore()
+
+ oauth2Type := params[ConfigParamType]
+ keyFile := params[ConfigParamKeyFile]
+ return NewAuthenticationOAuth2(issuer, st, oauth2Type, keyFile)
+}
+
+func NewAuthenticationOAuth2(
+ issuer oauth2.Issuer,
+ store store.Store,
+ oauth2Type, keyFilePath string) (Provider, error) {
Review comment:
I see that you changed this constructor from `NewAuthenticationOAuth2(issuer, store)` to `NewAuthenticationOAuth2(issuer, store, oauth2type, keyfile)`. Let me explain why I think that's not a good change.
The authentication provider requires an authorization grant to be able to obtain access tokens over time. The grant is a long-lived credential, such as a keyfile or a refresh token. In all cases, the +application+ obtains the grant beforehand and then stores it in a store. The store might be persistent. Later, the application creates an authorization provider with the store as a parameter. The provider loads the grant from the store, and uses it over time to obtain access tokens.
I am trying to explain that the provider is not responsible for obtaining or storing new grants. Hence the signature `NewAuthenticationOAuth2(issuer, store)`.
Now we discuss the special case of initializing the provider from parameters - `NewAuthenticationOAuth2WithParams`. One could imagine a combination of parameters that would open a pre-existing store but this would require more design work. A simple scenario that is possible now is the keyfile scenario in combination with an in-memory store (where the access token would be discarded when the process terminates). This scenario is implemented above.
Would you revert the change to this method that is seen in https://github.com/apache/pulsar-client-go/pull/313/commits/97d98f753995fc7879e69472efec6fb698ebed99?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] EronWright commented on a change in pull request #313: Support oauth2 authentication for pulsar-client-go
Posted by GitBox <gi...@apache.org>.
EronWright commented on a change in pull request #313:
URL: https://github.com/apache/pulsar-client-go/pull/313#discussion_r452471003
##########
File path: pulsar/internal/auth/oauth2.go
##########
@@ -0,0 +1,153 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package auth
+
+import (
+ "crypto/tls"
+ "fmt"
+
+ "github.com/apache/pulsar-client-go/oauth2"
+ "github.com/apache/pulsar-client-go/oauth2/cache"
+ "github.com/apache/pulsar-client-go/oauth2/store"
+ "k8s.io/utils/clock"
+)
+
+const (
+ ConfigParamType = "type"
+ ConfigParamTypeClientCredentials = "client_credentials"
+ ConfigParamIssuerURL = "issuerUrl"
+ ConfigParamAudience = "audience"
+ ConfigParamKeyFile = "privateKey"
+ ConfigParamClientID = "clientId"
+)
+
+type oauth2AuthProvider struct {
+ clock clock.Clock
+ issuer oauth2.Issuer
+ store store.Store
+ source cache.CachingTokenSource
+}
+
+// NewAuthenticationOAuth2WithParams return a interface of Provider with string map.
+func NewAuthenticationOAuth2WithParams(params map[string]string) (Provider, error) {
+ issuer := oauth2.Issuer{
+ IssuerEndpoint: params[ConfigParamIssuerURL],
+ ClientID: params[ConfigParamClientID],
+ Audience: params[ConfigParamAudience],
+ }
+
+ // initialize a store of authorization grants
+ st := store.NewMemoryStore()
+
+ oauth2Type := params[ConfigParamType]
+ keyFile := params[ConfigParamKeyFile]
+ return NewAuthenticationOAuth2(issuer, st, oauth2Type, keyFile)
+}
+
+func NewAuthenticationOAuth2(
+ issuer oauth2.Issuer,
+ store store.Store,
+ oauth2Type, keyFilePath string) (Provider, error) {
Review comment:
I see that you changed this constructor from `NewAuthenticationOAuth2(issuer, store)` to `NewAuthenticationOAuth2(issuer, store, oauth2type, keyfile)`. Let me explain why I think that's not a good change.
The authentication provider requires an authorization grant to be able to obtain access tokens over time. The grant is a long-lived credential, such as a keyfile or a refresh token. In all cases, the +application+ obtains the grant beforehand and then stores it in a store. The store might be persistent. Later, the application creates an authorization provider with the store as a parameter. The provider loads the grant from the store, and uses it over time to obtain access tokens.
I am trying to explain that the provider is not responsible for obtaining or storing new grants. Hence the signature `NewAuthenticationOAuth2(issuer, store)`.
Now we discuss the special case of initializing the provider from parameters - `NewAuthenticationOAuth2WithParams`. One could imagine a combination of parameters that would open a pre-existing store but this would require more design work. A simple scenario that is possible now is the keyfile scenario in combination with an in-memory store (where the access token would be discarded when the process terminates). This scenario is implemented in `NewAuthenticationOAuth2WithParams`.
Would you revert the change to this method that is seen in https://github.com/apache/pulsar-client-go/pull/313/commits/97d98f753995fc7879e69472efec6fb698ebed99?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] zymap commented on a change in pull request #313: Support oauth2 authentication for pulsar-client-go
Posted by GitBox <gi...@apache.org>.
zymap commented on a change in pull request #313:
URL: https://github.com/apache/pulsar-client-go/pull/313#discussion_r454761641
##########
File path: go.mod
##########
@@ -1,20 +1,25 @@
module github.com/apache/pulsar-client-go
-go 1.12
+go 1.13
require (
github.com/DataDog/zstd v1.4.6-0.20200617134701-89f69fb7df32
+ github.com/apache/pulsar-client-go/oauth2 v0.0.0-00010101000000-000000000000
github.com/beefsack/go-rate v0.0.0-20180408011153-efa7637bb9b6
github.com/bmizerany/perks v0.0.0-20141205001514-d9a9656a3a4b
github.com/gogo/protobuf v1.3.1
github.com/inconshreveable/mousetrap v1.0.0 // indirect
- github.com/klauspost/compress v1.10.8
+ github.com/klauspost/compress v1.10.5
+ github.com/kr/pretty v0.2.0 // indirect
github.com/pierrec/lz4 v2.0.5+incompatible
- github.com/pkg/errors v0.8.1
+ github.com/pkg/errors v0.9.1
github.com/sirupsen/logrus v1.4.1
github.com/spaolacci/murmur3 v1.1.0
github.com/spf13/cobra v0.0.3
github.com/spf13/pflag v1.0.3 // indirect
github.com/stretchr/testify v1.4.0
github.com/yahoo/athenz v1.8.55
+ k8s.io/utils v0.0.0-20200619165400-6e3d28b6ed19
Review comment:
Yes. Let me replace it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] EronWright commented on a change in pull request #313: Support oauth2 authentication for pulsar-client-go
Posted by GitBox <gi...@apache.org>.
EronWright commented on a change in pull request #313:
URL: https://github.com/apache/pulsar-client-go/pull/313#discussion_r454491977
##########
File path: go.mod
##########
@@ -1,20 +1,25 @@
module github.com/apache/pulsar-client-go
-go 1.12
+go 1.13
require (
github.com/DataDog/zstd v1.4.6-0.20200617134701-89f69fb7df32
+ github.com/apache/pulsar-client-go/oauth2 v0.0.0-00010101000000-000000000000
github.com/beefsack/go-rate v0.0.0-20180408011153-efa7637bb9b6
github.com/bmizerany/perks v0.0.0-20141205001514-d9a9656a3a4b
github.com/gogo/protobuf v1.3.1
github.com/inconshreveable/mousetrap v1.0.0 // indirect
- github.com/klauspost/compress v1.10.8
+ github.com/klauspost/compress v1.10.5
+ github.com/kr/pretty v0.2.0 // indirect
github.com/pierrec/lz4 v2.0.5+incompatible
- github.com/pkg/errors v0.8.1
+ github.com/pkg/errors v0.9.1
github.com/sirupsen/logrus v1.4.1
github.com/spaolacci/murmur3 v1.1.0
github.com/spf13/cobra v0.0.3
github.com/spf13/pflag v1.0.3 // indirect
github.com/stretchr/testify v1.4.0
github.com/yahoo/athenz v1.8.55
+ k8s.io/utils v0.0.0-20200619165400-6e3d28b6ed19
Review comment:
This dependency was due to the mock clock right? I recall that you forked it elsewhere, would that work here too?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org