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

Add proxy configuration to bootstrap node #1832

Merged
merged 1 commit into from
Jun 25, 2019

Conversation

jcpowermac
Copy link
Contributor

Modification of bootstrapTemplateData struct to include proxy requirements.
Modification of getTemplateData and addStorageFiles to support proxy

Added:

  • /etc/profile.d/proxy.sh.template
  • /etc/systemd/system.conf.d/10-default-env.conf.template

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 7, 2019
@jcpowermac
Copy link
Contributor Author

Tested with squid using basic auth.

proxy:
  httpsProxy: "jcallen:foo@1.1.1.1:3128"
  httpProxy: "jcallen:foo@1.1.1.1:3128"
  noProxy: "devcluster.openshift.com,127.0.0.1,10.128.0.0/14,10.0.0.0/16,172.30.0.0/16"

Proxy for podman:
https://access.redhat.com/solutions/3939131
https://bugzilla.redhat.com/show_bug.cgi?id=1682970

This needs @patrickdillon PR: #1827

}, nil
}

func (a *Bootstrap) addStorageFiles(base string, uri string, templateData *bootstrapTemplateData) (err error) {
proxy := map[string]struct{}{
Copy link
Contributor

@abhinavdahiya abhinavdahiya Jun 11, 2019

Choose a reason for hiding this comment

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

is it possible to move this to the call-site for this function ie in the Generate for bootstrap ignition asset...?

@@ -218,6 +225,11 @@ func (a *Bootstrap) addStorageFiles(base string, uri string, templateData *boots
}

name := info.Name()
if _, ok := proxy[name]; ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you make proxy a map[string]bool and populate it with true values, then you can right this expression as the more simple if proxy[name] {.

@@ -0,0 +1,5 @@
{{if .Proxy}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know that we care, but this will result in empty lines in the file.

To do it without empty lines,

{{ if .Proxy -}}
{{ if .Proxy.HTTPProxy -}}
export HTTP_PROXY="{{ .Proxy.HTTPProxy }}"
{{ end -}}
...
{{ end -}}

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 12, 2019
@jcpowermac jcpowermac changed the title [WIP] Add proxy configuration to bootstrap node Add proxy configuration to bootstrap node Jun 13, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 13, 2019
@jcpowermac
Copy link
Contributor Author

/retest

1 similar comment
@jcpowermac
Copy link
Contributor Author

/retest

@jcpowermac
Copy link
Contributor Author

/test e2e-openstack

@danehans
Copy link
Contributor

/test e2e-aws-scaleup-rhel7

@@ -183,10 +185,15 @@ func (a *Bootstrap) getTemplateData(installConfig *types.InstallConfig) (*bootst
PullSecret: installConfig.PullSecret,
ReleaseImage: releaseImage,
EtcdCluster: strings.Join(etcdEndpoints, ","),
Proxy: installConfig.Proxy,
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be the install-config, but the status from object from #1866

}, nil
}

func (a *Bootstrap) addStorageFiles(base string, uri string, templateData *bootstrapTemplateData) (err error) {
proxy := map[string]bool{
Copy link
Contributor

Choose a reason for hiding this comment

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

the files will automatically be empty if proxy is not set, right?

Is it a problem if we write empty file to disk? If is not a problem, I think it will be cleaner to always write those file, even though they might be empty..?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No issues were caused by having the files empty. The cluster installed normally. Updated the PR to remove this section.

Modification of `bootstrapTemplateData` struct to include proxy requirements.
Modification of `getTemplateData` and `addStorageFiles` to support proxy
Modification of proxy manifests

Added:
- /etc/profile.d/proxy.sh.template
- /etc/systemd/system.conf.d/10-default-env.conf.template
```yaml
---
apiVersion: v1
baseDomain: devcluster.openshift.com
Copy link
Contributor

Choose a reason for hiding this comment

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

include ... so that people know there's more that's required.

@abhinavdahiya
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 24, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, jcpowermac

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-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 24, 2019
@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 2b9afc6 into openshift:master Jun 25, 2019
@openshift-ci-robot
Copy link
Contributor

@jcpowermac: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/e2e-aws-scaleup-rhel7 e7edbf7 link /test e2e-aws-scaleup-rhel7

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@jcpowermac jcpowermac deleted the CORS-1076 branch June 27, 2019 19:02
wking added a commit to wking/openshift-installer that referenced this pull request Sep 1, 2019
This is a bit more accessible than pointing folks at Godocs, since it
allows us to focus on the YAML property names (while Godocs
understandably focus on Go property names) and YAML renderings.  Also
break up our old "one big example" install-config.yaml into a minimal
per-platform example and a series of small extentions excercising
groups of properties.

The vSphere docs are based heavily on [1].

Also drop proxy.md.  It was added in e7edbf7 (Add proxy
configuration to bootstrap node, 2019-06-24, openshift#1832), but:

* Proxy testing and Squid configuration information belongs in
  openshift/release, not in the installer repository.
* docs/user/customization.md now contains a more complete proxy-config
  fragment.

[1]: https://github.com/openshift/openshift-docs/blob/enterprise-4.2/modules/installation-vsphere-config-yaml.adoc
     Last touched by commit openshift/openshift-docs@25afc76 , 2019-08-19
wking added a commit to wking/openshift-installer that referenced this pull request Sep 4, 2019
This is a bit more accessible than pointing folks at Godocs, since it
allows us to focus on the YAML property names (while Godocs
understandably focus on Go property names) and YAML renderings.  Also
break up our old "one big example" install-config.yaml into a minimal
per-platform example and a series of small extentions excercising
groups of properties.

The vSphere docs are based heavily on [1].

Also drop proxy.md.  It was added in e7edbf7 (Add proxy
configuration to bootstrap node, 2019-06-24, openshift#1832), but:

* Proxy testing and Squid configuration information belongs in
  openshift/release, not in the installer repository.
* docs/user/customization.md now contains a more complete proxy-config
  fragment.

[1]: https://github.com/openshift/openshift-docs/blob/enterprise-4.2/modules/installation-vsphere-config-yaml.adoc
     Last touched by commit openshift/openshift-docs@25afc76 , 2019-08-19
wking added a commit to wking/openshift-installer that referenced this pull request Sep 9, 2019
This is a bit more accessible than pointing folks at Godocs, since it
allows us to focus on the YAML property names (while Godocs
understandably focus on Go property names) and YAML renderings.  Also
break up our old "one big example" install-config.yaml into a minimal
per-platform example and a series of small extentions excercising
groups of properties.

The vSphere docs are based heavily on [1].

Also drop proxy.md.  It was added in e7edbf7 (Add proxy
configuration to bootstrap node, 2019-06-24, openshift#1832), but:

* Proxy testing and Squid configuration information belongs in
  openshift/release, not in the installer repository.
* docs/user/customization.md now contains a more complete proxy-config
  fragment.

[1]: https://github.com/openshift/openshift-docs/blob/enterprise-4.2/modules/installation-vsphere-config-yaml.adoc
     Last touched by commit openshift/openshift-docs@25afc76 , 2019-08-19
wking added a commit to wking/openshift-installer that referenced this pull request Sep 9, 2019
This is a bit more accessible than pointing folks at Godocs, since it
allows us to focus on the YAML property names (while Godocs
understandably focus on Go property names) and YAML renderings.  Also
break up our old "one big example" install-config.yaml into a minimal
per-platform example and a series of small extentions excercising
groups of properties.

The vSphere docs are based heavily on [1].

Also drop proxy.md.  It was added in e7edbf7 (Add proxy
configuration to bootstrap node, 2019-06-24, openshift#1832), but:

* Proxy testing and Squid configuration information belongs in
  openshift/release, not in the installer repository.
* docs/user/customization.md now contains a more complete proxy-config
  fragment.

[1]: https://github.com/openshift/openshift-docs/blob/enterprise-4.2/modules/installation-vsphere-config-yaml.adoc
     Last touched by commit openshift/openshift-docs@25afc76 , 2019-08-19
wking added a commit to wking/openshift-installer that referenced this pull request Sep 9, 2019
This is a bit more accessible than pointing folks at Godocs, since it
allows us to focus on the YAML property names (while Godocs
understandably focus on Go property names) and YAML renderings.  Also
break up our old "one big example" install-config.yaml into a minimal
per-platform example and a series of small extentions excercising
groups of properties.

The vSphere docs are based heavily on [1].

Also drop proxy.md.  It was added in e7edbf7 (Add proxy
configuration to bootstrap node, 2019-06-24, openshift#1832), but:

* Proxy testing and Squid configuration information belongs in
  openshift/release, not in the installer repository.
* docs/user/customization.md now contains a more complete proxy-config
  fragment.

OpenStack computeFlavor precedence is based on [2].

[1]: https://github.com/openshift/openshift-docs/blob/enterprise-4.2/modules/installation-vsphere-config-yaml.adoc
     Last touched by commit openshift/openshift-docs@25afc76 , 2019-08-19
[2]: openshift#2162 (comment)
wking added a commit to wking/openshift-installer that referenced this pull request Sep 9, 2019
This is a bit more accessible than pointing folks at Godocs, since it
allows us to focus on the YAML property names (while Godocs
understandably focus on Go property names) and YAML renderings.  Also
break up our old "one big example" install-config.yaml into a minimal
per-platform example and a series of small extentions excercising
groups of properties.

The vSphere docs are based heavily on [1].

Also drop proxy.md.  It was added in e7edbf7 (Add proxy
configuration to bootstrap node, 2019-06-24, openshift#1832), but:

* Proxy testing and Squid configuration information belongs in
  openshift/release, not in the installer repository.
* docs/user/customization.md now contains a more complete proxy-config
  fragment.

OpenStack computeFlavor precedence is based on [2].

[1]: https://github.com/openshift/openshift-docs/blob/enterprise-4.2/modules/installation-vsphere-config-yaml.adoc
     Last touched by commit openshift/openshift-docs@25afc76 , 2019-08-19
[2]: openshift#2162 (comment)
jhixson74 pushed a commit to jhixson74/installer that referenced this pull request Dec 6, 2019
This is a bit more accessible than pointing folks at Godocs, since it
allows us to focus on the YAML property names (while Godocs
understandably focus on Go property names) and YAML renderings.  Also
break up our old "one big example" install-config.yaml into a minimal
per-platform example and a series of small extentions excercising
groups of properties.

The vSphere docs are based heavily on [1].

Also drop proxy.md.  It was added in e7edbf7 (Add proxy
configuration to bootstrap node, 2019-06-24, openshift#1832), but:

* Proxy testing and Squid configuration information belongs in
  openshift/release, not in the installer repository.
* docs/user/customization.md now contains a more complete proxy-config
  fragment.

OpenStack computeFlavor precedence is based on [2].

[1]: https://github.com/openshift/openshift-docs/blob/enterprise-4.2/modules/installation-vsphere-config-yaml.adoc
     Last touched by commit openshift/openshift-docs@25afc76 , 2019-08-19
[2]: openshift#2162 (comment)
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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants