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

LOG-4445: Forwarder must use hosted cluster_id on HCP cluster #2990

Merged
merged 1 commit into from
Mar 28, 2025

Conversation

Clee2691
Copy link
Contributor

@Clee2691 Clee2691 commented Mar 18, 2025

Description

This PR updates #2280 to use the hypershift API instead of the unstructured object.

The github.com/openshift/hypershift/api package is added as a dependency.

New permissions are added to the role that CLO's serviceAccount is bound to:

- apiGroups:
  - hypershift.openshift.io
  resources:
  - hostedcontrolplanes
  verbs:
  - list
  - get

Lastly, this PR adds the VRL transform that kubeAPI audit logs should go through as it was missing for http receiver inputs.

/cc @cahartma @vparfonov @alanconway
/assign @jcantrill

Links

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Mar 18, 2025
@openshift-ci-robot
Copy link

openshift-ci-robot commented Mar 18, 2025

@Clee2691: This pull request references LOG-4445 which is a valid jira issue.

In response to this:

Description

This PR updates #2280 to use the hypershift API instead of the unstructured object.

The github.com/openshift/hypershift/api package is added as a dependency.

/cc @cahartma @vparfonov @alanconway
/assign @jcantrill

Links

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 openshift-eng/jira-lifecycle-plugin repository.

Copy link
Contributor

@jcantrill jcantrill left a comment

Choose a reason for hiding this comment

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

/approve

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 18, 2025
@jcantrill
Copy link
Contributor

/hold

pending feedback from QE team

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 18, 2025
Copy link
Contributor

@alanconway alanconway 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 openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 19, 2025
Copy link
Contributor

openshift-ci bot commented Mar 19, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alanconway, Clee2691, jcantrill

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:
  • OWNERS [alanconway,jcantrill]

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

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 21, 2025
@openshift-ci-robot
Copy link

openshift-ci-robot commented Mar 21, 2025

@Clee2691: This pull request references LOG-4445 which is a valid jira issue.

In response to this:

Description

This PR updates #2280 to use the hypershift API instead of the unstructured object.

The github.com/openshift/hypershift/api package is added as a dependency.

New permissions are added to the role that CLO's serviceAccount is bound to:

- apiGroups:
 - hypershift.openshift.io
 resources:
 - hostedcontrolplanes
 verbs:
 - list
 - get

Lastly, this PR adds the VRL transform that kubeAPI audit logs should go through as it was missing for http receiver inputs.

/cc @cahartma @vparfonov @alanconway
/assign @jcantrill

Links

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 openshift-eng/jira-lifecycle-plugin repository.

@Clee2691
Copy link
Contributor Author

/retest

1 similar comment
@Clee2691
Copy link
Contributor Author

/retest

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 26, 2025
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 26, 2025
Copy link
Contributor

openshift-ci bot commented Mar 27, 2025

@anpingli: The label(s) /label qe_approved cannot be applied. These labels are supported: acknowledge-critical-fixes-only, platform/aws, platform/azure, platform/baremetal, platform/google, platform/libvirt, platform/openstack, ga, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, px-approved, docs-approved, qe-approved, ux-approved, no-qe, downstream-change-needed, rebase/manual, cluster-config-api-changed, approved, backport-risk-assessed, bugzilla/valid-bug, cherry-pick-approved, jira/valid-bug, stability-fix-approved, staff-eng-approved. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

In response to this:

/label qe_approved

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-sigs/prow repository.

@anpingli
Copy link

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Mar 27, 2025
@openshift-ci-robot
Copy link

openshift-ci-robot commented Mar 27, 2025

@Clee2691: This pull request references LOG-4445 which is a valid jira issue.

In response to this:

Description

This PR updates #2280 to use the hypershift API instead of the unstructured object.

The github.com/openshift/hypershift/api package is added as a dependency.

New permissions are added to the role that CLO's serviceAccount is bound to:

- apiGroups:
 - hypershift.openshift.io
 resources:
 - hostedcontrolplanes
 verbs:
 - list
 - get

Lastly, this PR adds the VRL transform that kubeAPI audit logs should go through as it was missing for http receiver inputs.

/cc @cahartma @vparfonov @alanconway
/assign @jcantrill

Links

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 openshift-eng/jira-lifecycle-plugin repository.

@anpingli
Copy link

anpingli commented Mar 27, 2025

QE Test pass

  • The hcp cluster id is used when CLF is deployed in the hypershift control panel namespace.
  • The management cluster ID is used when CLF is deployed in the other namespaces.

File a bug https://issues.redhat.com/browse/LOG-6921. Please feel free to close it as known issue.
As hcp cluster ID can only be injected in the hypershift control panel namespace, we need a release note about this point.

@Clee2691
Copy link
Contributor Author

/retest

@Clee2691
Copy link
Contributor Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 27, 2025
Copy link
Contributor

openshift-ci bot commented Mar 27, 2025

@Clee2691: all tests passed!

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

@@ -68,6 +68,7 @@ func init() {
utilruntime.Must(monitoringv1.AddToScheme(scheme))
utilruntime.Must(configv1.AddToScheme(scheme))
utilruntime.Must(securityv1.AddToScheme(scheme))
utilruntime.Must(hypershiftv1beta1.AddToScheme(scheme))
Copy link
Contributor

Choose a reason for hiding this comment

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

@Clee2691 @anpingli have we tested this in a non HCP cluster to ensure the operator does not fail to start because maybe it is missing the CRD for the hypershift apis?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't that what the e2e tests basically confirm? CLO is able to be installed and send logs even with the hypershift scheme added. On each reconcile, it will try to fetch the clusterID and version so if it failed, none of those tests would have passed.

Choose a reason for hiding this comment

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

Tested on regular cluster, the clusterID present correctly. No regression issue.

Copy link
Contributor

@jcantrill jcantrill left a comment

Choose a reason for hiding this comment

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

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 27, 2025
@jcantrill
Copy link
Contributor

/hold cancel
/lgtm

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Mar 28, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit bcc5421 into openshift:release-6.2 Mar 28, 2025
8 checks passed
@Clee2691
Copy link
Contributor Author

/cherry-pick master

@openshift-cherrypick-robot

@Clee2691: #2990 failed to apply on top of branch "master":

Applying: LOG-4445: Forwarder must use hosted cluster_id on HCP cluster
Using index info to reconstruct a base tree...
M	bundle/manifests/cluster-logging.clusterserviceversion.yaml
M	internal/controller/observability/clusterlogforwarder_controller.go
A	internal/generator/vector/filter/openshift/viaq/filter.go
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): internal/generator/vector/filter/openshift/viaq/filter.go deleted in HEAD and modified in LOG-4445: Forwarder must use hosted cluster_id on HCP cluster. Version LOG-4445: Forwarder must use hosted cluster_id on HCP cluster of internal/generator/vector/filter/openshift/viaq/filter.go left in tree.
Auto-merging internal/controller/observability/clusterlogforwarder_controller.go
CONFLICT (content): Merge conflict in internal/controller/observability/clusterlogforwarder_controller.go
Auto-merging bundle/manifests/cluster-logging.clusterserviceversion.yaml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 LOG-4445: Forwarder must use hosted cluster_id on HCP cluster

In response to this:

/cherry-pick master

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-sigs/prow repository.

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. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. qe-approved Signifies that QE has signed off on this PR release/6.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants