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

kvstore/etcd: also reload keypair using trusted-ca-file #10754

Merged
merged 1 commit into from Apr 2, 2020

Conversation

bpineau
Copy link
Contributor

@bpineau bpineau commented Mar 30, 2020

When we define the new trusted-ca-file attribute, etcd's
clientyaml.NewConfig would set cfg.TLS.RootCAs, which
shortcuts most of the (now deprecated) newConfig wrapper, and
prevents us from hooking reloads with TLS.GetClientCertificate:

if cfg.TLS == nil || cfg.TLS.RootCAs != nil {
        return cfg, nil
}

While at it, make it so the reload functionality survives
newConfig removal.

Signed-off-by: Benjamin Pineau benjamin.pineau@datadoghq.com

Support on-disk etcd client certificate and key reload when using trusted-ca-file

When we define the new `trusted-ca-file` attribute, etcd's
`clientyaml.NewConfig` would set `cfg.TLS.RootCAs`, which
shortcuts most of the (now deprecated) `newConfig` wrapper, and
prevents us from hooking reloads with `TLS.GetClientCertificate`:
```
if cfg.TLS == nil || cfg.TLS.RootCAs != nil {
        return cfg, nil
}
```

While at it, make it so the reload functionality survives
newConfig removal.

Signed-off-by: Benjamin Pineau <benjamin.pineau@datadoghq.com>
@bpineau bpineau requested a review from a team as a code owner March 30, 2020 10:41
@maintainer-s-little-helper
Copy link

Please set the appropriate release note label.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 45.517% when pulling 375a66c on DataDog:certificate-reload into d22fd69 on cilium:master.

@aanm aanm added pending-review release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Mar 30, 2020
@aanm
Copy link
Member

aanm commented Mar 30, 2020

test-me-please

Copy link
Member

@aanm aanm 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 the PR @bpineau

@bpineau
Copy link
Contributor Author

bpineau commented Mar 31, 2020

Is there anything I can do for integration tests? failures seems unrelated to the PR.

@aanm
Copy link
Member

aanm commented Mar 31, 2020

test-me-please

@aanm
Copy link
Member

aanm commented Mar 31, 2020

Is there anything I can do for integration tests? failures seems unrelated to the PR.

I've re-trigger the tests. They might have been caused by a flake.

@pchaigno
Copy link
Member

pchaigno commented Apr 1, 2020

restart-ginkgo

Restarting as CI hit #10760.

@aanm
Copy link
Member

aanm commented Apr 2, 2020

going to merge since it hit a known flake and flake is fixed on master.

@aanm aanm merged commit 99b51ab into cilium:master Apr 2, 2020
1.8.0 automation moved this from In progress to Merged Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
No open projects
1.8.0
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

4 participants