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

[WIP] Set FELIX_CHAININSERTMODE default to "append" #8836

Closed
wants to merge 1 commit into from
Closed

[WIP] Set FELIX_CHAININSERTMODE default to "append" #8836

wants to merge 1 commit into from

Conversation

hakman
Copy link
Member

@hakman hakman commented Apr 3, 2020

No description provided.

@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. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 3, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: hakman
To complete the pull request process, please assign justinsb
You can assign the PR to them by writing /assign @justinsb 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 files:

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

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/api and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 3, 2020
@hakman
Copy link
Member Author

hakman commented Apr 3, 2020

@KashifSaadat do you have any thoughts on making "append" the default for FELIX_CHAININSERTMODE in Kops?

With the current default "insert", one of the e2e tests fails:

[sig-network] Services should be rejected when no endpoints exist

@KashifSaadat
Copy link
Contributor

Hey @hakman, that suggests to me an error with the e2e tests / network policy misconfiguration.

Generally the default should always be left as insert, otherwise with other services potentially injecting iptables rules on a node you can run into a situation where calico chains are not hit, resulting in other issues (e.g. all traffic bypassing k8s network policies and accepted by default). I actually ran into this case in #4037.

@hakman
Copy link
Member Author

hakman commented Apr 3, 2020 •
edited
Loading

Hey @KashifSaadat:). Thanks for looking at this.
I saw some of the discussions related to that, but didn't know the whole context.
e2e tests pass with "append", but indeed may be risky.

For reference, there are a few issues related to this failing test and thought this may be an easy fix.
projectcalico/calico#1055
kubernetes/kubernetes#78994

@KashifSaadat
Copy link
Contributor

Aha I see, thanks for the context I'll have a read through those issues to understand it better!

@hakman
Copy link
Member Author

hakman commented Apr 3, 2020

No rush. Has been like that for years :).

johngmyers
johngmyers reviewed Apr 4, 2020
View reviewed changes
cmd/kops/create_cluster.go
@@ -174,7 +174,7 @@ func (o *CreateClusterOptions) InitDefaults() {
o.Yes = false
o.Target = cloudup.TargetDirect
o.Models = strings.Join(cloudup.CloudupModels, ",")
o.Networking = "kubenet"
o.Networking = "calico"
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be an unrelated change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Pretty much the only way to test if this fixes the failing e2e test.

@k8s-ci-robot
Copy link
Contributor

@hakman: PR needs rebase.

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.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 6, 2020
@hakman
Copy link
Member Author

hakman commented Apr 12, 2020

Seems that it's not worth the risk. Maybe there will be a better solution in the future.
kubernetes/kubernetes#78994 (comment)

@hakman hakman closed this Apr 12, 2020
@hakman hakman deleted the calico-append branch April 30, 2020 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johngmyers johngmyers johngmyers left review comments

@joshbranham joshbranham Awaiting requested review from joshbranham

@robinpercy robinpercy Awaiting requested review from robinpercy

Assignees
No one assigned
Labels
area/api area/documentation cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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.
4 participants
@hakman @k8s-ci-robot @KashifSaadat @johngmyers