Skip to content
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

[WIP] Proposal make oathkeeper fully configurable #96

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kayibal
Copy link

@kayibal kayibal commented Oct 29, 2021

This way we can support custom services while keeping ory as part of OML.

This is just a quick proposal to outline the changes needed for this to be supported.

Basically instead of templating yaml files we handle everything in HCL allowing us to accept overrides and additional access rules from outside. This way the ORY module is more reusable and OML just merges user provided configuration with it's own configuration.

Comment on lines +6 to +23
configuration_default_overrides = {
errors={
handlers={
redirect={
config={
to="${var.protocol}://${var.hostname}/profile/auth/login"
}
}
}
}
mutators={
id_token={
config={
issuer_url="${var.protocol}://${var.hostname}"
jwk_urls="file://${local.secrets_mount_path}/id_token.jwks.json"
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I don't really like this.

In my opinion, what would be ideal is to, when you need to override something, not touch OML code, but touch a separate yaml data file with the keys you want to override.
We could have support for "additional rules" and "override defaults", but ideally without needing to touch OML.
This because if someone is using release 0.3.0 and wants to upgrade to 0.4.0, it won't be easy with custom stuff written inside it's main.tf files

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All in all, if we can load an yaml for this local instead of writing into it, then it would be solved

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then that yaml can be somewhere in the user's project root and OML lives as a submodule, like we do for omigami.

Copy link
Author

@kayibal kayibal Oct 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't fully understand these comments....

In my opinion, what would be ideal is to, when you need to override something, not touch OML code, but touch a separate yaml data file with the keys you want to override.

This is exactly the point of this PR you have complete control over ory from outside OML now. E.g. if I wanted to touch some config other than hostname or protocol. I can do that like this:

module "oml" {
    ...
    ory_configuration_overrides = yamldecode("mycustom-oathkeeper-config.yaml")
    ...
}

I never have to touche OML's main.tf nor any other files to do that. I can even load that custom config from a yaml file if I already have an oathkeeper configuration and want to hand over controller of ory to OML.

I guess there are a few fine tuning decisions to make as in should we keep support for protocol and hostname? If yes should it take precedence over the overrides?

Copy link
Contributor

@bernardolk bernardolk Oct 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so as aligned in conversation, the user won't need to change main.tf code as I initially suspected. Great :)
Here we are basically "forcing" the values of these particular vars for proper OML functioning. It is to be decided whether we want to do it (if it is actually needed to enforce this) or not.

Comment on lines +230 to +231
access_rules = concat(local.default_access_rules, var.ory_additional_access_rules)
configuration_overrides = var.ory_configuration_overrides
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, I like this

Comment on lines +27 to +32
locals {
configuration = merge(
local.configuration_defaults, # lowest precedence
var.configuration_overrides,
local.configuration_default_overrides # highest precedence
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting

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