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

Allow egress-router to connect to cluster service network for DNS, etc. #20094

Merged
merged 1 commit into from
Jun 26, 2018
Merged

Allow egress-router to connect to cluster service network for DNS, etc. #20094

merged 1 commit into from
Jun 26, 2018

Conversation

pravisankar
Copy link

@pravisankar pravisankar commented Jun 25, 2018 •
edited
Loading

#19885 allows egress routers to
connect to the node IP but when openshift node is configured to use
service network IP as DNS IP then egress router pod will not be able
to resolve DNS requests. This change will address this issue.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1594958

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 25, 2018
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 25, 2018
@pravisankar
Copy link
Author

pravisankar commented Jun 25, 2018 •
edited
Loading

/hold
For 'oc adm create-node-config' do we expect --dns-ip flag to have IP other than cluster service network in any case?

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 25, 2018
@pravisankar
Copy link
Author

@openshift/sig-networking PTAL

danwinship
danwinship suggested changes Jun 25, 2018
View reviewed changes
Copy link
Contributor

@danwinship danwinship left a comment

Choose a reason for hiding this comment

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

yeah, looks good other than where you copied pre-existing lameness

pkg/network/sdn-cni-plugin/openshift-sdn.go Outdated
@@ -213,11 +213,23 @@ func (p *cniPlugin) CmdAdd(args *skel.CmdArgs) error {
},
Gw: defaultGW,
}
err = netlink.RouteAdd(route)
if err != nil {
if err := netlink.RouteAdd(route); err != nil {
return fmt.Errorf("failed to configure macvlan device: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

let's fix up the lame error messages here, for future debugging purposes. "failed to add route to node IP: %v"

pkg/network/sdn-cni-plugin/openshift-sdn.go Outdated
// Add a route to service network via SDN
_, serviceIPNet, err := net.ParseCIDR(config.ServiceNetworkCIDR)
if err != nil {
return fmt.Errorf("failed to configure macvlan device: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't really matter much since it should be "can't happen", but, "failed to parse ServiceNetworkCIDR: %v"

pkg/network/sdn-cni-plugin/openshift-sdn.go Outdated
Gw: defaultGW,
}
if err := netlink.RouteAdd(route); err != nil {
return fmt.Errorf("failed to configure macvlan device: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

"failed to add route to service network: %v"

@pravisankar pravisankar force-pushed the egress-router-add-svc-route branch from f25605a to d7c576b Compare June 25, 2018 20:37
@pravisankar
Copy link
Author

/retest

Allow egress-router to connect to cluster service network for DNS, etc.
339dccb 
openshift#19885 allows egress routers to
connect to the node IP but when openshift node is configured to use
service network IP as DNS IP then egress router pod will not be able
to resolve DNS requests. This change will address this issue.
@pravisankar pravisankar force-pushed the egress-router-add-svc-route branch from d7c576b to 339dccb Compare June 25, 2018 21:28
@imcsk8
Copy link
Contributor

imcsk8 commented Jun 26, 2018

code looks good

knobunc
knobunc approved these changes Jun 26, 2018
View reviewed changes
Copy link
Contributor

@knobunc knobunc left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@danwinship
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, pravisankar

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

@knobunc
Copy link
Contributor

knobunc commented Jun 26, 2018

@pravisankar Do you want to have this go in to 3.10? If so, you need to pull the hold today... are we concerned that there is risk introduced by this?

@pravisankar
Copy link
Author

@knobunc @danwinship @imcsk8
Reading more on '--dns-ip' option to openshift node config,
(1) DNS IP address is used by pods to access cluster DNS and is defaulted to service IP of the k8s.
(2) When running more complex local DNS configurations, DNS IP is often set to the local address of a DNS proxy like dnsmasq, which then will consult either the local DNS or the master DNS.

So this PR only handles (1) but not (2). To cover all the cases, we really need to pass DNS IP to SDN and to the sdn-cni-plugin and then need to add a route to DNS IP via SDN.

@pravisankar
Copy link
Author

/hold cancel
(2) in previous comment will be handled in a separate pr.

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 26, 2018
@knobunc
Copy link
Contributor

knobunc commented Jun 26, 2018

I'm okay with handling (2) above as a separate PR. Can you add a bug or card to track it please?

@openshift-merge-robot openshift-merge-robot merged commit db3845c into openshift:master Jun 26, 2018
@pravisankar
Copy link
Author

/cherrypick release-3.10

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Jun 26, 2018
Merged
@openshift-cherrypick-robot
Copy link

@pravisankar: new pull request created: #20102

In response to this:

/cherrypick release-3.10

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.

@pravisankar pravisankar mentioned this pull request Jun 27, 2018
Merged
@danwinship danwinship mentioned this pull request Dec 17, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danwinship danwinship danwinship approved these changes

@knobunc knobunc knobunc approved these changes

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

@rajatchopra rajatchopra Awaiting requested review from rajatchopra

Assignees

@danwinship danwinship

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. sig/networking size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@pravisankar @imcsk8 @danwinship @openshift-ci-robot @knobunc @openshift-cherrypick-robot @openshift-merge-robot