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.

Sign up for GitHub

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

Jump to bottom

🐛 (kustomize/v2): fix ServiceMonitor with TLS kustomize scaffolding #4536

Merged
merged 1 commit into from
Feb 8, 2025
Merged

🐛 (kustomize/v2): fix ServiceMonitor with TLS kustomize scaffolding #4536

merged 1 commit into from
Feb 8, 2025

Conversation

abhishekdwivedi3060
Copy link
Contributor

@abhishekdwivedi3060 abhishekdwivedi3060 commented Feb 2, 2025 •
edited
Loading

Fixes #4533

  • Fixed config/prometheus/monitor_tls_patch.yaml fiel
  • Added serverName field in monitor_tls_patch.yaml for metric server cert validation

@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Feb 2, 2025 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: abhishekdwivedi3060 / name: Abhisek Dwivedi (97071c1)

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Feb 2, 2025
@k8s-ci-robot
Copy link
Contributor

Welcome @abhishekdwivedi3060!

It looks like this is your first PR to kubernetes-sigs/kubebuilder 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kubebuilder has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

Hi @abhishekdwivedi3060. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

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.

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 2, 2025
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 2, 2025
camilamacedo86
camilamacedo86 reviewed Feb 5, 2025
View reviewed changes
...gins/common/kustomize/v2/scaffolds/internal/templates/config/prometheus/monitor_tls_patch.go
path: /spec/endpoints/0/tlsConfig
value:
# SERVICE_NAME and SERVICE_NAMESPACE will be substituted by kustomize
serverName: SERVICE_NAME.SERVICE_NAMESPACE.svc
Copy link
Member

Choose a reason for hiding this comment

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

Did we only need to change the kustomize default file just to pass the serviceName right?
But that is fixed. It will always be the same.
Could we not simplify the changes and the default kustomize by adding here the name

Suggested change
serverName: SERVICE_NAME.SERVICE_NAMESPACE.svc
serverName: controller-manager-metrics-monitor.SERVICE_NAMESPACE.svc

Instead?

Copy link
Member

Choose a reason for hiding this comment

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

I am trying to see if we can avoid do changes at: config/default/kustomization.yaml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to follow the existing convention followed in kubebuilder kustomize files.
For other scenarios where the service name is referred, we follow similar convention of replacements. See certificate
I'm not sure if this serverName replacement is any different. Let me know if my understanding is wrong

Copy link
Member

Choose a reason for hiding this comment

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

I will need a few time to check/think about this one.
But I will be back to you ASAP

Copy link
Member

@camilamacedo86 camilamacedo86 Feb 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I am not too happy with the complexities in the kustomize, but I think that is not part of the scope, and it makes sense you follow the same approach. Thank you.

@camilamacedo86
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-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 Feb 8, 2025
@camilamacedo86
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 8, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhishekdwivedi3060, camilamacedo86

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 8, 2025
@k8s-ci-robot k8s-ci-robot merged commit 9abbd42 into kubernetes-sigs:master Feb 8, 2025
7 checks passed
camilamacedo86 added a commit to camilamacedo86/kubebuilder that referenced this pull request Feb 10, 2025
@camilamacedo86
Revert "Merge pull request kubernetes-sigs#4536 from abhishekdwivedi3…
c5ac95e 
…060/master"

This reverts commit 9abbd42, reversing
changes made to 5845cb3.
@camilamacedo86 camilamacedo86 mentioned this pull request Feb 10, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camilamacedo86 camilamacedo86 camilamacedo86 left review comments

@Kavinjsir Kavinjsir Awaiting requested review from Kavinjsir

@varshaprasad96 varshaprasad96 Awaiting requested review from varshaprasad96

Assignees

@camilamacedo86 camilamacedo86

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Kubebuilder scaffolding with HTTPS endpoint for metrics endpoint with custom certs doesn't work
3 participants
@abhishekdwivedi3060 @k8s-ci-robot @camilamacedo86