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.

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

Service Internal Traffic Policy #96600

Merged
merged 3 commits into from Mar 8, 2021

Conversation

maplain
Copy link

@maplain maplain commented Nov 16, 2020

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR implements the Service Internal Traffic Policy KEP

Which issue(s) this PR fixes:
Fixes kubernetes/enhancements#2086

Special notes for your reviewer:

Does this PR introduce a user-facing change?:
Yes

One new field "InternalTrafficPolicy" in Service is added.
It specifies if the cluster internal traffic should be routed to all endpoints or node-local endpoints only.
"Cluster" routes internal traffic to a Service to all endpoints.
"Local" routes traffic to node-local endpoints only, and traffic is dropped if no node-local endpoints are ready.
The default value is "Cluster".

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

TBD

@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. release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 16, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @maplain. Thanks for your PR.

I'm waiting for a kubernetes 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.

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-priority Indicates a PR lacks a `priority/foo` label and requires one. label Nov 16, 2020
@k8s-ci-robot k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 16, 2020
@andrewsykim
Copy link
Member

/triage accepted
/milestone v1.21
/assign

Thanks for starting this @maplain!

@k8s-ci-robot k8s-ci-robot added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Nov 16, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Nov 16, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Nov 16, 2020
pkg/apis/core/types.go Outdated Show resolved Hide resolved
pkg/apis/core/types.go Outdated Show resolved Hide resolved
@andrewsykim
Copy link
Member

andrewsykim commented Nov 30, 2020

I know this PR is a WIP, but some comments for what is still required:

  1. We need a ServiceInternalTrafficPolicy feature gate in https://github.com/kubernetes/kubernetes/blob/master/pkg/features/kube_features.go that is alpha status and off by default.
  2. We need to drop the new field in the Service REST code when the feature gate is off. See spec.allocateLoadBalancerNodePorts as one example: https://github.com/kubernetes/kubernetes/blob/master/pkg/registry/core/service/strategy.go#L185-L190
  3. We need to set the default value to Cluster if the feature gate is enabeld, see SetDefaults_Service: https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/core/v1/defaults.go#L97
  4. kube-proxy changes

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 2, 2020
staging/src/k8s.io/api/core/v1/types.go Outdated Show resolved Hide resolved
pkg/apis/core/types.go Outdated Show resolved Hide resolved
pkg/registry/core/service/strategy.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 4, 2020
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 6, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 6, 2021
@maplain
Copy link
Author

maplain commented Mar 6, 2021

/test pull-kubernetes-verify

@thockin
Copy link
Member

thockin commented Mar 8, 2021

Needs rebase

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2021
Fangyuan Li added 2 commits March 7, 2021 16:52
1. Add API definitions;
2. Add feature gate and drops the field when feature gate is not on;
3. Set default values for the field;
4. Add API Validation
5. add kube-proxy iptables and ipvs implementations
6. add tests
Fields:
1. rename onlyNodeLocalEndpoints to nodeLocalExternal;
2. rename onlyNodeLocalEndpointsForInternal to nodeLocalInternal;
Methods:
1. rename OnlyNodeLocalEndpoints to NodeLocalExternal;
2. rename OnlyNodeLocalEndpointsForInternal to NodeLocalInternal;
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2021
@JornShen
Copy link
Member

JornShen commented Mar 8, 2021

for failed check pull-kubernetes-verify, need to run hack/update-openapi-spec.sh

@maplain
Copy link
Author

maplain commented Mar 8, 2021

@JornShen I've tried to run it a few times, but it doesn't update anything locally.. I actually run FORCE_ALL=true make update every time before I push to make sure it is in sync.

let me try it one more time

@maplain
Copy link
Author

maplain commented Mar 8, 2021

➜  kubernetes git:(internal-traffic-policy) hack/update-openapi-spec.sh
+++ [0307 18:48:44] Building go targets for darwin/amd64:
    ./vendor/k8s.io/code-generator/cmd/prerelease-lifecycle-gen
Generating prerelease lifecycle code for       26 targets
+++ [0307 18:48:51] Building go targets for darwin/amd64:
    ./vendor/k8s.io/code-generator/cmd/deepcopy-gen
