-
Notifications
You must be signed in to change notification settings - Fork 44
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
Update openstack-k8s-operators #843
base: main
Are you sure you want to change the base?
Conversation
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/62709f3b0d0640b3a0da84fdd233d330 ✔️ openstack-k8s-operators-content-provider SUCCESS in 3h 32m 33s |
f90db8f
to
8416bb7
Compare
/retest |
/retest-required |
8416bb7
to
9dec273
Compare
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/576ebc76f8004dc49a2de4183c1a1442 ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 44m 17s |
9dec273
to
bfb5fa3
Compare
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/4b7f4a5617764ae78b52347a2ef91a54 ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 28m 33s |
recheck |
1 similar comment
recheck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK we need this adaptation due to openstack-k8s-operators/lib-common@3824fa1
The controller change looks good to me, we want to set a condition. I have a suggestion to improve on the test assert.
@@ -933,13 +933,11 @@ var _ = Describe("NovaAPI controller", func() { | |||
}) | |||
|
|||
It("reports that the CA secret is missing", func() { | |||
th.ExpectConditionWithDetails( | |||
th.ExpectCondition( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be better to keep the detailed check. E.g. it shows that we basically lost a bit of details from the error message like the namespace of the secret, and the fact that what is missing is a Secret.
th.ExpectConditionWithDetails(
novaNames.APIName,
ConditionGetterFunc(NovaAPIConditionGetter),
condition.TLSInputReadyCondition,
corev1.ConditionFalse,
condition.RequestedReason,
"TLSInput is missing: combined-ca-bundle",
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
bfb5fa3
to
08c2cfc
Compare
condition.ErrorReason, | ||
fmt.Sprintf("TLSInput error occured in TLS sources Secret %s/internal-tls-certs not found", novaNames.Namespace), | ||
condition.RequestedReason, | ||
"TLSInput is missing: combined-ca-bundle", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that seems to be a bug. This test case does create a the CA bunde, but does not create the internal tls cert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I differed errors by using "one or more cert secrets" error log like other operators do (to be consistent) but if we want be more specific we can go with separate errors for internal and vencrypt cert
condition.ErrorReason, | ||
fmt.Sprintf("TLSInput error occured in TLS sources Secret %s/internal-tls-certs not found", novaNames.Namespace), | ||
condition.RequestedReason, | ||
"TLSInput is missing: combined-ca-bundle", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
condition.ErrorReason, | ||
fmt.Sprintf("TLSInput error occured in TLS sources Secret %s/vencrypt-tls-certs not found", novaNames.Namespace), | ||
condition.RequestedReason, | ||
"TLSInput is missing: combined-ca-bundle", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
condition.ErrorReason, | ||
fmt.Sprintf("TLSInput error occured in TLS sources Secret %s/internal-tls-certs not found", novaNames.Namespace), | ||
condition.RequestedReason, | ||
"TLSInput is missing: combined-ca-bundle", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
condition.ErrorReason, | ||
fmt.Sprintf("TLSInput error occured in TLS sources Secret %s/vencrypt-tls-certs not found", novaNames.Namespace), | ||
condition.RequestedReason, | ||
"TLSInput is missing: combined-ca-bundle", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
condition.ErrorReason, | ||
fmt.Sprintf("TLSInput error occured in TLS sources Secret %s/internal-tls-certs not found", novaNames.Namespace), | ||
condition.RequestedReason, | ||
"TLSInput is missing: combined-ca-bundle", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
condition.ErrorReason, | ||
fmt.Sprintf("TLSInput error occured in TLS sources Secret %s/public-tls-certs not found", novaNames.Namespace), | ||
condition.RequestedReason, | ||
"TLSInput is missing: combined-ca-bundle", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
08c2cfc
to
6046be0
Compare
controllers/novaapi_controller.go
Outdated
condition.TLSInputReadyCondition, | ||
condition.RequestedReason, | ||
condition.SeverityInfo, | ||
fmt.Sprintf(condition.TLSInputReadyWaitingMessage, "one or more cert secrets"))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is what I commented about during our call. In the past the error we get from the lib-common stated which secret was missing but now we don't know that exactly if the internal or the public tls cert is missing. This should be fixed in lib-common as this makes it impossible to us to tell the user which secret is missing via the condition.
As this was changed in openstack-k8s-operators/lib-common#547 by @abays lets see what he suggest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes agree that we now missing info about what service secret is missing, but it's hard to find a way to inform user more specific than adding additional info what secrets we are require for service (public and/or internal)
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/20d58766de344f6fa654afa566ceda72 ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 22m 29s |
recheck requested autohold on node |
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/24d7d0ceefd44958a88e51a54eb45fcf ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 30m 25s |
recheck autohold (now with the right project 🤦🏻♂️ ) |
6046be0
to
ceead51
Compare
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/bccf9c9437654bd3b2a7aab98961baf1 ✔️ openstack-meta-content-provider SUCCESS in 3h 03m 29s |
/recheck |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gibizer, openstack-k8s-ci-robot 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 |
This PR contains the following updates:
91b64b2
->12ffed6
ba2309d
->990fe66
7fd3da6
->174296c
7fd3da6
->174296c
2d771bf
->64a741b
Configuration
📅 Schedule: Branch creation - "every weekend" in timezone America/New_York, Automerge - At any time (no schedule defined).
🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.
♻ Rebasing: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.
👻 Immortal: This PR will be recreated if closed unmerged. Get config help if that's undesired.
This PR has been generated by Renovate Bot.