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

support custom CA bundle for AWS API #372

Merged
merged 1 commit into from Dec 7, 2020

Conversation

staebler
Copy link

When a cluster is installed in a AWS C2S region, access to the AWS API requires a custom CA bundle for trust. The custom CA bundle is read from the "ca-bundle.pem" key of the kube-cloud-config ConfigMap in the openshift-config-managed namespace.

https://issues.redhat.com/browse/CORS-1584

Copy link

@michaelgugino michaelgugino left a comment

Choose a reason for hiding this comment

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

/lgtm

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

// useCustomCABundle will set up a custom CA bundle in the AWS options if a CA bundle is configured in the
// kube cloud config.
func useCustomCABundle(awsOptions *session.Options, ctrlRuntimeClient client.Client) error {

Choose a reason for hiding this comment

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

Fetching this object each reconcile is a little spammy, but this allows users to potentially update the CA bundle day 2 (or whenever) and we don't need to worry about restarting our controller.

Copy link
Author

Choose a reason for hiding this comment

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

The resource will be cached locally. The client will not have to reach out to the server on every reconcile.

Copy link

@elmiko elmiko left a comment

Choose a reason for hiding this comment

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

this makes sense to me, but i have a couple requests. could we add a unit test to ensure that the custom bundle logic doesn't break in the future?

i think it would be nice to document this as well, maybe a small section at the bottom of the readme about how to use a custom ca bundle? (or even just an example in the examples directory)

@staebler
Copy link
Author

staebler commented Dec 3, 2020

this makes sense to me, but i have a couple requests. could we add a unit test to ensure that the custom bundle logic doesn't break in the future?

Yes.

i think it would be nice to document this as well, maybe a small section at the bottom of the readme about how to use a custom ca bundle? (or even just an example in the examples directory)

In my opinion, knowledge about how to update the custom CA bundle does not need to live in the cluster-api-provider. The custom CA bundle is used globally in the cluster for all interactions with the AWS API. So it makes sense to me to document maintenance of it more globally as well.

@elmiko
Copy link

elmiko commented Dec 3, 2020

In my opinion, knowledge about how to update the custom CA bundle does not need to live in the cluster-api-provider. The custom CA bundle is used globally in the cluster for all interactions with the AWS API. So it makes sense to me to document maintenance of it more globally as well.

that makes good sense to me, thanks!

edit: probably something we should document in the main machine-api-operator docs

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

@elmiko elmiko left a comment

Choose a reason for hiding this comment

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

thanks for adding the test @staebler !
/approve

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: elmiko

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 Dec 3, 2020
@elmiko
Copy link

elmiko commented Dec 4, 2020

/retest

@elmiko
Copy link

elmiko commented Dec 4, 2020

/test unit

@elmiko
Copy link

elmiko commented Dec 4, 2020

@staebler it looks like we might have done something that is affecting the unit tests. the failure seems unrelated to your change, i'm looking into a fix now.

@elmiko
Copy link

elmiko commented Dec 4, 2020

seems like we do have a fix, it is in #332

@brenton
Copy link

brenton commented Dec 7, 2020

/test unit

1 similar comment
@brenton
Copy link

brenton commented Dec 7, 2020

/test unit

@elmiko
Copy link

elmiko commented Dec 7, 2020

@brenton @staebler looks like we might need a rebase, there is a conflict in one of the files

@brenton
Copy link

brenton commented Dec 7, 2020

Is the "error: build error: error building at STEP "RUN umask 0002...--max-args 100 --no-run-if-empty chmod g+xw": exit status 1" message a flake? Searching chat history I see references to this resulting from cloning private repos.

@elmiko
Copy link

elmiko commented Dec 7, 2020

Is the "error: build error: error building at STEP "RUN umask 0002...--max-args 100 --no-run-if-empty chmod g+xw": exit status 1" message a flake? Searching chat history I see references to this resulting from cloning private repos.

it looks like a flake, that isn't how the test was failing before. seems like that is happening during the test setup.

@staebler
Copy link
Author

staebler commented Dec 7, 2020

Is the "error: build error: error building at STEP "RUN umask 0002...--max-args 100 --no-run-if-empty chmod g+xw": exit status 1" message a flake? Searching chat history I see references to this resulting from cloning private repos.

it looks like a flake, that isn't how the test was failing before. seems like that is happening during the test setup.

It is just an artifact of the merge conflict, right?

@elmiko
Copy link

elmiko commented Dec 7, 2020

could be

When a cluster is installed in a AWS C2S region, access to the AWS
API requires a custom CA bundle for trust. The custom CA bundle
is read from the "ca-bundle.pem" key of the kube-cloud-config
ConfigMap in the openshift-config-managed namespace.

https://issues.redhat.com/browse/CORS-1584
@staebler
Copy link
Author

staebler commented Dec 7, 2020

Unit tests are passing now after the rebase.

@michaelgugino
Copy link

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 7, 2020
@openshift-merge-robot openshift-merge-robot merged commit aca83e3 into openshift:master Dec 7, 2020
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.

None yet

6 participants