⚠️ Kustomizations usable off the shelf #1438
Conversation
metal3-io-bot
added
the
size/XL
lentzi90
force-pushed
the
lentzi90/e2e-basic-auth-tls
branch
from
c0d6448
to
1e4ad8f
Compare
|
/metal3-bmo-e2e-test |
test/e2e/e2e_suite_test.go
Outdated
There was a problem hiding this comment.
The changes here is just to install Ironic before BMO, since BMO will try to mount the CA certificate that comes with Ironic.
|
/hold |
metal3-io-bot
added
the
do-not-merge/hold
lentzi90
force-pushed
the
lentzi90/e2e-basic-auth-tls
branch
from
1e4ad8f
to
c324f5a
Compare
|
/metal3-bmo-e2e-test |
|
/test-ubuntu-integration-main |
lentzi90
force-pushed
the
lentzi90/e2e-basic-auth-tls
branch
from
c324f5a
to
9bc093b
Compare
|
/test-ubuntu-integration-main |
lentzi90
force-pushed
the
lentzi90/e2e-basic-auth-tls
branch
from
9bc093b
to
8e0492c
Compare
|
/test-ubuntu-integration-main |
lentzi90
force-pushed
the
lentzi90/e2e-basic-auth-tls
branch
from
8e0492c
to
85c12e2
Compare
|
/test-ubuntu-integration-main |
lentzi90
force-pushed
the
lentzi90/e2e-basic-auth-tls
branch
from
85c12e2
to
9610e66
Compare
|
/test-ubuntu-integration-main |
1 similar comment
|
/test-ubuntu-integration-main |
lentzi90
force-pushed
the
lentzi90/e2e-basic-auth-tls
branch
from
e716ac9
to
ab0631c
Compare
|
/test-ubuntu-integration-main |
|
/metal3-bmo-e2e-test |
|
Tests looks good! |
metal3-io-bot
removed
the
do-not-merge/hold
There was a problem hiding this comment.
These credentials would be used in the e2e tests. I'm curious to hear opinions on this. My reasoning for including them in the repo is that
- They act like a concrete example that users can look at to understand both how to create the secrets and what to put in them
- The tests should (ideally) not expose Ironic to anything outside the CI system anyway and they are relatively short lived
I'm open for other solutions though. We could generate them automatically for the tests and static examples can be provided without being part of the CI.
/cc @tuminoid
There was a problem hiding this comment.
I agree that we need to have proper usable examples for the users, so they don't need to guess what they're supposed to look like etc, but I'd prefer generating them, and just documenting the proper format/process.
- While the risk of exposure is small, it still exists, better remove it
- There is always someone who will deploy them in production anyways ...
- If we generate them, we also test our codebase versus accidental hardcoding, ie. make it more realistic check
If in step 3 we use random username/password, it'll make debugging/developing a bit harder though, so let's document it well where/how to fetch the creds.
What do you think?
There was a problem hiding this comment.
It does make sense to me, but makes things a bit more complicated. I need to think about how to do it.
There was a problem hiding this comment.
Done, and looks like it is working!
I have not added any more documentation yet. Perhaps that can be in a separate PR? We don't have any docs for it at the moment anyway so no degradation...
The credentials are created in the ci-e2e.sh script. This means that outside of CI, users can still choose to set up a static overlay for use with a baremetal lab or similar.
config/default/kustomization.yaml
Outdated
There was a problem hiding this comment.
This was basically copied to config/base/kustomization.yaml and then simplified by using that as base
tools/deploy.sh
Outdated
There was a problem hiding this comment.
The changes to this file can be summarized like this:
- Gereration of credentials was moved up so that the files will be in place when we start building the kustomizations.
- A temporary overlay is used for BMO (just like Ironic was already doing).
config/base/kustomization.yaml
Outdated
There was a problem hiding this comment.
This was copied from config/default and then the paths were adapted. The only other change is that this does NOT create the ironic configmap. This is because the content of the configmap depends on the environment so the user will anyway need to change it in almost all cases.
|
/metal3-bmo-e2e-test |
To be clear, it will still be possible to use the deploy script for those who want to do so. I don't expect people in general want to use a script to deploy though, and this will make it possible to just use the kustomizations as is, without modifications or scripts. |
metal3-io-bot
added
the
needs-rebase
lentzi90
force-pushed
the
lentzi90/e2e-basic-auth-tls
branch
from
a09bdba
to
8725ce0
Compare
metal3-io-bot
removed
the
needs-rebase
lentzi90
force-pushed
the
lentzi90/e2e-basic-auth-tls
branch
2 times, most recently
from
47b482d
to
4eccf2b
Compare
This restructures the BMO kustomizations to make use of components (similar to what was already done for the Ironic kustomizations). It also makes the kustomizations usable "off the shelf" by removing the secret generation. Previously it was necessary to clone the repository and add the relevant files (e.g. by running `tools/deploy.sh`) before it was possible to build the kustomizations that included secrets generated from the files. With this commit, the secrets will NOT be generated as part of the kustomizations and they can thus be used as is. However, the user is instead required to add the necessary secrets. The secrets can be added in any way. One common way is that the users creates their own overlays that add the secrets. These kustomizations can be based on and use the kustomizations in the repo since they are now buildable without changes. Another way to add the secrets is via the external secrets operator. Signed-off-by: Lennart Jern <[email protected]>
lentzi90
force-pushed
the
lentzi90/e2e-basic-auth-tls
branch
from
4eccf2b
to
86ab714
Compare
Signed-off-by: Lennart Jern <[email protected]>
lentzi90
force-pushed
the
lentzi90/e2e-basic-auth-tls
branch
from
86ab714
to
d895031
Compare
|
/metal3-bmo-e2e-test |
|
/test-centos-e2e-integration-main |
2 similar comments
|
/test-centos-e2e-integration-main |
|
/test-centos-e2e-integration-main |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kashifest 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 |
metal3-io-bot
added
the
approved
|
I wonder how much easier this change becomes if we throw Inspector out in the window. |
Not that much I think. I would still want to shuffle around the files like I do here. The Ironic and inspector auth configs would reduce to just one I guess, but that is just a few lines. |
|
I cannot wait for ironic-(standalone-)operator to make most of this redundant. But for now, great job refactoring! /lgtm |
The PR is marked as⚠️ breaking because the structure of the kustomizations changed. In practice I do not think it should cause problems, because the kustomizations that we change here were not usable without modifications (adding credentials) anyway. The
deploy.shscript should work just as before.What this PR does / why we need it:
This restructures the BMO kustomizations to make use of components
(similar to what was already done for the Ironic kustomizations).
It also makes the kustomizations usable "off the shelf" by removing the
secret generation. Previously it was necessary to clone the repository
and add the relevant files (e.g. by running
tools/deploy.sh) before itwas possible to build the kustomizations that included secrets generated
from the files. With this commit, the secrets will NOT be generated as
part of the kustomizations and they can thus be used as is. However, the
user is instead required to add the necessary secrets.
The secrets can be added in any way. One common way is that the users
creates their own overlays that add the secrets. These kustomizations
can be based on and use the kustomizations in the repo since they are
now buildable without changes. Another way to add the secrets is via the
external secrets operator.
Finally, basic-auth and TLS is enabled for the e2e tests. This also works as an example for how to make use of the kustomizations and for what secrets are needed.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #1361, #1375
Note for reviewers:
Most changes are just from moving files around to get the kustomize components structure. Some files needed slight modifications because of the new structure though and this can sometimes show as a large diff.