add nodeportisolation filter in yurthub #1209

rambohe-ch · 2023-02-08T06:36:37Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:
/kind bug
/kind documentation
/kind enhancement
/kind good-first-issue
/kind feature
/kind question
/kind design
/sig ai
/sig iot
/sig network
/sig storage

/kind feature

What this PR does / why we need it:

add a new filter named nodeportisolation in yurthub in order to make only nodes in specified nodepool can listen on nodePort of service.

Which issue(s) this PR fixes:

Fixes #1183

Special notes for your reviewer:

Does this PR introduce a user-facing change?

other Note

openyurt-bot · 2023-02-08T06:36:41Z

@rambohe-ch: GitHub didn't allow me to assign the following users: your_reviewer.

Note that only openyurtio members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:
/kind bug
/kind documentation
/kind enhancement
/kind good-first-issue
/kind feature
/kind question
/kind design
/sig ai
/sig iot
/sig network
/sig storage

/kind feature

What this PR does / why we need it:

add a new filter named nodeportisolation in yurthub in order to make only nodes in specified nodepool can listen on nodePort of service.

Which issue(s) this PR fixes:

Fixes #1183

Special notes for your reviewer:

Does this PR introduce a user-facing change?
other Note

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.

openyurt-bot · 2023-02-08T06:36:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rambohe-ch

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rambohe-ch]

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

rambohe-ch · 2023-02-08T06:37:16Z

@Congrool @yuanmaomao PTAL

codecov · 2023-02-08T07:41:17Z

Codecov Report

Merging #1209 (f6eca59) into master (7488bda) will increase coverage by 0.08%.
The diff coverage is 60.95%.

@@            Coverage Diff             @@
##           master    #1209      +/-   ##
==========================================
+ Coverage   53.49%   53.58%   +0.08%     
==========================================
  Files         101      102       +1     
  Lines       12604    12751     +147     
==========================================
+ Hits         6742     6832      +90     
- Misses       5314     5363      +49     
- Partials      548      556       +8

