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

Combine included VCL modules into main and parse once #93

Merged
merged 8 commits into from
Sep 4, 2022

Conversation

ysugimoto
Copy link
Owner

@ysugimoto ysugimoto commented Aug 15, 2022

Fixes #78

Resolve include modules that are declared at include "xxx"; statement, replace the parsed AST into its declaration position, and lint whole codes as a single VCL tree

This implementation still does not use DFS style parsing but it's should be lint properly.

Example

From #78, the example case of VCLs:

# dependency1.vcl
sub foo {
   set req.backend = httpbin_org;
}
# main.vcl

include "dependency1";

backend httpbin_org {
  .connect_timeout = 1s;
  .dynamic = true;
  .port = "443";
  .host = "httpbin.org";
  .first_byte_timeout = 20s;
  .max_connections = 500;
  .between_bytes_timeout = 20s;
  .share_key = "xei5lohleex3Joh5ie5uy7du";
  .ssl = true;
  .ssl_sni_hostname = "httpbin.org";
  .ssl_cert_hostname = "httpbin.org";
  .ssl_check_cert = always;
  .min_tls_version = "1.2";
  .max_tls_version = "1.2";
}

sub vcl_recv {
   call foo;
}

Then resolved VCL should be:

sub foo {
   set req.backend = httpbin_org;
}

backend httpbin_org {
  .connect_timeout = 1s;
  .dynamic = true;
  .port = "443";
  .host = "httpbin.org";
  .first_byte_timeout = 20s;
  .max_connections = 500;
  .between_bytes_timeout = 20s;
  .share_key = "xei5lohleex3Joh5ie5uy7du";
  .ssl = true;
  .ssl_sni_hostname = "httpbin.org";
  .ssl_cert_hostname = "httpbin.org";
  .ssl_check_cert = always;
  .min_tls_version = "1.2";
  .max_tls_version = "1.2";
}

sub vcl_recv {
   call foo;
}

And linter should not raise any errors.

Additional Specs

  • Remote snippets (EdgeDictionary, ACL) also valid
  • Nested inclusion also valid
  • VCL error of included module still displays its filename and line

Comment on lines +54 to +59
type RunMode int

const (
RunModeLint RunMode = 0x000001
RunModeStat RunMode = 0x000010
)
Copy link
Owner Author

Choose a reason for hiding this comment

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

Runner no longer needs to recognize the VCL is main or not, so RunMode could be simple as bit mode.

Comment on lines +287 to +302
case *ast.IncludeStatement:
module, err := r.resolver.Resolve(t.Module.Value)
if err != nil {
return nil, err
}

vcl, err := r.parseVCL(module.Name, module.Data)
if err != nil {
return nil, err
}

vcl.Statements, err = r.resolveStatements(vcl.Statements)
if err != nil {
return nil, err
}
resolved = append(resolved, vcl.Statements...)
Copy link
Owner Author

Choose a reason for hiding this comment

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

Resolve include statement here.

Comment on lines +303 to +307
case *ast.IfStatement:
if err := r.resolveIfStatement(t); err != nil {
return nil, err
}
resolved = append(resolved, t)
Copy link
Owner Author

Choose a reason for hiding this comment

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

If statement may have a block statement so need to resolve recursively.

Copy link
Collaborator

Choose a reason for hiding this comment

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

what does block statement mean here? Can you give an example?

Copy link
Collaborator

Choose a reason for hiding this comment

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

(also we should probably move this to the code as a comment)

Copy link
Owner Author

Choose a reason for hiding this comment

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

The block statement may have include statement of embedding dynamic snippets inside (and probably include file is also valid).

For example:

sub vcl_fetch {
  if (req.restarts > 0) {
    include "snippet::<snippet_name>"; # Embed dynamic snippet here
  }
  ...
}

According to include statement, it is available in all subroutines so the above syntax should be valid and resolve inclusion recursively.

Comment on lines +308 to +314
case *ast.SubroutineDeclaration:
ss, err := r.resolveStatements(t.Block.Statements)
if err != nil {
return nil, err
}
t.Block.Statements = ss
resolved = append(resolved, t)
Copy link
Owner Author

Choose a reason for hiding this comment

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

Subroutine may have a block statement so need to resolve recursively.

@davinci26
Copy link
Collaborator

That is soooooo dope, super excited for this PR. will review tomorrow

@ysugimoto ysugimoto changed the title Combile included VCL modules into main and parse once Combine included VCL modules into main and parse once Aug 15, 2022
@ysugimoto ysugimoto added the bug Something isn't working label Aug 19, 2022
@ysugimoto
Copy link
Owner Author

I'll resolve the conflict lately

@ysugimoto
Copy link
Owner Author

@davinci26 I've resolved conflict with main branch, PTAL?

Copy link
Collaborator

@davinci26 davinci26 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for working on this

@davinci26
Copy link
Collaborator

when this + #99 is merged I will pull it in to our internal branch and test it on our VCL corpus as well and report back if I run into any issues

@ysugimoto ysugimoto merged commit bc3f060 into main Sep 4, 2022
@ysugimoto ysugimoto deleted the feature/issue-78 branch September 4, 2022 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Parser/Linter Include statement order resolution can be improved
2 participants