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

Parse term gws #10743

Merged
merged 1 commit into from
Jul 7, 2021
Merged

Parse term gws #10743

merged 1 commit into from
Jul 7, 2021

Conversation

holtwilkins
Copy link
Contributor

@holtwilkins holtwilkins commented Jun 11, 2021

Parse terminating gateway stanzas. While Nomad supports terminating GWs, its own parse function doesn't actually support them and populate the structs. This attempts to address that.

@hashicorp-cla
Copy link

hashicorp-cla commented Jun 11, 2021

CLA assistant check
All committers have signed the CLA.

@holtwilkins
Copy link
Contributor Author

Not sure what's going on with the failed checks, but it doesn't look like it's because of my changes?

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

@holtwilkins The HCL1 parser you're editing here will be going away once we're satisfied the HCL2 parser is fully correct. So generally speaking new features are only being parsed by the HCL2 parser. (And we verify that terminating gateways are being parsed with one of our E2E tests)

Going to ping @shoenig and @notnoop for their thoughts on this though.

@notnoop
Copy link
Contributor

notnoop commented Jun 11, 2021

Thanks @holtwilkins for contributing. I wanted to highlight that Nomad has switched to using jobspec2 package that supports HCLv2 introduced in 1.0.0. The jobspec package is is to be deprecated and is only kept is a fallback to ease adoption from pre-1.0.0 and handle incompatibilities. We realize we haven't documented this - we plan to update the package docs and adding a sunset date for jobspec package.

Would love to hear the motivation of the change further. Would using the jobspec2 package satisfy your needs?

@holtwilkins
Copy link
Contributor Author

Unfortunately we're not quite ready to move to hcl2 for Nomad. We're in the process of migrating stacks, and will move fully to hcl2 once the migration is complete in a few months. I tried using jobspec2 for our jobs, but it throws lots of errors because of new paradigms required for escaping templates:

Invalid template directive; A template directive keyword ("if", "for", etc) is expected at the beginning of a %{ sequence.

In the meantime, HCL1 supports terminating gateways, so it should be the case that the HCL1 job parser does as well, no?

@holtwilkins
Copy link
Contributor Author

Also @notnoop & @tgross , the same error gateway -> invalid key: terminating is thrown by the nomad provider when trying to instantiate a job using the default behaviour of nomad_job (i.e. HCL1 job spec).

The fact that nomad plan <HCL1 jobfile that declares a term gw> works, but the same jobfile when using Parse in the go API or with the nomad_job terraform provider doesn't, makes it seem like this is a bug that needs to be addressed, no?

@notnoop
Copy link
Contributor

notnoop commented Jun 17, 2021

Very good points. Indeed, the new escaping change has been painful for our users, and we do intend to follow up with a disable interpolation option (e.g. heredoc syntax with <<'EOF').

We initially intended the hcl1 parser as a fallback to help users upgrade to 1.0.x but faced unexpected incompatibilities in hcl2 and don't want to change the job files yet. We reasoned that users that modify the job spec and take advantage of the new 1.X features can also resolve the incompatibilities.

However, as you pointed out, it can be more painful that we like, and it makes sense to backport 1.0.X features back to HCL1 parser until we set better expectation and update the terraform provider.

In the meantime, HCL1 supports terminating gateways, so it should be the case that the HCL1 job parser does as well, no?

The fact that nomad plan <HCL1 jobfile that declares a term gw> works, but the same jobfile when using Parse in the go API or with the nomad_job terraform provider doesn't, makes it seem like this is a bug that needs to be addressed, no?

nomad plan by default uses HCLv2, using the jobspec2 package. HCL1 doesn't support gw, neither does its parser. So it make sense why it will pass, while jobspec and the terraform plan fails. We did not intend HCL1 to support new 1.X features.

That being said, I agree this is confusing and that the documentation is lacking. Nomad users should not need to know Nomad's parser internals to navigate their work.

I'll discuss this PR and the terraform provider with the team and follow up. Thank you again for your the PR and for highlighting the problem!

Copy link
Contributor

@notnoop notnoop left a comment

Choose a reason for hiding this comment

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

The code looks great. I made a couple of suggestions and intend to the PR and follow up with other missed fields. Thank you so much!

jobspec/parse_service.go Show resolved Hide resolved
jobspec/parse_service.go Outdated Show resolved Hide resolved
jobspec/parse_service.go Outdated Show resolved Hide resolved
Comment on lines +1563 to +1564
Name: "service2",
SNI: "myhost",
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for including the test and testing the nil ca_file cases.

@holtwilkins
Copy link
Contributor Author

Hey @notnoop I think everything should be good now, if you could give another look.

Copy link
Contributor

@notnoop notnoop left a comment

Choose a reason for hiding this comment

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

Thank you @holtwilkins for the PR again. This looks good - I'll inspect the other fields and update them too!

@notnoop notnoop merged commit 712ad49 into hashicorp:main Jul 7, 2021
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants