Sig-Auth Pre-Acceptance 2nd Review

[ibihim]

What

Issues collected during the second round of review with @enj.

Why

kube-rbac-proxy should become a project owned by Kubernetes.

Issues

cmd/kube-rbac-proxy/app/kube-rbac-proxy.go:

• [x] #317
• [x] (2) Rename to ApplyTo to conform naming convention link.
• [x] (3) Replace func with cobra.Args link.
• [x] (4) Run should run with a final config version. Complete should finish the necessary changes and those shouldn't happen anymore in Run. link.
• [x] (5) Use context from Signals. link.
• [x] #318
• [x] #316
• [x] #319
• [x] (9) H2C is fine as plaintext if listening to 127.0.0.1 / ::1 or UDS link.
• [x] (10) Describe what an empty request info factory means link.
• [x] (11) Use safe wait group from k/k instead of run.Group link
• [x] (12) Avoid nested code blocks link
• [x] (13) Comment why we have two listeners on different ports link
• [ ] #320
• [x] (15) Add serverconfig.SetupSignalContext lin.
• [x] (16) Log error of serverCtxCancel link.
• [x] (17) Authorizers should only be added, when used link.
• [x] (18) Verify that Rewrite Attributes is definitely turned off if not configured link.
• [x] (19) Discuss in comment, why regular path k/k authorizer can't be used link.
• [x] (20) What should happen if an Authorizer errs? link.
  • [x] Maybe fail? link.

cmd/kube-rbac-proxy/app/options/legacyoptions.go:

• [x] (21) Add whitespace link.
• [x] (22) Remove validation or add validation link.
• [x] (23) Rename to ApplyTo due to naming conventions link.
• [x] (24) Same: why do we have empty functions? link.

cmd/kube-rbac-proxy/app/options/proxyoptions.go:

• [x] #331
• [x] #332
• [ ] #336
• [ ] #371

pkg/authn/config.go:

• [x] (30) What does this do? link.

pkg/authn/identityheaders/identityheaders.go:

• [x] (31) Allow authHeaders only with mTLS or 127.0.0.1/::1 link.
• [x] (32) If !ok err-out link.
• [ ] #330

pkg/authorization/path/path.go:

• [x] (34) Better NewDenyPathAuthorizerUnlessAllowed name? link
• [x] (35) Just use authorizationpath.NewAuthorizer link

pkg/authorization/rewrite/rewrite.go:

• [x] (36) Use dedicated type link.
• [x] (37) Better link.
• [x] (38) Emphasize AND logic link.
• [x] (39) Better link.
• [ ] #333
• [ ] #356
• [x] (42) Describe that this turns dynamic path authz checks into static resource checks link.
• [x] (43) Verify and parse templates on process startup link.
• [x] #334
• [x] (45) Noop on nil link.
• [ ] #368
• [ ] (47) Collapse logic into a custom request info parser, if possible link.
• [ ] #355
• [x] (49) Quit early on no params link.

pkg/authorization/static/static.go:

• [ ] (50) Seems to be unused.
• [ ] #335

pkg/server/config.go:

• [x] (52) Check conditional statement on ForceAttemptHTTP2 link.

Epic

• [ ] (53) Should we have rewrites in general link OR
• [ ] (54) several SAR requests in particular per incoming request OR
• [ ] (55) support only header based config link OR
• [ ] #345
• [ ] (57) highlight as a very scary config link.

[liouk]

@ibihim the code related to (2) appears to have changed since the pre-acceptance review as it's no longer on brancz:sig-auth-acceptance. Could you double-check whether it still applies please?

[liouk]

@ibihim (12) seems to not apply anymore; nesting has been simplified in the latest version in brancz:sig-auth-acceptance. Could you please verify?

[ibihim]

@ibihim the code related to (2) appears to have changed since the pre-acceptance review as it's no longer on brancz:sig-auth-acceptance. Could you double-check whether it still applies please?

If there are no options, that have ConvertToNewOptions insterad of ApplyTo, you can check it off.

[ibihim]

@ibihim (12) seems to not apply anymore; nesting has been simplified in the latest version in brancz:sig-auth-acceptance. Could you please verify?

Yes, the nesting seems to have been gone. We should keep an eye open, in case we stumble into it somewhere else / try to avoid doing it.

Feel free to check it off.

[liouk]

@ibihim link for 17 points to the task/link for 16 -- does 17 maybe refer to this block instead?

[liouk]

FYI I do not have permissions to edit the issue description therefore I can't tick off any checkboxes :/

[ibihim]

@ibihim link for 17 points to the task/link for 16 -- does 17 maybe refer to this block instead?

Yes, I think so. Sorry. The markdown with all the hash-links makes it super hard to read.

[ibihim]

FYI I do not have permissions to edit the issue description therefore I can't tick off any checkboxes :/

You can enumerate the tasks you've done in a comment. I assigned the issue to both of us, but I assume that won't suffice. I have limited permission on this repository too, so I can't edit permissions.

[liouk]

List of issues I'm working on and their status:

cmd/kube-rbac-proxy/app/kube-rbac-proxy.go:

• [x] (2) Rename to ApplyTo to conform naming convention link.
  • not applicable anymore; has already been done
• [ ] (3) Replace func with cobra.Args link.
• [ ] (4) Run should run with a final config version. Complete should finish the necessary changes and those shouldn't happen anymore in Run. link.
• [ ] (5) Use context from Signals. link.
• [ ] (10) Describe what an empty request info factory means link.
• [ ] (11) Use safe wait group from k/k instead of run.Group link
• [x] (12) Avoid nested code blocks link
  • not applicable anymore; nesting has been already removed
• [ ] (13) Comment why we have two listeners on different ports link
• [ ] (15) Add serverconfig.SetupSignalContext link.
• [ ] (17) Authorizers should only be added, when used link.
• [ ] (18) Verify that Rewrite Attributes is definitely turned off if not configured link.

I'll keep this comment up-to-date to help coordination.

The issues in this comment are addressed in the following PR:

• #243

[liouk]

Also working on the following issues:

• [ ] (1) Log deprecation warnings link.
• [ ] (7) Don't use deprecated Director func link.
• [ ] (14) Validate if a simple response by the health endpoint is enough to proof healthiness link
• [ ] (19) Discuss in comment, why regular path k/k authorizer can't be used link.