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

Fix NLB icmp permission duplication #56759

Merged
merged 1 commit into from Jan 9, 2018

Conversation

@aledbf
Member

aledbf commented Dec 3, 2017

What this PR does / why we need it:

Fixes an issue with the ICMP rule for MTU during the creation of a NLB

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

Fixes #56703

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Dec 3, 2017

@aledbf: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#write-release-notes-if-needed.

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.

@aledbf

This comment has been minimized.

Member

aledbf commented Dec 3, 2017

/assign @micahhausler

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Dec 3, 2017

@aledbf: GitHub didn't allow me to assign the following users: sig-aws.

Note that only kubernetes members can be assigned.

In response to this:

/assign sig-aws

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.

@aledbf

This comment has been minimized.

Member

aledbf commented Dec 3, 2017

/sig aws

@k8s-ci-robot k8s-ci-robot added the sig/aws label Dec 3, 2017

@aledbf

This comment has been minimized.

Member

aledbf commented Dec 3, 2017

/assign @micahhausler

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Dec 3, 2017

@aledbf: GitHub didn't allow me to assign the following users: micahhausler.

Note that only kubernetes members can be assigned.

In response to this:

/assign @micahhausler

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.

@aledbf

This comment has been minimized.

Member

aledbf commented Dec 3, 2017

/test pull-kubernetes-unit

@aledbf

This comment has been minimized.

Member

aledbf commented Dec 4, 2017

@micahhausler

LGTM! Thanks for doing this!

adds = append(adds, permission)
} else {
removes = append(removes, permission)
if !icmpPermAdded {

This comment has been minimized.

@micahhausler

micahhausler Dec 4, 2017

Member

Small nit: you could probably move the mtuRuleAnnotation into this block

This comment has been minimized.

@aledbf

aledbf Dec 4, 2017

Member

done

@aledbf aledbf force-pushed the aledbf:fix-nlb-icmp branch from 2c7024f to 51719a3 Dec 4, 2017

@aledbf

This comment has been minimized.

Member

aledbf commented Dec 4, 2017

/test pull-kubernetes-unit

@aledbf

This comment has been minimized.

Member

aledbf commented Dec 4, 2017

/assign @justinsb

@aledbf

This comment has been minimized.

Member

aledbf commented Dec 4, 2017

/test pull-kubernetes-unit

1 similar comment
@aledbf

This comment has been minimized.

Member

aledbf commented Dec 4, 2017

/test pull-kubernetes-unit

if add {
adds = append(adds, permission)
} else {
removes = append(removes, permission)

This comment has been minimized.

@justinsb

justinsb Dec 4, 2017

Member

If we have 2 ports and we remove one, we don't want to remove the MTU rule, do we?

I suspect the MTU rule belongs outside the port changes loop.

@aledbf aledbf force-pushed the aledbf:fix-nlb-icmp branch from 51719a3 to 6e4621c Dec 4, 2017

if len(adds) > 0 {
adds = append(adds, mtuPermission)

This comment has been minimized.

@micahhausler

micahhausler Dec 4, 2017

Member

This is not going to work, if a service port was changed from 80 to 8080, you'd get both an add and remove.

The ICMP rules really need their own tracking structure, you could probably do something similar to the
portChanges map:

// securityGroupID, clientCIDR, add/remove
icmpPortChanges := map[string]map[string]bool{}

You'd need to

  • get the desired state (which is just an ICMP for each clientCidr for each security group)
  • get the actual state (probably just reuse the for _, actualGroup := range actualGroups loop)
  • append to adds/ removes as necessary

@aledbf aledbf force-pushed the aledbf:fix-nlb-icmp branch from 6e4621c to 41011df Dec 13, 2017

@kubernetes kubernetes deleted a comment from k8s-merge-robot Dec 13, 2017

@aledbf aledbf force-pushed the aledbf:fix-nlb-icmp branch from 41011df to bea2d8d Dec 13, 2017

@aledbf

This comment has been minimized.

Member

aledbf commented Dec 14, 2017

/test pull-kubernetes-e2e-gce-device-plugin-gpu

@aledbf

This comment has been minimized.

Member

aledbf commented Dec 14, 2017

/test pull-kubernetes-e2e-kops-aws

@micahhausler

This looks good tome
/lgtm

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Dec 14, 2017

@micahhausler: changing LGTM is restricted to assignees, and only kubernetes org members may be assigned issues.

In response to this:

This looks good tome
/lgtm

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.

@aledbf

This comment has been minimized.

Member

aledbf commented Dec 17, 2017

ping @justinsb

@justinsb

This comment has been minimized.

Member

justinsb commented Dec 19, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Dec 19, 2017

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Dec 19, 2017

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, justinsb, micahhausler

Associated issue: #56703

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

@justinsb

This comment has been minimized.

Member

justinsb commented Dec 19, 2017

Thanks @aledbf !

@aledbf

This comment has been minimized.

Member

aledbf commented Dec 19, 2017

/test pull-kubernetes-e2e-kops-aws

@micahhausler

This comment has been minimized.

Member

micahhausler commented Dec 19, 2017

@justinsb How do we get this into the release-1.9 branch?

@aledbf

This comment has been minimized.

Member

aledbf commented Dec 24, 2017

/test pull-kubernetes-unit

1 similar comment
@aledbf

This comment has been minimized.

Member

aledbf commented Dec 24, 2017

/test pull-kubernetes-unit

@jsafrane

This comment has been minimized.

Member

jsafrane commented Jan 9, 2018

/release-note-none

@jsafrane

This comment has been minimized.

Member

jsafrane commented Jan 9, 2018

How do we get this into the release-1.9 branch?

Fill release note or tell the bot there is none.

@aledbf

This comment has been minimized.

Member

aledbf commented Jan 9, 2018

/test pull-kubernetes-verify

@fejta-bot

This comment has been minimized.

fejta-bot commented Jan 9, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Jan 9, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Jan 9, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit eff07b6 into kubernetes:master Jan 9, 2018

12 of 13 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-verify
Details
cla/linuxfoundation aledbf authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke-gci Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@rajatjindal

This comment has been minimized.

Contributor

rajatjindal commented Apr 24, 2018

Hi @aledbf , do we plan to merge this to 1.9.x? we are running 1.9.6, and running into this issue

@aledbf

This comment has been minimized.

Member

aledbf commented Apr 24, 2018

@rajatjindal I don't think so. Please check all the comments asking the same thing.

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