-
Notifications
You must be signed in to change notification settings - Fork 113
Deprecate use of emailClaim. #87
Deprecate use of emailClaim. #87
Conversation
In order to support multiple clusters in the future we need each username in the kubeconfig to be unique. Therefore rather than using the user's email from their oidc auth, we should construct it from usernameClaim and clusterName. Of course we should also deprecate it nicely, so if emailClaim is set it will override that default and still use their OIDC email address and print a warning message in the logs. Signed-off-by: Paul Czarkowski <username.taken@gmail.com>
Resolves #86 |
@@ -57,14 +57,14 @@ $ sudo mv ./kubectl /usr/local/bin/kubectl | |||
<code class="language-bash"> | |||
echo "{{ .ClusterCA }}" \ > ca-{{ .ClusterName }}.pem | |||
kubectl config set-cluster {{ .ClusterName }} --server={{ .APIServerURL }} --certificate-authority=ca-{{ .ClusterName }}.pem --embed-certs | |||
kubectl config set-credentials {{ .Username }}@{{ .ClusterName }} \ | |||
kubectl config set-credentials {{ .Email }} \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulczar in the issue we discussed making this user@cluster
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, which it is ... just its doing it in the logic that was fetching emailClaim rather than in the template. see here. Instead of completely ignoring emailClaim it will use it if provided in a config (with a deprecation warning ) and if its not set it sets the existing email
field to be username@cluster.
Thus both the new and the old work, and the deprecation can be remove later at a suitable time with a major version cut.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh I see. My only nit is that it's confusing if you just look at the template file. Maybe a different variable name would make sense just to make it clear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I thought that as well, but I thought for the sake of not messing with the various data structures it would be best to leave it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok thats fine, we can rework for v2. 😁
log.Println("email Handler") | ||
return | ||
email := strings.Join([]string{username, cfg.ClusterName}, "@") | ||
if cfg.EmailClaim != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add some tests to cover this new logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tests added!
Signed-off-by: Paul Czarkowski <username.taken@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
In order to support multiple clusters in the future we need each
username in the kubeconfig to be unique. Therefore rather than using
the user's email from their oidc auth, we should construct it from
usernameClaim and clusterName.
Of course we should also deprecate it nicely, so if emailClaim is set
it will override that default and still use their OIDC email address and
print a warning message in the logs.
Signed-off-by: Paul Czarkowski username.taken@gmail.com