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

feat:An ability to override Kibana image URL in DA #345

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
+21 −3
Open

feat:An ability to override Kibana image URL in DA #345

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e0afd6d
feat:An ability to override Kibana image URL in DA
Nov 19, 2024
f7020fd
changed the description for kibana_image_reference ,enable_kibana_das…
Nov 20, 2024
52680ff
Merge branch 'main' into override_kibana_image_11649
Nov 20, 2024
c1b72fa
FORMATTED THE VARIABLE DESCRIPTION
Nov 20, 2024
2cafdb4
Merge branch 'main' into override_kibana_image_11649
aatreyee2506 Nov 28, 2024
22e9715
Merge branch 'main' into override_kibana_image_11649
Ak-sky Dec 2, 2024
8a33912
Merge branch 'main' into override_kibana_image_11649
Ak-sky Dec 4, 2024
3d19ed9
updated variable description
Ak-sky Dec 6, 2024
b01dee5
Merge branch 'main' into override_kibana_image_11649
Ak-sky Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions ibm_catalog.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,18 @@
"value": ".elser_model_2_linux-x86_64"
}
]
},
{
"key": "kibana_image_reference"
},
{
"key": "existing_code_engine_project_id"
},
{
"key": "enable_kibana_dashboard"
},
{
"key": "elasticsearch_full_version"
Copy link
Member

Choose a reason for hiding this comment

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

I actually added these in my PR which just got merged. You should rebase your branch with main and you will only need to add kibana_image_reference here

}
]
}
Expand Down
2 changes: 1 addition & 1 deletion solutions/standard/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,7 @@ module "code_engine_kibana" {

apps = {
(local.code_engine_app_name) = {
image_reference = "docker.elastic.co/kibana/kibana:${local.es_full_version}"
image_reference = var.kibana_image_reference != null ? var.kibana_image_reference : "docker.elastic.co/kibana/kibana:${local.es_full_version}"
image_port = 5601
run_env_variables = [{
type = "literal"
Expand Down
10 changes: 8 additions & 2 deletions solutions/standard/variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,12 +351,18 @@ variable "existing_code_engine_project_id" {

variable "enable_kibana_dashboard" {
type = bool
description = "Set it true to deploy Kibana in code engine. NOTE: Kibana image is coming direcly from the official registry (https://www.docker.elastic.co/) and not certified by the IBM."
description = "Set to true to deploy Kibana in Code Engine. NOTE: By default, the Kibana image will be pulled from the official Elastic registry (docker.elastic.co) and is not certified by IBM, however this can be overridden using the `kibana_image_reference` input."
default = false
}

variable "elasticsearch_full_version" {
Copy link
Member

Choose a reason for hiding this comment

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

I really am not a fan of this variable name. It should be consistent with the kibana_image_reference input, and name kibana_image_version - thoughts @Ak-sky ?

Copy link
Member

@Ak-sky Ak-sky Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Why do you end it with _full_version ? The version here is the kibana image version (image tag). I know it has to match the Ealsticsearch version, but its I dont think it needs the word elasticsearch in it?
If we are consistent with kibana_image_reference we should call it kibana_image_version IMHO

Copy link
Member

@Ak-sky Ak-sky Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We can use kibana_image_version, but below are the reasons why I added-

_full_version- because we need major.minor.patch version number and not just major.minor. Kibana and Elasticsearch versions must match exactly for major, minor, and patch versions to avoid runtime errors.

elasticsearch- because to get the full version of the deployed database (elastic search) which is required to deploy Kibana.

Here is a reference which says-

# The data object below calls the ES URL in order to establish the full version of the deployed database
# because that is needed to deploy Kibana.
# The full version gets stored in a local variable es-full-version and then used in the codengine resources

Copy link
Member

@ocofaigh ocofaigh Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@Ak-sky But kibana don't release image tags with major.minor format. All their tags are major.minor.patch format:
image

Also its actually a better practise to lock into a digest instead of a tag version (as they may not be static), so we need to support that too.

So if we breakdown what an image reference looks like with correct terminology, it looks like this:
[registry-url]/[namespace]/[image][:tag|@digest]. So for kibana, that would be:

  • registry-url: docker.elastic.co
  • namespace: kibana
  • image: kibana
  • tag: :8.15.4
    OR
  • digest: @sha256:0ba5cab0b781c3b2a242ef8b5f2544638fa9741fad03c218c58798c06c2bb175

With all this in mind, I think we should have 2 variables:

  • kibana_registry_namespace_image with a default value of docker.elastic.co/kibana/kibana
  • kibana_image_digest with a default value of null. The variable description should say this:

By default, when enable_kibana_dashboard is set to true, the DA will deploy a kibana using an image tag version that is applicable with the version of Elasticsearch deployed. Alternatively you can override this by supplying an image digest in the format of sha256:xxxxx..., however if doing so ensure the digest entered is an applicable version for your Elasticsearch instance.

Copy link
Member

Choose a reason for hiding this comment

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

This is better and even if the user is going to use private registery, they can replace kibana_registry_namespace_image value.

But from security review aspect we will ask the user to input the registry url as you suggested? then no need to set the default values?

description = "(Optional) Full version of the Elasticsearch instance in the format `x.x.x` to deploy Kibana dashboard. If no value is passed, data lookup will fetch the full version using the Elasticsearch API, see https://github.com/elastic/kibana?tab=readme-ov-file#version-compatibility-with-elasticsearch"
description = "Full version of the Elasticsearch instance in the format `x.x.x` to deploy Kibana dashboard. Value is only used if `enable_kibana_dashboard` is true and if no value is passed for `kibana_image_reference`. If no value is passed, data lookup will fetch the full version using the Elasticsearch API, see https://github.com/elastic/kibana?tab=readme-ov-file#version-compatibility-with-elasticsearch"
type = string
default = null
}

aatreyee2506 marked this conversation as resolved.
Show resolved Hide resolved
variable "kibana_image_reference" {
description = "The docker image reference to use for Kibana if `enable_kibana_dashboard` is set to true. If no value is set, it will pull the image from the official Elastic registry (https://www.docker.elastic.co). Ensure to use a version that is compatible with the Elasticsearch version being used."
Copy link
Member

Choose a reason for hiding this comment

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

how about we give this a default value of docker.elastic.co/kibana/kibana and then users will know the expected format if they want to override it?

type = string
default = null
}
Expand Down