Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[IND-369] TFE FDO on Nomad #168
[IND-369] TFE FDO on Nomad #168
Changes from 2 commits
854f864
bb96c17
3550383
fcb204c
fdfa330
bf0df66
027820c
307763c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 so that TFE agent can talk to Nomad, right?
There's no
NOMAD_TOKEN
being set anywhere, which makes me think we should be using the Task API socket and not TLS certificates for having TFE talk to Nomad anyways. But the Pack doesn't include what Nomad ACL policies need to be written for the TFE job either.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.
TFE Agent is not supposed to talk to Nomad. Only the TFE container will.
NOMAD_TOKEN
will be injected throughidentity
stanza in TFE job file (not the TFE agent job file) if I'm not wrong. For versions not supportingidentity
we can putNOMAD_TOKEN
in the job file.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.
Ok, good to hear that you're using Workload Identity, but then you don't need the certs to use the Task API socket.
Oops, missed that! Ok.
I was told this was urgently shipping to production and that's why we needed it to be at the top of our priority list to review. Let's just finish the job so that the Nomad Engineering team doesn't have to service another unscheduled interrupt for this Pack.
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.
@tgross if I am not wrong, Task APIs are not a special type of job, right? It's an HTTP API which we can call to start a new job on Nomad.
I'm not sure how we can update the job specs here to accommodate Task API.
The agent job here is started by TFE task worker (https://github.com/hashicorp/tfe-task-worker/blob/main/driver/nomad/nomad.go).
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.
The Task API provides a Unix domain socket at
${NOMAD_SECRETS_DIR}/api.sock
which you'd use instead of the external-facing HTTP endpoint. So no TLS required, just the Workload Identity token.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.
yes, correct, so essentially we will have to modify the backend HTTP call to trigger TFE agent and not the job file.
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.
Why would you need to change the backend? You just need to swap out the
NOMAD_ADDR
environment variable. See hashicorp/nomad#16872 for an example.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.
oh ok, now it makes sense! thanks @tgross
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.
@tgross this won't work with TFE because of this code - https://github.com/hashicorp/terraform-enterprise/blob/b9f00dc90468660cd9ab99ff5334b9504d020974/config/config.go#L476
I'll have to pick this up, change, test and merge before the next release cycle.
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.
Very well. I'll approve.