Update README, bug fixes, and initial targets API

[davidallendj]

This PR updates the README and makes a number of small changes that simplifies the generator API and removes some of the complexity to making changes to the code. It also adds the initial implementation of the targets API for creating new targets via requests to the server, and fixes a couple of smaller bugs.

[synackd]

I left some initial thoughts and requests. I got build/test errors on the current tip of this branch so I can give more feedback once that is working.

[synackd]

The config command looks to be working now. One thing I'll see regarding here:

configurator/pkg/config/config.go

Line 111 in b6c3533

err = os.WriteFile(path, data, os.ModePerm)

os.ModePerm uses 0777 masked with the umask, which we probably don't want. More reasonably, we will probably want to set this to 0644. E.g, see here and then here for where that gets used.

[synackd]

I think this line needs to change:

configurator/pkg/client/smd.go

Line 154 in eda3cce

url := fmt.Sprintf("%s:%d/hsm/v2%s", client.Host, client.Port, endpoint)

When starting the server, configurator expects there to have :<port> appended to the host string despite using the port directive in the config, or it will err. I think instead of having host and port directives, having a single base-uri directive and parsing into that would be simpler, less confusing, and more robust. I see that the default config sets the host to http://172.0.0.1:27779 for SMD anyway, so it looks like that behavior is expected.

This PR is already huge, so I'm going to file a separate PR for this.

[davidallendj]

The config command looks to be working now. One thing I'll see regarding here:

configurator/pkg/config/config.go

Line 111 in b6c3533

err = os.WriteFile(path, data, os.ModePerm)

os.ModePerm uses 0777 masked with the umask, which we probably don't want. More reasonably, we will probably want to set this to 0644. E.g, see here and then here for where that gets used.

Interesting. I saw the comment and though it was setting it to 0511 by default. I'll update it to use 0644 explicitly.

[davidallendj]

I think this line needs to change:

configurator/pkg/client/smd.go

Line 154 in eda3cce

url := fmt.Sprintf("%s:%d/hsm/v2%s", client.Host, client.Port, endpoint)

When starting the server, configurator expects there to have :<port> appended to the host string despite using the port directive in the config, or it will err. I think instead of having host and port directives, having a single base-uri directive and parsing into that would be simpler, less confusing, and more robust. I see that the default config sets the host to http://172.0.0.1:27779 for SMD anyway, so it looks like that behavior is expected.

This PR is already huge, so I'm going to file a separate PR for this.

I'll change this here to fix the issue, but I plan to remove the port variable in the future.

[synackd]

The config command looks to be working now. One thing I'll see regarding here:

configurator/pkg/config/config.go

Line 111 in b6c3533

err = os.WriteFile(path, data, os.ModePerm)

os.ModePerm uses 0777 masked with the umask, which we probably don't want. More reasonably, we will probably want to set this to 0644. E.g, see here and then here for where that gets used.

Interesting. I saw the comment and though it was setting it to 0511 by default. I'll update it to use 0644 explicitly.

To cite my sources, os.ModePerm is defined as an fs.ModePerm, which is hard coded to 0777.

[synackd]

I think this line needs to change:

configurator/pkg/client/smd.go

Line 154 in eda3cce

url := fmt.Sprintf("%s:%d/hsm/v2%s", client.Host, client.Port, endpoint)

When starting the server, configurator expects there to have :<port> appended to the host string despite using the port directive in the config, or it will err. I think instead of having host and port directives, having a single base-uri directive and parsing into that would be simpler, less confusing, and more robust. I see that the default config sets the host to http://172.0.0.1:27779 for SMD anyway, so it looks like that behavior is expected.
This PR is already huge, so I'm going to file a separate PR for this.

I'll change this here to fix the issue, but I plan to remove the port variable in the future.

I think having ip and port make sense for the server directive, since it doesn't make sense to bind to a URL. But for communicating with SMD, a base URL makes more sense.

[synackd]

I think this is fine for now, but I'm getting:

{"level":"error","time":1733940654,"message":"failed to generate file: failed to fetch ethernet interfaces with client: failed to read HTTP response: failed to make request: Get \"/hsm/v2/Inventory/EthernetInterfaces\": unsupported protocol scheme \"\""}

even when the host is defined.

I think we need to pass all of the ClientOptions to the SmdClient when creating here:

configurator/pkg/generator/generator.go

Line 390 in 5b351d2

client = configurator.NewSmdClient()

[davidallendj]

Is this the right branch? I don't see a line 390 in the PR branch.

[synackd]

Argh... GitHub switched back to main when I clicked on the symbol. Here is the line on this branch:

configurator/pkg/server/server.go

Line 55 in ebe4e02

c := config.New()

[davidallendj]

Which plugin are you trying to use here? Also, are you trying to run the server here?

I think it's probably worth to include more information in the error.

[synackd]

I'm running the server as follows (config displayed):

# ./configurator serve --verbose --cacert /opt/shared/ochami.pem --config ./config.yaml
{
        "Version": "",
        "Server": {
                "Host": "127.0.0.1:3334",
                "Port": 0,
                "Jwks": {
                        "Uri": "",
                        "Retries": 5
                }
        },
        "SmdClient": {
                "Host": "http://127.0.0.1:27779",
                "Port": 0,
                "AccessToken": ""
        },
        "AccessToken": "eyJhb...",
        "Targets": {
                "dnsmasq": {
                        "Plugin": "",
                        "TemplatePaths": null,
                        "FilePaths": null,
                        "RunTargets": null
                }
        },
        "PluginDirs": [],
        "CertPath": ""
}
{"level":"error","time":1734034071,"message":"failed to generate file: failed to fetch ethernet interfaces with client: failed to read HTTP response: failed to make request: Get \"/hsm/v2/Inventory/EthernetInterfaces\": unsupported protocol scheme \"\""}                   
1:07PM INF Request bytes_in=0 bytes_out=203 duration=0.115345 method=GET remote_addr=127.0.0.1:44354 request_id=ochami-vm/WwwPpCGpkq-000001 request_uri=/generate?target=dnsmasq status="Internal Server Error" status_code=500 user_agent=configurator                          
2024/12/12 13:07:51 [ochami-vm/WwwPpCGpkq-000001] "GET http://127.0.0.1:3334/generate?target=dnsmasq HTTP/1.1" from 127.0.0.1:44354 - 500 203B in 214.879µs 

The response above is printed when I run:

./configurator fetch --target dnsmasq --host http://127.0.0.1:3334

I believe the line I linked is related to this.

[synackd]

Actually, I think the problem is here:

configurator/pkg/server/server.go

Lines 92 to 95 in cccf632

	opts := []client.Option{
	client.WithAccessToken(s.Config.AccessToken),
	client.WithCertPoolFile(s.Config.CertPath),
	}

None of the SmdClient options are getting read in the serve command, and so the host field is blank when making the request.

[davidallendj]

I think the issue may actually be because of this:
https://github.com/OpenCHAMI/configurator/blob/readme-and-improvements/cmd/serve.go#L45

[synackd]

So I have been tracking down the issue by print statements, etc. and I found that, when passing --target dnsmasq to the fetch command, the output of s.getTarget is nil here:

configurator/pkg/server/server.go

Line 142 in cccf632

target *Target = s.getTarget(targetParam)

and so GenerateWithTarget is called here:

configurator/pkg/server/server.go

Line 160 in cccf632

outputs, err = generator.GenerateWithTarget(s.Config, targetParam)

instead of Generate here:

configurator/pkg/server/server.go

Line 154 in cccf632

outputs, err = generator.Generate(target.PluginPath, s.GeneratorParams)

which would pass the s.GeneratorParams containing the SMD host configuration.

It looks like s.getTarget returns nil when the target (dnsmasq in this case) doesn't exist in the server's Targets map. Does this get set somewhere? The server.New() function doesn't seem to set this.

[synackd]

The new commit (pending review comment) seems to fix this, however the configurator doesn't seem to contact SMD despite being configured.

# ./configurator serve --verbose --cacert /opt/shared/ochami.pem --config ./config.yaml
{"level":"warn","time":"2024-12-17T22:27:27-07:00","message":"No token found. Continuing without one...\n"}
{                                   
        "Version": "",
        "Server": {       
                "Host": "127.0.0.1:3334",
                "Port": 0,                            
                "Jwks": {
                        "Uri": "",          
                        "Retries": 5
                }                          
        },                      
        "SmdClient": {
                "Host": "http://127.0.0.1:27779",      
                "Port": 0,                                      
                "AccessToken": ""                        
        },                                                                    
        "AccessToken": "",                                                                                                                                                                                  
        "Targets": {                                                     
                "dnsmasq": {                                                  
                        "Plugin": "",                                        
                        "TemplatePaths": null,                               
                        "FilePaths": null,                                     
                        "RunTargets": null                                     
                }                                                                   
        },                                                                       
        "PluginDirs": [],                                                        
        "CertPath": ""                                                           
}                                                          
{"level":"error","time":1734499653,"message":"could not find target in default generators"}
10:27PM INF Request bytes_in=0 bytes_out=2 duration=0.076701 method=GET remote_addr=127.0.0.1:39508 request_id=te-head.si.usrc/XNIz1WqTBL-000001 request_uri=/generate?target=dnsmasq status=OK status_code=2
00 user_agent=configurator                
2024/12/17 22:27:33 [te-head.si.usrc/XNIz1WqTBL-000001] "GET http://127.0.0.1:3334/generate?target=dnsmasq HTTP/1.1" from 127.0.0.1:39508 - 200 2B in 149.241µs
{"level":"error","time":1734499667,"message":"could not find target in default generators"}

Using the following client command:

./configurator fetch --target dnsmasq --host http://127.0.0.1:3334 --verbose