Pomerium: [discussion] Change Options and URL fields to values

[travisgroth]

This came up in #150 while working on Checksum(). Related to #115 as call/struct signatures were retained as much as possible.

It appears that on service creation, Authenticate.New() tacks on a path to the AuthenticateURL from Options and then uses that to initialize further data structures. It isn't 100% obvious in the code but, as-written, it actually makes the change to the url field in the Options structure.

https://github.com/pomerium/pomerium/blob/e982e72146b23914ee0a03dec75cffd279f80faa/authenticate/authenticate.go#L81-L82

While possibly less efficient, my instinct is to protect Options from accidental changes and dereference as much as possible. I think just changing the field types would make it apparent when Options was being mutated, but not passing options as a pointer at all should make it impossible.

This would touch a few things, so I wanted to discuss before I try to tackle the changes. I think this makes sense (and wouldn't break anything...) but I'm open to input or correction.

[nareddyt]

I might be misunderstanding the example, but it sounds like we just want
the Options struct to be immutable. We could do that by not exposing
setters/mutators outside the package, right?

https://stackoverflow.com/a/47632959/4402434

On Sun, Jun 2, 2019, 2:10 PM Travis Groth [e-mail] wrote:

This came up in #150 https://github.com/pomerium/pomerium/pull/150
while working on Checksum(). Related to #115
https://github.com/pomerium/pomerium/issues/115 as call/struct
signatures were retained as much as possible.

It appears that on service creation, Authenticate.New() tacks on a path to
the AuthenticateURL from Options and then uses that to initialize further
data structures. It isn't 100% obvious in the code but, as-written, it
actually makes the change to the url field in the Options structure.

https://github.com/pomerium/pomerium/blob/e982e72146b23914ee0a03dec75cffd279f80faa/authenticate/authenticate.go#L81-L82

While possibly less efficient, my instinct is to protect Options from
accidental changes and dereference as much as possible. I think just
changing the field types would make it apparent when Options was being
mutated, but not passing options as a pointer at all should make it
impossible.

This would touch a few things, so I wanted to discuss before I try to
tackle the changes. I think this makes sense (and wouldn't break
anything...) but I'm open to input or correction.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/pomerium/pomerium/issues/155?email_source=notifications&email_token=ACVAIG5NVLBPXXCWLXS7HBLPYQEINA5CNFSM4HSDF7AKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4GXFBQIA,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACVAIGYK5FNLX72HKLRMXFLPYQEINANCNFSM4HSDF7AA
.

[travisgroth]

Yes, we could definitely go that route if we wanted to completely guarantee immutability. My immediate concern was more accidental changes that would escape a given service's scope.

A pile of Getter() methods isn't really something I want to bring in just for this, though I'm up for convincing and am willing to do it if we feel that's the right approach. Getters in front of simple fields doesn't feel very idiomatic, though. My thought was more to make it hard/impossible to do unintentionally outside of your scope. Getting rid of URL pointers alone would at least make it hard and obvious (you'd have to basically replace the entire Options struct).

[desimone]

It appears that on service creation, Authenticate.New() tacks on a path to the AuthenticateURL from Options and then uses that to initialize further data structures. It isn't 100% obvious in the code but, as-written, it actually makes the change to the url field in the Options structure.

Yuck! I'm totally to blame for that.

I think just changing the field types would make it apparent when Options was being mutated, but not passing options as a pointer at all should make it impossible.

I might be misunderstanding the example, but it sounds like we just want
the Options struct to be immutable. We could do that by not exposing
setters/mutators outside the package, right?

You guys are both right and the answer is yes. Ideally options would be immutable, but passing a copy of Options would probably solve the immediate problem.

The problem with creating an immutable struct (as shown in that example) is that unexpected fields (I believe I am remembering correctly) aren't compatible with viper/hash struct tags because they cannot be reflected on and unmarshalled.

Since we would only be copying the options struct between 1...3 times per instance, I think the copy approach makes sense. The alternative would be to make an unexported/exported struct wrapper shim where all the fields were gated by an accessor func.

Thoughts?

[travisgroth]

Yuck! I'm totally to blame for that.

I'd blame go's eagerness to help, to be honest. Very easy to not know what you're manipulating there.

The problem with creating an immutable struct (as shown in that example) is that unexpected fields (I believe I am remembering correctly) aren't compatible with viper/hash struct tags because they cannot be reflected on and unmarshalled.

Actually, yes, I think that's true. If those fields aren't public, unmarshalling will probably break. I can think of some patterns that might sidestep that (passing .Get() through to viper for instance or your Shim) but those all feel like over engineering.

I agree the copy overhead seems acceptable. Will take a pass at that approach.