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

Hide the Elasticsearch field in CRDs #1653

Closed
charith-elastic opened this issue Aug 28, 2019 · 6 comments · Fixed by #1687
Closed

Hide the Elasticsearch field in CRDs #1653

charith-elastic opened this issue Aug 28, 2019 · 6 comments · Fixed by #1687
Assignees

Comments

@charith-elastic
Copy link
Contributor

From the discussion over at #1141 we came to the realisation that the Elasticsearch field in the Kibana and APM CRDs should not be user configurable. This is because a user who wishes to configure access to an external Elasticsearch cluster should be using the native configuration options provided by Kibana and APM products.

The operators still use the Elasticsearch field to store information about the association with Elasticsearch. There are several options we can consider here:

  • Use naming conventions to discover information about the resources that are currently stored in the BackendElasticsearch and ElasticsearchOutput objects.
  • Make the Elasticsearch field private so that it doesn't appear in the generated CRD spec (Kubebuilder does not seem to have an annotation to hide a field) and add methods to the parent object to allow the operators to read and modify the field.
  • Introduce a new CRD for Elasticsearch association that is managed by the operator to store the data it needs.
@charith-elastic charith-elastic changed the title Review the Elasticsearch field in CRDs Hide the Elasticsearch field in CRDs Aug 28, 2019
@pebrc
Copy link
Collaborator

pebrc commented Aug 28, 2019

Introduce a new CRD for Elasticsearch association that is managed by the operator to store the data it needs.

We already had that and removed it again because CRDs are too user visible and we did not want to give that implementation detail so much room. See #747 for example

@charith-elastic
Copy link
Contributor Author

@pebrc My understanding from looking at #747 is that previously we required users to explicitly create an association object to create the link between stack products. That is indeed too much to ask from users. However, what I am suggesting is that the association object should be created and managed solely by the operator as a way to manage its internal state. Typical users should not even be aware that it exists. The flow would be pretty much that same as what we have right now but instead of using the Elasticsearch field to store the state, the operator will store it in a separate association object that users are not allowed to modify. WDYT?

@charith-elastic charith-elastic self-assigned this Aug 29, 2019
@charith-elastic
Copy link
Contributor Author

Another option might be to use annotations to store the required information.

@pebrc
Copy link
Collaborator

pebrc commented Aug 30, 2019

The flow would be pretty much that same as what we have right now but instead of using the Elasticsearch field to store the state, the operator will store it in a separate association object that users are not allowed to modify. WDYT?

I think my concern is that CRDs are very visible see as an example https://operatorhub.io/operator/elastic-cloud-eck We would 'leak' these implementation details by making it a CRD.

var expectedEsConfig kbtype.BackendElasticsearch
expectedEsConfig.CertificateAuthorities.SecretName = caSecretName
expectedEsConfig.URL = services.ExternalServiceURL(es)
expectedEsConfig.Auth.SecretKeyRef = association.ClearTextSecretKeySelector(&kibana, kibanaUserSuffix)

Looking at the implementation of the Kibana association controller, it seems that the association controller itself is already using conventions to determine what to configure in the BackendElasticsearch attributes. So I wonder if it would be acceptable to go down the conventions route (leaving always the door open for explicit configuration via the config attribute by the user)

@charith-elastic
Copy link
Contributor Author

It is certainly possible to use conventions for discovering related K8S objects but that won't extend to things like URLs. The naming PR I am currently working on has highlighted some edge cases surrounding names so my preference right now is to not rely on naming and use annotations to store the information instead.

Some of the information we need such as references to secrets are Kubernetes specific and will not fit in the config section. If we want user-configurability, my feeling is that annotations would help with that as well.

@pebrc
Copy link
Collaborator

pebrc commented Aug 30, 2019

I am OK with using annotations, but I am not following the argument regarding unspecified edge cases around naming here.

If the association controller calls services.ExternalServiceURL only to inject the result of that into BackendElasticsearch then I don't see a fundamental problem with the Kibana controller deriving the URL itself using the exact same function. The only exception being separation of concerns and creating a dependency on the elasticsearch package (which is a valid concern).

As to how to solve the indirection between the two resources: annotations are an option the other is a secret/config map containing the necessary configuration which again could be discovered by convention or a label (we are already doing that for the CA certs and user/pw, there is even an argument for consolidating everything in one secret maybe).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants