Re-enable the authorizer webhook by default.#364
Re-enable the authorizer webhook by default.#364renormalize wants to merge 7 commits intoai-dynamo:mainfrom
Conversation
|
@renormalize you are unable to see the problem because we never print the admission denied error and the response from the webhook is not captured as part of the log. After i explicitly added a log the error is now visible: There seems to be a Only for testing, when i added |
c368155 to
33befa3
Compare
a31db38 to
8ccb90d
Compare
e6adaa4 to
5fbf148
Compare
gflarity
left a comment
There was a problem hiding this comment.
Thanks for the PR!
A few small things in my comments. I'd like to see the Clientset and hardcoded (valid for 1 hour) fixed if you don't disagree. The printDiff is just a nit. I won't request changes so this doesn't get blocked on me. Ping me for a fast approval after, I'll also chat with @sanjaychatterjee and ask him to approve it I'm not immediately available.
operator/internal/webhook/admission/pcs/authorization/handler.go
Outdated
Show resolved
Hide resolved
3d9c52b to
af61dbc
Compare
4e4fe52 to
c99a837
Compare
Signed-off-by: Saketh Kalaga <saketh.kalaga@sap.com>
Signed-off-by: Madhav Bhargava <madhav.bhargava@sap.com>
grove serviceaccount. * Adapted setup and tests to use operator dynamic client where tests need to change managed resources. * Changed the verbosity level for Info log from authorizer webhook that logs every request. Signed-off-by: Madhav Bhargava <madhav.bhargava@sap.com>
Signed-off-by: Madhav Bhargava <madhav.bhargava@sap.com>
Signed-off-by: Madhav Bhargava <madhav.bhargava@sap.com>
Signed-off-by: Saketh Kalaga <saketh.kalaga@sap.com>
Signed-off-by: Madhav Bhargava <madhav.bhargava@sap.com>
c99a837 to
2cb9b2d
Compare
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
The authorizer webhook was disabled by default due to a potential regression observed around the second alpha release in #223.
However, I have been unable to replicate this issue since, and would like to re-enable it by default. It being enabled by default will also help us in catching any potential bugs before the next release as all new PRs will indirectly check the behavior of the authorizer webhook, along with the E2E tests.
This PR also fixes E2E tests and introduces a new dynamic client which creates a bearer token via
TokenRequestAPI using the service account with which Grove operator is started. The authorizer webhook allows calls to change managed resources when it comes from grove service account.Previous to this PR, user
system:adminwas making calls to scale-out/in PCLQ/PCSG and with authorizer webhook enabled this will be disallowed assystem:adminis not an exempted account. Also we would not like to change thevalues.yamlto include test users that k3d brings in.Does this PR introduce a API change?
Additional documentation e.g., enhancement proposals, usage docs, etc.: