-
Notifications
You must be signed in to change notification settings - Fork 39k
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
Validate kubeconfig files in case of external CA mode #70537
Conversation
@yagonobre thanks.
|
/priority important-longterm |
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 on a quick look.
added two minor nits.
err := ValidateKubeConfig(outDir, filename, config) | ||
if err != nil { | ||
// Check if the file exist, and if it doesn't, just write it to disk | ||
if os.IsNotExist(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.
can be done as follows, optionally:
if !os.IsNotExist(err) {
return err
}
...
|
||
err = kubeconfigutil.WriteToDisk(kubeConfigFilePath, config) | ||
if err != nil { | ||
return errors.Wrapf(err, "failed to save kubeconfig file %s on disk", kubeConfigFilePath) |
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" -> "%q"
@neolit123 done |
/ok-to-test |
@@ -116,7 +125,14 @@ func runKubeConfigFile(kubeConfigFileName string) func(workflow.RunData) error { | |||
|
|||
// if external CA mode, skip certificate authority generation | |||
if data.ExternalCA() { | |||
//TODO: implement validation of existing kubeconfig files | |||
config, err := getKubeConfig(data.Cfg(), kubeConfigFileName) |
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 new behavior covered either directly or indirectly in the _tests file?
/assign @fabriziopandini
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.
Not yet, I'll update the tests tomorrow.
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.
@yagonobre thanks for this PR, this is definitely a step in the right direction!
However, IMO we should change where the validation is triggered in order to fail as soon as possible in case we are in external CA mode (certificate authority certs existing without keys), but without all proper certificates and kubeconfig file in places.
In order to do so, I think that the kubeconfig validation should happen before, ideally when building ìnitData/inside the
externalCA` function here. wdyt?
@timothysc @neolit123 opinions?
+1 |
@fabriziopandini options:
my vote is to try 1, then 2. |
@neolit123 @yagonobre IMO |
@fabriziopandini make senses, I'll update soon |
26f5cd4
to
315002d
Compare
315002d
to
a5cf391
Compare
a5cf391
to
0b991fc
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.
@yagonobre thanks for this update to this PR!
IMO this is ready to merge as soon as the new ValidateKubeconfigsForExternalCA
func is covered by tests as required by @timothysc
/approve
0b991fc
to
e7623b2
Compare
Updated with some tests |
e7623b2
to
eb49aa4
Compare
eb49aa4
to
e1320bb
Compare
/test pull-kubernetes-e2e-gce-100-performance |
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
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini, timothysc, yagonobre 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 |
/retest |
1 similar comment
/retest |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes kubernetes/kubeadm#1203
Does this PR introduce a user-facing change?:
/assign @timothysc
/assign @fabriziopandini