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

upgrade es and kb to version 8 #3598

Merged
merged 2 commits into from
Nov 19, 2024
Merged

Conversation

vara2504
Copy link
Contributor

@vara2504 vara2504 commented Nov 16, 2024

https://github.com/tigera/calico-private/pull/8119

Description

For PR author

  • Tests for change.
  • If changing pkg/apis/, run make gen-files
  • If changing versions, run make gen-versions

For PR reviewers

A note for code reviewers - all pull requests must have the following:

  • Milestone set according to targeted release.
  • Appropriate labels:
    • kind/bug if this is a bugfix.
    • kind/enhancement if this is a a new feature.
    • enterprise if this PR applies to Calico Enterprise only.

Copy link
Contributor

@hjiawei hjiawei left a comment

Choose a reason for hiding this comment

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

There are some UT failures need to be addressed.

"SYS_CHROOT",
}
// Set the user and group to be the default elasticsearch ID
sc.RunAsUser = ptr.Int64ToPtr(1000)
Copy link
Member

@rene-dekker rene-dekker Nov 18, 2024

Choose a reason for hiding this comment

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

IIUC, the init containers still need to run as root to:

  • set os settings
  • chown the file system

and so the namespace still needs to have the privileged context. The main container now runs as 1000:1000, which is the es user. This is also the user the prepareFS script will chown to.

Is my understanding correct? Have we considered changing the user to 10000:10000 (our default user)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this SecurityContext is for the main ElasticSearch container. The SecurityContext for each init container still uses the root context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Init container "elastic-internal-init-filesystem" still run as root because we need to set the vm max map count "echo 262144 > /proc/sys/vm/max_map_count". This change is only for elasticsearch container.
Currently, Elasticsearch 7, we override the security context of the Elasticsearch container to run as the root user and group. The docker-entrypoint.sh script in Elasticsearch contained logic to switch to user 1000 when it encountered a root user. However, this logic has been removed in Elasticsearch 8. Since the ownership of the /data folder is set to the default Elasticsearch UID, we are encountering an unwritable error when running as the root user.

Copy link
Member

Choose a reason for hiding this comment

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

We spoke offline about a potential follow-up to explore using uid:gid of 10001:10001, but continue with the PR in its current form.

"data",
"ingest",
"master",
"remote_cluster_client",
Copy link
Member

Choose a reason for hiding this comment

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

Should we remove this role?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might need these role - data, ingest, master same as the previous version.
Reg:remote_cluster_client : if we are setting node.roles, then remote_cluster_client role must be explicitly included to enable cross-cluster search. Otherwise, this could result in an access denied issue when loading the Stack Monitoring page in Kibana.

Copy link
Member

Choose a reason for hiding this comment

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

:/ so be it...

@@ -359,14 +360,9 @@ func (es *elasticsearchComponent) podTemplate() corev1.PodTemplateSpec {
}

sc := securitycontext.NewRootContext(false)
Copy link
Contributor

@hjiawei hjiawei Nov 18, 2024

Choose a reason for hiding this comment

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

I think this securitycontext is for the main ElasticSearch container. It should use securitycontext.NewNonRootContext() now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@rene-dekker rene-dekker left a comment

Choose a reason for hiding this comment

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

lgtm

@rene-dekker rene-dekker merged commit c06505e into tigera:master Nov 19, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants