-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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 etcd-manager for the cilium etcd cluster #8750
Use etcd-manager for the cilium etcd cluster #8750
Conversation
Hi @olemarkus. Thanks for your PR. I'm waiting for a kubernetes 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. |
/ok-to-test |
f201e46
to
dd16f82
Compare
/test pull-kops-e2e-kubernetes-aws |
e97f3df
to
7528654
Compare
I think it is a good time to create a |
I wouldn't do that as part of this PR, but if this one goes in, I can do a follow-up creating that dir. |
How does this interact with legacy etcd mode? Do we need to add a validation to prevent this combination? |
7528654
to
e5d41aa
Compare
Yeah. We need to ensure that the cilium etcd cluster is using etcd-manager. I will add some validation on that. |
7f33d8a
to
68608b0
Compare
68608b0
to
4d6cb7a
Compare
@justinsb anything you think needs to be changed in this MR? |
0aa621a
to
9014ded
Compare
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 few minor comments but looks good overall. I know that on the March 27th office hours no one seemed to have any concerns or objections with this, so once we address the comments we can get this merged asap. Thanks! and sorry for the delay.
nodeup/pkg/model/cilium.go
Outdated
privateKeyBytes := pkiutil.EncodePrivateKeyPEM(privateKey) | ||
|
||
certConfig := &certutil.Config{ | ||
CommonName: "kube-apiserver", |
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 a copy/paste from etcd_manager_tls.go, should this cert have a different CN?
fb8c0e5
to
3db9d58
Compare
Co-Authored-By: Peter Rifel <rifelpet@users.noreply.github.com>
3db9d58
to
a7f631e
Compare
Thanks for the additional tests! the new integration tests confirm the security group rule change is only present if cilium etcdManaged is enabled. /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: olemarkus, rifelpet 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 |
This will use etcd-manager to handle the cilium etcd kvstore instead of using cilium-etcd-operator.
Just brute-forced my way until it worked, so it still needs some cleanup. But have a look if there are any concerns with this approach.
cc @johngmyers