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.

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

Support raw/plain secret pull from AWS Secret Manager #269

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

obabec
Copy link

@obabec obabec commented Nov 27, 2024

Hi Packer community!

This PR solves the issue described in Packer Issue . Currently, the secret manager is only able to work with simple key:value pairs in JSON format. This is unfortunate as there are other types of secrets in the world.

In this PR, I have added an additional argument to secretsmanager, which allows you to select whether you want the secret in raw/plain form or in the default key:value pair format.

Essentially, the only change is that the awsmanager function GetSecret() now takes one additional argument raw, which is a simple boolean. This boolean instructs the function to return the plain string value of the secret even if the secret is valid JSON.

There are two tests implemented for this feature. If you are still unsure about the issue, please take a look at the tests. I think it will be quite clear after that.

Of course, to expose this feature, we will also have to add a new function to Packer that will fully expose this to users.

As this is my first contribution to Packer/Packer SDK, I am glad for all the feedback and advice on how to push this PR forward.

@obabec obabec requested a review from a team as a code owner November 27, 2024 09:50
Copy link

hashicorp-cla-app bot commented Nov 27, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

CLA assistant check

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes

Have you signed the CLA already but the status is still pending? Recheck it.

@obabec
Copy link
Author

obabec commented Dec 3, 2024

Hi @nywilken, could you provide some feedback please? Thank you!

@lbajolet-hashicorp
Copy link
Contributor

Hi @obabec,

FYI @nywilken is not a Hashicorp employee anymore, but I can take it from here in his stead.

Regarding the secretsmanager function, I have two concerns, please let me know what you think about those:

  1. The change you propose seems to change the signature of the function, making current uses incompatible with this change. This is a breaking change at first glance, and as such I would not consider this mergeable because of the ramifications such a change is bound to have on existing templates. If you want to support this use case, I would suggest adding a new function instead, but even this might be a problem possibly regarding my second point.
  2. We have a secretsmanager datasource on the amazon plugin, which already supports those raw objects (or at least I believe so, if you don't define key?), in terms of usage I would strongly advise we focus our efforts on this one instead of here, especially since the SDK is meant to be a common building block for all plugins, and as such having an amazon-specific function in it feels out of place. Unless there's a compelling argument in favour of not using it, I suggest using the datasource instead of the function for such a use-case.

Hope that explains my concerns adequately, please let me know what you think and we can discuss this PR some more!

@obabec
Copy link
Author

obabec commented Dec 10, 2024

Hi @lbajolet-hashicorp, thanks for response 👍

So regarding the first concern. I am not really familiar with all the dependencies in the code but to my understanding I am proposing change just in the internal implementation, and I am adding new exposed function in the template so what this proposal should do, is to expose two functions:

  • GetAWSSecret with unchanged signature
  • GetAWSSecretRaw

So imho that should not break anything in packer, as GetAWSSecret has still same signature. But as I said I might be mistaken.

Regarding the second point. We didn't even know that secret manager data source exists. We can try that and get back to you :)

@obabec
Copy link
Author

obabec commented Dec 10, 2024

@lbajolet-hashicorp I have briefly checked the code of data source and I am not really sure that it will help us without any modification. See this line - https://github.com/hashicorp/packer-plugin-amazon/blob/main/datasource/secretsmanager/data.go#L142 It will try to unmarshall all the secrets and if that fails id does not fallback to raw secret value.

I will be more than happy to elaborate on this during some quick huddle or something like that, if you find some free time slot.

@lbajolet-hashicorp
Copy link
Contributor

Hey @obabec,

Thanks for the clarification, I misunderstood the change, you do add a second function to get the raw secret, so in terms of backwards-compatibility, we're good :)

I understand some people still use the function as it has historically been the way to get those secrets before the introduction of data sources and HCL2, but in terms of separation of concerns, it would be more maintenable/logic to gather AWS-related code into the Amazon plugin, hence I'm still reticent to accept this PR in the current state, even if the change won't break existing configurations, although I will still push towards improving the data source instead before we can consider merging this change, I hope this is not too disappointing.

I have misunderstood how the datasource works it seems also. Indeed if you don't specify a key, it'll return the first value of the JSON object returned instead of the raw data, which is what you're aiming for here; I believe this might be a good idea to add a raw option to this datasource then, if it fits your use-case, I don't have a problem adding this to the component itself.
Did you want to take a jab at this? If not, we can pick that up from here, as you prefer!

Also open to huddle if you want, I believe we're on very different timezones (I am North-America ET), but if relevant, we can probably make something work, let me know how you want to proceed with that!

@obabec
Copy link
Author

obabec commented Dec 10, 2024

@lbajolet-hashicorp not dissapointing at all. I am not owner nor have good overview how you want proceed to the future. I am more that happy to implement that into the data source myself, but I would probably prefer to have some chat about it so I can write it once in a way that we are both happy with it :) I am still not sure if data source will fix our problem but that is yet another story.

ET is not that bad I think we should be able to find some time. Is there some slack/whatever that I can join so we can discuss the details in dms? :)

@lbajolet-hashicorp
Copy link
Contributor

lbajolet-hashicorp commented Dec 10, 2024

I don't think we have a slack for users to interact with us directly for now. I honestly think that might be a good idea at some point to have a real-time discussion channel, though I cannot promise this will ever happen as it will involve some company involvement, and there are bound to be concerns about interrupts :)

That said, feel free to contact me at my <first name>.<last name>@hashicorp.[com] email, and we should be able to schedule that!

Thanks for the quick update and involvement!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants