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.

Sign up for GitHub

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

Jump to bottom

gce: Add support for clusters without DNS #14769

Merged
merged 2 commits into from
Dec 16, 2022
Merged

gce: Add support for clusters without DNS #14769

merged 2 commits into from
Dec 16, 2022

Conversation

hakman
Copy link
Member

@hakman hakman commented Dec 11, 2022 •
edited
Loading

Quick version of --dns=none for GCE.

Refs #14859

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 11, 2022
@k8s-ci-robot k8s-ci-robot added area/api area/provider/gcp Issues or PRs related to gcp provider size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 11, 2022
@hakman
Copy link
Member Author

hakman commented Dec 11, 2022

/cc @justinsb
/assign @justinsb

@hakman hakman changed the title WIP gce: Add support for clusters without DNS gce: Add support for clusters without DNS Dec 13, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 13, 2022
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 14, 2022
@hakman
Copy link
Member Author

hakman commented Dec 14, 2022

/retest

justinsb
justinsb approved these changes Dec 15, 2022
View reviewed changes
pkg/model/gcemodel/api_loadbalancer.go
TargetPool: targetPool,
IPAddress: ipAddress,
IPProtocol: "TCP",
})
if b.Cluster.UsesNoneDNS() {
c.AddTask(&gcetasks.ForwardingRule{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may be able to lock this down e.g. only to the nodes... We/I can try that separately though!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My idea is that, we could add another internal LB for this purpose, making it secure. Though, that would diverge from the original concept and a refactor would be needed to return the IP(s) for kops-controller separately from API ones.
My goal in this PR was to get it supported as simple as possible. To the very least, this gives us a chance to add some tests and see how the output changes later.

pkg/model/gcemodel/api_loadbalancer.go
})
if b.Cluster.UsesNoneDNS() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this? This would be for e.g. humans running kubectl?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite pretty, but with Public LB, the nodes need access to it and this was the best idea at the time except for allowing all traffic.

upup/pkg/fi/cloudup/apply_cluster.go
break
}
}
if bootConfig.APIServerIP != "" {
case kops.CloudProviderGCE:
// Use any IP address that is found (including public ones)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (for a future PR): We probably should just have nodeup accept a list of IPs, probably easier than choosing!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this a little. Would be nice to do the choosing on nodeup side. For sure it would be better equipped to find the best bootstrap address for itself.

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 15, 2022
@justinsb
Copy link
Member

This looks good. One quibble about whether we need to open the ports on the Load Balancers or can restrict it a bit more, but we can likely merge this and then try locking it down.

/approve
/lgtm

/hold in case you want to address why we need to open the kops-controller port to the kubernetesAPIAccessRange - I don't understand that, I don't think it necessarily includes the nodes! But if not, no worries, we can look at this as part of follow-on

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 15, 2022
@k8s-ci-robot
Copy link
Contributor

[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.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@hakman
Copy link
Member Author

hakman commented Dec 16, 2022

Thanks for the review @justinsb. I also agree about the firewall rule for kops-controller. Hopefully we can find a solution there and implement it in a separate PR.
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2022
@k8s-ci-robot k8s-ci-robot merged commit 0896ea2 into kubernetes:master Dec 16, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Dec 16, 2022
@hakman hakman deleted the gce_none_dns branch December 16, 2022 03:32
@hakman hakman mentioned this pull request Dec 22, 2022
Merged
@hakman hakman mentioned this pull request Jan 14, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinsb justinsb justinsb approved these changes

@johngmyers johngmyers Awaiting requested review from johngmyers

@olemarkus olemarkus Awaiting requested review from olemarkus

Assignees

@justinsb justinsb

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/api area/provider/gcp Issues or PRs related to gcp provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.26
Development

Successfully merging this pull request may close these issues.
3 participants
@hakman @justinsb @k8s-ci-robot