Additional trust bundle #1152

Avielyo10 · 2020-11-17T10:48:39Z

Adding the option to add certificate bundle that will be added to the nodes' trusted certificate store.

openshift-ci-robot · 2020-11-17T10:48:48Z

Hi @Avielyo10. Thanks for your PR.

I'm waiting for a openshift-metal3 member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

hardys · 2020-11-17T11:26:08Z

ocp_install_env.sh

+    if [ ! -z "$ADDITIONAL_TRUST_BUNDLE" ]; then
+cat <<EOF
+additionalTrustBundle: |
+  $(echo ${ADDITIONAL_TRUST_BUNDLE} | sed -e ':a;N;$!ba;s/\n/\n  /g')


This will create a duplicate section when image_mirror_config is called (when MIRROR_IMAGES or ipv6 is specified), we'll need some way to merge the content.

Why duplicated?does image_mirror_config adds this section?

Yes exactly, it already adds this section https://github.com/openshift-metal3/dev-scripts/blob/master/utils.sh#L331

hardys · 2020-11-17T11:27:36Z

Thanks for the PR @Avielyo10 - I made a comment since we do already configure this when using a local registry, can you add this additional var in a way that's compatible with that please?

Also interested to understand the reason this is needed, can you expand on what you're testing at all?

Avielyo10 · 2020-11-17T11:33:57Z

@hardys We are testing OCP deployment & upgrades with our operators, so instead of deploying the cluster and then Adding the certs and waiting for the MC to finish we can do it in one shot
And yes of course I'll

When MIRROR_IMAGES is set or when using ipv6 (this is always true for ipv6) additionalTrustBundle section gets created, so give precedence to MIRROR_IMAGES CA

Avielyo10 · 2020-11-17T11:59:53Z

@hardys Please review :)

ocp_install_env.sh

hardys · 2020-11-18T11:28:10Z

/ok-to-test

hardys · 2020-11-18T17:15:28Z

/label tide/merge-method-squash
/approve

openshift-ci-robot · 2020-11-18T17:15:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hardys

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [hardys]

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

Avielyo10 · 2020-11-19T09:30:26Z

@hardys @dhellmann @zaneb Can this get /lgtm?

stbenjam · 2020-11-20T12:15:09Z

/lgtm

zaneb · 2020-11-20T14:59:20Z

config_example.sh

+# PEM-encoded X.509 certificate bundle that will be added to the nodes' trusted 
+# certificate store. This trust bundle may also be used when a proxy has 
+# been configured.
+# export ADDITIONAL_TRUST_BUNDLE=$(cat ca)


If we're expecting this to start as a file, and we need it as a stream, it seems a little odd to be passing around the contents (rather than just the filename) in an environment variable.

@zaneb I believe you are right, I'll work to fix this

zaneb · 2020-11-20T15:05:54Z

ocp_install_env.sh

+cat <<EOF
+$(echo ${ADDITIONAL_TRUST_BUNDLE} | awk '{ print " ", $0 }')
+EOF
+    fi


A less complicated version of this function:

if [ -n "${ADDITIONAL_TRUST_BUNDLE}" ]; then if [ -z "${MIRROR_IMAGES}" ]; then echo "additionalTrustBundle: |" fi echo "${ADDITIONAL_TRUST_BUNDLE}" | awk '{ print " ", $0 }' fi

This reverts commit 1e82cb5.

