-
Notifications
You must be signed in to change notification settings - Fork 423
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use the apiversion from KUBERNETES_EXEC_INFO #439
Conversation
Signed-off-by: Jyoti Mahapatra <[email protected]>
Signed-off-by: Jyoti Mahapatra <[email protected]>
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jyotimahapatra The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Jyoti Mahapatra <[email protected]>
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.
Couple small questions, @jyotimahapatra but overall looks solid.
@@ -70,6 +72,7 @@ var verifyCmd = &cobra.Command{ | |||
|
|||
func init() { | |||
rootCmd.AddCommand(verifyCmd) | |||
metrics.InitMetrics(prometheus.DefaultRegisterer) |
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.
is this change unrelated?
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. it fixes #438
i can get rid of it if causing confusion. it was in my workspace, so i let it be part of the PR
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.
Probably would be best to isolate this to a different PR, thanks Jyoti. Then you can link to #438 from that other PR summary :)
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.
will do
pkg/token/token.go
Outdated
@@ -338,11 +341,23 @@ func (g generator) GetWithSTS(clusterID string, stsAPI stsiface.STSAPI) (Token, | |||
|
|||
// FormatJSON formats the json to support ExecCredential authentication | |||
func (g generator) FormatJSON(token Token) string { | |||
apiVersion := clientauthv1beta1.SchemeGroupVersion.String() | |||
for _, e := range os.Environ() { |
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.
Don't parse this manually, instead use LoadExecCredentialFromEnv()
. Example usage:
package main
import (
"fmt"
"k8s.io/client-go/tools/auth/exec"
)
func main() {
obj, config, err := exec.LoadExecCredentialFromEnv()
fmt.Printf("%#v, %#v, %#v\n", obj, config, err)
}
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.
nice. Thank you!!
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.
Looks like https://github.com/kubernetes/client-go/blob/master/pkg/apis/clientauthentication/types.go#L54-L59 is sending an error ExecCredential does not contain cluster information
.. We have to keep this implementation and work on fixing the error in client-go. wdyt?
@@ -70,6 +72,7 @@ var verifyCmd = &cobra.Command{ | |||
|
|||
func init() { | |||
rootCmd.AddCommand(verifyCmd) | |||
metrics.InitMetrics(prometheus.DefaultRegisterer) |
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.
fixes #438
@@ -70,6 +72,7 @@ var verifyCmd = &cobra.Command{ | |||
|
|||
func init() { | |||
rootCmd.AddCommand(verifyCmd) | |||
metrics.InitMetrics(prometheus.DefaultRegisterer) |
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. it fixes #438
i can get rid of it if causing confusion. it was in my workspace, so i let it be part of the PR
lgtm, works for me . plz squash commits. in 1.24 alpha api will be removed from client-go and probably kubectl as well, not sure what their migration plan exactly (will there be some way for me to find/replace all the v1alpha1 entries in my kubeconfig??) but we will see
|
When using the latest builds of aws-iam-authenticator(v0.5.3-v0.5.5), the default value of apiVersion in the
ExecCredential
object isclient.authentication.k8s.io/v1beta1
. This change was done here. However, when using the newer builds, clients have to update the apiVersion in kubeConfig to match the apiVersion being sent as part of the stdout here.A difference in the values causes errors
The current PR adds a migration path such that the aws-iam-authenticator will return the apiVersion being used in the
KUBERNETES_EXEC_INFO
env var being set from the client-go code. This code will be removed in the future after a period of deprecation and with a minor version change in aws-iam-authenticator.Similar change in aws-cli https://github.com/aws/aws-cli/pull/6476/files