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

templates: add apiserver-url.env file #2232

Merged
merged 1 commit into from
Dec 4, 2020
Merged

templates: add apiserver-url.env file #2232

merged 1 commit into from
Dec 4, 2020

Conversation

squeed
Copy link
Contributor

@squeed squeed commented Nov 17, 2020

A few components need to know the URL of the internal apiserver load balancer. Most notably, the network operator does.

Right now, the only way to get this is to parse the kubelet's kubeconfig, but this is fragile. So, write down an environment file that just sets the "usual" service environment variables. Then we can just seamlessly source the environment file.

Ref: SDN-1347

@squeed
Copy link
Contributor Author

squeed commented Nov 18, 2020

Nice, CI seems basically green.

@@ -0,0 +1,7 @@
mode: 0644
path: "/etc/kubernetes/apiserver-url.env"
# used by the cluster network operator to learn the URL to the internal apiserver load balancer
Copy link
Member

Choose a reason for hiding this comment

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

For things like the binaries extracted to the host? Or code running in a container? Both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The motivating use case is the CNO, which is in a container (and thus bind-mounts it), but I suspect we'll find a few more uses for it.

@@ -0,0 +1,7 @@
mode: 0644
path: "/etc/kubernetes/apiserver-url.env"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should add something like a "MCO internals" package that things like the SDN could vendor in that would have this as a constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, that's a decent idea - as long as it won't annoy go mod.

Copy link
Contributor Author

@squeed squeed Nov 23, 2020

Choose a reason for hiding this comment

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

That said, this path is certainly not changeable, so parameterizing it isn't really useful.

Up to you.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 18, 2020
A few components need to know the URL of the internal apiserver load
balancer. Most notably, the network operator does.

Right now, the only way to get this is to parse the kubelet's
kubeconfig, but this is fragile. So, write down an environment file
that just sets the "usual" service environment variables. Then we can
just seamlessly souce the environment file.
@squeed
Copy link
Contributor Author

squeed commented Nov 30, 2020

@cgwalters I updated to add a pkg/constants reference. Let me know what you think. I'm not super happy with the naming.

package constants

// constants defines some file paths that are shared outside of the
// MCO package; and thus consumed by other users
Copy link
Member

Choose a reason for hiding this comment

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

Thanks! This should help us in the future in doing things like searching across the OpenShift repos to see which other projects are using bits from the MCO.

Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

Thanks!

@squeed
Copy link
Contributor Author

squeed commented Dec 1, 2020

/retest

@squeed
Copy link
Contributor Author

squeed commented Dec 1, 2020

@cgwalters could I get a lgtm again?

@cgwalters
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 2, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, squeed

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

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot
Copy link
Contributor

@squeed: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/okd-e2e-aws 230863c link /test okd-e2e-aws
ci/prow/e2e-aws-workers-rhel7 230863c link /test e2e-aws-workers-rhel7

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 60f66d0 into openshift:master Dec 4, 2020
Fedosin added a commit to Fedosin/machine-config-operator that referenced this pull request Jul 9, 2021
Azure node controller needs to know the URL of the internal apiserver
load balancer.

To do it, this commit creates an environment file with required env
variables, that can be consumed by the controller.

This approach is similar to what we do on master nodes:
openshift#2232
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants