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

feat: general mechanism to allow a container (or init-container) environment to overridden from the OperatorOverride #742

Merged
merged 3 commits into from
Jun 16, 2022

Conversation

k-wall
Copy link
Contributor

@k-wall k-wall commented Jun 8, 2022

A general mechanism to allow a container (or init-container) environment to overridden from the OperatorOverride. Currently it is just applied to the canary image.

(The scope of this PR has been broadened. Originally we wanted a mechanism to allow canary reconcile and connection check interval to be overridden to help investigate MGDSTRM-8698 by having the ability to turn off the connection check.

The scope has been extended to include admin server. I note that strimzi does let let you override envvars for any of its deployments.

@k-wall k-wall requested a review from MikeEdgar June 8, 2022 11:33
@github-actions github-actions bot added the operator changes related to operator label Jun 8, 2022
@k-wall k-wall requested a review from grdryn June 8, 2022 11:33
@k-wall k-wall changed the title feat: expose canary reconcile and connection check interval for confi… feat: expose canary reconcile and connection check interval for config Jun 8, 2022
@MikeEdgar
Copy link
Contributor

MikeEdgar commented Jun 8, 2022

I think this change would be a prime candidate to use additionalProperties in OperandOverride. For example, the canary could look for a Map<String, EnvVar> List<EnvVar> entry under additionalProperties.get("env") that could contain any user-provided environment entries and pass them through to the canary container. That leaves a door open to configure individual canary instances rather than having the config apply to the entire fleet.

@k-wall
Copy link
Contributor Author

k-wall commented Jun 9, 2022

I think this change would be a prime candidate to use additionalProperties in OperandOverride. For example, the canary could look for a Map<String, EnvVar> List<EnvVar> entry under additionalProperties.get("env") that could contain any user-provided environment entries and pass them through to the canary container. That leaves a door open to configure individual canary instances rather than having the config apply to the entire fleet.

I like that idea. I think in the cm it would look like:

canary:
  image: quay..
  env:
   - name: FOO
     value: bar

but I don't immediately see how we'd persuade Jackson to manufacture EnvVar objects rather (than Map). What am I missing?

We could just spin off a separate List<EnvVar> env property on the OperandOverride I suppose rather than use the catch-all.

@shawkins
Copy link
Contributor

shawkins commented Jun 9, 2022

but I don't immediately see how we'd persuade Jackson to manufacture EnvVar objects rather (than Map). What am I missing?

Use something like Serialization.jsonMapper().convertValue(map, EnvVar[].class) with the map from the env field.

@MikeEdgar
Copy link
Contributor

We could just spin off a separate List<EnvVar> env property on the OperandOverride I suppose rather than use the catch-all.

+1

@k-wall
Copy link
Contributor Author

k-wall commented Jun 10, 2022

@shawkins @MikeEdgar how does this look?

I needed to make a decision about whether the override feature should apply to any environment variable, even those that probably shouldn't be overridden. I thought about separating a container's env var into a set of those which are reasonable to override and those which are not (KAFKA_BOOTSTRAP_SERVERS for instance). In the end I decided it were simpler to assume that the feature would be used wisely.

I think we should apply the same pattern to the other containers, but what to give you chance to comment first.

@k-wall k-wall changed the title feat: expose canary reconcile and connection check interval for config feat: general mechanism to allow a container (or init-container) environment to overridden from the OperatorOverride Jun 10, 2022
Copy link
Contributor

@MikeEdgar MikeEdgar left a comment

Choose a reason for hiding this comment

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

This looks good. Just one comment which supports what you said about making this more generally usable for any of the operands.

@k-wall k-wall added this to the 0.24.0 milestone Jun 16, 2022
@k-wall k-wall removed the request for review from grdryn June 16, 2022 12:59
Copy link
Contributor

@SamBarker SamBarker left a comment

Choose a reason for hiding this comment

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

LGTM

@SamBarker SamBarker merged commit 18186ed into bf2fc6cc711aee1a0c2a:main Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
operator changes related to operator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants