Skip to content

fix(*): replace deprecated pl.xml module with luaexpat #61

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

Merged
merged 6 commits into from
Jul 18, 2023

Conversation

windmgc
Copy link
Member

@windmgc windmgc commented Jul 12, 2023

Summary

This PR has the following changes:

  • Replace deprecated pl.xml module with luaexpat. Currently, the XML parser is only used in TokenFileWebIdentityCredentials.lua. Since STS service is responding with XML response, it is also needed in ChainableTemporaryCredentials.lua. This should solve Dependency on deprecated Penlight XML (pl.xml) module #37 (as long as luaexpat does not get deprecated :P)
  • Fix the logic that fetches temporary credentials from STS service, this is necessary for implementing assumeRole-related features.

@windmgc windmgc force-pushed the remove-deprecated-pl-xml branch from bb47a2f to 482b821 Compare July 17, 2023 05:44
@windmgc
Copy link
Member Author

windmgc commented Jul 17, 2023

By running the original tests I found that Luaexpat's behaviour seems quite different from Penlight's implementation of simple XML parser...

(kong-dev) ubuntu@ip-172-31-93-219:~/kong-ee$ cat test3.lua
local a = [[<?xml version="1.0" encoding="UTF-8"?>
<mainXmlElement xmlns="cool-name-space">
  <someSubStructure>
    <hello>the default hello thinghy</hello>
    <world>the default world thinghy</world>
  </someSubStructure>
  <subList>
    <listELement>1</listELement>
    <listELement>2</listELement>
    <listELement>3</listELement>
  </subList>
  <RoleArn>hello</RoleArn>
  <RoleSessionName>world</RoleSessionName>
</mainXmlElement>
]]

local parser = require("lxp.lom")
local inspect = require("inspect")

print("======LUAEXPAT======")
print(inspect(parser.parse(a)))

print("======PL.XML======")
local pl_parser = require("pl.xml")
print(inspect(pl_parser.parse(a, false, true)))
(kong-dev) ubuntu@ip-172-31-93-219:~/kong-ee$ resty test3.lua                                                                                                                                                                                                          [42/18206]
======LUAEXPAT======
{ "\n  ", { "\n    ", { "the default hello thinghy",
      attr = {},
      tag = "hello"
    }, "\n    ", { "the default world thinghy",
      attr = {},
      tag = "world"
    }, "\n  ",
    attr = {},
    tag = "someSubStructure"
  }, "\n  ", { "\n    ", { "1",
      attr = {},
      tag = "listELement"
    }, "\n    ", { "2",
      attr = {},
      tag = "listELement"
    }, "\n    ", { "3",
      attr = {},
      tag = "listELement"
    }, "\n  ",
    attr = {},
    tag = "subList"
  }, "\n  ", { "hello",
    attr = {},
    tag = "RoleArn"
  }, "\n  ", { "world",
    attr = {},
    tag = "RoleSessionName"
  }, "\n",
  attr = { "xmlns",
    xmlns = "cool-name-space"
  },
  tag = "mainXmlElement"
}
======PL.XML======
{ { { "the default hello thinghy",
      attr = {},
      tag = "hello",
      <metatable> = <1>{
        __index = <table 1>,
        __tostring = <function 1>,
        __type = "doc",
        add_child = <function 2>,
        add_direct_child = <function 3>,
        addtag = <function 4>,
        child_with_name = <function 5>,
        children = <function 6>,
        childtags = <function 7>,
        filter = <function 8>,
        first_childtag = <function 9>,
        get_attribs = <function 10>,
        get_elements_with_name = <function 11>,
        get_text = <function 12>,
        maptags = <function 13>,
        match = <function 14>,
        matching_tags = <function 15>,
        reset = <function 16>,
        set_attrib = <function 17>,
        set_attribs = <function 18>,
        subst = <function 19>,
        text = <function 20>,
        tostring = <function 21>,
        up = <function 22>
      }
    }, { "the default world thinghy",
      attr = {},
      tag = "world",
      <metatable> = <table 1>
    },
    attr = {},
    tag = "someSubStructure",
    <metatable> = <table 1>
  }, { { "1",
      attr = {},
      tag = "listELement",
      <metatable> = <table 1>
    }, { "2",
      attr = {},
      tag = "listELement",
      <metatable> = <table 1>
    }, { "3",
      attr = {},
      tag = "listELement",
      <metatable> = <table 1>
    },
    attr = {},
    tag = "subList",
    <metatable> = <table 1>
  }, { "hello",
    attr = {},
    tag = "RoleArn",
    <metatable> = <table 1>
  }, { "world",
    attr = {},
    tag = "RoleSessionName",
    <metatable> = <table 1>
  },
  attr = {
    xmlns = "cool-name-space"
  },
  tag = "mainXmlElement",
  <metatable> = <table 1>
}

It seems that luaexpat is parsing those new line characters into items which is not expected

@windmgc
Copy link
Member Author

windmgc commented Jul 17, 2023

@Tieske Do you think it is a good idea that we remove the newline chars when concatenating the result of poor_mans_xml_encoding? It is not necessary for the XML format after all

request.body = table.concat(xml_data, "\n")

@Tieske
Copy link
Contributor

Tieske commented Jul 17, 2023

By running the original tests I found that Luaexpat's behaviour seems quite different from Penlight's implementation of simple XML parser...

Yes, because PL was taking shortcuts, and dismissed whitespace, which is significant in the XML standard. Though in most use cases it is not.

@windmgc windmgc force-pushed the remove-deprecated-pl-xml branch from 2e0c43d to 61aee17 Compare July 18, 2023 07:27
@windmgc
Copy link
Member Author

windmgc commented Jul 18, 2023

OpenResty 1.17.8.2's image seems too old to have a libexpat >= 2.3.0, so I changed it to build libexpat from source.

@Tieske Tieske force-pushed the remove-deprecated-pl-xml branch from 753c9c5 to 7ffb98a Compare July 18, 2023 17:58
@Tieske Tieske merged commit ebe3871 into Kong:main Jul 18, 2023
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.

2 participants