NO-JIRA: quote bootstrap IP in bootkube etcd endpoints manifest#1640
Conversation
WalkthroughThe etcd bootstrap annotation is quoted in the manifest template, render code now accepts an injected bootstrap IP locator, and render tests add IPv6 coverage by reading the generated ConfigMap and checking the bootstrap annotation. ChangesBootstrap IP rendering
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @Rusty-Gopher. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
Hey, While working on this fix for my usecase, i noticed that testrender() was actually never called with IPv6 bootstrap it, which was exactly my usecase. The IPv6 network configs were added in 876221a but only for testTemplateData() — the full render pipeline was never exercised with an IPv6 address. Now i have added a TestRenderIPv6 to cover this gap. Thanks |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/cmd/render/render_test.go`:
- Around line 313-325: The TestRenderIpv6 function currently only verifies that
render.Run() completes without error, but does not validate that the rendered
etcd-endpoints ConfigMap annotation contains the properly quoted IPv6 address.
Replace the testRender call with testTemplateData to enable proper validation of
the TemplateData structure. Add a validator that checks the EscapedBootstrapIP
field equals the correctly bracketed IPv6 address "[2001:db8::1]" to ensure the
IPv6 quoting fix is properly tested and the test would fail if the bracketing
were removed from the template.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 314396bd-9b13-4ded-8eff-c02ee8f62c79
📒 Files selected for processing (1)
pkg/cmd/render/render_test.go
| {{- if ne .BootstrapScalingStrategy "BootstrapInPlaceStrategy" }} | ||
| annotations: | ||
| alpha.installer.openshift.io/etcd-bootstrap: {{ .BootstrapIP }} | ||
| alpha.installer.openshift.io/etcd-bootstrap: "{{ .BootstrapIP }}" |
There was a problem hiding this comment.
There was a problem hiding this comment.
from my prespective, no, this one uses the k8s api client, not raw yaml files, so ipv6 colons are fine in json.
not the same problem.
| func testRender(t *testing.T, tc *testConfig) { | ||
| renderManifests(t, tc) | ||
| } |
There was a problem hiding this comment.
no need to have a function just to call another function. Let's keep it at testRender
| testRender(t, config) | ||
| } | ||
|
|
||
| func TestRenderIpv6(t *testing.T) { |
There was a problem hiding this comment.
funny that we don't already have a render test that looks at the generated yaml. Good addition.
| defaultBootstrapIPLocator = &fakeBootstrapIPLocator{ip: net.ParseIP("2001:db8::1")} | ||
| defer func() { defaultBootstrapIPLocator = orig }() |
There was a problem hiding this comment.
that looks strange, especially when we run tests in parallel. Do you mind to refactor the testConfig with it?
I don't really get why this needs to be on TestMain, I think we can remove that
There was a problem hiding this comment.
this probably is a larger refactoring, but it would be useful to have it injected through TemplateData directly
There was a problem hiding this comment.
this probably is a larger refactoring, but it would be useful to have it injected through TemplateData directly
There was a problem hiding this comment.
Done, i have moved bootstrapIP into testConfig and removed TestMain.
Also i have added bootstrapIPLocator as a field on renderOpts, so the fake locator in injected through this structt, avoiding any gloabal state mutation.
Atleast from my side. tests are fully isolated and safe to run in parallel
Can you please check it and let me know if thats ok for you, thanks for the hint!
There was a problem hiding this comment.
yeah works for me, thanks :)
There was a problem hiding this comment.
Thanks to you, that was quick, appericiate it.
If anything is needed from my side, just let me know :)
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
|
/lgtm Thank you! I'll tag verified once everything passes in CI. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tjungblu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/retitle NO-JIRA: quote bootstrap IP in bootkube etcd endpoints manifest |
openshift-ci
Bot
changed the title
|
/retest |
openshift-ci-robot
added
the
jira/valid-reference
|
@Rusty-Gopher: This pull request explicitly references no jira issue. DetailsIn response to this:
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. |
|
/verified by ci |
openshift-ci-robot
added
the
verified
|
@tjungblu: This PR has been marked as verified by DetailsIn response to this:
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. |
|
/override ci/prow/e2e-aws-ovn-serial-1of2 unrelated and known failure in both jobs, no bootstrap error. Neither of them runs ipv6 bootstrap anyway. |
|
@tjungblu: Overrode contexts on behalf of tjungblu: ci/prow/e2e-aws-ovn-serial-1of2, ci/prow/e2e-aws-ovn-single-node DetailsIn response to this:
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. |
|
@Rusty-Gopher: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
3 participants
During bootstrap, the rendered etcd-endpoints ConfigMap writes the bootstrap
IP into an annotation.
IPv4 bootstrap IPs render safely either way, but IPv6 addresses can contain
colon sequences such as
::. When emitted as an unquoted YAML plain scalar,a value like this can fail YAML parsing:
Quote the rendered annotation value so the bootstrap IP is always treated as
a string. This preserves the rendered value for IPv4 and fixes IPv6 bootstrap
addresses.
Changed:
to:
Tested:
Summary by CodeRabbit