* Simplify additional_trust_bundle() * Pass ca via path instead of via env vars Improve readability Fix loca-bmo script (openshift-metal3#1144) (openshift-metal3#1084) Co-authored-by: Andrea Fasano <60063538+andfasano@users.noreply.github.com> Fix incorrect group name (openshift-metal3#1155) `whoami` does not always match the group name for the user. Use `id -gn` to set the group name properly. Also create GROUP environment variable and replace USER where appropriate for chown commands. Signed-off-by: Melvin Hillsman <mrhillsman@redhat.com> Additional trust bundle (openshift-metal3#1152) * Adding the option to add certs to install-config.yaml * Add ADDITIONAL_TRUST_BUNDLE to the config_example.sh * Avoid additionalTrustBundle duplication & move from sed to awk When MIRROR_IMAGES is set or when using ipv6 (this is always true for ipv6) additionalTrustBundle section gets created, so give precedence to MIRROR_IMAGES CA * In case MIRROR_IMAGES is set, concat ADDITIONAL_TRUST_BUNDLE Co-authored-by: Aviel Yosef <ayosef@redhat.com> Account for podman version < 2.0.0 (openshift-metal3#1151) The default for CentOS 8 is 1.6.4 which does not support units in `podman wait -i`. Add a version check to support both. Follow-up to commit 42af610. Update version detection and make dnsVIP conditional (openshift-metal3#1146) * Store local copy of release info We retrive this several times, so caching locally will be faster, and also provides a useful reference for debugging * Replace OPENSHIFT_VERSION reference with function This variable isn't always set, in particular in CI, so instead of relying on it, we can use a new openshift_version function which derives the version from the locally cached release info. * Only specify dnsVIP for < 4.5 This was removed in openshift/installer#3304

Aviel Yosef added 2 commits November 17, 2020 12:42

Adding the option to add certs to install-config.yaml

c942986

Add ADDITIONAL_TRUST_BUNDLE to the config_example.sh

9f6d132

openshift-ci-robot requested review from dhellmann and zaneb November 17, 2020 10:48

openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 17, 2020

hardys reviewed Nov 17, 2020

View reviewed changes

Avoid additionalTrustBundle duplication & move from sed to awk

3bbe825

When MIRROR_IMAGES is set or when using ipv6 (this is always true for ipv6) additionalTrustBundle section gets created, so give precedence to MIRROR_IMAGES CA

Avielyo10 requested a review from hardys November 17, 2020 12:04

hardys reviewed Nov 17, 2020

View reviewed changes

ocp_install_env.sh Show resolved Hide resolved

Avielyo10 requested a review from hardys November 17, 2020 15:47

In case MIRROR_IMAGES is set, concat ADDITIONAL_TRUST_BUNDLE

185f76c

openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 18, 2020

openshift-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Nov 18, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 18, 2020

openshift-ci-robot assigned stbenjam Nov 20, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 20, 2020

zaneb reviewed Nov 20, 2020

View reviewed changes

openshift-merge-robot merged commit 1e82cb5 into openshift-metal3:master Nov 20, 2020

Avielyo10 added a commit to Avielyo10/dev-scripts that referenced this pull request Nov 22, 2020

Revert "Additional trust bundle (openshift-metal3#1152)"

d3ff091

This reverts commit 1e82cb5.

hardys mentioned this pull request Nov 23, 2020

Additional trust bundle #1159

Merged

Additional trust bundle #1152

Additional trust bundle #1152

Uh oh!

Conversation

Avielyo10 commented Nov 17, 2020

Uh oh!

openshift-ci-robot commented Nov 17, 2020

Uh oh!

hardys Nov 17, 2020

Choose a reason for hiding this comment

Uh oh!

Avielyo10 Nov 17, 2020

Choose a reason for hiding this comment

Uh oh!

hardys Nov 17, 2020

Choose a reason for hiding this comment

Uh oh!

hardys commented Nov 17, 2020

Uh oh!

Avielyo10 commented Nov 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Avielyo10 commented Nov 17, 2020

Uh oh!

Uh oh!

hardys commented Nov 18, 2020

Uh oh!

hardys commented Nov 18, 2020

Uh oh!

openshift-ci-robot commented Nov 18, 2020

Uh oh!

Avielyo10 commented Nov 19, 2020

Uh oh!

stbenjam commented Nov 20, 2020

Uh oh!

zaneb Nov 20, 2020

Choose a reason for hiding this comment

Uh oh!

Avielyo10 Nov 22, 2020

Choose a reason for hiding this comment

Uh oh!

zaneb Nov 20, 2020

Choose a reason for hiding this comment

Uh oh!

Avielyo10 Nov 22, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Avielyo10 commented Nov 17, 2020 •

edited

Loading