Generating deepcopy code for      226 targets
+++ [0307 18:49:03] Building go targets for darwin/amd64:
    ./vendor/k8s.io/code-generator/cmd/defaulter-gen
Generating defaulter code for       88 targets
+++ [0307 18:49:14] Building go targets for darwin/amd64:
    ./vendor/k8s.io/code-generator/cmd/conversion-gen
Generating conversion code for      123 targets
+++ [0307 18:49:37] Building go targets for darwin/amd64:
    ./vendor/k8s.io/kube-openapi/cmd/openapi-gen
Generating openapi code for KUBE
Generating openapi code for AGGREGATOR
Generating openapi code for APIEXTENSIONS
^A^KGenerating openapi code for CODEGEN
Generating openapi code for SAMPLEAPISERVER
+++ [0307 18:49:52] Building go targets for darwin/amd64:
    ./vendor/github.com/go-bindata/go-bindata/go-bindata
+++ [0307 18:49:54] Building go targets for darwin/amd64:
    cmd/kube-apiserver
etcd --advertise-client-urls http://127.0.0.1:2379 --data-dir /var/folders/l5/66tl8_29065g0jrk7t92qdjm0000gn/T/tmp.vlrfbPAq --listen-client-urls http://127.0.0.1:2379 --log-level=debug > "/dev/null" 2>/dev/null
Waiting for etcd to come up.
+++ [0307 18:51:25] On try 3, etcd: : {"health":"true"}
{"header":{"cluster_id":"14841639068965178418","member_id":"10276657743932975437","revision":"2","raft_term":"2"}}+++ [0307 18:51:25] Starting kube-apiserver
+++ [0307 18:51:34] On try 9, apiserver: : ok
+++ [0307 18:51:34] Updating
    hack/../api/openapi-spec
+++ [0307 18:51:35] SUCCESS
+++ [0307 18:52:35] Clean up complete


➜  kubernetes git:(internal-traffic-policy) gst
On branch internal-traffic-policy
Your branch is up to date with 'my/internal-traffic-policy'.

nothing to commit, working tree clean

@JornShen is it possible that this is related to the go version?

go version go1.16 darwin/amd64

@maplain
Copy link
Author

maplain commented Mar 8, 2021

/test pull-kubernetes-e2e-gce-alpha-features

@maplain
Copy link
Author

maplain commented Mar 8, 2021

/test pull-kubernetes-verify

@JornShen
Copy link
Member

JornShen commented Mar 8, 2021

@maplain using linux robot will be ok.

JornShen@c1a7b78

I have upload it to my repo. You can cherry-pick it, maybe.

1. `git clean -fxd`
2. `make update`
@andrewsykim
Copy link
Member

Go version seems right, I think we're on 1.16

@thockin
Copy link
Member

thockin commented Mar 8, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 8, 2021
@maplain
Copy link
Author

maplain commented Mar 8, 2021

Thanks @thockin for approval, and @andrewsykim and @JornShen for your help!

I manually copied the file attached by Shen and it passed the test.

Though after copying the file and running hack/update-openapi-spec.sh locally again, I do see changes this time, which is clearly wrong:

diff --git a/api/openapi-spec/swagger.json b/api/openapi-spec/swagger.json
index 39807afb663..ec45c84199a 100644
--- a/api/openapi-spec/swagger.json
+++ b/api/openapi-spec/swagger.json
@@ -11795,6 +11795,552 @@
       },
       "type": "object"
     },
+    "io.k8s.api.flowcontrol.v1alpha1.FlowDistinguisherMethod": {
+      "description": "FlowDistinguisherMethod specifies the method of a flow distinguisher.",
+      "properties": {
+        "type": {
+          "description": "`type` is the type of flow distinguisher method The supported types are \"ByUser\" and \"ByNamespace\". Required.",
+          "type": "string"
+        }
+      },
+      "required": [
+        "type"
+      ],
+      "type": "object"
+    },
+    "io.k8s.api.flowcontrol.v1alpha1.FlowSchema": {
+      "description": "FlowSchema defines the schema of a group of flows. Note that a flow is made up of a set of inbound API requests with similar attributes and is identified by a pair of strings: the name of the FlowSchema and a \"flow distinguisher\".",

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. area/ipvs cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/network Categorizes an issue or PR as relevant to SIG Network. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Service Internal Traffic Policy
9 participants