Skip to content
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

Add --master-public-name argument to kops create_cluster #3385

Merged
merged 3 commits into from
Nov 7, 2017
Merged

Add --master-public-name argument to kops create_cluster #3385

merged 3 commits into from
Nov 7, 2017

Conversation

mdavidsen
Copy link
Contributor

Hi,

As outlined in #3384 I've created the following patch, and would like to get some feedback.

This PR exposes the MasterPublicName from ClusterSpec (pkg/apis/kops/cluster.go) to cmd/kops/create_cluster.go. This solves the issues mentioned in #3384

The changes are minimal and should not break anything as far as I can tell.

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
We suggest the following additional approver: zmerlynn

Assign the PR to them by writing /assign @zmerlynn in a comment when ready.

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

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

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

@k8s-ci-robot
Copy link
Contributor

Hi @mdavidsen. 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 /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.

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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 15, 2017
@mdavidsen mdavidsen changed the title [RFC] Add --master-public-name argument to kops create_cluster Add --master-public-name argument to kops create_cluster Sep 15, 2017
@mdavidsen
Copy link
Contributor Author

/assign @zmerlynn

@justinsb
Copy link
Member

So this is great - I just want to look more at the use case, because we do we want to support pointing a real name at a cluster using gossip DNS, but I wonder if we should also support doing a real TLS certificate (whether LetsEncrypt or externally issued). One way to do that might be to put it into the AccessSpec section of the API, though that might just be overcomplicated as the field you're using feels like it doesn't have another use. That said, there's probably a bug in that kops edit should work anyway.

So I'm going to try to look at this in 1.8, and likely merge this - thank you!

BTW if you haven't seen it, we now have an extension mechanism for short-circuiting the kops create/kops edit dance - you can pass an --override flag. The intention is that will allow specification of any field(s), but currently the implementation is hard-coded to just one field path while we figure out the right approach to make this generic: https://github.com/kubernetes/kops/blob/master/cmd/kops/create_cluster.go#L1073-L1074

Anyway, if you wanted to add an override field in there I would merge that immediately (cluster.spec.masterPublicName), and eventually I'll figure out how to make it generic and you'll be able to use that approach anyway. But whatever approach we decide is best for this in general, we should use the flag for - I think it's likely your way, I just want to retrace your steps and better understand :-)

@justinsb justinsb assigned justinsb and unassigned zmerlynn Sep 25, 2017
@blakebarnett
Copy link

blakebarnett commented Sep 25, 2017

Ah, didn't realize this was for a gossip cluster. Either way, I think if we allow setting a field named like this, it should work for all cases (private/public/gossip) and manage DNS entries appropriately, which is why I said we need #1919 to work first.

@k8s-github-robot
Copy link

@mdavidsen PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 5, 2017
@justinsb justinsb merged commit 9298547 into kubernetes:master Nov 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants