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.

Sign up for GitHub

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

Jump to bottom

CRD: Add basic Stack spec definition #4

Merged
merged 3 commits into from
Oct 30, 2018
Merged

CRD: Add basic Stack spec definition #4

merged 3 commits into from
Oct 30, 2018

Conversation

marclop
Copy link
Contributor

@marclop marclop commented Oct 30, 2018 •
edited
Loading

Description

Added 3 fields to the Stack spec:

  spec:
   version: "6.4.1"
   elasticsearch:
     nodeCount: 3
     # image: docker.elastic.co/an/image:version

@marclop
CRD: Add basic Stack spec definition
40d9fa0 
Signed-off-by: Marc Lopez <marc5.12@outlook.com>
@marclop marclop requested a review from nkvoll October 30, 2018 15:29
@marclop marclop self-assigned this Oct 30, 2018
@marclop
fix json annotation from snake case to camel case
b3833ac 
Signed-off-by: Marc Lopez <marc5.12@outlook.com>
@marclop marclop requested a review from sebgl October 30, 2018 15:56
sebgl
sebgl approved these changes Oct 30, 2018
View reviewed changes
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.

Added a small concern on having the Docker image in the CRD, but that should not prevent from going forward with the PR.

pkg/apis/deployments/v1alpha1/stack_types.go
// ElasticsearchSpec defines the desired state of an Elasticsearch deployment.
type ElasticsearchSpec struct {
// Image represents the docker image that will be used.
Image string `json:"image,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we want the docker image in the resource definition? (sounds like the user could set it to what he wants but we'd want control on that I guess)

Copy link
Contributor

Choose a reason for hiding this comment

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

However I'm also fine with keeping it there for now and remove it later if we need to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd keep it on here for now since it allows us to easily test & swap images, but I agree that we might need to remove this in the future

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with having it for now. We can hardcode a default / hardconfig a default once we get there.

In the future I hope we can still provide it through some advanced opt-in stuff. Of course with all the caveats that come with it, but it's actually a potentially important thing to support.

nkvoll
nkvoll requested changes Oct 30, 2018
View reviewed changes
Copy link
Member

@nkvoll nkvoll left a comment

Choose a reason for hiding this comment

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

Would remove the v-prefix on the version. LGTM otherwise. No need for 2nd round of reviews for me and merge. :)

config/samples/deployments_v1alpha1_stack.yaml Outdated
@@ -6,4 +6,6 @@ metadata:
name: stack-sample
spec:
# Add fields here
foo: bar
version: "v6.4.1"
Copy link
Member

Choose a reason for hiding this comment

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

I would drop the v prefix. It's already a version string by context

pkg/apis/deployments/v1alpha1/stack_types.go
// ElasticsearchSpec defines the desired state of an Elasticsearch deployment.
type ElasticsearchSpec struct {
// Image represents the docker image that will be used.
Image string `json:"image,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with having it for now. We can hardcode a default / hardconfig a default once we get there.

In the future I hope we can still provide it through some advanced opt-in stuff. Of course with all the caveats that come with it, but it's actually a potentially important thing to support.

@marclop
Address review comments
51168cb 
Signed-off-by: Marc Lopez <marc5.12@outlook.com>
@marclop marclop merged commit c2fd9ca into elastic:master Oct 30, 2018
sebgl added a commit that referenced this pull request Mar 10, 2020
@sebgl
Label Pods with a hash of the config for auto rotation (#4)
612a409 
We want Pods to be rotated with the new config.
sebgl added a commit that referenced this pull request Mar 13, 2020
@sebgl
Add the EnterpriseSearch CRD and controller (#2688)
a1bd830 
Introduce the EnterpriseSearch CRD and controller in ECK.

It seems to be working fine so far but has not been extensively tested.
To be used, it requires the image field to be set with a valid Docker image.

This is obviously work in progress, since:

parts of the controller should be refactored to share common code with APMServer and Kibana
most of the association controller should be refactored to share common code with APMServer and Kibana
there are 0 unit tests (mostly because I wanted to go fast in the prototype phase, and because many unit tests should be written along with the refactoring above)
there are 0 E2E tests
it is missing a few features (podDisruptionBudget, secret config refs, version upgrade handling, etc.)

---

Built from many commits:

* Add Enterprise Search CRD

* Add the Enterprise Search controller - minimal deployment ok

* Add a working minimal Enterprise Search sample

* Reconcile a configuration secret instead of env vars (#2)

This is cleaner, more similar to what we do for other resources, and
avoids leaking secrets in Pod environment variables.

Note that Pods do not get automatically rotated when config changes so
far.

* Setup the EnterpriseSearch - Elasticsearch association controller

This is heavily based on the APMServer association controller.
Would largely benefit from some common refactoring in the future.

* Patch the EnterpriseSearch CRD to fix the subresources

I don't know why this isn't there in the first place.
I plan to investigate in the future, but for now this is required to
perform status updates.

* Label Pods with a hash of the config for auto rotation (#4)

We want Pods to be rotated with the new config.

* Reuse the JAVA_OPTS const (#5)

* Setup a readiness probe for smooth deployment rotation (#6)

Let's query /swiftype-app-version which seems to be the default out
there.
The initial bootstrap is pretty slow so I set an InitialDelaySeconds of
60sec.

* Generate the default enterprise_search user in a secret (#7)

I *think* this user can end up being useful if Elasticsearch is down
hence the native realm cannot be used.

* Strip out private Docker image from default sample (#8)

* Fix EnterpriseSearch CRD generation (#9)

This fixes CRD generation following a rebase on master.

* Use self-signed TLS certificates by default (#10)

Generate self-signed certificates for Enterprise Search and enable TLS
encryption by default. This can be disabled in the EnterpriseSearch
spec, and user-provided certificates can be used instead.

* Switch http/https protocol in readiness probe and config (#11)

Depending on the TLS spec, let's use the correct protocol: http or
https.

* Inject a hash of TLS certs for pod rotation (#12)

Automatically rotate the Pod if:
- the ent search config changes
- the ent search TLS certs change
- the referenced Elasticsearch certs change

This is done through hashing them all and using that hash as a label in
the podTemplate.

* Cleanup the code files (#13)

* Minor cosmetic housekeeping

* goimports files

* Add missing license headers

* Fix linter warnings

* Add api docs

* Struct comments improvements

* Regenerate CRDs and api docs

* Fixes from PR review

* Rely on the global scheme.Scheme

* Setup RBAC for Enterprise Search
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nkvoll nkvoll nkvoll requested changes

@sebgl sebgl sebgl approved these changes

Assignees

@marclop marclop

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@marclop @nkvoll @sebgl