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

https://issues.redhat.com/browse/ACM-14417 Ver. Strategy doc #7222

Merged
merged 18 commits into from
Nov 21, 2024

Conversation

oafischer
Copy link
Contributor

Train 19

@oafischer oafischer requested a review from zhujian7 November 12, 2024 15:08
@zhujian7
Copy link
Contributor

zhujian7 commented Nov 12, 2024

@oafischer can we also mention the strategy when creating/importing a cluster, if it is a ROSA-HCP cluster, should check if the UseSystemTruststore is configured, for example:

1.6.4 Cluster creation
Add a pre-flight for the cluster creation
If your hub cluster API server certificate is issued by a public CA(e.g. Let’s Encrypt). For example, ROSA-HCP. Please refer to 1.6.18.1 to check if the hub server verification strategy is configured to UseSystemTruststore.

1.6.5 Cluster import
Add a pre-flight for the cluster import
If your hub cluster API server certificate is issued by a public CA(e.g. Let’s Encrypt). For example, ROSA-HCP. Please refer to 1.6.18.1 to check if the hub server verification strategy is configured to UseSystemTruststore.

@zhujian7
Copy link
Contributor

@elgnay please help take a review, thx!

@oafischer
Copy link
Contributor Author

@zhujian7 We can add the pre-flight info but I'm not sure where a good spot for it would be. In our creating and import doc, we do not have anything related to ROSA.

HCP content has also moved to OCP docs entirely in 2.12, so we don't have any HCP docs left in ACM docs where this could fit in either.

When reading pre-flight info, it sounds like it applies to more than just ROSA HCP. Is that the case?

@zhujian7
Copy link
Contributor

HCP content has also moved to OCP docs entirely in 2.12, so we don't have any HCP docs left in ACM docs where this could fit in either.

If so, I think it's OK without the pre-flight info. @elgnay WDYT?

@elgnay
Copy link
Contributor

elgnay commented Nov 13, 2024

@oafischer @zhujian7 I still think we need to add one more item to the prerequisites of cluster creation and cluster importing:

Since the default strategy 'UseAutoDetectedCABundle' may not work always, it necessary for the user to review the hub API server certificate verification strategy before they start creating or importing managed clusters.

@oafischer
Copy link
Contributor Author

@zhujian7 @elgnay Thanks for the feedback, I've added the requested info to the general prereq sections for import and create. I think it works well there. Let me know if it looks good now. Thanks!

@elgnay
Copy link
Contributor

elgnay commented Nov 14, 2024

LGTM

@zhujian7
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Nov 14, 2024
@dockerymick dockerymick self-requested a review November 15, 2024 15:39
clusters/cluster_lifecycle/adv_config_cluster.adoc Outdated Show resolved Hide resolved
clusters/cluster_lifecycle/adv_config_cluster.adoc Outdated Show resolved Hide resolved
clusters/cluster_lifecycle/adv_config_cluster.adoc Outdated Show resolved Hide resolved
clusters/cluster_lifecycle/adv_config_cluster.adoc Outdated Show resolved Hide resolved
clusters/cluster_lifecycle/adv_config_cluster.adoc Outdated Show resolved Hide resolved
clusters/cluster_lifecycle/adv_config_cluster.adoc Outdated Show resolved Hide resolved
clusters/cluster_lifecycle/adv_config_cluster.adoc Outdated Show resolved Hide resolved
Comment on lines +367 to +370
See the examples in one of the following three strategies to learn how to manually configure a strategy:

[#config-hub-strategy-auto-detect-ca]
=== Configuring the strategy with `UseAutoDetectedCABundle`
Copy link
Contributor

Choose a reason for hiding this comment

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

Here's an example of what I was talking about. Related comment: https://github.com/stolostron/rhacm-docs/pull/7222/files#r1844139775

The tricky part is adding callouts to describe the values in the YAML sample. We can probably still keep the examples, and the only line reduction would come from removing the anchors and titles

Suggested change
See the examples in one of the following three strategies to learn how to manually configure a strategy:
[#config-hub-strategy-auto-detect-ca]
=== Configuring the strategy with `UseAutoDetectedCABundle`
See the examples in one of the following three strategies to learn how to manually configure a strategy:
`UseAutoDetectedCABundle`:: The default configuration strategy is `UseAutoDetectedCABundle`. The {mce-short} automatically detects the certificate on the hub cluster and merges the certificate configured in the `trustedCABundles` list of config map references to the real CA bundles, if there are any.
`UseSystemTruststore`:: With `UseSystemTruststore`, {mce-short} does not detect any certificate and ignores the certificates configured in the `trustedCABundles` parameter section. This configuration does not pass any certificate to the managed clusters. Instead, the managed clusters use certificates from the system trusted store of the managed clusters to verify the hub cluster API server. This applies to situations where a public CA, such as `Let's Encrypt`, issues the hub cluster certificate.
`UseCustomCABundles`:: You can use `UseCustomCABundles` if you know the CA of the hub cluster API server and do not want {mce-short} to automatically detect it. For this strategy, {mce-short} adds your configured certificates from the `trustedCABundles` parameter to the managed clusters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for sharing an example. As you mentioned, integrating the YAML samples will be difficult. I'm almost leaning more to keeping the titles, since those make it a bit clearer that there are steps that the user needs to do. Since there are only 3 titles , are you OK with just keeping it as is?

@openshift-ci openshift-ci bot added the lgtm label Nov 18, 2024
Copy link

openshift-ci bot commented Nov 18, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dockerymick, oafischer, zhujian7

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

@oafischer oafischer merged commit 8f4f7e1 into 2.12_stage Nov 21, 2024
0 of 2 checks passed
@oafischer oafischer deleted the of-train19 branch November 21, 2024 09:31
@@ -30,6 +30,11 @@ spec:
clusterSet: global
----

[#creating-managedclusterset-prereq]
Copy link
Contributor

Choose a reason for hiding this comment

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

HI @oafischer the prerequisite is NOT for "create_clusterset", but for "create cluster", because the strategy is used for importing a cluster, and after a cluster is created, it will be imported to the hub by default. so we may need to:

  • delete the prerequisite here
  • instead, add the prerequisite in create_cluster_cli.adoc and create_cluster_on_prem.adoc
  • add the prerequisite in "import_cli.adoc" and "import_gui.adoc" as well

Copy link
Contributor

Choose a reason for hiding this comment

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

I created a task for this https://issues.redhat.com/browse/ACM-16098

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for creating the issue, I'll take care of it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants