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

Run pulp container on https #73

Closed
wants to merge 1 commit into from
Closed

Conversation

fao89
Copy link
Member

@fao89 fao89 commented Apr 23, 2021

@pulpbot
Copy link
Member

pulpbot commented Apr 23, 2021

WARNING!!! This PR is not attached to an issue. In most cases this is not advisable. Please see our PR docs for more information about how to attach this PR to an issue.

@fao89 fao89 force-pushed the https_container branch 3 times, most recently from 8591e5e to 34a0f06 Compare April 23, 2021 22:15
}
openssl ec -in /etc/pulp/certs/token_private_key.pem -pubout -out /etc/pulp/certs/token_public_key.pem
openssl x509 -req -in /etc/pulp/certs/pulp_webserver.csr -CA /etc/pulp/certs/ca.crt -CAkey /etc/pulp/certs/ca.key \
-CAcreateserial -out /etc/pulp/certs/pulp_webserver.crt
Copy link
Member

Choose a reason for hiding this comment

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

That is not how execlineb works...
You need to foreground every single command that does not support chain calling, despite the last one.

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 got confused when reading about foreground, thank you for the help!

@fao89 fao89 force-pushed the https_container branch 5 times, most recently from 76dc21c to 295b8d4 Compare April 26, 2021 17:13
@mdellweg
Copy link
Member

I believe, you need to set verify-ssl or similar to false for the curl calls.

@fao89 fao89 force-pushed the https_container branch 3 times, most recently from fe60008 to 23375b2 Compare April 26, 2021 17:55
@daviddavis
Copy link
Contributor

This work looks great. Can I ask for a huge favor? Can we wait until after April 30 to merge this work (or at least hold off publishing the new containers until then)? The students working on the UI are using these and have their final project deadline on April 30. I don't want to mess them up.

@fao89 fao89 force-pushed the https_container branch 2 times, most recently from 3c69af4 to e933527 Compare April 26, 2021 19:42
@fao89
Copy link
Member Author

fao89 commented Apr 26, 2021

This work looks great. Can I ask for a huge favor? Can we wait until after April 30 to merge this work (or at least hold off publishing the new containers until then)? The students working on the UI are using these and have their final project deadline on April 30. I don't want to mess them up.

yes, this is just a POC, and it may break CI for almost every plugin. It needs to be proper advertised before merged, so people can have time to improve functional tests to deal with https

@fao89 fao89 force-pushed the https_container branch from e933527 to 5a13ae0 Compare April 26, 2021 20:15
@fao89
Copy link
Member Author

fao89 commented Apr 26, 2021

@fao89 fao89 force-pushed the https_container branch 4 times, most recently from 291d7f7 to 58d2c19 Compare May 4, 2021 00:21
@mdellweg
Copy link
Member

mdellweg commented May 4, 2021

This is a major change in the way the single container works.
Can we somehow make the switch to https optional? So we do not break every automation we have when merging this?

@daviddavis
Copy link
Contributor

We created an https tag that @fao89 is using to test things out.

Ideally I think we'd like to get all the other tags moved over to https but this seems like a monumental undertaking when one considers all the repos, release branches, etc.

@mdellweg
Copy link
Member

mdellweg commented May 4, 2021

What about the opposite tag (http / nossl / ...), and some day, we announce the switchover of latest?
Can we make the ssl version to be a thin container layer on top of the nossl version?

@fao89 fao89 force-pushed the https_container branch 2 times, most recently from 46efe5a to 48e7e0e Compare May 6, 2021 00:08
@fao89
Copy link
Member Author

fao89 commented May 6, 2021

@fao89 fao89 force-pushed the https_container branch 10 times, most recently from 7a7fb19 to 0cf64fc Compare May 7, 2021 14:12
@fao89 fao89 marked this pull request as ready for review May 7, 2021 14:20
@@ -0,0 +1,8 @@
FROM pulp/pulp-ci-centos:latest
Copy link
Member

Choose a reason for hiding this comment

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

💯

@@ -37,6 +37,13 @@ jobs:
if [ "$VERSION" = "latest" ]; then
docker build --file pulp_ci_centos/Containerfile --tag pulp/pulp-ci-centos:latest .
docker build --file pulp_galaxy_ng/Containerfile --tag pulp/pulp-galaxy-ng:latest .
# SSL
docker build --file pulp_ci_centos/SSL/Containerfile --tag pulp/pulp-ci-centos:https .
sed -i "s/latest/https/g" pulp_galaxy_ng/Containerfile
Copy link
Member

Choose a reason for hiding this comment

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

This is changing the files checked into this repository.
May be OK in CI, but the moment we are going to write up these steps in the README, it may get weird. WDYT?

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 didn't want to have a new container file just because of the tag, maybe we need to use a template

Copy link
Member Author

Choose a reason for hiding this comment

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

template + makefile?

Copy link
Member

Choose a reason for hiding this comment

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

So you would turn the SSL/Containerfile into a template? Would you template it on the fly? Or use the Makefile to template files we want to check into git? I really don't know what's the best way.
Another thought: We are now building the images with 3 layers of dependency. Maybe it is time to start using make to sort out the build sequence.
BTW: we could call that file pulp-ci-centos8/Containerfile.https. No one really says that it must be called Containerfile. So one nice naming schema would be <image_name>/Containerfile.<tag>. Wdyt?

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 agree with everything you said, including the part: "I really don't know what's the best way"
<image_name>/Containerfile.<tag>sounds really good!

@himdel
Copy link

himdel commented May 10, 2021

I'd like to request this feature be optional, with an option to go back to http.

We're using this for Cypress testing, and having to generate a self-signed certificate only to ignore it seems unecessary for that case, if it can be reasonably avoided :).

(context: ansible/ansible-hub-ui#378 (comment) )

Verified

This commit was created on github.com and signed with GitHub’s verified signature. The key has expired.
[noissue]
@fao89 fao89 force-pushed the https_container branch from a5c5ab1 to 6eeb819 Compare July 5, 2021 13:37
@fao89 fao89 closed this Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants