strimzi-kafka-operator icon indicating copy to clipboard operation
strimzi-kafka-operator copied to clipboard

Add support for custom CC API users

Open kyguy opened this issue 1 year ago • 11 comments

Type of change

  • Enhancement / new feature

Description

Based on this proposal, this PR allows advanced users the ability to create REST API users for the Cruise Control REST API. This would allow roles and permissions to be defined to allow advanced users and third-party applications to access the Cruise Control REST API without having to disable HTTP basic authentication.

For example, advanced users could define their custom API users in a secret called cruise-control-api-users-secret putting their API user credentials in the Jetty's HashLoginService's file format like this:

userOne: passwordOne, USER
userTwo: passwordTwo, VIEWER

Create the secret:

kubectl create secret generic cruise-control-api-users-secret  --from-file=key=cruise-control-auth.txt

Then update their Kafka custom resource like this:

apiVersion: kafka.strimzi.io/v1beta2
kind: Kafka
metadata:
  ...
spec:
  cruiseControl:
    apiUsers:
      type: hashLoginService (1)
      valueFrom: (2)
        secretKeyRef:
          name: cruise-control-api-users-secret
          key: key
     ...

(1) A type field is added here to describe the format of the data and for configuration flexibility in the future. This gives us the option to add different data format types in the future. In this example we use Jetty's HashLoginService format since that is the format which Cruise Control uses for its API user configuration.

(2) The valueFrom construct allows us to add more sources in the future if needed. This is also a pattern used in other Strimzi APIs already, for example Strimzi's logging configuration, password configuration in KafkaUser resources, metrics configuration, and more.

For more information, checkout the proposal.

Checklist

Please go through this checklist and make sure all applicable tasks have been done

  • [x] Write tests
  • [x] Make sure all tests pass
  • [x] Update documentation
  • [x] Check RBAC rights for Kubernetes / OpenShift roles
  • [x] Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
  • [x] Reference relevant issue(s) and close them after merging
  • [x] Update CHANGELOG.md
  • [ ] Supply screenshots for visual changes, such as Grafana dashboards

kyguy avatar May 15 '24 20:05 kyguy

/packit test --labels sanity

kyguy avatar Jun 05 '24 22:06 kyguy

I just wanted to check something with Packit. The failure is not connected to this PR but to Packit/TF config, you can ignore the failed TF checks, thanks.

Frawless avatar Jun 06 '24 04:06 Frawless

/azp run regression

scholzj avatar Jun 08 '24 18:06 scholzj

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar Jun 08 '24 18:06 azure-pipelines[bot]

/azp run regression

scholzj avatar Jun 13 '24 07:06 scholzj

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar Jun 13 '24 07:06 azure-pipelines[bot]

/azp run regression

scholzj avatar Jun 21 '24 00:06 scholzj

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar Jun 21 '24 00:06 azure-pipelines[bot]

@kyguy The code looks good to me. But there seems to be some issue as all tests with CC failed in regression.

scholzj avatar Jun 21 '24 08:06 scholzj

/azp run regression

scholzj avatar Jun 27 '24 00:06 scholzj

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar Jun 27 '24 00:06 azure-pipelines[bot]

@kyguy You seem to have some Spotbugs issue that failed the build.

scholzj avatar Jul 01 '24 17:07 scholzj

Based on the feedback I moved a bunch of the methods of ApiCredentials class into the ApiUsers class messing around with the access controls and attempting to simplify the logic. Please take another look to make sure everything looks ok!

@scholzj @ppatierno Could you take a look at the recent changes when you get a chance?

kyguy avatar Jul 03 '24 21:07 kyguy

Can't we just revert to the point when I approved it and start again from there? 😞 To be honest, I'm not sure how did we managed to mess up so much something what looked good :-(. Sorry that I missed the comments and changes here earlier @kyguy.

scholzj avatar Jul 17 '24 21:07 scholzj

Can't we just revert to the point when I approved it and start again from there? 😞 To be honest, I'm not sure how did we managed to mess up so much something what looked good :-(. Sorry that I missed the comments and changes here earlier @kyguy.

I think that "mess up" is a too strong wording (at least for how it actually translates to Italian). I asked for some abstraction and Kyle just followed my suggestions. Maybe there is something still to tweak but saying that it's a mess (and the one before was good) sounds to be not fair to me.

ppatierno avatar Jul 18 '24 07:07 ppatierno

Refactored based on feedback, can you both take another pass when you get a chance @ppatierno @scholzj

kyguy avatar Jul 25 '24 12:07 kyguy

/azp run regression

scholzj avatar Jul 26 '24 00:07 scholzj

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar Jul 26 '24 00:07 azure-pipelines[bot]

@kyguy FYI: The system test that failed is flaky, so while it uses Cruise Control, it is possibly not related to your PR.

scholzj avatar Jul 26 '24 16:07 scholzj

/azp run regression

scholzj avatar Jul 30 '24 20:07 scholzj

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar Jul 30 '24 20:07 azure-pipelines[bot]

@scholzj Just addressed nits, once the base unit tests pass again we are are ready for another round of system tests

kyguy avatar Aug 01 '24 21:08 kyguy

/azp run regression

scholzj avatar Aug 03 '24 09:08 scholzj

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar Aug 03 '24 09:08 azure-pipelines[bot]

@kyguy Could you please resolve the conflict?

scholzj avatar Aug 05 '24 13:08 scholzj

@scholzj COnflict resolved and now have all approvals, could you execute regression tests one more time

kyguy avatar Aug 06 '24 12:08 kyguy

Yeah, I need to first merge #10415 that fixes some flaky tests.

scholzj avatar Aug 06 '24 13:08 scholzj

/azp run regression

scholzj avatar Aug 06 '24 16:08 scholzj

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar Aug 06 '24 16:08 azure-pipelines[bot]

Thanks for the PR @kyguy

scholzj avatar Aug 06 '24 21:08 scholzj