Skip to content
This repository has been archived by the owner on Dec 7, 2020. It is now read-only.

Ingest --tags, --response-headers from CLI options #497

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sampsonj
Copy link

This PR addresses KEYCLOAK-12433, the --flags CLI option would not be parsed.
See JIRA issue for further details.

Changes include:

  • Allow the --flags and --response-headers CLI options to be ingested
  • Test that ensures all map based CLI options as defined in the config get parsed
  • decodeKeyPairs agnostic of value on right side of assignment (ie, can include the = character)
  • Test decodeKeyPairs updated to ensure value parsing is agnostic

cli.go Show resolved Hide resolved
cli.go Show resolved Hide resolved
cli.go Show resolved Hide resolved
utils.go Show resolved Hide resolved
cli_test.go Show resolved Hide resolved
cli_test.go Show resolved Hide resolved
cli_test.go Show resolved Hide resolved
cli_test.go Show resolved Hide resolved
cli_test.go Show resolved Hide resolved
cli_test.go Show resolved Hide resolved
@slaskawi
Copy link

slaskawi commented Jan 9, 2020

@sampsonj LGTM

@abstractj Perhaps we should disable wsl linter? It's very picky..

@abstractj abstractj added this to the 9 milestone Jan 16, 2020
Copy link

@abstractj abstractj left a comment

Choose a reason for hiding this comment

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

@slaskawi @sampsonj sorry for the very late feedback, the PR fixes the issues with --tags which is great and thanks for that @sampsonj. But it also introduces the injection of --response-headers, something out of the scope for https://issues.redhat.com/browse/KEYCLOAK-12433.

Do we have any use case on why we should introduce the injection of --response-headers? I could confirm that the issue with tags is gone, but trying to inject --response-headers fails, at least for me. That means that we might be fixing the bug for tags and introducing a new bug with response-headers.

Maybe I'm wrong, but I'd rather to have anything that's out of scope for the injection of attributes on "Custom pages" discussed on the dev mailing list.

@abstractj
Copy link

@slaskawi regards wsl I'd say let's leave it for now and if it starts to becomes an annoyance we can disable.

@sampsonj
Copy link
Author

Thanks for having a look @slaskawi and @abstractj - much appreciated!

The reason for the increase in scope from the original JIRA ticket is due to the general nature of the unit test - it finds all map based properties as defined in the config and ensures they can be ingested from CLI flags. In writing that test, it identified response headers not being ingested and in order for that test to pass, I needed to add the --response-headers to the cli.go stanza that pulls in map based flags and merges with the config.
The utilization of that flag then happens upstream in middleware:
https://github.com/keycloak/keycloak-gatekeeper/blob/master/middleware.go#L379
Called from:
https://github.com/keycloak/keycloak-gatekeeper/blob/master/server.go#L197

As for the --response-headers flag not adding response headers, I don't get the same result as I am able to pass for example, --response-headers="X-Custom-Header=xcustomheadervalue" from the command line arguments, and have the header show up in the response. What might I be missing here?

Thanks for your time - let me know if I've missed something here and I'd be happy to push an update.

@stianst stianst removed this from the 9 milestone Apr 29, 2020
@abstractj abstractj changed the title [KEYCLOAK-12433] Ingest --tags, --response-headers from CLI options Ingest --tags, --response-headers from CLI options Jun 4, 2020
@abstractj abstractj removed their assignment Aug 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants