-
Notifications
You must be signed in to change notification settings - Fork 345
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
Add settings security to Maven build #3185
Conversation
I think the correct way to proceed would be to have:
We need to put in place what is described in https://maven.apache.org/guides/mini/guide-encryption.html and/or allowing the final user to provide such |
So you agree that we should turn configMapKeyRef to array type? Because currently I see that the type of configMapKeyRef is object.
Here you mean we should turn secretKeyRef to array type too? |
That's correct. We don't have the |
16d5c91
to
7f81b97
Compare
@squakez I added settingsSecurity to MavenSpec. Can you check please? |
@haanhvu that's a good start. I guess we need to develop the related logic and make use of that settings during the build now. I suggest a TDD approach, where you develop a test (similar to this https://github.com/apache/camel-k/blob/main/pkg/builder/project_test.go#L193) and then you complete the logic to pass that test. |
@squakez From my understanding, we need to test |
656a6ea
to
0243dce
Compare
@squakez can you pls check the test cases? |
The unit test looks okey. I think you must now develop the logic to make use of that settings. You can make reference to the implementation done for settings as well, see https://github.com/apache/camel-k/blob/main/pkg/builder/project.go#L107 |
@squakez could you help me a little here: From my understanding, in the logic there's a step to encrypt server passwords with the command line How can I implement this step in the code? |
So, that step must be performed manually by the user which ends up with this file: |
@squakez I understand that the user needs to create a master password manually. What you just described is this: What I asked is about encrypting server password: From what I read in https://maven.apache.org/guides/mini/guide-encryption.html , once we have the |
@haanhvu that link was there just to illustrate the reason why the feature is meant to be. We need to focus on passing whatever the |
852c8f5
to
b989e4f
Compare
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.
I think it's okey. It would be good to have an E2E test however, in order to confirm that we really use those settings-security correctly when provided. Do you think can you try? alternatively you may create an example in the example directory which we can use as a tester for this new feature.
@squakez e2e test seems to be a better idea. I'll give it a try. |
899d585
to
873fbb5
Compare
@squakez Could you guide me a little here? We need to add a test case to an existing test, a create a whole new test? Do we have a similar test for |
@haanhvu we probably need a new test. You can use the tests in this directory as a reference https://github.com/apache/camel-k/tree/main/e2e/common/build as they are all dealing with maven configuration. Feel free to create a new file which contains your set of test (if you can take the opportunity and provide the settings, it would be very welcome). In order to execute them locally, please, have a look at the guide to perform E2E locally. If you need any further support, don't hesitate to ask. |
@squakez I guess the |
@haanhvu can you try to execute |
58778a3
to
5a49f41
Compare
@squakez I fixed the problem with I ran However in the end
I pushed the generated changes from Or is there a problem with my
Also, as I said, I use the go version 1.17.9. Is it a recommended version for camel-k? |
The Golang version we support is actually 1.16. As for the Custom Resource, probably it's because you still have some reference to the previous installation. Ideally you should delete all Custom Resources (ie, via |
d0e2ca8
to
d3ab9d6
Compare
@squakez I uninstalled camel-k. I knew it's uninstalled because when I checked with Then I reinstalled with However, the
I highly suspect this is a golang version problem again during build. Should I downgrade to go 1.16.x? I read in #3236 that some |
Hi @haanhvu I think here it is complaining about not correctly installed CRDS. Can you run the following and get the output:
What we need to do is to install the proper CRDs into the cluster in order to have your changes applied. As a last resort, if the above does not show any |
@squakez yeah manually install the crd could be the last simple choice. I just wanted to inspect if my environment has any problems with the build and the install. Anyway I'll dive deeper into this later... I set up the crd manually and it worked:
Should I add this example to the PR and we get this done now? I'll add the e2e test in a later PR. |
Yes please. Provide a detailed example on how to use the new configuration. |
d985cce
to
96f9960
Compare
@squakez I added an example and also edit the maven doc. Please check if those work |
@@ -1,18 +1,19 @@ | |||
= Configure Maven | |||
|
|||
[[maven-settings]] | |||
== Maven Settings | |||
== Maven Settings and Maven Settings Security |
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.
+1 for the documentation update. However, I'd have a separate section nested under == Maven Settings
called === Security Settings
. In there you may link also to the Maven website which explain how to setup the security settings file
examples/user-config/maven/README.md
Outdated
@@ -0,0 +1,17 @@ | |||
# Camel K Maven configuration examples |
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. Let's move it under /examples/maven. user-config directory was thought for kamel run
parameters.
@@ -2301,6 +2301,13 @@ base Maven specification | |||
|
|||
additional repositories | |||
|
|||
|`servers` + |
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.
Where is this servers
coming from?
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.
@squakez it was generated from make generate
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, can you regenerate? it looks like it was picking that from source, ie, maven_types.go
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.
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.
it may happen, yes. Make sure to rebase with the latest from main
, so you will get the freshest things.
112ffe5
to
d54deb5
Compare
@squakez I regenerated and nothing's changed. Also moved the maven example directory and updated the maven doc properly. |
@squakez A few updates: I just rebased with the latest I don't know if it's thanks to the new genarated or not. But the new change is now successfully installed and I don't need manually install the crd anymore. I also moved the example directory and updated the doc. Could you review? |
Merged. Thanks for the contribution! |
Release Note
fixes #2747
My idea of the fix is to enable spec.build.maven.settings to have multiple configMapKeyRef. For example:
@squakez can you check if this is the right direction? This is not done yet. But at first I need to know if this is the right direction.