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

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

Conversation

aatreyee2506
Copy link

@aatreyee2506 aatreyee2506 commented Nov 19, 2024

Description

Added a variable "kibana_image_reference" to give the users an option to override the kibana image.
Git issue

Release required?

  • No release
  • Patch release (x.x.X)
  • Minor release (x.X.x)
  • Major release (X.x.x)
Release notes content

This PR adds variable "kibana_image_reference" to give the users an option either to go with the default image or give their own image as an input.

Run the pipeline

If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.

Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:

/run pipeline

Checklist for reviewers

  • If relevant, a test for the change is included or updated with this PR.
  • If relevant, documentation for the change is included or updated with this PR.

For mergers

  • Use a conventional commit message to set the release level. Follow the guidelines.
  • Include information that users need to know about the PR in the commit message. The commit message becomes part of the GitHub release notes.
  • Use the Squash and merge option.

@aatreyee2506
Copy link
Author

/run pipeline

@aatreyee2506
Copy link
Author

aatreyee2506 commented Nov 19, 2024

cc: @SarikaSinha @maheshwarishikha

Copy link
Member

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

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

See comments. Also please update the variable description for enable_kibana_dashboard and make sure to reference the new kibana_image_reference input.

Also any time you add a new input variable to a DA solution, you need to also add it to the ibm_catalog.json file. The variables ion that file are sorted in an order that keeps related variables group together. So ensure to add the new variable with the other kibana related variable inputs

solutions/standard/variables.tf Outdated Show resolved Hide resolved
solutions/standard/variables.tf Outdated Show resolved Hide resolved
solutions/standard/main.tf Outdated Show resolved Hide resolved
solutions/standard/variables.tf Show resolved Hide resolved
…hboard and elasticsearch_full_version,changed empty string to null added variable in ibm_catalog.json
@aatreyee2506
Copy link
Author

/run pipeline

Copy link
Member

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

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

see latest comments

ibm_catalog.json Outdated Show resolved Hide resolved
solutions/standard/variables.tf Outdated Show resolved Hide resolved
solutions/standard/variables.tf Outdated Show resolved Hide resolved
solutions/standard/variables.tf Outdated Show resolved Hide resolved
@ocofaigh
Copy link
Member

@aatreyee2506 before running the pipeline, ensure your branch is up to date with latest code in main branch - otherwise the pipeline needs to be re-run again after rebase (which has an associated cost to it since we provision actual resources in our account)

@aatreyee2506
Copy link
Author

/run pipeline

@aatreyee2506
Copy link
Author

/run pipeline

@ocofaigh
Copy link
Member

ocofaigh commented Dec 4, 2024

@aatreyee2506 @Ak-sky Why was the PR closed?

@Ak-sky
Copy link
Member

Ak-sky commented Dec 4, 2024

@aatreyee2506 @Ak-sky Why was the PR closed?

@ocofaigh, it was by mistake, reopening this.

@Ak-sky Ak-sky reopened this Dec 4, 2024
solutions/standard/variables.tf Outdated Show resolved Hide resolved
@Ak-sky Ak-sky requested review from Ak-sky and shemau December 6, 2024 13:11
@aatreyee2506
Copy link
Author

/run pipeline

@ocofaigh
Copy link
Member

ocofaigh commented Dec 9, 2024

We need to fix this regression first: #359 (Pr is under review)
Then come back to this PR

}

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?

@@ -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

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

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

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?

"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

Copy link
Member

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

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

see comments

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

Successfully merging this pull request may close these issues.

4 participants