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

roachprod: pass --tenant-scope on cert gen from old binaries #98198

Closed
wants to merge 1 commit into from

Conversation

healthy-pod
Copy link
Contributor

Previously, TLS client certs were scoped to the system tenant by default. After #97585, they became valid for all tenants by default (unless --tenant-scope is pass).

This caused the multitenant-upgrade roachtest to fail when certs are generated using the latest roachprod binary from an old cockroach binary.

This change ensures that such certs are not scoped to the system tenant only by passing --tenant-scope.

Release note: None
Epic: none

Closes #97016

Previously, TLS client certs were scoped to the system tenant
by default. After cockroachdb#97585, they became valid for all tenants by
default (unless `--tenant-scope` is passed).

This caused the multitenant-upgrade roachtest to fail when
certs are generated using the latest roachprod binary from
an old cockroach binary.

This change ensures that such certs are not scoped to the
system tenant only by passing `--tenant-scope`.

Release note: None
Epic: none

Closes cockroachdb#97016
@healthy-pod healthy-pod requested review from knz and ajstorm March 8, 2023 03:02
@healthy-pod healthy-pod requested a review from a team as a code owner March 8, 2023 03:02
@healthy-pod healthy-pod requested review from srosenberg and smg260 and removed request for a team March 8, 2023 03:02
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @healthy-pod, @smg260, and @srosenberg)


pkg/roachprod/install/cluster_synced.go line 1233 at r1 (raw file):

rm -fr certs
mkdir -p certs
VERSION=$(%[1]s version | grep "Build Tag:" | sed -e 's/Build Tag:[[:space:]]*//' | cut -d- -f1)
VERSION=$(%[1]s version --build-tag)
VERSION=${VERSION::3}
if [[ $VERSION = v22 ]]; then
   ...

This makes me think we might want to backport my change to v22.1 and v22.2 and call it a day. Do we still expect to publish v22.1 releases? what do you think?

@healthy-pod
Copy link
Contributor Author

VERSION=$(%[1]s version --build-tag)
VERSION=${VERSION::3}
if [[ $VERSION = v22 ]]; then
   ...

Nice

This makes me think we might want to backport my change to v22.1 and v22.2 and call it a day. Do we still expect to publish v22.1 releases? what do you think?

My concerns are

  1. We would have to wait for a 22.2.x release to have the multitenant-upgrade test running normally again.
  2. A more general solution (i.e. in roachprod) will be future proof
  3. cli: don't scope TLS client certs to a specific tenant by default #97585 changes the behaviour of a flag. Would it still meet backport requirements?

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.

roachtest: multitenant-upgrade failed
3 participants