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 first version for docs of kubermatic proxy whitelisting #166

Closed
wants to merge 5 commits into from

Conversation

toschneck
Copy link
Member

Based on my research, I added a frist version for proxy whitelisting

@kubermatic-bot kubermatic-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 13, 2019
@toschneck
Copy link
Member Author

/assign @thz @alvaroaleman

@kubermatic-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: toschneck
To complete the pull request process, please assign alvaroaleman
You can assign the PR to them by writing /assign @alvaroaleman in a comment when ready.

The full list of commands accepted by this bot can be found 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

@toschneck
Copy link
Member Author

/assign @alvaroaleman

Copy link
Contributor

@alvaroaleman alvaroaleman left a comment

Choose a reason for hiding this comment

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

Would it maybe make more sense to simply make this a table?

Name | URL | What is this needed for?

cc @thz

quay.io/coreos/container-linux-update-operator
```
{{% notice note %}}
[Kubermatic offline mode](https://docs.kubermatic.io/advanced/offline_mode/#kubermatic-offline-mode):
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont think this note makes sense here, the page is about proxy whitelisting. You could probably add a link in the head "hey, offline mode also exists" but here its not really adding value IMHO

Copy link
Member Author

Choose a reason for hiding this comment

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

will change the meaning. The point here is, if you use the offline mode, you maybe don't need to white-list some images

https://github.com/containernetworking/plugins/releases/

# Kubermatic health-monitor
https://raw.githubusercontent.com/kubermatic/machine-controller/8b5b66e4910a6228dfaecccaa0a3b05ec4902f8e/pkg/userdata/scripts/health-monitor.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make to reference a specific commit? This could change over time

Copy link
Member Author

Choose a reason for hiding this comment

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

no, I will change it

@toschneck
Copy link
Member Author

toschneck commented Oct 11, 2019

Would it maybe make more sense to simply make this a table?

Name | URL | What is this needed for?

cc @thz

For quickly copy and paste it from there to e.g. a proxy configuration, I decided to keep it in one file.

@kubermatic-bot kubermatic-bot added the dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. label Oct 11, 2019
@toschneck
Copy link
Member Author

@alvaroaleman / @thz can you please review the change, so we can merge this one?

quay.io/coreos/container-linux-update-operator
```
{{% notice note %}}
If you use the [Kubermatic offline mode](https://docs.kubermatic.io/advanced/offline_mode/#kubermatic-offline-mode), images will get pulled from the defined private registry (e.g. `172.20.0.2:5000`) instead of the public registries. This means, that some above mentioned whitelistings are maybe not required. As result the following images will get pulled from the private registry:
Copy link
Contributor

Choose a reason for hiding this comment

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

The comma after the means must go and the maybe in the second sentence is wrong. Also its not entirely clear that the two images are samples and this is done for all images.

I'd personally just remove the whole passage here and mention in the beginning that this is for an env that uses a http proxy and that if the env is airgapped, the docs are elsewhere.

Copy link
Member Author

@toschneck toschneck Nov 11, 2019

Choose a reason for hiding this comment

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

Also its not entirely clear that the two images are samples and this is done for all images.

I think this should get described here https://docs.kubermatic.io/advanced/offline_mode/. There the description say to me that only this images get pulled from other locations.

Anyway I removed the details and put it to the top.

Copy link
Contributor

Choose a reason for hiding this comment

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

The maybe is still there and its still wrong. If you want this to be just a doc about the whitelisting and don't want to do a doc about other proxy/offline env related stuff, please just remove this passage. It doesn't help if we have something that partly explains topic B on a page that is supposed to explain topic A.

Copy link
Member Author

Choose a reason for hiding this comment

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

my pushed didn't worked due to the change of permissions. I created a new PR #196 see commit b38b73e

@@ -0,0 +1,141 @@
+++
title = "Kubermatic Proxy Whitelisting"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes more sense to have a page that describes the whole HTTP proxy configuration and not just the whitelisting part of it.

Also I still think putting the images and binaries in a table would make all of this a lot easier and quicker to grasp.

Copy link
Member Author

@toschneck toschneck Nov 11, 2019

Choose a reason for hiding this comment

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

I agree that we should have a dedicated HTTP proxy description. But I don't think this is my task and part of this issue. This should part of the development cycle.

I'm wonder why we now search for reason to not merge at least this one? In my opinion this doesn't help ether.
So I create a separate issue for the HTTP proxy configuration: #195

@toschneck toschneck closed this Nov 13, 2019
@alvaroaleman alvaroaleman deleted the proxy-withelisting branch November 13, 2019 08:15
kubermatic-bot pushed a commit that referenced this pull request Nov 28, 2019
…#166' (#196)

* add first version for docs of kubermatic proxy whitelisting

* add let's encrypt URL to white listing

* update proxy-white listing docs, due to review comments

* update offline mode note for proxy whitelisting

* modify note for offline mode

* fix lint warnings

* fix typos and adjust PR to new version structure
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. 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.

None yet

4 participants