Implement AccessorID resolution and client ip propagation

[ncode]

Hi @mxab,

I've started the patch last week and would like to double check the direction it's going so far :).

My idea here was to resolve the token as early as possible to propagate down. The upstream ip address is very cheap to propagate, so I thought about always passing it so we can have cidr based validators and mutators even when not resolving the token

Addresses #18

---

This pull request includes significant updates to the admissionctrl package, focusing on refactoring the Job handling to use a new types.Payload structure and adding support for context headers in webhook mutators. The most important changes are as follows:

Refactoring to use types.Payload:

• admissionctrl/controller.go: Changed method signatures and internal logic to use types.Payload instead of api.Job. This includes updates to JobMutator, JobValidator, JobHandler, and their respective methods. [1] [2] [3] [4]
• admissionctrl/controller_test.go: Updated test cases to use types.Payload and modified TestJobHandler_ApplyAdmissionControllers to include the new resolveToken parameter. [1] [2] [3]

Adding context headers in webhook mutators:

• admissionctrl/mutator/json_patch_webhook.go: Enhanced the Mutate method to add context headers from types.Payload to the HTTP request. [1] [2]
• admissionctrl/mutator/webhook_mutator.go: Similar updates to add context headers from types.Payload to the HTTP request.

Updates to mutator and OPA logic:

• admissionctrl/mutator/opa_json_patch.go: Refactored Mutate method to use types.Payload and updated logging and error handling accordingly. [1] [2] [3] [4]
• admissionctrl/opa/opa.go: Modified Query method to accept types.Payload as input.

Test updates:

• admissionctrl/mutator/json_patch_webhook_test.go: Updated tests to use types.Payload.
• admissionctrl/mutator/opa_json_patch_test.go: Updated tests to use types.Payload.
• admissionctrl/mutator/webhook_mutator_test.go: Updated tests to use types.Payload.
• admissionctrl/opa/opa_test.go: Updated tests to use types.Payload.

[mxab]

Hi @ncode, awesome! This goes definitely in the right direction!

Some thoughts so far but theses are things we also could stabilise later:

• the token resolution provides additional context, e.g. policies afaik, should we pass those to the webhooks as well?
• afaik X-... headers are discouraged, should we use NACP-.... ?
• passing everything as headers might get a bit messy in cases there will be more values, we could break the API so far and pass it as a combined document to the webhook ´{ job: , context : { remote_ip_addr: ..., ... }}`

I'm also currently waiting for a recommendation from the OPA communityhow to model this there

[ncode]

Hi @mxab,

Hi @ncode, awesome! This goes definitely in the right direction!

Some thoughts so far but theses are things we also could stabilise later:

• the token resolution provides additional context, e.g. policies afaik, should we pass those to the webhooks as well?

I didn't think of it but it's a pretty nice idea to pass all the available information since we've already resolved it :)

• afaik X-... headers are discouraged, should we use NACP-.... ?

Yes. Let's use NACP-...

• passing everything as headers might get a bit messy in cases there will be more values, we could break the API so far and pass it as a combined document to the webhook ´{ job: , context : { remote_ip_addr: ..., ... }}`

I agree that passing the context like you describe will make things a bit more flexible, specially with the extra metadata from the token it will be cleaner. I'm only in doubt about X-FORWARD-FOR and the NACP-Nomad-Accessor-ID, these would be useful to have available as readers without the need to parse the content. What do you think?

I'm also currently waiting for a recommendation from the OPA communityhow to model this there

Nice!

[mxab]

Sorry for the late response.

X-FORWARD-FOR and the NACP-Nomad-Accessor-ID, these would be useful to have available as readers without the need to parse the content

Ah so if I get you correctly your "validation" part in this case is solely on the Client IP Address + Accessor ID. Yeah I guess it makes sense.

Regarding the OPA part, they recommended to create an input consisting out of the context and the job. So I guess we need to update this part here and create a input struct the consists out of a caller context (client ip addr, resolved token) and the job

So for the remote hooks I think it make sense to use the same structure, we could still leave the headers as well so it works for your use case

[ncode]

Sorry for the late response.

No worries!

X-FORWARD-FOR and the NACP-Nomad-Accessor-ID, these would be useful to have available as readers without the need to parse the content

Ah so if I get you correctly your "validation" part in this case is solely on the Client IP Address + Accessor ID. Yeah I guess it makes sense.

Regarding the OPA part, they recommended to create an input consisting out of the context and the job. So I guess we need to update this part here and create a input struct the consists out of a caller context (client ip addr, resolved token) and the job

So for the remote hooks I think it make sense to use the same structure, we could still leave the headers as well so it works for your use case

Deal! I will start working on it this weekend!

[ncode]

Hi @mxab,

I think at this stage we have the bare bones of how to propagate the data. Now I'm staring to add the context passing to mutators and validators. I will add those to the same PR and we can break it into different pieces if it's a too big of a PR. One thing I'm not entire sure is if we should break the api and have:

type Payload struct {
	Job     *api.Job               `json:"job"`
	Context *config.RequestContext `json:"context,omitempty"`
}

I will add a few extra tests and deploy a release to test this week to validate in production :)

[mxab]

Hi,
Nice. I'll review it asap, just a bit stuffed until the weekend.

[ncode]

Hi, Nice. I'll review it asap, just a bit stuffed until the weekend.

No worries!

[mxab]

I'm actual not too knowledgeable when it comes to http lib of golang, what is it we do with the transport here :)

[mxab]

ah this was for the tls part right?

[ncode]

Yeap. Since we have a transport coming from the upstream function call, we use the same settings to call the nomad endpoint. To ensure if we have the tls set we use it.

[mxab]

Overall I would say this looks really great. I guess it would be a good time to start CHANGELOG.md to document those breaking changes :D

[ncode]

Overall I would say this looks really great. I guess it would be a good time to start CHANGELOG.md to document those breaking changes :D

Nice! It indeed make sense to add those to a CHANGELOG.md. I'm happy to update MR the with a changelog section that details the change to help :)

[ncode]

@mxab I've added a changelog section to the PR message. Now that I'm thinking about it, should I actually push the CHANGELOG.md file?

[mxab]

Looks really great, if you don't mind put it in a CHANGELOG.md file

[ncode]

Looks really great, if you don't mind put it in a CHANGELOG.md file

Done :)

[mxab]

Hi @ncode thanks so much for your contribution, I merged in. Will create a release asap

[ncode]

@mxab thank you for creating this awesome tool!