-
Notifications
You must be signed in to change notification settings - Fork 314
Ubuntu/Rhel Certificate Renewal Implementation (Part 2 of the certificate renewal command feature) #9781
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
Conversation
Hi @howard-junec. Thanks for your PR. I'm waiting for a aws member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
fbe65d4
to
51d856f
Compare
b2dad8b
to
6eb3e86
Compare
6fcf2db
to
c56b9b5
Compare
pkg/certificates/renewer.go
Outdated
err = r.kube.Get(ctx, secretName, constants.EksaSystemNamespace, secret) | ||
if err != nil { | ||
if apierrors.IsNotFound(err) { | ||
logger.V(2).Info("Secret not found—skipping creation", "name", secretName) |
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.
When will we run into this case?
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 for skipping creating the secret if not found, otherwise, we will update the apiserver etcd client secret
pkg/certificates/renewerubuntu.go
Outdated
return fmt.Errorf("backup certs: %v", err) | ||
} | ||
if err := ssh.RunCommand(ctx, node, | ||
[]string{"sudo", "etcdadm", "join", "phase", "certificates", "http://eks-a-etcd-dumb-url"}); err != nil { |
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.
why are we running this?
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 for run certificates join phase to regenerate the deleted certificates
pkg/certificates/renewerubuntu.go
Outdated
"cd %[1]s && cp -r pki pki.bak_%[2]s && rm -rf pki/* && cp pki.bak_%[2]s/ca.* pki/", | ||
linuxEtcdCertDir, backup, | ||
) | ||
return []string{"sudo", "sh", "-c", script} |
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: can we just move the first 3 params here to a separate func and make it accept any script and just call it here and all other use cases?
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
pkg/certificates/ssh.go
Outdated
} | ||
|
||
func (r *DefaultSSHRunner) buildCommandString(cmds []string) string { | ||
if len(cmds) >= 3 && cmds[0] == "sudo" && cmds[1] == "sh" && cmds[2] == "-c" { |
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.
Why do we need this only for this command?
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.
since commands are passed as string slices, but sudo sh -c needs proper quoting to preserve script integrity, and in later, bottlerocket's sudo sheltie commands need special handling to maintain heredoc syntax. This function will formats both cases correctly.
|
||
kubeClient := deps.UnAuthKubeClient.KubeconfigClient(kubeCfgPath) | ||
|
||
osKey := cfg.OS |
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.
can we just call it os?
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
pkg/certificates/ssh.go
Outdated
// RunCommand runs a command on the remote host | ||
RunCommand(ctx context.Context, node string, cmds []string) error | ||
// RunCommandWithOutput runs a command on the remote host and returns the output | ||
RunCommandWithOutput(ctx context.Context, node string, cmds []string) (string, error) |
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.
do we need both RunCommandWithOutput
and RunCommand
? I think we only need RunCommandWithOutput, just don't assign the output if we don't need it
_, err := ssh.RunCommandWithOutput
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 tried replacing RunCommand with RunCommandWithOutput as suggested, but found that it loses detailed command output when using high verbosity levels like -v=9. RunCommand has special logging behavior that streams command output directly to the console at high verbosity levels, which is critical for debugging. Both methods serve different purposes, I think we can keep both.
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.
How are we deciding when to run RunCommand
or RunCommandWithOutput
?
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.
for nor we only use RunCommandWithOutput when we need to programmatically capture and process the command's output, like captures details steps in "copyEtcdCerts", and all other steps we just use RunCommand.
2851169
to
160214e
Compare
…tialization, and handle environment variables more robustly
fe2d729
to
c59d637
Compare
…lified SSH handling
a454c69
to
9e664e6
Compare
c1ae8ea
to
9e664e6
Compare
… instead of all nodes
Thanks for all the work! |
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: 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 |
@panktishah26: new pull request created: #9957 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. |
/cherry-pick release-0.23 |
@panktishah26: new pull request created: #9959 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. |
Issue #, if available:
Description of changes:
This PR implements certificate renewal functionality for Ubuntu and RHEL nodes in EKS Anywhere clusters. This is part 2 of the certificate renewal feature, focusing on Linux-based operating systems. Key features include:
EKSA_SSH_KEY_PASSPHRASE_ETCD
andEKSA_SSH_KEY_PASSPHRASE_CP
to avoid manual password inputExample Config File:
This PR builds on the configuration structure from part 1, implementing the renewal logic for traditional Linux distributions.
Testing (if applicable):
Documentation added/planned (if applicable):
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.