feat(operator): Add support for properly configuring the CORS allowed origins header #5906
Conversation
|
@carlesarnal this may need to be updated once you implement the Keycloak support |
|
|
||
| // If the QUARKUS_HTTP_CORS_ORIGINS env var is configured in the "env" section of the CR, | ||
| // then make sure to add those configured values to the set of allowed origins we want to | ||
| // configure. |
There was a problem hiding this comment.
This is a valid functionality, but I think we should follow a principle regarding configuration in the env section:
The user should be able to completely override any env. variable set by the operator. i.e. we should not touch variables set by the user, and they should override the ones set by the operator.
The advantage of this is that (together with PTS), there is a workaround for users to handle most situations even if there is a bug in the operator (e.g. setting an incorrect env. var value), or there is some circumstance we were not able to predict. This is also simple to explain to the users.
Disadvantage might be a slight decrease in user experience, where the operator could do more things automatically, but I don't think this would affect a lot of feature.
WDYT?
There was a problem hiding this comment.
Note: The method io.apicurio.registry.operator.resource.app.AppDeploymentResource#addEnvVar is designed to do this, and we should consolidate a way env. vars are added to a handful of methods, maybe in EnvironmentVariables.
There was a problem hiding this comment.
Do you mean that the value configured in the env section should take precedent (override) the rest of the logic? I can see why that makes sense. Please confirm and I'll change it.
operator/controller/src/main/java/io/apicurio/registry/operator/utils/IngressUtils.java
Show resolved
Hide resolved
| value: https://ui.example.org | ||
| """; | ||
|
|
||
| private static final String WITH_ENV_VAR_AND_INGRESS = """ |
There was a problem hiding this comment.
I think we should use the files in operator/controller/src/test/resources/k8s/examples as much as possible, even if they are simple (or slightly edited in the test). This would provide resources we can use in the docs and where we can point users to. The examples will end up in the "install examples" release artifact.
(BTW similarly I have an aspiration to eventually do the same thing for the registry examples and run them as tests).
There was a problem hiding this comment.
I was having trouble getting those resources to load in a unit test vs an IT test (the context classloader is different). I didn't want to duplicate the resource loading, but I can of course. I'll go ahead and move these into k8s/exmaples.
| Assertions.assertThat(allowedOrigins).containsExactlyInAnyOrder(values); | ||
| } | ||
|
|
||
| public static String load(String path) { |
There was a problem hiding this comment.
Similar functionality is already in io.apicurio.registry.operator.resource.ResourceFactory#load. Can we a single place for this functionality? I don't have a preference for where to put it.
There was a problem hiding this comment.
The resource loading in ResourceFactory didn't work from a unit test, I assume due to the difference in thread context classloader. I can consolidate as long as you're OK with multiple variants of load() for the different use cases. For the unit test I will need to pass a classloader to load() so it can properly load the resource.
| @@ -24,6 +24,14 @@ public class Mapper { | |||
| YAML_MAPPER.configure(FAIL_ON_UNKNOWN_PROPERTIES, true); | |||
| } | |||
|
|
|||
| public static <T> T deserialize(String data, Class<T> klass) { | |||
There was a problem hiding this comment.
Similar functionality is already in io.apicurio.registry.operator.resource.ResourceFactory#deserialize. Can we a single place for this functionality? I don't have a preference for where to put it.
There was a problem hiding this comment.
Same answer as above.
This adds support for configuring the CORS allowed origins operator based on what is found in the CR. The value of the allowed origins is influenced by:
envconfigurationIf none of those sources has information we can use, then the value of the allowed origins defaults to
*.This PR contains an update to the smoke IT test, but also more thorough unit tests.