Feature: Enhance allow-port to support multiple ports #24
Conversation
|
@Hector295 Can you please open an issue under https://github.com/rancher/rancher/issues, describing the usecase and fix needed here |
cmd/client/main.go
Outdated
| @@ -52,7 +52,7 @@ var ( | |||
| tokenPath = kingpin.Flag("token-path", "Uses an OAuth 2.0 Bearer token found in this path to make scrape requests").String() | |||
| insecureSkipVerify = kingpin.Flag("insecure-skip-verify", "Disable SSL security checks for client").Default("false").Bool() | |||
| useLocalhost = kingpin.Flag("use-localhost", "Use 127.0.0.1 to scrape metrics instead of FQDN").Default("false").Bool() | |||
| allowPort = kingpin.Flag("allow-port", "Restricts the proxy to only being allowed to scrape the given port").Default("*").String() | |||
| allowPort = kingpin.Flag("allow-port", "Restricts the proxy to only being allowed to scrape the given ports, separated by commas").Default("*").String() | |||
There was a problem hiding this comment.
I think it is bad form to add parsing strategies to existing strings. It is more natural to add allowPorts as an array (or slice if you prefer golang terms)
There was a problem hiding this comment.
@richardelling Thank you for the feedback! My goal with this change is to keep everything backward compatible while adding the flexibility to specify multiple ports using a comma-separated list. This way, existing setups remain unchanged, but we gain more options for new configurations
There was a problem hiding this comment.
I'd prefer if this used kingping.Strings() as well. In this case an empty slice would be equivalent to *. Keeping the flag as --allow-port will keep backward compatibility, but should rename the allowPort var to allowPorts.
cmd/client/main.go
Outdated
| @@ -149,7 +149,7 @@ func (c *Coordinator) doScrape(request *http.Request, client *http.Client) { | |||
|
|
|||
| port := request.URL.Port() | |||
| if len(port) > 0 { | |||
| if *allowPort != "*" && *allowPort != port { | |||
| if *allowPort != "*" && !matchPort(*allowPort, port) { | |||
There was a problem hiding this comment.
Edge case: what happens when * is provided as an argument (ex --allow-port 8080,*,8081 or --allow-port 8081 --allow-port '*').
I would argue that this should be an invalid specification and should result in an error. But I'm ok with allowing it and if * is in the list all ports are allowed even if others are specified.
There was a problem hiding this comment.
I would concur on it being an invalid state, so I would rather we error when len(allowPorts) > 1 && strings.Contains(allowPorts, "*").
cmd/client/main.go
Outdated
| @@ -52,7 +52,7 @@ var ( | |||
| tokenPath = kingpin.Flag("token-path", "Uses an OAuth 2.0 Bearer token found in this path to make scrape requests").String() | |||
| insecureSkipVerify = kingpin.Flag("insecure-skip-verify", "Disable SSL security checks for client").Default("false").Bool() | |||
| useLocalhost = kingpin.Flag("use-localhost", "Use 127.0.0.1 to scrape metrics instead of FQDN").Default("false").Bool() | |||
| allowPort = kingpin.Flag("allow-port", "Restricts the proxy to only being allowed to scrape the given port").Default("*").String() | |||
| allowPort = kingpin.Flag("allow-port", "Restricts the proxy to only being allowed to scrape the given ports, separated by commas").Default("*").String() | |||
There was a problem hiding this comment.
I'd prefer if this used kingping.Strings() as well. In this case an empty slice would be equivalent to *. Keeping the flag as --allow-port will keep backward compatibility, but should rename the allowPort var to allowPorts.
Hector295
force-pushed
the
feature/allow-multiple-ports
branch
3 times, most recently
from
a7de1a0 to
9827332
Compare
cmd/client/main.go
Outdated
| @@ -52,7 +52,7 @@ var ( | |||
| tokenPath = kingpin.Flag("token-path", "Uses an OAuth 2.0 Bearer token found in this path to make scrape requests").String() | |||
| insecureSkipVerify = kingpin.Flag("insecure-skip-verify", "Disable SSL security checks for client").Default("false").Bool() | |||
| useLocalhost = kingpin.Flag("use-localhost", "Use 127.0.0.1 to scrape metrics instead of FQDN").Default("false").Bool() | |||
| allowPort = kingpin.Flag("allow-port", "Restricts the proxy to only being allowed to scrape the given port").Default("*").String() | |||
| allowPorts = kingpin.Flag("allow-port", "Restricts the proxy to only being allowed to scrape the given ports, separated by commas").Default("*").String() | |||
There was a problem hiding this comment.
Based on joshmeranda's previous feedback:
| allowPorts = kingpin.Flag("allow-port", "Restricts the proxy to only being allowed to scrape the given ports, separated by commas").Default("*").String() | |
| allowPorts = kingpin.Flag("allow-port", "Restricts the proxy to only being allowed to scrape the given ports, separated by commas").Default("*").Strings() |
|
@Hector295 looks like you implemented some of these changes in 4420338 but then reverted then in 9827332. Do you need more guidance on what we are looking for? |
@Hector295 could you confirm which CIS Kubernetes version and check you are referring to ? This is mostly to add context to the initial description. Does it concern 4.3.1 Ensure that the kube-proxy metrics service is bound to localhost ? |
|
@pjbgf @prachidamle What you suggest I have tried and it changes the way to use the flag --allow-port. Now, you would have to use --allow-port for each port you want to allow. Is this what is expected from the change? I was thinking of using --allow-port 1010,1011,1012, but with your change from String to Strings, it would be something like --allow-port 1010 --allow-port 1011 --allow-port 1012. Is this still backwards compatible? It would change how they are handled in the charts, right? |
|
@andypitcher sure, we are using version 1.9 and these are the other references: |
|
@Hector295 Yes that is backwards compatible. As it stands you can only specify --add-port once, our suggestions would just let you specify it multiple times. It would change how they are handled in charts, but that is ok. As we've said this is backwards compatible so charts won't break with this change. If a chart user wants to specify multiple ports they can simply update the logic in the chart to range over the desired ports. |
Hector295
force-pushed
the
feature/allow-multiple-ports
branch
from
7cb3084 to
ecad97f
Compare
cmd/client/main.go
Outdated
| func (c *Coordinator) matchPort(allowedPorts []string, port string) bool { | ||
| for _, s := range allowedPorts { | ||
| if s == port { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
There was a problem hiding this comment.
I'd also like to see the git history cleaned up a bit here. There are 4 commits with the same message that are adding and removing much of the same content. Plus there is a "debug" commit that should not have been pushed.
Ideally this could all be under once Feature: Enhance allow-port to support multiple ports commit, thanks!
Hector295
force-pushed
the
feature/allow-multiple-ports
branch
from
ecad97f to
9571051
Compare
Hector295
force-pushed
the
feature/allow-multiple-ports
branch
from
9571051 to
934ac46
Compare
|
@joshmeranda @pjbgf @andypitcher could you please review the PR again? |
|
@joshmeranda @pjbgf @andypitcher, could you please assist by providing your approval for the PR? |
|
@richardelling please, your review so I can close this PR |
|
@pjbgf @joshmeranda @richardelling Could this PR be merged, please? |
This change updates the allow-port flag to support multiple ports separated by commas.
The update is backward compatible.
Use Case: Metrics that are exposed only in localhost, such as
kube-proxy,kube-controller-manager, andkube-schedulerwhen CIS Kubernetes must be complied. Previously, each component needed a dedicated client and proxy with an specific port.