Allows additional Subject Alternate Names #2063

pdh · 2017-03-07T18:06:45Z

Adds support for specifying additional SANs for issue #1922

This change is

k8s-ci-robot · 2017-03-07T18:06:46Z

Hi @pdh. 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 @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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.

yissachar · 2017-03-08T05:31:16Z

@k8s-bot ok to test

chrislovecnm

Questions for you

chrislovecnm · 2017-04-21T02:03:47Z

cmd/kops/create_cluster.go

@@ -167,6 +168,8 @@ func NewCmdCreateCluster(f *util.Factory, out io.Writer) *cobra.Command {
 	cmd.Flags().StringSliceVar(&options.NodeSecurityGroups, "node-security-groups", options.NodeSecurityGroups, "Add precreated additional security groups to nodes.")
 	cmd.Flags().StringSliceVar(&options.MasterSecurityGroups, "master-security-groups", options.MasterSecurityGroups, "Add precreated additional security groups to masters.")

+	cmd.Flags().StringSliceVar(&options.AdditionalSANs, "additional-sans", options.AdditionalSANs, "Add additional Subject Alternate Names to the kops generated apiserver cert")


Should we have this cli option? We try not to have all api level option via the cli.

Yeah, I agree that this may not be desired to add as a CLI option. Happy to remove.

chrislovecnm · 2017-04-21T02:04:56Z

nodeup/pkg/model/tests/dockerbuilder/simple/cluster.yaml

@@ -21,6 +21,8 @@ spec:
  kubernetesVersion: v1.4.6
  masterInternalName: api.internal.minimal.example.com
  masterPublicName: api.minimal.example.com
+  additionalSans:
+   - proxy.api.minimal.example.com


Do we need this in the dockerbuilder test? Or do we need another test??

I'll remove this.

chrislovecnm · 2017-04-21T02:05:59Z

pkg/apis/kops/v1alpha1/cluster.go

@@ -68,6 +68,9 @@ type ClusterSpec struct {
 	// MasterInternalName is the internal DNS name for the master nodes
 	MasterInternalName string `json:"masterInternalName,omitempty"`

+	// AdditionalSANs adds additional Subject Alternate Names to apiserver cert that kops generates
+	AdditionalSANs []string `json:"additionalSans,omitempty"`


@justinsb et al ... AdditionalSANs ... is SANs a common reference. SSL and TLS are probably one of my weakest areas ... lol

chrislovecnm · 2017-04-21T02:07:22Z

pkg/apis/kops/cluster.go

@@ -67,6 +67,9 @@ type ClusterSpec struct {
 	// MasterInternalName is the internal DNS name for the master nodes
 	MasterInternalName string `json:"masterInternalName,omitempty"`

+	// AdditionalSANs adds additional Subject Alternate Names to apiserver cert that kops generates
+	AdditionalSANs []string `json:"additionalSans,omitempty"`


So this adds the API value. Where is the biz logic to add the SAN to the cert? I am probably missing it.

snoby · 2017-04-28T13:56:08Z

👍
Definitely need this.

justinsb · 2017-04-28T18:59:06Z

This looks good.. I'll just have to ponder whether additionalSANs being under spec is the right place. It feels like we should associate it with the API server, but we don't really have a block for that. We do have the api block, but .... I'm not sure. I'll think for a bit and then likely merge.

Do you mind if I move it if I decide it should go somewhere else @pdh? The top candidate in my mind is under the api block - any thoughts?

Also, additionalSANs is a little redundant, but it does seem to be what everyone else does, so :-)

pdh · 2017-04-28T19:32:23Z

Thanks for taking a look.

@justinsb definitely feel free to move it. api does seem like it might be a better place, but I defer to you all for what makes the most sense. Let me know if you'd like me to make any changes here.

chrislovecnm · 2017-04-28T20:59:16Z

@justinsb I would like the docker unit test removed, and had a couple of other questions. May be off base ;)

snoby · 2017-05-01T14:07:05Z

Love for this to be accepted, as I'm having to run with the flag --insecure-skip-tls-verify in my clusters that are being accessed through an NGINX proxy. :-(

snoby · 2017-05-08T12:42:13Z

bump

prat0318 · 2017-06-08T21:44:32Z

bump.. this will be super helpful to access apiserver from outside the network. Current stuck on this.

chrislovecnm · 2017-06-09T18:57:49Z

Once the reviews are resolved we can get this in. If the author wants someone else can branch and push the needed changes.

pdh · 2017-06-12T05:03:51Z

I think all should be addressed now. Let me know if anything else is needed.

chrislovecnm · 2017-06-12T19:50:44Z

Assigning to @justinsb as he had the last comments

bernielomax · 2017-06-27T04:16:08Z

Any chance this can be reviewed?

blakebarnett · 2017-07-03T22:30:06Z

pkg/apis/kops/v1alpha2/cluster.go

@@ -65,6 +65,9 @@ type ClusterSpec struct {
 	// MasterInternalName is the internal DNS name for the master nodes
 	MasterInternalName string `json:"masterInternalName,omitempty"`

+	// AdditionalSANs adds additional Subject Alternate Names to apiserver cert that kops generates
+	AdditionalSANs []string `json:"additionalSans,omitempty"`


why not additionalSAN? The capitalization is bit odd given there's networkCIDR.

blakebarnett · 2017-07-03T22:36:35Z

Maybe there should be a PKI section in the spec? I can imagine a few other sections like Node CSR approval and certificate expiration/revocation that could go under there also.

k8s-github-robot · 2017-08-04T00:00:12Z

@pdh PR needs rebase

blakebarnett · 2017-09-22T19:05:19Z

I think this should go into the next release.

chrislovecnm · 2017-09-22T23:30:36Z

Needs a rebase :(

llpdh · 2017-09-28T17:18:16Z

@chrislovecnm rebased 👍

chrislovecnm · 2017-09-28T20:06:06Z

@blakebarnett can you take a look at this? Let me know if you cannot

blakebarnett · 2017-09-28T20:14:04Z

My only issues are with the naming and capitalization of the JSON parameters (additionalSans vs additionalSANs). UX wise I think we should probably start moving some more of these top-level keys into logical groups.

If everyone else is happy with it as is, fine with merging, would really like to use this.

chris-h-phillips · 2017-10-27T18:37:30Z

We'd love to use this as well 👍

llpdh · 2017-11-06T21:43:12Z

@chrislovecnm @justinsb Hi folks, just giving this a nudge.

justinsb · 2017-11-07T03:51:43Z

/lgtm

k8s-github-robot · 2017-11-07T03:54:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [justinsb]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

dermyrddin · 2018-04-27T06:49:40Z

Does this also works for creating cluster through terraform?
I've just tried and it seems that certificate does not include ELB domain name.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 7, 2017

chrislovecnm reviewed Apr 21, 2017

View reviewed changes

snoby mentioned this pull request Apr 28, 2017

Additions to the master cert via a SANS entry should be possible. #2459

Closed

pdh force-pushed the additional-sans branch from 025702d to c029f27 Compare June 6, 2017 04:55

chrislovecnm assigned justinsb Jun 12, 2017

blakebarnett reviewed Jul 3, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 4, 2017

pdh force-pushed the additional-sans branch from 3fa03b4 to 1d2d020 Compare September 28, 2017 16:35

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 28, 2017

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 28, 2017

pdh force-pushed the additional-sans branch from 1d2d020 to 4a05fc5 Compare September 28, 2017 16:48

chrislovecnm assigned blakebarnett Sep 28, 2017

pdh force-pushed the additional-sans branch 3 times, most recently from 07b2529 to 14f22d0 Compare November 1, 2017 23:24

Allows additional Subject Alternate Names

fc6f33d

pdh force-pushed the additional-sans branch from 14f22d0 to fc6f33d Compare November 2, 2017 17:26

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2017

justinsb merged commit 7066368 into kubernetes:master Nov 7, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 7, 2017

ghost mentioned this pull request Nov 24, 2017

Private DNS + private topology on AWS: problems regarding the certificate #2032

Closed

blakebarnett mentioned this pull request Jan 3, 2018

Add ability to specify additional SANs for master TLS certs? #1922

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allows additional Subject Alternate Names #2063

Allows additional Subject Alternate Names #2063

pdh commented Mar 7, 2017 •

edited by k8s-reviewable

Loading

k8s-ci-robot commented Mar 7, 2017

yissachar commented Mar 8, 2017

chrislovecnm left a comment

chrislovecnm Apr 21, 2017

pdh Apr 28, 2017

chrislovecnm Apr 21, 2017

pdh Apr 28, 2017

chrislovecnm Apr 21, 2017

chrislovecnm Apr 21, 2017

snoby commented Apr 28, 2017

justinsb commented Apr 28, 2017

pdh commented Apr 28, 2017

chrislovecnm commented Apr 28, 2017

snoby commented May 1, 2017

snoby commented May 8, 2017

prat0318 commented Jun 8, 2017

chrislovecnm commented Jun 9, 2017

pdh commented Jun 12, 2017

chrislovecnm commented Jun 12, 2017

bernielomax commented Jun 27, 2017

blakebarnett Jul 3, 2017

blakebarnett commented Jul 3, 2017

k8s-github-robot commented Aug 4, 2017

blakebarnett commented Sep 22, 2017

chrislovecnm commented Sep 22, 2017

llpdh commented Sep 28, 2017

chrislovecnm commented Sep 28, 2017 •

edited

Loading

blakebarnett commented Sep 28, 2017

chris-h-phillips commented Oct 27, 2017

llpdh commented Nov 6, 2017

justinsb commented Nov 7, 2017

k8s-github-robot commented Nov 7, 2017

dermyrddin commented Apr 27, 2018

Allows additional Subject Alternate Names #2063

Allows additional Subject Alternate Names #2063

Conversation

pdh commented Mar 7, 2017 • edited by k8s-reviewable Loading

k8s-ci-robot commented Mar 7, 2017

yissachar commented Mar 8, 2017

chrislovecnm left a comment

Choose a reason for hiding this comment

chrislovecnm Apr 21, 2017

Choose a reason for hiding this comment

pdh Apr 28, 2017

Choose a reason for hiding this comment

chrislovecnm Apr 21, 2017

Choose a reason for hiding this comment

pdh Apr 28, 2017

Choose a reason for hiding this comment

chrislovecnm Apr 21, 2017

Choose a reason for hiding this comment

chrislovecnm Apr 21, 2017

Choose a reason for hiding this comment

snoby commented Apr 28, 2017

justinsb commented Apr 28, 2017

pdh commented Apr 28, 2017

chrislovecnm commented Apr 28, 2017

snoby commented May 1, 2017

snoby commented May 8, 2017

prat0318 commented Jun 8, 2017

chrislovecnm commented Jun 9, 2017

pdh commented Jun 12, 2017

chrislovecnm commented Jun 12, 2017

bernielomax commented Jun 27, 2017

blakebarnett Jul 3, 2017

Choose a reason for hiding this comment

blakebarnett commented Jul 3, 2017

k8s-github-robot commented Aug 4, 2017

blakebarnett commented Sep 22, 2017

chrislovecnm commented Sep 22, 2017

llpdh commented Sep 28, 2017

chrislovecnm commented Sep 28, 2017 • edited Loading

blakebarnett commented Sep 28, 2017

chris-h-phillips commented Oct 27, 2017

llpdh commented Nov 6, 2017

justinsb commented Nov 7, 2017

k8s-github-robot commented Nov 7, 2017

dermyrddin commented Apr 27, 2018

pdh commented Mar 7, 2017 •

edited by k8s-reviewable

Loading

chrislovecnm commented Sep 28, 2017 •

edited

Loading