Skip to content

k8s: allow setting multiple k8s API server addresses#20090

Closed
fristonio wants to merge 5 commits intocilium:masterfrom
fristonio:pr/fristonio/feat-19038
Closed

k8s: allow setting multiple k8s API server addresses#20090
fristonio wants to merge 5 commits intocilium:masterfrom
fristonio:pr/fristonio/feat-19038

Conversation

@fristonio
Copy link
Member

@fristonio fristonio commented Jun 6, 2022

See commit message for detailed description

Introduce a new command line parameter(--k8s-api-server-urls) and helm option(k8s.apiServerURLs) to specify multiple k8s API server addresses for the client to use.

Fixes: #19038

Allow setting of multiple k8s API server URLs for client

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 6, 2022
@fristonio fristonio added the area/daemon Impacts operation of the Cilium daemon. label Jun 6, 2022
@brb
Copy link
Member

brb commented Jun 7, 2022

@fristonio Welcome back! 😅 Would it be possible to set the new param from Helm? Currently, we use this hack to pass the API server endpoint addr - https://github.com/cilium/cilium/blob/master/install/kubernetes/cilium/templates/cilium-agent/daemonset.yaml#L193.

@fristonio
Copy link
Member Author

fristonio commented Jun 7, 2022

Hey @brb 👋
Its good to be back. 😄
Yeah, I have some changes locally for the helm option that I need to test. Will update the PR soon.

@fristonio fristonio added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Jun 7, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 7, 2022
@fristonio fristonio force-pushed the pr/fristonio/feat-19038 branch 2 times, most recently from d978a26 to dfc36a5 Compare June 7, 2022 23:15
@fristonio fristonio marked this pull request as ready for review June 8, 2022 01:45
@fristonio fristonio requested a review from a team June 8, 2022 01:45
@fristonio fristonio requested a review from a team as a code owner June 8, 2022 01:45
@fristonio fristonio requested a review from a team June 8, 2022 01:45
@fristonio fristonio requested review from a team as code owners June 8, 2022 01:45
@fristonio
Copy link
Member Author

/test

Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

👋 super happy to see you around 🎖️

One small comment as per below, the rest looks reasonable to me.

@fristonio fristonio requested a review from a team as a code owner June 8, 2022 08:31
@fristonio fristonio requested a review from qmonnet June 8, 2022 08:31
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Looks good, with a few nits - see below (plus a typo on --k8s-api-erver-addresses in your first commit log).

One question: I'm wondering if it would make sense to have “URLs” instead of “Addresses” in the flag (and Helm value) name? Reading “addresses” I'd first think of IP addresses, maybe URLs would make it more explicit?

Awesome to have you contributing again! ❤️

@fristonio fristonio force-pushed the pr/fristonio/feat-19038 branch from 13aa021 to dd44542 Compare June 8, 2022 17:58
@fristonio
Copy link
Member Author

Hey @qmonnet! 👋
Yeah, changing the CLI flag to use urls instead of addresses makes sense to me.
I have updated the PR and addressed the changes you requested.

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Good points raised by Tom, but I'm good for my own review. Thanks again! :)

@fristonio fristonio force-pushed the pr/fristonio/feat-19038 branch from dd44542 to 96f167d Compare June 8, 2022 22:04
@fristonio fristonio force-pushed the pr/fristonio/feat-19038 branch from 53a2159 to e61c4bb Compare June 27, 2022 20:22
@fristonio
Copy link
Member Author

fristonio commented Jun 28, 2022

/test

Job 'Cilium-PR-K8s-GKE' hit: #17628 (93.58% similarity)

@joestringer joestringer added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Jul 5, 2022
@joestringer joestringer removed their request for review July 5, 2022 18:43
@joestringer
Copy link
Member

Looks like this just needs rebase, address any CI failures, plus operator / cli codeowner review. If there's something particular you'd like me to look at, then let me know - otherwise I'll let the other core reviewers follow up.

* This commit introduces a new command line flag to specify multiple
  kubernetes addresses that can be used with clients. By default the
  client will pick the first APIServer address for setting up the
  client. During a failed heartbeat check for k8s API server we will
  rotate the API server to use with the client.

* cilium-agent --k8s-api-server-urls=IP1,IP2,IP3 ... to use this
  feature.

* The flag takes precedence over already existing `--k8s-api-server`
  flag.

* The flag modfies the host address for client-go http.RoundTripper to
  use configured APIServerURL in k8s.config. The heartbeat thread
  rotates the APIServer URL when there is a failure causing subsequent
  request to be sent to the rotated host.

Signed-off-by: Deepesh Pathak <deepeshpathak09@gmail.com>
* Use helm option `k8s.apiServerURLs` to set multiple k8s server
  urls for the client to use.

* Example helm command

```
helm install cilium cilium/cilium \
    --namespace kube-system \
    --set k8s.apiServerURLs="https://172.18.0.4:6443 https://172.18.0.3:6443 https://172.18.0.5:6443"
```

Signed-off-by: Deepesh Pathak <deepeshpathak09@gmail.com>
Signed-off-by: Deepesh Pathak <deepeshpathak09@gmail.com>
Signed-off-by: Deepesh Pathak <deepeshpathak09@gmail.com>
* This commit adds APIServer URL rotation logic during initial client
  setup.

* Update tests helper to use the new helm option `k8s.apiServerURLs` to
  specify Kubernetes API server address in kube-proxy free setup.

Signed-off-by: Deepesh Pathak <deepeshpathak09@gmail.com>
@fristonio fristonio force-pushed the pr/fristonio/feat-19038 branch from e61c4bb to ea41042 Compare July 19, 2022 01:16
@fristonio
Copy link
Member Author

/test

@fristonio fristonio removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Jul 19, 2022
Comment on lines +108 to +109
config.APIServerURL = nil
config.APIServerURLs = []*url.URL{}
Copy link
Member

Choose a reason for hiding this comment

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

These fields are being accessed without being protected by their mutex

Comment on lines +158 to +171
type httpRoundTripper struct {
delegate http.RoundTripper
}

func (rt *httpRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
req.URL.Host = GetAPIServerURL().Host
return rt.delegate.RoundTrip(req)
}

func defaultHTTPRoundTripper(rt http.RoundTripper) http.RoundTripper {
return &httpRoundTripper{
delegate: rt,
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you leave a comment stating what this is doing?

Comment on lines +3711 to +3719
if viper.IsSet(K8sAPIServer) {
if len(c.K8sAPIServerURLs) > 0 {
log.Warningf("The option %s has been deprecated in favour of %s. Ignoring %s: %s",
K8sAPIServer, K8sAPIServerURLs, K8sAPIServer, viper.GetString(K8sAPIServer))
} else {
c.K8sAPIServer = viper.GetString(K8sAPIServer)
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

in the daemon/flags.go we should be marking the flag as deprecated.

@twpayne twpayne removed their request for review August 14, 2022 22:44
@christarazi
Copy link
Member

Moving to draft, please mark it ready for review when you are ready to pick it up.

@christarazi christarazi marked this pull request as draft August 31, 2022 15:37
type configuration struct {
// APIServerURL is the URL address of the API server
APIServerURL string
APIServerURL *url.URL
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this member be dropped?

@github-actions
Copy link

github-actions bot commented Oct 1, 2022

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Oct 1, 2022
@github-actions
Copy link

This pull request has not seen any activity since it was marked stale.
Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/daemon Impacts operation of the Cilium daemon. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

k8sServiceHost multiple IPs/Hosts

9 participants