Set application location even if server.xml defines app config

[hibell]

Summary

The buildpack now generates app.xml if app config has been provided in the server.xml or not. This allows the Liberty buildpack to override the app location and context root (if BP_LIBERTY_CONTEXT_ROOT is provided). This change requires an ID to be set on the app config element in order for app config to be merged properly; if not set, the build will fail.

The buildpack also now checks server.xml for any app configs defined with webApplication or enterpriseApplication in addition to application.

Use Cases

Allows the buildpack to override the location and context root even if app config is provided in the server.xml.

Checklist

• I have viewed, signed, and submitted the Contributor License Agreement.
• I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
• I have added an integration test, if necessary.
• I have reviewed the styleguide for guidance on my code quality.
• I'm happy with the commit history on this PR (I have rebased/squashed as needed).

[kevin-ortega]

@dmikusa this is possibly considered a breaking change. if a server.xml with an app config element does not have an ID, the build will fail. This is only the case if a server.xml is provided via config, server directory or server package. It is difficult to know how prevalent this scenario is and how many, if any, users it would affect. It is really a bug in the buildpack that necessitated this PR.

[dmikusa]

@kevin-ortega I agree with what you're saying about the breaking change. If there is the possibility, even a remote one, that someone built an app yesterday and the build worked then after this code change is released that same app build would fail, that would be a breaking change to me.

How prevalent is the use of ids outside of buildpacks? Is this something a user will have seen before, or is this something created new to solve a buildpack-specific problem? We generally try to avoid the latter.

Also, do you have any more details on the nature of the present bug? Is there a possibility that we could work around this in a different, non-breaking way? The main reason I ask is that we could cut a new major release of the Liberty buildpack, but it might be a little bit before we could integrate that into the Java buildpack (composite). We're trying to be careful with how frequently we bump the major version of the Java buildpack, and we'll likely need to batch up major bumps from other composites into one major bump for the Java buildpack. That could take a couple of weeks, I know of a few others at the moment.

[hibell]

@kevin-ortega I agree with what you're saying about the breaking change. If there is the possibility, even a remote one, that someone built an app yesterday and the build worked then after this code change is released that same app build would fail, that would be a breaking change to me.

Yes, agreed.

How prevalent is the use of ids outside of buildpacks? Is this something a user will have seen before, or is this something created new to solve a buildpack-specific problem? We generally try to avoid the latter.

IDs outside the buildpack are probably not too prevalent since users would have full control over their config. Since the buildpack generates some app config (e.g. location, context root), we would need to either

• edit whichever app config element (application, webApplication, or enterpriseApplication) in the server.xml directly to do this configuration, or
• we can use the Liberty config merging.

In the latter case, the ID is required in order for the config to be merged correctly. The CF Liberty for Java buildpack uses the former method using xpath to set the location for the application. However, it does not look like we can do this in Golang without an extra dependency (such as antchfx/xmlquery).

Also, do you have any more details on the nature of the present bug? Is there a possibility that we could work around this in a different, non-breaking way? The main reason I ask is that we could cut a new major release of the Liberty buildpack, but it might be a little bit before we could integrate that into the Java buildpack (composite). We're trying to be careful with how frequently we bump the major version of the Java buildpack, and we'll likely need to batch up major bumps from other composites into one major bump for the Java buildpack. That could take a couple of weeks, I know of a few others at the moment.

The nature of the bug is that the user's custom app config was being ignored. The first issue we found was that they were using webApplication where we were only using application. The second issue was that the server was seeing two different applications: the user's custom app (with no location set) and the app we configured with the location set. The server was unable to find the app the user configured (since no location was set) resulting in the basic app config we provided to be deployed.

@dmikusa Would adding that xmlquery library be an option? It is MIT licensed. We could use that library to set the ID for the user in any custom app config they provide so that the change would be non-breaking. Something like:

func updateServerConfig(configPath string) error {
	config, err := os.ReadFile(configPath)
	if err != nil {
		return err
	}
	reader := bytes.NewReader(config)
	doc, err := xmlquery.Parse(reader)
	if err != nil {
		return err
	}

        // Retrieve the application, webApplication, or enterpriseApplication node
	appConfig, err := getApplicationNode(doc) 
	if err != nil {
		return err
	}
	appConfig.SetAttr("id", "app")

	fmt.Println(doc.OutputXML(true))

	return nil
}

[dmikusa]

In the latter case, the ID is required in order for the config to be merged correctly.

Ok, but it sounds like this is functionality in Liberty, so it's not something we're inventing for the buildpack. I'm Ok with that approach.

The CF Liberty for Java buildpack uses the former method using xpath to set the location for the application. However, it does not look like we can do this in Golang without an extra dependency (such as antchfx/xmlquery).

We generally want to avoid dependencies but where it makes sense it is OK. You're not going to implement XPath query support in the buildpack, so taking a dependency on that library would make sense. For any library, we need to do some basic evaluation to make sure it a.) has no presently known security vulnerabilities, b.) appears to be active (replies on GH issues, recent releases, etc...), c.) has reasonable documentation/usage instructions d.) has an OSS licenses compatible with Apache v2 (all Paketo is under Apache v2) and e.) appears to be "popular" in the Go community for solving a particular problem.

@dmikusa Would adding that xmlquery library be an option? It is MIT licensed. We could use that library to set the ID for the user in any custom app config they provide so that the change would be non-breaking. Something like:

👍 This particular library seems to check those boxes, so I'd be OK with taking that dependency since it would meet a functionality goal in the buildpack.

[hibell]

@dmikusa Thanks for the response. I made the changes to add the ID into server.xml if it is not set.

[dmikusa]

Looks good. Approved.

There is a merge conflict on the go.sum file and those are a pain to resolve manually. What I typically do is check out the branch and run go mod tidy (you might need to delete the conflict markers first, so it's in a valid format). That command will just regenerate the go.mod file. If you can push that up, then we can go ahead and merge.