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

proposal(operator-config): initial proposal for persisting configuration #941

Merged

Conversation

ecordell
Copy link
Member

@ecordell ecordell commented Jul 3, 2019

of operators

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 3, 2019
- If the global proxy object is not set / api doesn't exist, do nothing different.
- If the global proxy object is set and none of `HTTPS_PROXY`, `HTTP_PROXY`, `NO_PROXY` are set on the `Subscription`
- Then set those env vars on the deployment and ensure the deployment on the cluster matches.
- If the global proxy object is set and at least one of `HTTPS_PROXY`, `HTTP_PROXY`, `NO_PROXY` are set on the `Subscription`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a use case where the user wants to use the global proxy but wants to override NO_PROXY?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is, but my understanding from @shawn-hurley was that we won't support that. Correct me if I'm wrong.

Copy link
Member

Choose a reason for hiding this comment

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

I think it probably will make everyone's lives easier if we don't try to collide the override and global configs.

I think it will be easier to reason through for a user and for the proxy code. WDYT? I am not tied to it

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's fine to be conservative at first and change it if we need to later.

Copy link

Choose a reason for hiding this comment

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

yeah the path we took w/ the build component (which supports this same proxy information, specified at various levels of precedence) is "all or nothing." We take all the values from the highest level of precedence that the user configured.

so in @tkashem's scenario, if the user wants to override NO_PROXY, they'd have to also copy the global proxy values into the operator specific http/https proxy env vars.


When reconciling an operator's `Deployment`:

- If the global proxy object is not set / api doesn't exist, do nothing different.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question: If the global proxy object is not set - technically there can be N (N>1) CR instance(s) of Proxy type in the cluster. Will the global proxy object have a predefined name or will there be only one instance of Proxy? @ecordell @shawn-hurley

Copy link
Member Author

Choose a reason for hiding this comment

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

There will be many proxy objects, but the one named cluster should be considered the global proxy config.

@tkashem
Copy link
Collaborator

tkashem commented Jul 8, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2019
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ecordell, tkashem

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ecordell
Copy link
Member Author

ecordell commented Jul 8, 2019

/test unit

@ecordell
Copy link
Member Author

ecordell commented Jul 8, 2019

/retest


- `HTTP_PROXY`
- `HTTPS_PROXY`
- `NO_PROXY`
Copy link

Choose a reason for hiding this comment

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

@knrc somewhat relevant to your socks proxy question for istio.....there's still no socks proxy field being added to the global proxy config today that i know of, but if one is added, this is how i'd expect the value to get passed down to your OLM operator.

@openshift-merge-robot openshift-merge-robot merged commit 18b6c0d into operator-framework:master Jul 8, 2019
@rcernich
Copy link

Have you considered support for generic configuration options to configure the behavior of the operator, e.g. cli args and values? Or is that what the configMap option is for? I'm thinking this could be applicable to other types of controllers that could be installed through OLM (i.e. instead of having an operator manage those controllers, promote them and save the resources).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants