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
create command to create new adminkubeconfig #1452
create command to create new adminkubeconfig #1452
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k 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 |
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.
I glanced at this, I just had some superficial nits, nonblocking.
newAdminKubeconfigLong = templates.LongDesc(i18n.T(` | ||
Generate, make the server trust, and display a new admin.kubeconfig. | ||
|
||
The key is produced locally and never hits disk. The public half is pushed to the 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.
s/never hits disk/is not persisted/ maybe? "hits disk" is a little colloquial.
if len(args) > 0 { | ||
fmt.Errorf("no arguments allowed") |
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.
This is just https://pkg.go.dev/github.com/spf13/cobra#ExactArgs with zero.
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.
if err := cobra.ExactArgs(0)(cmd, args); err != nil {
return err
}
Doesn't seem to read easier to future maintainers to me.
} | ||
|
||
// update the in-cluster configmap | ||
existingConfigMap, err := r.KubeClient.CoreV1().ConfigMaps("openshift-config").Get(ctx, "admin-kubeconfig-client-ca", metav1.GetOptions{}) |
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.
A very minor style thing, I would hoist admin-kubeconfig-client-ca
to a const
at the top so it's easily found (and use it in multiple places to avoid typos).
} | ||
|
||
func createNewAdminClientCert() (*crypto.TLSCertificateConfig, []byte, error) { | ||
tenYears := 24 * time.Hour * 365 * 10 |
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.
This would be a good toplevel const
too
0d5360b
to
636d40f
Compare
updated for comments. |
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.
no blocking comments
for i := range certificates { | ||
found := false | ||
for j := range finalCertificates { | ||
if reflect.DeepEqual(certificates[i].Raw, finalCertificates[j].Raw) { |
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.
bytes.Equal
would probably be good enough
} | ||
} | ||
|
||
caBytes, err := crypto.EncodeCertificates(finalCertificates...) |
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.
nit: you could just return here
/override ci/prow/e2e-agnostic-ovn-cmd |
@deads2k: Overrode contexts on behalf of deads2k: ci/prow/e2e-agnostic-ovn-cmd In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
got an lgtm via slack, labelling. |
/override ci/prow/e2e-aws-ovn-serial |
@deads2k: Overrode contexts on behalf of deads2k: ci/prow/e2e-aws-ovn-serial In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@deads2k: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Add
oc config new-admin-kubeconfig
to create a new admin.kubeconfig using an existing connection./assign @stlaz