Added cert auth method for auto auth #58
Conversation
|
Hi @qvicksilver, sorry for the delay, I've been away for some time! It seems we have introduced a number of conflicts. Would you kindly resolve them so I can review this pull request? Thank you! |
qvicksilver
force-pushed
the
add-cert-auth-method
branch
from
60faa13
to
ce3417d
Compare
|
Rebased! |
tvoran
added
enhancement
| @@ -123,12 +123,15 @@ const ( | |||
| // AnnotationVaultAuthPath specifies the mount path to be used for the Kubernetes auto-auth | |||
| // method. | |||
| AnnotationVaultAuthPath = "vault.hashicorp.com/auth-path" | |||
|
|
|||
| // AnnotationVaultAuthMethod specifies the auto-auth method to use | |||
| AnnotationVaultAuthMethod = "vault.hashicorp.com/auth-method" | |||
There was a problem hiding this comment.
would it be better to have auth config instead, cause there might be a need for other additional params in future? What do you think?
| @@ -126,6 +120,16 @@ func (a *Agent) newConfig(init bool) ([]byte, error) { | |||
| Templates: a.newTemplateConfigs(), | |||
| } | |||
|
|
|||
| if a.Vault.AuthMethod == "" || a.Vault.AuthMethod == DefaultVaultAuthMethod { | |||
There was a problem hiding this comment.
Wouldn't it better to abstract this logic rather than having it all over the place? If were to do it I would create an interface with methods GetAutoAuthConfig and GetAutoAuthPatches and create structs like KubernetesAutoAuth and CertAutoAuth which implements this interface. I can see this function growing as now there are 2 open PRs for adding different autoauthmethods soon there will be more autoauth methods and we will end up with a very big function. Apart from cognitive overhead it would also make it hard to test this method in future. What do you think?
There was a problem hiding this comment.
I totally agree, however that was never the scope of this. At the time of implementation we had the need to support authentication using certificates. We don't forsee any need for anything else in the future and there seemed to be no efforts in the upstream implementation to provide an abstract version.
If you feel this is not up to the projects standards it is perfectly fine to dismiss this PR, there will be no hard feelings. We don't use this solution anymore and will not put more time into it.
|
I am not maintainer of this project. I came across this PR and thought I
will try to help.
…On Sat, Mar 7, 2020 at 00:40 qvicksilver ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In agent-inject/agent/config.go
<#58 (comment)>:
> @@ -126,6 +120,16 @@ func (a *Agent) newConfig(init bool) ([]byte, error) {
Templates: a.newTemplateConfigs(),
}
+ if a.Vault.AuthMethod == "" || a.Vault.AuthMethod == DefaultVaultAuthMethod {
I totally agree, however that was never the scope of this. At the time of
implementation we had the need to support authentication using
certificates. We don't forsee any need for anything else in the future and
there seemed to be no efforts in the upstream implementation to provide an
abstract version.
If you feel this is not up to the projects standards it is perfectly fine
to dismiss this PR, there will be no hard feelings. We don't use this
solution anymore and will not put more time into it.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#58?email_source=notifications&email_token=ACN3OKXEMUFJL2XZTPRVA6TRGFNVLA5CNFSM4KK7MCJ2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCYLRTQA#discussion_r389133078>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACN3OKVNJMK23XWHIOO563TRGFNVLANCNFSM4KK7MCJQ>
.
|
|
Hey, I've got a PR open (#213) that supports all auth methods (including Cert) via annotations. Check that out to see if it covers your use case - going off of historical PRs, it seems like a common ask. |
|
All Auth Methods are supported. Closing! |
No description provided.