Skip to content

Bug 1800746: baremetal: only respond to dhcp for control plane mac's #3079

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
stbenjam:dhcp-allowlist
Feb 19, 2020
Merged

Bug 1800746: baremetal: only respond to dhcp for control plane mac's #3079
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
stbenjam:dhcp-allowlist

Conversation

@stbenjam
Copy link
Copy Markdown
Member

@stbenjam stbenjam commented Feb 7, 2020

Note: This is solving a similar problem for baremetal as
openshift/machine-config-operator#1421 did for MCO.

The bootstrap can now co-exist with machine-api being online. That
means there could be an instance of Ironic, dnsmasq, etc running in
both the cluster and the bootstrap. This causes problems, as it's not
deterministic which dnsmasq instance the worker provisioned by the
machine-api will use. If it uses the bootstrap, then the worker will not
come online.

This is causing a percentage of baremetal installs to fail, with the
worker being offline, ingress and other operators never come up.

This change blocks dhcp requests from anything but control plane hosts,
using iptables. DHCPv6 relies on DUID's instead which makes things more
complicated to use dnsmasq's dhcp-host abilities, which prefers DUIDS
for IPv6.

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 7, 2020
@stbenjam
Copy link
Copy Markdown
Member Author

stbenjam commented Feb 7, 2020

/label platform/baremetal

@openshift-ci-robot openshift-ci-robot added the platform/baremetal IPI bare metal hosts platform label Feb 7, 2020
@stbenjam stbenjam changed the title [WIP] baremetal: provide dnsmasq with allowlist for control plane mac's baremetal: provide dnsmasq with allowlist for control plane mac's Feb 7, 2020
@openshift-ci-robot openshift-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 Feb 7, 2020
@stbenjam

This comment has been minimized.

@stbenjam
Copy link
Copy Markdown
Member Author

stbenjam commented Feb 7, 2020

/retest

@stbenjam
Copy link
Copy Markdown
Member Author

stbenjam commented Feb 7, 2020

/cc @sadasu @hardys @dhellmann @wking

Result of our discussion on Slack.

@metal3ci
Copy link
Copy Markdown

metal3ci commented Feb 7, 2020

Build SUCCESS, see build http://10.8.144.11:8080/job/dev-tools/1499/

@stbenjam
Copy link
Copy Markdown
Member Author

stbenjam commented Feb 7, 2020

/retitle Bug 1800746: baremetal: provide dnsmasq with allowlist for control plane mac's

@openshift-ci-robot openshift-ci-robot changed the title baremetal: provide dnsmasq with allowlist for control plane mac's Bug 1800746: baremetal: provide dnsmasq with allowlist for control plane mac's Feb 7, 2020
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

@stbenjam: This pull request references Bugzilla bug 1800746, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Bug 1800746: baremetal: provide dnsmasq with allowlist for control plane mac's

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.

@openshift-ci-robot openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Feb 7, 2020
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

@stbenjam: This pull request references Bugzilla bug 1800746, which is valid.

Details

In response to this:

Bug 1800746: baremetal: provide dnsmasq with allowlist for control plane mac's

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.

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 14, 2020
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 14, 2020
@metal3ci
Copy link
Copy Markdown

Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/1503/

@derekhiggins
Copy link
Copy Markdown
Contributor

Seems to be working, iptables rules are created and seem to be catching DHCP packets

[root@localhost core]# iptables-save -c | grep -i DHCP                                                                                                                                                             
:DHCP - [0:0]
[71:24812] -A PREROUTING -p udp -m udp --dport 67 -j DHCP
[0:0] -A PREROUTING -p udp -m udp --dport 546 -j DHCP
[18:6698] -A DHCP -m mac --mac-source 00:5D:B3:59:26:EF -j ACCEPT
[16:5706] -A DHCP -m mac --mac-source 00:5D:B3:59:26:F3 -j ACCEPT
[19:6918] -A DHCP -m mac --mac-source 00:5D:B3:59:26:F7 -j ACCEPT
[18:5490] -A DHCP -j DROP

@stbenjam
Copy link
Copy Markdown
Member Author

/hold cancel

@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 Feb 18, 2020
@stbenjam
Copy link
Copy Markdown
Member Author

/cc @hardys PTAL

@openshift-ci-robot
Copy link
Copy Markdown
Contributor

@stbenjam: GitHub didn't allow me to request PR reviews from the following users: PTAL.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @hardys PTAL

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.

@metal3ci
Copy link
Copy Markdown

Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/1508/

@metal3ci
Copy link
Copy Markdown

Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/1509/

@openshift-ci-robot
Copy link
Copy Markdown
Contributor

@stbenjam: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-openstack 01f8849 link /test e2e-openstack
ci/prow/e2e-aws-upgrade 01f8849 link /test e2e-aws-upgrade
ci/prow/e2e-libvirt 01f8849 link /test e2e-libvirt
ci/prow/e2e-aws-scaleup-rhel7 01f8849 link /test e2e-aws-scaleup-rhel7

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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.

@hardys
Copy link
Copy Markdown

hardys commented Feb 18, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 18, 2020
@hardys
Copy link
Copy Markdown

hardys commented Feb 19, 2020

/approve

@hardys
Copy link
Copy Markdown

hardys commented Feb 19, 2020

Hmm seems we're missing an OWNERS file entry for one of the files touched?

@stbenjam
Copy link
Copy Markdown
Member Author

Hm, every dir should have it. I fixed the missing ones a few days ago. Maybe it’s not looking at https://github.com/openshift/installer/blob/master/data/data/bootstrap/baremetal/OWNERS for startironic.sh?

@sdodson
Copy link
Copy Markdown
Member

sdodson commented Feb 19, 2020

/approve

Need the baremetal OWNERS in installer/pkg/asset/ignition/bootstrap/baremetal/

@sdodson
Copy link
Copy Markdown
Member

sdodson commented Feb 19, 2020

/overide ci/prow/e2e-aws-upgrade

@openshift-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hardys, sdodson

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

The pull request process is described here

Details 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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 19, 2020
@sdodson
Copy link
Copy Markdown
Member

sdodson commented Feb 19, 2020

/override ci/prow/e2e-aws-upgrade

@openshift-ci-robot
Copy link
Copy Markdown
Contributor

@sdodson: Overrode contexts on behalf of sdodson: ci/prow/e2e-aws-upgrade

Details

In response to this:

/override ci/prow/e2e-aws-upgrade

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.

@sdodson
Copy link
Copy Markdown
Member

sdodson commented Feb 19, 2020

/override ci/prow/e2e-aws

@openshift-bot
Copy link
Copy Markdown
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@stbenjam
Copy link
Copy Markdown
Member Author

/cherry-pick release-4.4

@openshift-cherrypick-robot
Copy link
Copy Markdown

@stbenjam: once the present PR merges, I will cherry-pick it on top of release-4.4 in a new PR and assign it to you.

Details

In response to this:

/cherry-pick release-4.4

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.

@openshift-merge-robot openshift-merge-robot merged commit 21740e0 into openshift:master Feb 19, 2020
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

@stbenjam: All pull requests linked via external trackers have merged. Bugzilla bug 1800746 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1800746: baremetal: only respond to dhcp for control plane mac's

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.

@openshift-cherrypick-robot
Copy link
Copy Markdown

@stbenjam: new pull request created: #3138

Details

In response to this:

/cherry-pick release-4.4

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.

@yuvalk
Copy link
Copy Markdown
Contributor

yuvalk commented Feb 19, 2020

FWIW - This PR looks good on my BM env (1master+2workers)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged. platform/baremetal IPI bare metal hosts platform size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants