Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove User CRD #948

Merged
merged 35 commits into from
Jun 6, 2019
Merged

Remove User CRD #948

merged 35 commits into from
Jun 6, 2019

Conversation

barkbay
Copy link
Contributor

@barkbay barkbay commented May 27, 2019

Fixes:

Still a WIP:

  • Check if it is relevant to add some tests
  • Check if we should cache the hashed password as an annotation to save some CPU cycles
  • Check/trim usernames

Todo in an other PR:

  • Factorize some code between Kibana/APM (lots of TODO in the APM code)

@barkbay
Copy link
Contributor Author

barkbay commented May 28, 2019

jenkins test this please

@barkbay
Copy link
Contributor Author

barkbay commented May 29, 2019

Todo in some other PRs (I will create them after this one is merged):

  • Factorize some code between Kibana/APM (lots of TODO in the APM code)
  • Resource name length is only addressed in the context of Elasticsearch (see Trim cluster and pod names up to a limit #318), we should do the same for Kibana/APM and associations as they are used in labels
  • They are no (premature) optimization for the password comparison, it has some implications that need to be discussed

Copy link
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good 👍 For me the only open question before merging is about the ownership of the user secret (see my comments)

operators/pkg/controller/common/user/label.go Outdated Show resolved Hide resolved
operators/pkg/controller/common/user/provided.go Outdated Show resolved Hide resolved
operators/pkg/controller/common/user/provided.go Outdated Show resolved Hide resolved
operators/pkg/controller/common/user/utils.go Outdated Show resolved Hide resolved
}

// ChecksUser checks that a secret contains the required fields expected by the user reconciler.
func ChecksUser(t *testing.T, secret *corev1.Secret, expectedUsername string, expectedRoles []string) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this introduce a dependency to testing in production code?

Copy link
Contributor Author

@barkbay barkbay Jun 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question !
Functions in *_test.go files are not available for import by other tests. Looks like there is no easy way to share some code between tests.

I did some tests and it looks like there is not dependency as long as the function is not explicitly used in some production code.

For example if ChecksUser is explicitly called elsewhere in the production code the binary is 1MB bigger and all the test tooling seems to be embedded in the binary:

$ strings elastic-operator|grep -i 'testing\.'
*testing.T
**testing.T
*[]*testing.T
*testing.common
**testing.common
*testing.matcher
*func(*testing.T)
*testing.indenter
...

If the function ChecksUser is shared by some tests but not in the production code (like in this PR):

$ strings elastic-operator|grep -i 'testing\.'
testing.init
/usr/local/Cellar/go/1.11.4/libexec/src/testing/testing.go
$

it is the same result with the code on the current master branch btw

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Nice analysis!

operators/pkg/controller/kibanaassociation/user.go Outdated Show resolved Hide resolved
@barkbay barkbay requested a review from pebrc June 5, 2019 07:33
@barkbay
Copy link
Contributor Author

barkbay commented Jun 6, 2019

jenkins test this please

Copy link
Contributor

@sebgl sebgl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, great work!

@barkbay
Copy link
Contributor Author

barkbay commented Jun 6, 2019

jenkins test this please

@barkbay barkbay merged commit be4440b into elastic:master Jun 6, 2019
@pebrc pebrc added >enhancement Enhancement of existing functionality v0.9.0 labels Jul 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement Enhancement of existing functionality v0.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants