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

Prefix concatenation when using stages-multitenant results in prefix longer than 9 chars #1314

Closed
derailed-dash opened this issue Apr 10, 2023 · 10 comments · Fixed by #1346 or #2016
Closed

Comments

@derailed-dash
Copy link
Contributor

  • The variable var.prefix in stages/0-bootstrap/terraform.tfvars must be no longer than 9 chars.
  • The short_name in stages-multitenant/0-bootstrap-tenant/terraform.tfvars should be 3 or 4 letters.
  • In stages-multitenant/0-bootstrap-tenant/main.tf, the output prefix var is created by concatenating the two above variables, i.e.
    • prefix = join("-", compact([var.prefix, var.tenant_config.short_name]))
  • Consequently, the newly created prefix variable in stages-multitenant/0-bootstrap-tenant.auto-tfvars.json is longer than 9 characters.
  • But the validation in stages-multitenant/1-resman-tenant/variables.tf requires that this prefix be no longer than 9 characters.

Is the validation in stages-multitenant/1-resman-tenant/variables.tf correct? If so, then the instructions need to be updated to explain the consequences of using a prefix and short_name that, when concatenated, will be over 9 characters.

@derailed-dash
Copy link
Contributor Author

In the meantime, my workaround for completing stage 1-resman-multitenant was to change the prefix validation in variables.tf to be:

  validation {
    condition     = try(length(var.prefix), 0) < 14
    error_message = "Use a maximum of 13 characters for prefix (which incudes tenant short name)."
  }

This seems to have worked fine, and I've been able to execute this stage. I haven't yet checked it in with a PR, as I wasn't sure if there are any downstream negative consequences.

@ludoo
Copy link
Collaborator

ludoo commented Apr 11, 2023

Thanks for opening this. I have a vage recollection of checking when I developed the multitenant stages, and 13 or 14 being the magic number like you say. I also remember fixing the validation, but I'm eveidently wrong.

This is a sample of project ids created by different stages minus prefix, with their word count

12 audit-logs-0
10 iac-core-0
16 prod-net-spoke-0
15 prod-sec-core-0
19 prod-gke-clusters-0
19 prod-data-orc-cmp-0

Tenant prefix by default combines org-level prefix and tenant shortname, so 30 (maximum project id length) minus 1 (dash joining org-level and tenant-level), minus 19 (longest project id) leaves 10 characters.

Which is larger than the current condition in tenant stages, but by only 1 char. And it's also suboptimal as an org-level prefix of 9 would only leave 1 character for tenant short names.

The only leeway we have is in stage 3 names: if we set a limit to 16 for those, we get back 3 characters that combined with the 1 above give a maximum of 4 characters for tenant shortnames, which actually matches what we've seen at customers. GKE is easy as we just need to shorten clusters, the data platform is a bit more complex we need to sync with @lcaggio .

The other alternative is to shorten the check for org-level stages so the maximum prefix length is 7, and we get 3 characters minimum for tenants.

This was probably longer than you asked for. :) What are your thoughts on this? And @juliocc please chime in too of course.

@derailed-dash
Copy link
Contributor Author

derailed-dash commented Apr 11, 2023

Thank you for sharing this info!

What about...

  • Shorten the org-level prefix to a max of 8 chars. (Gives +1.)
  • Set stage 3 limit to 18 chars. Easier to achieve for Data Platform than reducing to 16, I'm guessing. (Gives +1.)

Together with the original 1 character, this gives us a 3 char tenant short name.

@ludoo
Copy link
Collaborator

ludoo commented Apr 11, 2023

Sounds like a plan. We can also leave the org-level to 9, then with the 18 for stage 3 you have 2 chars minimum for tenants, plus any left over from the org 9. WDYT?

@derailed-dash
Copy link
Contributor Author

derailed-dash commented Apr 11, 2023

I guess it depends if you think 2 chars are sufficient for the top-level tenants? I guess most enterprises don't have too many at this level, so probably okay. But 2 chars doesn't allow for a very descriptive short name.

However, you could encourage users to think about this from the start of the process. So, if we stuck with keeping 9 chars for org prefix, maybe add a note to the 0-bootstrap readme to say something like "If you plan to use multitenant, and you want your top-level tenant short names to be more than 2 characters long, do not use the full 9 characters for your organization prefix."

Would that work? If so, and if you'd like me to contribute to the 0-bootstrap readme, then I can do that tonight. (But I understand if you'd rather do it yourselves!)

@ludoo
Copy link
Collaborator

ludoo commented Apr 11, 2023

Yes, that is exactly what I meant: users should take into account limitations right from the start. If you want to go ahead with a PR, by all means do so! And thanks again for raising this. :)

@derailed-dash
Copy link
Contributor Author

Awesome, will do! Thanks for coming back so quickly, and for giving me the opportunity to contribute!

@drebes
Copy link
Member

drebes commented Apr 11, 2023

Food for thought: on a recent multi-tenant implementation, we used the global prefix for the tenants (that is, no tenant prefix, each tenant chooses their own global prefix). Since tenants are "mostly independent", we thought it made sense.

@derailed-dash
Copy link
Contributor Author

@ludoo Thanks for approving the PR. I haven't closed the issue yet, as I haven't touched anything in stage 3.

@juliocc
Copy link
Collaborator

juliocc commented Jan 12, 2024

This seems to be an issue again. We have to double check the length of all DP SAs

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