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

template: check namespace and region before submitting templates to the client #366

Conversation

pkazmierczak
Copy link
Contributor

@pkazmierczak pkazmierczak commented Jun 26, 2023

Description

If Nomad uses ACLs that restrict our access by namespace or region, we need to extract namespace/region information from the raw templates before submitting them to the client; otherwise we get a 403.

Resolves #364

Reminders

  • Add CHANGELOG.md entry

@pkazmierczak pkazmierczak requested review from jrasell and angrycub June 26, 2023 18:51
@pkazmierczak pkazmierczak force-pushed the pkazmierczak/b-parse-template-properties-before-initializing-the-client branch from 67a968d to 10eef40 Compare June 27, 2023 06:37
Copy link
Member

@jrasell jrasell left a comment

Choose a reason for hiding this comment

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

LGTM.

@pkazmierczak pkazmierczak merged commit ff62241 into main Jun 28, 2023
@pkazmierczak pkazmierczak deleted the pkazmierczak/b-parse-template-properties-before-initializing-the-client branch June 28, 2023 14:03
@pschichtel
Copy link

@pkazmierczak I'm not sure I follow the code correctly, but it seems to apply the regexes to the templates before rendering. is that correct? and if so, what if the region is based on a template parameter like region = [[ .my.some_var | quote ]]?

@pschichtel pschichtel mentioned this pull request Jul 9, 2023
@slonopotamus
Copy link
Contributor

slonopotamus commented Aug 13, 2023

This doesn't work properly. My job has

variable "regions" {
  type = list(object({
    region = string
    count  = number
  }))
  default = [
    {
      region = "EU"
      count  = 1
    }
  ]
  description = "Specifies dedicated server instance count per region"
}

This variable is not a nomad region, it is just a random variable. However, nomad-pack thinks that

  1. This is a Nomad region
  2. It tries to apply this region to all dependent pack jobs too, which is completely wrong because different jobs from different packs might be deployed to different Nomad regions.

Obviously, nomad-pack run fails with No path to region error while nomad-pack render + nomad run just works.

So, this regex-based approach is fundamentally broken because it picks up completely unrelated strings from job file. I suggest reverting this change.

@pkazmierczak
Copy link
Contributor Author

@slonopotamus thanks for reporting this bug and apologies for a late response. I created a ticket and we'll pick it up asap.

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.

Permission denied to plan and run a job with namespace using client type token
4 participants