Flag	Coverage Δ
unittests	`53.58% <60.95%> (+0.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/yurthub/filter/nodeportisolation/filter.go	`59.85% <59.85%> (ø)`
pkg/yurthub/filter/manager/manager.go	`69.35% <100.00%> (+0.50%)`	⬆️
pkg/yurthub/util/fs/store.go	`68.00% <0.00%> (+0.74%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Congrool · 2023-02-09T14:39:34Z

pkg/yurthub/filter/nodeportisolation/filter.go

+	}
+
+	if len(nif.nodeName) == 0 {
+		return fmt.Errorf("node name for nodePortIsolationFilter is not ready")


"node name for nodePortIsolationFilter is not set" will be more clear

Congrool · 2023-02-09T15:01:09Z

pkg/yurthub/filter/nodeportisolation/filter.go

+		if len(svc.Annotations[ServiceAnnotationNodePortIsolation]) != 0 {
+			enabledNodePools, disabledNodePools := parseNodePoolFromAnnotation(svc.Annotations[ServiceAnnotationNodePortIsolation])
+			if disabledNodePools.Has(nodePoolName) {
+				klog.V(2).Infof("nodePort service(%s) is isolated in nodePool(%s) by nodePortIsolationFilter", nsName, nodePoolName)


"nodePort service(%s) is disabled in nodePool(%s) by nodePortIsolationFilter"

Congrool · 2023-02-09T15:01:15Z

pkg/yurthub/filter/nodeportisolation/filter.go

+				return svc
+			}
+
+			klog.V(2).Infof("nodePort service(%s) is isolated in nodePool(%s) by nodePortIsolationFilter", nsName, nodePoolName)


Congrool · 2023-02-09T15:15:27Z

pkg/yurthub/filter/nodeportisolation/filter.go

+
+	// node is not located in NodePool, skip this filter
+	if len(nodePoolName) == 0 {
+		return svc


Is it suitable that skipping filter for node that does not in any nodepool?

If users only set enabled nodepools for svcs, it looks strange that such nodePort svcs will also work on these orphan nodes.

But if users only set disabled nodepools for svcs, it's intuitive that these svcs should work on all nodes in other nodepools and all orphan nodes.

If users set both enable nodepools and disable nodepools, it maybe confused because such two sets may overlap. For example, "np1,np2,-np1,-np3", we have to make more clear definition for such case.

I have updated nodepool definition in #1183 as following:

The details of nodeport.openyurt.io/isolation annotation of service:

key: nodeport.openyurt.io/isolation

value: you can combine [np, -np, *] these three string to configure value, the priority is: [ np > -np > * ]

np: enable specified NodePort service listening in this nodePool

-np: disable specified NodePort service listening in this nodePool

*: enable specified NodePort service listening in all nodePools except specified by -np.

!: is used for orphan nodes(don't locate in nodepool), if ! is set, disable NodePort service listening on orphan nodes. if ! is not set, enable NodePort service listening on orphan nodes.

for example:

*: enable NodePort service listening in all nodePools
nodepool1, nodepool2: enable NodePort service listening in nodePool1 and nodePool2
-nodepool1, *: enable NodePort service listening in all nodePools except nodePool1
nodepool1,-nodepool2: only enable NodePort service listening in nodePool1
nodepool1, *: enable NodePort service listening in all nodePools
nodepool1, -nodepool1, *: enable NodePort service listening in of nodePools except nodePool1
*, !: enable NodePort service listening in of nodePools except orphan nodes.

nodepool1, -nodepool1, *: enable NodePort service listening in of nodePools except nodePool1

If [ np > -np > * ], I think it should enable NodePort service on all nodepools.

Is there a better way to indicate orphan nodes? Using ! is hard to read I think.

@Congrool As discussed in community meeting, the design has been changed as following:

I will name the new filter as nodeportisolation, and add a new annotation nodeport.openyurt.io/listen for NodePort service in order to trigger nodeportisolation filter in Yurthub.

The details of nodeport.openyurt.io/listen annotation of service:

key: nodeport.openyurt.io/listen

value: A list of nodepool names that separated by ','

foo: enable specified NodePort service listening on nodes of NodePool named foo

-foo: disable specified NodePort service listening on nodes of NodePool named fool

*: enable specified NodePort service listening on nodes of all NodePools

if NodePool name is duplicated in the configuration, we will take the first configuration

if NodePool name is not configured, we will disable NodePort listening on nodes of these unconfigured NodePools.

orphan nodes(don't locate in NodePool) will be kept the same as native Kubernetes, so NodePort will be listened on orphan nodes.

for example:

foo, bar: enable NodePort service listening on nodes in foo and bar NodePool.

foo, *: enable NodePort service listening on nodes of all NodePools

-foo, -bar: disable NodePort service listening on nodes of all NodePools

-foo, *: disable NodePort service listening only on nodes in foo NodePool

foo,-foo: enable NodePort service listening on nodes in foo NodePool

-foo: disable NodePort service listening on nodes of all NodePools(include foo NodePool).

Congrool · 2023-02-09T15:21:47Z

pkg/yurthub/filter/nodeportisolation/filter.go

+			}
+
+			klog.V(2).Infof("nodePort service(%s) is isolated in nodePool(%s) by nodePortIsolationFilter", nsName, nodePoolName)
+			return nil


In other case, we return nil to disable the nodeport svc. It looks confusing. For example, we have "np1,-np2" which also implicitly disable this svc in np3, np4 and so on. Users may be confused when encountering this scenario.

Congrool · 2023-02-09T15:53:15Z

Another problem, this filter seems cannot react to the nodepool change, for example the node is moved to another nodepool.

rambohe-ch · 2023-02-10T03:49:52Z

Another problem, this filter seems cannot react to the nodepool change, for example the node is moved to another nodepool.

@Congrool I suppose that node should be reset and re-join when node want to change the nodePool. or maybe we can adapt nodepool change with a new controller in the other pull request.

Congrool · 2023-02-13T02:16:56Z

I suppose that node should be reset and re-join when node want to change the nodePool. or maybe we can adapt nodepool change with a new controller in the other pull request.

If so, we may need to add such instruction to our manual.

Congrool · 2023-02-20T03:03:08Z

Refer to the design of #1183 (comment):

if NodePool name is not configured, we will disable NodePort listening on nodes of these unconfigured NodePools.

When the svc has the annotation key but with no value, like nodeport.openyurt.io/listen:, I think it's more suitable to disable this svc in all nodepools. But currently, it will enable this svc in all nodepools. What do you think ? @rambohe-ch

rambohe-ch · 2023-02-20T07:12:10Z

Refer to the design of #1183 (comment):

if NodePool name is not configured, we will disable NodePort listening on nodes of these unconfigured NodePools.

When the svc has the annotation key but with no value, like nodeport.openyurt.io/listen:, I think it's more suitable to disable this svc in all nodepools. But currently, it will enable this svc in all nodepools. What do you think ? @rambohe-ch

@Congrool you are right, we need to disable this svc in all nodepools if no value configured. i have fixed it, PTAL

Congrool · 2023-02-21T06:02:50Z

/lgtm

openyurt-bot added the kind/feature kind/feature label Feb 8, 2023

openyurt-bot requested review from charleszheng44 and Fei-Guo February 8, 2023 06:36

openyurt-bot added approved approved size/XL size/XL: 500-999 labels Feb 8, 2023

rambohe-ch force-pushed the add-nodeportisolation-filter branch 2 times, most recently from 0ea2f18 to a285f94 Compare February 8, 2023 07:37

Congrool reviewed Feb 9, 2023

View reviewed changes

rambohe-ch force-pushed the add-nodeportisolation-filter branch from a285f94 to 0b6f4bb Compare February 10, 2023 03:47

rambohe-ch force-pushed the add-nodeportisolation-filter branch 2 times, most recently from b31b9d3 to a75b1c7 Compare February 16, 2023 09:37

add nodeportisolation filter in yurthub

f6eca59

rambohe-ch force-pushed the add-nodeportisolation-filter branch from a75b1c7 to f6eca59 Compare February 20, 2023 07:10

openyurt-bot assigned Congrool Feb 21, 2023

openyurt-bot added the lgtm lgtm label Feb 21, 2023

openyurt-bot merged commit a8c48d2 into openyurtio:master Feb 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add nodeportisolation filter in yurthub #1209

add nodeportisolation filter in yurthub #1209

rambohe-ch commented Feb 8, 2023

openyurt-bot commented Feb 8, 2023

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

other Note

openyurt-bot commented Feb 8, 2023

rambohe-ch commented Feb 8, 2023

codecov bot commented Feb 8, 2023 •

edited

Loading

Congrool Feb 9, 2023

rambohe-ch Feb 10, 2023

Congrool Feb 9, 2023

rambohe-ch Feb 10, 2023

Congrool Feb 9, 2023

rambohe-ch Feb 10, 2023

Congrool Feb 9, 2023

rambohe-ch Feb 10, 2023 •

edited

Loading

Congrool Feb 13, 2023

rambohe-ch Feb 16, 2023

Congrool Feb 9, 2023

rambohe-ch Feb 10, 2023

Congrool commented Feb 9, 2023

rambohe-ch commented Feb 10, 2023

Congrool commented Feb 13, 2023

Congrool commented Feb 20, 2023

rambohe-ch commented Feb 20, 2023

Congrool commented Feb 21, 2023

add nodeportisolation filter in yurthub #1209

add nodeportisolation filter in yurthub #1209

Conversation

rambohe-ch commented Feb 8, 2023

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

other Note

openyurt-bot commented Feb 8, 2023

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

other Note

openyurt-bot commented Feb 8, 2023

rambohe-ch commented Feb 8, 2023

codecov bot commented Feb 8, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rambohe-ch Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Congrool commented Feb 9, 2023

rambohe-ch commented Feb 10, 2023

Congrool commented Feb 13, 2023

Congrool commented Feb 20, 2023

rambohe-ch commented Feb 20, 2023

Congrool commented Feb 21, 2023

codecov bot commented Feb 8, 2023 •

edited

Loading

rambohe-ch Feb 10, 2023 •

edited

Loading