Skip to content

Conversation

@papanito
Copy link

Fixes #2319

@jetstack-bot jetstack-bot added the dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. label Apr 26, 2020
@jetstack-bot jetstack-bot requested a review from JoshVanL April 26, 2020 18:32
@jetstack-bot jetstack-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 26, 2020
@jetstack-bot
Copy link
Contributor

Hi @papanito. Thanks for your PR.

I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@jetstack-bot jetstack-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Apr 26, 2020
@papanito
Copy link
Author

@JoshVanL did you already had a chance to look at my PR?

@papanito
Copy link
Author

papanito commented Jun 4, 2020

Hi @JoshVanL, @munnerz this pr is open since quite a while. Do you mind check it?

@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 3, 2020
Copy link
Member

@wallrj wallrj left a comment

Choose a reason for hiding this comment

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

Thanks for this @papanito

And sorry for the not replying sooner.

There are some merge conflicts right now.
Will you have some time to rebase the branch?
If not, I'll do it and hopefully we can get this merged finally.

@jetstack-bot jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 19, 2020
@papanito
Copy link
Author

@wallrj can you have a look?

@wallrj
Copy link
Member

wallrj commented Dec 21, 2020

/ok-to-test

@jetstack-bot jetstack-bot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 21, 2020
Copy link
Member

@wallrj wallrj left a comment

Choose a reason for hiding this comment

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

Thanks @papanito

I forgot to add the /ok-to-test tag in the last review, and now I see that there are some spellcheck errors ....which are difficult to see because the CI output isn't syntax highlighted.

You'll need to add iptables, nftables, distros RHEL etc to the .spelling file in the repo root.

You can also run make verify locally to get clearer spell check output.

(ignore all the grammar and other suggestions that it spits out).

Comment on lines 12 to 18
- [GKE](#gke)
- [AWS EKS](#aws-eks)
- [Webhook](#webhook)
- [iptables vs. nftables](#iptables-vs-nftables)
- [Switch to `iptables`](#switch-to-iptables)
- [Configure network provider](#configure-network-provider)
- [Calico](#calico)
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this extra ToC.
There is already a document ToC on the right which is maintained automatically.

Copy link
Author

Choose a reason for hiding this comment

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

Removed the toc, please have a look again

@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: papanito
To complete the pull request process, please assign munnerz
You can assign the PR to them by writing /assign @munnerz in a comment when ready.

The full list of commands accepted by this bot can be found 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

@jetstack-bot jetstack-bot added dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. and removed dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. labels Dec 21, 2020
Fixes #2319

Signed-off-by: papanito <papanito@wyssmann.com>
@jetstack-bot jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Dec 21, 2020
@jetstack-bot
Copy link
Contributor

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

Test name Commit Details Rerun command
pull-cert-manager-website-verify 2985243 link /test pull-cert-manager-website-verify

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.

@papanito
Copy link
Author

Just read your comment, will do the missing stuff as well

@wallrj
Copy link
Member

wallrj commented Dec 22, 2020

Hey @papanito

Thanks for the updates. I discussed this with the team at standup this morning and it was mentioned that we should link to the Kubernetes website documentation on nftables compatibility ...but when I went looking I found that they've removed it from the doc: kubernetes/website#19773 because kube-proxy / kubeadm now support nftables (since Kubernetes 1.17)...algthough they didn't remove the documentation until 1.18.

So perhaps we should simply link to the original Kubernetes docs for 1.17 and note that this workaround is only necessary on K8S < 1.17?

The docs for 1.17 add one extra command than in the 1.16 docs:

# ensure legacy binaries are installed
sudo apt-get install -y iptables arptables ebtables
...

Anyway, the consensus is that we should try to avoid duplicating workarounds that are already in the Kubernetes docs because ours will likely get out of date. Let me know what you think?

@papanito
Copy link
Author

should try to avoid duplicating workarounds that are already in the Kubernetes docs

Fully agree. I will give it a try and improve my changes...

@wallrj
Copy link
Member

wallrj commented Feb 5, 2021

Hey @papanito I'll close this PR now, but if you have time, please followup with a shorter PR which notes the nftables problem and which versions of Kubernetes are affected, with a link to the K8S documented workaround for those versions.

Thanks.

@wallrj wallrj closed this Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. ok-to-test 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.

3 participants