Add support for LDAP authentication #145

sbernauer · 2022-03-09T11:25:47Z

Description

For #5
Integration-Tests: https://github.com/stackabletech/integration-tests/pull/175
Use ./create_test_cluster.py --kind --operator superset=0.4.0-pr145 before running the tests

Review Checklist

Code contains useful comments
(Integration-)Test cases added (or not applicable)
Documentation added (or not applicable)
Changelog updated (or not applicable)
Cargo.toml only contains references to git tags (not specific commits or branches)
Helm chart can be installed and deployed operator works (or not applicable)

Once the review is done, comment bors r+ (or bors merge) to merge. Further information

deploy/helm/superset-operator/templates/roles.yaml

rust/crd/src/authentication.rs

nightkr · 2022-03-14T14:48:15Z

rust/operator-binary/src/config.rs

+    // We don't calculate the secrets here directly, as the operator should not be able to see the actual credentials.
+    // Instead we add a env-var which gets it's value from the secret with the credentials and read that with Python.
+    let common = r#"
+# common configs


Nit: Is it worth pulling this out into a file that we include_str!?

If you take a look at the current config.rs it now has a lot more format strings and i think i would make it difficult to read if we pull all of them out into separate files

Yeah.. those are all currently completely unescaped (https://github.com/stackabletech/superset-operator/pull/145/files/e54c1406da7ca2eb76db0f1558750c8d3e67d656#r826035108). I'd much rather stick them in through a secondary channel (either env vars or a JSON file we deploy next to the config.py perhaps?) and deploy the same config.py everywhere which then reads that secondary channel.

I would like to establish like a "common pattern" of how to do that in all the products supporting LDAP.
And not all products support executing arbitrary code to collect the Env/Json i guess. Maybe we should do a proper validation/escaping in the AuthenticationClass so that we validate all properties in a single place and the operators can behave all the same way? (I'm afraid we need proper validation/escaping anyhow)

Most product operators that don't support turing-complete configuration languages already use a common and well-defined configuration format where this isn't a problem anyway (JSON, XML, Java properties, whatever). This is just adapting Superset to follow the same convention.

Hmm would be ok for me to put this into env vars and than read them with Python

That should be fine.

I think this is fine for now because @siegfriedweber is working on an overhaul of the configuration for Superset and escaping Python-Strings and so on

rust/operator-binary/src/config.rs

rust/operator-binary/src/superset_controller.rs

nightkr · 2022-03-14T14:58:06Z

rust/operator-binary/src/superset_controller.rs

+) -> Result<ConfigMap> {
+    let mut cm_conf_data = BTreeMap::new(); // filename -> filecontent
+
+    for property_name_kind in rolegroup_config.keys() {


Is there any value in piping through the product-config machinery if we don't actually use it anyway?

That's a good question which came also to my mind. Maybe I'm doing it wrong but i haven't understand the benefit of the product-config machinery yet.

I think we can ignore this for now as @siegfriedweber is working on an overhaul of the product machinery for Superset and we focus on getting the feature in at the moment

rust/operator-binary/src/superset_controller.rs

sbernauer · 2022-04-08T06:54:50Z

Thanks for your review @teozkr!
Only open issue is to create a release of the commons-operator so that we can reference a fixed tag version.
I will work on that but requires some time (we decided to move the common structs into operator-rs - see Architecture meeting notes)

sbernauer · 2022-04-12T06:53:59Z

@teozkr I added the needed common structures to operator-rs 0.16 and upgraded to it.
So everything should be finally done™

examples/superset-with-ldap.yaml

rust/crd/src/lib.rs

rust/operator-binary/src/config.rs

rust/operator-binary/src/main.rs

rust/operator-binary/src/superset_controller.rs

Co-authored-by: Teo Klestrup Röijezon <teo@nullable.se>

…erator into feat/ldapAuthN

…ope>

sbernauer · 2022-04-12T12:27:47Z

Thanks again for another round of feedback, it should be addressed by now.
integration-tests run successfully.

sbernauer · 2022-04-21T12:48:09Z

Thanks for approving @teozkr!
As we already merged the new config validation mechanism by @siegfriedweber in #173 i need to adopt the implementation to use the new approach.
I created #180 for this, i will request review when it is ready to be reviewed. The actual implementation should not change, only using the new validation mechanism.

sbernauer · 2022-04-22T06:56:22Z

Closed in favor of #180

## Description Superseding #145, completely reworked to use new configuration mechanism introduced in #173 For #5 Integration-Tests: https://github.com/stackabletech/integration-tests/pull/175 Use `./create_test_cluster.py --kind --operator superset=0.4.0-pr180` before running the tests

sbernauer added 5 commits March 4, 2022 11:55

Add AuthenticationClass CRD

8f9c301

Add missing AuthenticationClas to CRD command

60f15cc

Update CRD

f89a330

Working version of ldap bind secret

9a1eaf5

First working version

d2a6912

sbernauer changed the title ~~Draft: Support LDAP authentication~~ Draft: Overall concept of LDAP authentication Mar 9, 2022

Avoid clone()

bb257f9

sbernauer requested a review from siegfriedweber March 9, 2022 11:37

sbernauer added 3 commits March 9, 2022 16:45

WIP for Siggi :)

c7dd84d

Deleted examples/openldap.yaml

d42dde5

Clean up comments

6e43c27

sbernauer force-pushed the feat/ldapAuthN branch from 01c8961 to 6e43c27 Compare March 10, 2022 10:18

sbernauer changed the title ~~Draft: Overall concept of LDAP authentication~~ Overall concept of LDAP authentication Mar 10, 2022

sbernauer marked this pull request as draft March 10, 2022 10:21

sbernauer mentioned this pull request Mar 10, 2022

Support LDAP authentication #5

Closed

formatting

929acf3

siegfriedweber requested review from a team and removed request for siegfriedweber March 11, 2022 14:04

sbernauer added 2 commits March 14, 2022 10:18

RBAC: allow superset operator to read authenticationclasses

a8ec102

fix reviewdog secrets

98d0c33

sbernauer force-pushed the feat/ldapAuthN branch from 937a535 to 98d0c33 Compare March 14, 2022 11:59

Remodel optional bools

e54c140

nightkr reviewed Mar 14, 2022

View reviewed changes

sbernauer added 5 commits March 16, 2022 09:43

WIP TLS

23f01ad

Implement tls server verification

876336f

Fix link in docs

e9b27d2

Refactor controller

d691e0f

Refactor controller

b672bb8

sbernauer force-pushed the feat/ldapAuthN branch from 7c0c91c to b672bb8 Compare March 16, 2022 10:30

Use SSL instead of TLS

e789400

sbernauer added 5 commits April 8, 2022 08:56

YAML linter...

1bb1398

Adopt to commons now reside in operators-rs. PART 1

71ae649

Adopt to commons now reside in operators-rs. PART 2

c007c00

Adopt to ommons now reside in operators-rs. PART 3

db705e7

Remove stackable-commons-crd from deny.toml

1898d7d

fix wrong usage of STARTTLS

6082162

nightkr reviewed Apr 12, 2022

View reviewed changes

sbernauer and others added 12 commits April 12, 2022 13:32

Update rust/crd/src/lib.rs

c6299b6

Co-authored-by: Teo Klestrup Röijezon <teo@nullable.se>

Update rust/crd/src/lib.rs

e77ec03

Co-authored-by: Teo Klestrup Röijezon <teo@nullable.se>

Update rust/crd/src/lib.rs

c90ab32

Co-authored-by: Teo Klestrup Röijezon <teo@nullable.se>

Make LdapRolesSyncMoment PascalCase

e313a6a

Update rust/crd/src/lib.rs

94ab162

Co-authored-by: Teo Klestrup Röijezon <teo@nullable.se>

Use IANA port numbers for openldap

f63bbd3

Merge branch 'feat/ldapAuthN' of github.com:stackabletech/superset-op…

7d31126

…erator into feat/ldapAuthN

Remove explicit setting of STARTTLS (AUTH_LDAP_USE_TLS)

d9bb1e2

cargo fmt

f12ef0b

Prefix volume mounts with /stackable

6b3f0eb

make regenerate-charts

ae1e46a

Change &Option<SecretClassVolumeScope> to Option<&SecretClassVolumeSc…

1410743

…ope>

sbernauer mentioned this pull request Apr 14, 2022

[Merged by Bors] - Enable configuration and environment overrides #173

Closed

6 tasks

nightkr approved these changes Apr 21, 2022

View reviewed changes

sbernauer mentioned this pull request Apr 21, 2022

[Merged by Bors] - Add support for LDAP authentication #180

Closed

6 tasks

sbernauer closed this Apr 22, 2022

sbernauer deleted the feat/ldapAuthN branch April 27, 2023 11:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for LDAP authentication #145

Add support for LDAP authentication #145

sbernauer commented Mar 9, 2022 •

edited

Loading

nightkr Mar 14, 2022

sbernauer Mar 21, 2022

nightkr Mar 21, 2022

sbernauer Mar 21, 2022 •

edited

Loading

nightkr Mar 21, 2022

sbernauer Mar 21, 2022

nightkr Mar 21, 2022

sbernauer Mar 31, 2022

nightkr Mar 14, 2022

sbernauer Mar 17, 2022

sbernauer Mar 31, 2022

sbernauer commented Apr 8, 2022

sbernauer commented Apr 12, 2022

sbernauer commented Apr 12, 2022

sbernauer commented Apr 21, 2022

sbernauer commented Apr 22, 2022

Add support for LDAP authentication #145

Add support for LDAP authentication #145

Conversation

sbernauer commented Mar 9, 2022 • edited Loading

Description

Review Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbernauer Mar 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbernauer commented Apr 8, 2022

sbernauer commented Apr 12, 2022

sbernauer commented Apr 12, 2022

sbernauer commented Apr 21, 2022

sbernauer commented Apr 22, 2022

sbernauer commented Mar 9, 2022 •

edited

Loading

sbernauer Mar 21, 2022 •

edited

Loading