Add bootstrap Authenticator and CSR registration #510

dharmjit · 2022-04-25T16:58:54Z

What this PR does / why we need it:
This PR implements CSR registration and BootstrapAuthenticator controller as per the design for feature #484.
Both CSR registration/BootstrapAuthenticator Controller logic is disabled by default and will be kept behind the SecureAccess feature flag, which can be enabled explicitly while starting the HostAgent.

Which issue(s) this PR fixes :
Fixes #508 #509(partially)

codecov-commenter · 2022-04-25T17:09:14Z

Codecov Report

Merging #510 (ac03f57) into main (6ba32a1) will decrease coverage by 3.35%.
The diff coverage is 53.06%.

@@            Coverage Diff             @@
##             main     #510      +/-   ##
==========================================
- Coverage   67.32%   63.97%   -3.36%     
==========================================
  Files          22       25       +3     
  Lines        1732     1904     +172     
==========================================
+ Hits         1166     1218      +52     
- Misses        494      608     +114     
- Partials       72       78       +6

Impacted Files	Coverage Δ
agent/main.go	`13.79% <0.00%> (-1.70%)`	⬇️
agent/authenticator/bootstrap_authenticator.go	`65.38% <65.38%> (ø)`
agent/registration/csr.go	`66.03% <66.03%> (ø)`
agent/registration/host_registrar.go	`0.00% <0.00%> (ø)`

agent/authenticator/authenticator_suite_test.go

agent/authenticator/authenticator_test.go

agent/registration/registration_test.go

agent/registration/csr.go

agent/host_agent_test.go

anusha94 · 2022-04-25T20:15:41Z

Also, this part of the code is untested?
https://app.codecov.io/gh/vmware-tanzu/cluster-api-provider-bringyourownhost/compare/510/tree/agent/main.go#D1L227

CSR registration wnd BootstrapAuthenticator will be handled behind SecureAccess feature flag. CSR resource will be created in the management cluster on the start of the HostAgent. BootstrapAuthenticator controller will watch the CSR resources. Currently placeholders are created for different flows and the implementation will be raised in differemt PRs

dharmjit · 2022-04-26T17:27:37Z

Also, this part of the code is untested? https://app.codecov.io/gh/vmware-tanzu/cluster-api-provider-bringyourownhost/compare/510/tree/agent/main.go#D1L227

There is already a test case added should create BYOHost CSR in the management cluster, and the code block is only initializing struct type and calling some methods.

agent/registration/csr.go

agent/registration/csr_test.go

agent/registration/registration_test.go

pshail · 2022-04-27T09:44:11Z

Codecov Report

Merging #510 (1e8f8bc) into main (6ba32a1) will decrease coverage by 3.75%.
The diff coverage is 49.49%.

❗ Current head 1e8f8bc differs from pull request most recent head e73f193. Consider uploading reports for the commit e73f193 to get more accurate results
@@            Coverage Diff             @@
##             main     #510      +/-   ##
==========================================
- Coverage   67.32%   63.56%   -3.76%     
==========================================
  Files          22       25       +3     
  Lines        1732     1894     +162     
==========================================
+ Hits         1166     1204      +38     
- Misses        494      611     +117     
- Partials       72       79       +7     
Impacted Files Coverage Δ
agent/main.go 14.11% <0.00%> (-1.37%) ⬇️
agent/authenticator/bootstrap_authenticator.go 50.00% <50.00%> (ø)
agent/registration/csr.go 66.66% <66.66%> (ø)
agent/reconciler/host_reconciler.go 79.23% <0.00%> (-2.25%) ⬇️
agent/registration/host_registrar.go 0.00% <0.00%> (ø)

💭 We should try to keep the coverage as is and not reduce it from achieved higher value 💭

The CSR naming is changed to byoh-csr-<hostname> so that the same can be picked in ByoAdmisisonController. As CSR is clusterscoped resource, usage of namespace is removed. EnvTest k8s version is updated to 1.23.3 as ExpirationSeconds is supported after v1.22.x

