-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Add additional Linux distros and Consul editions to Enos scenarios #25983
Add additional Linux distros and Consul editions to Enos scenarios #25983
Conversation
CI Results: |
Build Results: |
df70052
to
a6d67d1
Compare
enos/modules/vault_verify_raft_auto_join_voter/scripts/verify-raft-auto-join-voter.sh
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor questions/nits, but looks good AFAICT!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What a journey!
I'm very excited to see this land. I've interspersed some thoughts along with feedback.
Before merge:
- Fix sampling for zip bundles. Only test on Amazon linux and Ubuntu.
- Remove the Consul Enterprise filtering out of CE samples
- Remove the leap exclusions from the sample since they're excluded already by the scenario
- Remove the host info module that we're not using
- Address feedback for
install-packages.sh
- Use install_packages module in the
vault_cluster
module - An enterprise counterpart of the PR with enterprise samples. For that we'd just create a branch when this is ready to go against enterprise and add the samples as separate commit. That will allow us to test all of these changes there before we merge it here. When it comes time to merge you could rebase away the commits that aren't samples and then merge it when it CE change gets to enterprise.
Some followup tasks which can be done on this or later:
- Move
consul_version
to the sample attributes and out of scenario matrices - Update the distro-repo-setup script to take in the DISTRO as well
- Update the RHEL versions we test
- Add Ubuntu 24.04 to the distros for Ubuntu
@@ -97,6 +97,11 @@ jobs: | |||
ENOS_VAR_vault_product_version: ${{ needs.metadata.outputs.vault-version }} | |||
ENOS_VAR_vault_revision: ${{ inputs.vault-revision }} | |||
ENOS_VAR_vault_license_path: ./support/vault.hclic | |||
ENOS_VAR_distro_version_amzn2: ${{ matrix.attributes.distro_version_amzn2 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should move the consul_version
into the sample attributes. Definitely not required in this PR but something we should track and do after this is merged. It'll reduce our matrix size but I think the entire sample field with attributes will become the new source of truth and we should make the enos CLI capable of reporting that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will create a ticket for this.
ENOS_VAR_distro_version_leap: ${{ matrix.attributes.distro_version_leap }} | ||
ENOS_VAR_distro_version_rhel: ${{ matrix.attributes.distro_version_rhel }} | ||
ENOS_VAR_distro_version_sles: ${{ matrix.attributes.distro_version_sles }} | ||
ENOS_VAR_distro_version_ubuntu: ${{ matrix.attributes.distro_version_ubuntu }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite excited for additional RHEL and Ubuntu versions in the sample field! Related: Ubuntu 24.04 drops next month. I wonder if there are already AMIs out that we can add into our module so we can get a head start on testing for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will opt not to include this in this particular PR, but will create a ticket for it.
enos/enos-globals.hcl
Outdated
artifact_sources = ["local", "crt", "artifactory"] | ||
artifact_types = ["bundle", "package"] | ||
backends = ["consul", "raft"] | ||
backend_license_path = abspath(var.backend_license_path != null ? var.backend_license_path : joinpath(path.root, "./support/consul.hclic")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote it but couldn't remember if joinpath()
requires the file to actually exist or not, the latter of which I didn't want to be the case. But I've looked it up and it just creates the path but doesn't stat it.
enos/enos-globals.hcl
Outdated
# When installing Vault RPM packages, SLES searches for openssl by a different name | ||
# than the one that comes pre-installed on the AMI. Therefore we add the | ||
# "correctly" named one in our package installation before installing Vault. | ||
# sles = ["ncat", "openssl"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# sles = ["ncat", "openssl"] |
enos/enos-globals.hcl
Outdated
amzn2 = ["nc"] | ||
leap = ["netcat", "openssl"] | ||
rhel = ["nc"] | ||
# When installing Vault RPM packages, SLES searches for openssl by a different name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is such a bummer. The other package does provide openssl
but isn't named openssl
, which RPM doesn't know how to handle.
@@ -0,0 +1,45 @@ | |||
#!/bin/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As they do the same thing, we probably ought to remove this script and use the install_packages
module in vault_cluster
instead.
enos/modules/vault_cluster/scripts/start-audit-socket-listener.sh
Outdated
Show resolved
Hide resolved
@@ -1,6 +1,12 @@ | |||
# Copyright (c) HashiCorp, Inc. | |||
# SPDX-License-Identifier: BUSL-1.1 | |||
|
|||
variable "arch" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see where we use the arch
, consul_bind_addr
, distro
or package_manager
variables in the module.
enos/enos-samples-ce-build.hcl
Outdated
@@ -200,6 +217,7 @@ sample "build_ce_linux_amd64_zip" { | |||
arch = ["amd64"] | |||
artifact_type = ["bundle"] | |||
artifact_source = ["crt"] | |||
consul_edition = ["ce"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For zip bundles I think we ought to only test on ubuntu
and amz2
while leaving all of the distros for the system packages. The idea is that when we're testing our zip bundles we'll favor testing on distros that don't have vendor markup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few tiny nits but otherwise I think we should get an Ent PR created to test with.
The only thing I'm concerned about is the install location on RHEL. Did that change on purpose/how did it work before?
@@ -191,6 +191,7 @@ sample "build_ce_linux_amd64_zip" { | |||
arch = ["amd64"] | |||
artifact_type = ["bundle"] | |||
artifact_source = ["crt"] | |||
distro = ["amzn2", "ubuntu"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍
# and accept SUSE's terms of use. You can do this at the links below. If the AWS account | ||
# you are using is already subscribed, this confirmation will be displayed on each page. | ||
# openSUSE Leap arm64 subscription: https://aws.amazon.com/marketplace/server/procurement?productId=a516e959-df54-4035-bb1a-63599b7a6df9 | ||
# openSUSE leap amd64 subscription: https://aws.amazon.com/marketplace/server/procurement?productId=5535c495-72d4-4355-b169-54ffa874f849 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we already subscribe to these in the vault_enterprise_ci account as well? I guess we'll find out with the ent PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet but I will do that with the ent PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we need to put this information in the scenario descriptions so it's easy to see. I'll make a note do that in my next PR that adds descriptions when I rebase on this.
54d078d
to
feba8cd
Compare
feba8cd
to
6d51387
Compare
75d5c76
to
7d4a65a
Compare
7d4a65a
to
273d624
Compare
273d624
to
a7c0e1a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple small things I think we should clean up before merge but after lets push the green!
@@ -139,7 +142,7 @@ scenario "dev_pr_replication" { | |||
// If you are using an ent edition, you will need a Vault license. Common convention | |||
// is to store it at ./support/vault.hclic, but you may change this path according | |||
// to your own preference. | |||
vault_install_dir = matrix.artifact == "zip" ? var.vault_install_dir : global.vault_install_dir_packages[matrix.distro] | |||
vault_install_dir = matrix.artifact == "zip" || matrix.artifact == "local" ? global.vault_install_dir["bundle"] : global.vault_install_dir["package"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on this, it seems like we have no real use of vault_install_dir
being a variable at all since we ignore it and use our global map. Seems like we should remove it entirely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our global map uses the artifact_type (bundle or package), which we don't have in this scenario, so this is like a re-mapping of our matrix.artifact to those.
aws_region = ["us-east-1", "us-west-2"] | ||
aws_region = ["us-east-1", "us-west-2"] | ||
distro_version_amzn2 = ["2"] | ||
distro_version_leap = ["15.4", "15.5"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great use of sample attrs. I'm still trying to figure out when it's better to use samples attrs or a matrix variant, but it feels like to me if it doesn't change logic it should be a sample attr going forward.
"ent.fips1402" = "vault-enterprise-fips1402_", | ||
"ent.hsm" = "vault-enterprise-hsm_", | ||
"ent.hsm.fips1402" = "vault-enterprise-hsm-fips1402_", | ||
amzn2 = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Copyright (c) HashiCorp, Inc. | ||
# SPDX-License-Identifier: BUSL-1.1 | ||
|
||
terraform { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is my new favorite module in the entire codebase
enos/modules/disable_selinux/main.tf
Outdated
resource "enos_remote_exec" "make_selinux_permissive" { | ||
for_each = var.hosts | ||
|
||
# environment = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove this comment?
echo "Installing Dependencies: ${PACKAGES}" | ||
|
||
# Use the default package manager of the current Linux distro to install packages | ||
case $PACKAGE_MANAGER in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Massive improvement in our implementation here.
enos/modules/seal_pkcs11/main.tf
Outdated
@@ -95,6 +95,7 @@ module "target" { | |||
ami_id = module.ec2_info.ami_ids["arm64"]["ubuntu"]["22.04"] | |||
cluster_tag_key = local.id | |||
common_tags = var.common_tags | |||
disable_selinux = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, won't this make RHEL 9.3 not work with pkcs11 since we disable it for vault to start up there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this was from earlier work, it's not needed now. Removed.
@@ -24,6 +24,12 @@ variable "common_tags" { | |||
default = { "Project" : "vault-ci" } | |||
} | |||
|
|||
variable "disable_selinux" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we modified this interface we should also update the other target modules so that if for some reason we decide to try spot instances again it should work.
enos/modules/vault_cluster/main.tf
Outdated
@@ -15,6 +15,10 @@ terraform { | |||
data "enos_environment" "localhost" {} | |||
|
|||
locals { | |||
distro_version_sles = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see where we use this. Also, we should be getting this from variables, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep this is leftover from earlier attempts. Removed.
@@ -37,6 +41,13 @@ locals { | |||
"pkcs11" = null | |||
} | |||
leader = toset(slice(local.instances, 0, 1)) | |||
netcat_command = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I though we created this in globals. Why are we redefining it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In globals, we have a list of the packages that need to be installed for each distro:
distro_packages = {
amzn2 = ["nc"]
leap = ["netcat", "openssl"]
rhel = ["nc"]
sles = ["nc", "openssl"]
ubuntu = ["netcat"]
We use a separate set of package names here for two reasons:
- We are getting the name of the distro from
enos_host_info
, which returns slightly different distro names than the ones we use throughout the scenarios (e.g.opensuse-leap
vsleap
,amzn
vsamzn2
). - We include only the netcat CLI command used by the distro, as a single string. It seemed rather fragile to try to pick "the netcat one" from a list of multiple packages in our globals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. Too many maps!
6a14482
to
a7559f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think testing each of the other target modules really quick and fixing up the disable section and then merge.
count = var.disable_selinux == true ? 1 : 0 | ||
|
||
hosts = { for idx in range(var.instance_count) : idx => { | ||
public_ip = aws_instance.targets[idx].public_ip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we test this change? I feel like this probably needs to be data.aws_instance.targets....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recording here that we discovered some issues with the target_ec2_fleet module so will skip adding disable_selinux to this and the target_ec2_spot_fleet module for now, since we are not actively using either of those.
count = var.disable_selinux == true ? 1 : 0 | ||
|
||
hosts = { for idx in range(var.instance_count) : idx => { | ||
public_ip = aws_instance.targets[idx].public_ip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same goes for this instance
@@ -37,6 +41,13 @@ locals { | |||
"pkcs11" = null | |||
} | |||
leader = toset(slice(local.instances, 0, 1)) | |||
netcat_command = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. Too many maps!
be777a0
to
a8151e5
Compare
…ios and modules Add Consul edition support to Enos scenarios and modules Add Linux distros and Consul edition to Enos samples Bump RHEL versions to 9.3 and 8.9
675d40d
to
b6c82dd
Compare
This PR:
This more than doubles the number of Linux distros/versions we test Vault on, and allows us to test integrations of Vault with Consul enterprise in addition to Consul community edition.