dharmjit · 2022-04-28T16:55:46Z

We should try to keep the coverage as is and not reduce it from achieved a higher value

@pshail, I agree. Currently, the code coverage is less

bootstrap_authenticator.go is mostly bootstrap code and once we implement more CSR workflows (approved, denied, etc.), the code coverage will improve for this file.
Similarly for agent/main.go it is mostly variable initialization code.

anusha94

minor nits. LGTM otherwise.

agent/authenticator/authenticator_suite_test.go

agent/authenticator/authenticator_test.go

agent/registration/csr.go

anusha94

lgtm

pshail

Overall LGTM just need clarity on main error handling related to CSR creation failure(s)

agent/authenticator/bootstrap_authenticator.go

agent/host_agent_test.go

agent/main.go

pshail

LGTM

vmwclabot added the cla-not-required label Apr 25, 2022

github-actions bot added the area/test-release label Apr 25, 2022

dharmjit requested a review from anusha94 April 25, 2022 17:02

anusha94 reviewed Apr 25, 2022

View reviewed changes

Dharmjit Singh added 3 commits April 26, 2022 17:11

Add CSR creation logic

de1c43c

Changes as per review comments

4d5be9c

anusha94 reviewed Apr 27, 2022

View reviewed changes

agent/registration/csr.go Show resolved Hide resolved

agent/registration/csr_test.go Outdated Show resolved Hide resolved

agent/registration/registration_test.go Outdated Show resolved Hide resolved

sachinkumarsingh092 mentioned this pull request Apr 28, 2022

Add admission controller #513

Merged

anusha94 reviewed Apr 28, 2022

View reviewed changes

agent/authenticator/authenticator_suite_test.go Outdated Show resolved Hide resolved

agent/authenticator/authenticator_test.go Outdated Show resolved Hide resolved

agent/registration/csr.go Show resolved Hide resolved

agent/registration/csr.go Show resolved Hide resolved

Dharmjit Singh added 2 commits April 29, 2022 07:39

created a builder to hold CSR generation

2a40297

fix test flake

8cbdc23

anusha94 approved these changes May 2, 2022

View reviewed changes

dharmjit requested a review from shubham14bajpai May 4, 2022 04:30

pshail suggested changes May 4, 2022

View reviewed changes

agent/authenticator/bootstrap_authenticator.go Show resolved Hide resolved

agent/host_agent_test.go Show resolved Hide resolved

agent/main.go Show resolved Hide resolved

agent/main.go Show resolved Hide resolved

agent/main.go Show resolved Hide resolved

pshail approved these changes May 4, 2022

View reviewed changes

dharmjit merged commit cd30026 into vmware-tanzu:main May 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bootstrap Authenticator and CSR registration #510

Add bootstrap Authenticator and CSR registration #510

dharmjit commented Apr 25, 2022

codecov-commenter commented Apr 25, 2022 •

edited

Loading

anusha94 commented Apr 25, 2022

dharmjit commented Apr 26, 2022

pshail commented Apr 27, 2022 •

edited

Loading

Codecov Report

dharmjit commented Apr 28, 2022

anusha94 left a comment

anusha94 left a comment

pshail left a comment

pshail left a comment

Add bootstrap Authenticator and CSR registration #510

Add bootstrap Authenticator and CSR registration #510

Conversation

dharmjit commented Apr 25, 2022

codecov-commenter commented Apr 25, 2022 • edited Loading

Codecov Report

anusha94 commented Apr 25, 2022

dharmjit commented Apr 26, 2022

pshail commented Apr 27, 2022 • edited Loading

Codecov Report

dharmjit commented Apr 28, 2022

anusha94 left a comment

Choose a reason for hiding this comment

anusha94 left a comment

Choose a reason for hiding this comment

pshail left a comment

Choose a reason for hiding this comment

pshail left a comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 25, 2022 •

edited

Loading

pshail commented Apr 27, 2022 •

edited

Loading