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

Create secret for service account if k8s version >= 1.24 #274

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

larrywax
Copy link
Contributor

What

This PR adds the capability to create the service account secret, during helm install, if the kubernetes cluster version is >= 1.24
Since k8s v1.24 secrets API objects containing service account tokens are no longer auto-generated.
https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.24.md#urgent-upgrade-notes

How

Create a Secret API object linked to the service account if k8s version is >= 1.24.0

Can this PR be safely reverted / rolled back?

  • YES 💚
  • NO ❌

🚨 User Impact 🚨

If the user manually created the Secret object before this change, the helm install/upgrade command will fail.

@pmossman
Copy link
Contributor

Hey @larrywax! I'm interested to learn more about your specific use case for needing a Secret-backed service account token, rather than letting Kubernetes use the TokenRequest API under the hood as described in the Note: block here: https://kubernetes.io/docs/concepts/configuration/secret/#service-account-token-secrets

I think we'd want to avoid automatically creating this Secret by default for all K8s installations, so if you do need this Secret to be created, I'd probably want to see this being an opt-in setting, ie an installer could set some helm value to "turn on" the creation of this service account secret. I hope that makes sense! Let me know if you have any questions/clarifications

@larrywax
Copy link
Contributor Author

larrywax commented Sep 19, 2023

I think we'd want to avoid automatically creating this Secret by default for all K8s installations, so if you do need this Secret to be created, I'd probably want to see this being an opt-in setting, ie an installer could set some helm value to "turn on" the creation of this service account secret. I hope that makes sense! Let me know if you have any questions/clarifications

Hey @pmossman! I think what you are saying makes total sense, I will update the PR to create the secret conditionally. Do you think something along the lines of create_secret or create_token_secret could work well?

Hey @larrywax! I'm interested to learn more about your specific use case for needing a Secret-backed service account token, rather than letting Kubernetes use the TokenRequest API under the hood

I need it because I'm trying to use IRSA to let airbyte use an S3 bucket as storage. It's not working, and my first guess was the missing service account secret.

@oleksandr-gubchenko
Copy link

any updates on this one? thanks.

@pmossman
Copy link
Contributor

@larrywax sorry for the slow reply here, I realized I misconfigured my Github mentions awhile back so I didn't see a notification that you had replied, my mistake!

I know that we've had other open-source installers successfully use IRSA (see #263 for some context in case it's useful).

Were you able to try this secret creation from your branch to verify that it fixes the issue? Either way I think this PR looks reasonable so I'll open it as an internal PR where I can run our full tests and merge if things check out.

@pmossman
Copy link
Contributor

/create-oss-pr

@larrywax
Copy link
Contributor Author

Were you able to try this secret creation from your branch to verify that it fixes the issue? Either way I think this PR looks reasonable so I'll open it as an internal PR where I can run our full tests and merge if things check out.

Unfortunately this PR doesn't seem to fix the issue
From the error I'm getting it seems the PR you mentioned did not fixed the issue neither.
Here is the log

software.amazon.awssdk.core.exception.SdkClientException: Unable to load credentials from any of the providers in the chain AwsCredentialsProviderChain(credentialsProviders=[SystemPropertyCredentialsProvider(), EnvironmentVariableCredentialsProvider(), WebIdentityTokenCredentialsProvider(), ProfileCredentialsProvider(profileName=default, profileFile=ProfileFile(profilesAndSectionsMap=[])), ContainerCredentialsProvider(), InstanceProfileCredentialsProvider()]) :
[
    SystemPropertyCredentialsProvider(): Unable to load credentials from system settings. Access key must be specified either via environment variable (AWS_ACCESS_KEY_ID) or system property (aws.accessKeyId).,
    EnvironmentVariableCredentialsProvider(): Unable to load credentials from system settings. Access key must be specified either via environment variable (AWS_ACCESS_KEY_ID) or system property (aws.accessKeyId).,
    WebIdentityTokenCredentialsProvider(): To use web identity tokens, the 'sts' service module must be on the class path.,
    ProfileCredentialsProvider(profileName=default, profileFile=ProfileFile(profilesAndSectionsMap=[])): Profile file contained no credentials for profile 'default': ProfileFile(profilesAndSectionsMap=[]),
    ContainerCredentialsProvider(): Cannot fetch credentials from container - neither AWS_CONTAINER_CREDENTIALS_FULL_URI or AWS_CONTAINER_CREDENTIALS_RELATIVE_URI environment variables are set.,
    InstanceProfileCredentialsProvider(): Failed to load credentials from IMDS.
]

@pmossman
Copy link
Contributor

@larrywax sorry to hear that things still aren't working for you. If you'd like to share your helm configuration files (ie any values that you've set that differ from the repo defaults, or any custom helm value files that you're providing, with any sensitive info redacted), I could try to take a look and see if it looks like there are any missing configurations.

We have an objective on our roadmap to stand up a dedicated, persistent AWS environment for end to end testing and reproduction of user issues so that we can provide better support for these types of investigations in the future. For now though, I think our best bet is to review your configurations and see if we can catch any issues there

@larrywax
Copy link
Contributor Author

larrywax commented Oct 3, 2023

@larrywax sorry to hear that things still aren't working for you. If you'd like to share your helm configuration files (ie any values that you've set that differ from the repo defaults, or any custom helm value files that you're providing, with any sensitive info redacted), I could try to take a look and see if it looks like there are any missing configurations.

We have an objective on our roadmap to stand up a dedicated, persistent AWS environment for end to end testing and reproduction of user issues so that we can provide better support for these types of investigations in the future. For now though, I think our best bet is to review your configurations and see if we can catch any issues there

Hi @pmossman thanks again for your kind answer.
Here is the relevant part of our helm values file, thanks


global:
  database:
    secretName: "airbyte"
    secretValue: "db_password"
    host: "xxxxxxxxxxxxxxx"
    port: "5432"
  state:
    storage:
      type: "S3"
  logs:
    storage:
      type: "S3"

    minio:
      enabled: false

    s3:
      enabled: true
      bucket: xxxxxxxxxxxx
      bucketRegion: "xxxxxxxxx"

serviceAccount:
  create: true
  annotations: 
    eks.amazonaws.com/role-arn: xxxxxxxxxxx

externalDatabase:
  host: "xxxxxxxxxxxxxxx"
  user: airbyte
  existingSecret: "airbyte"
  existingSecretPasswordKey: "db_password"
  database: airbyte
  port: 5432

minio:
  enabled: false

@weichunnn
Copy link

weichunnn commented Oct 4, 2023

@larrywax @pmossman Following this issue since our team is also facing similar errors.

I had a slight suspicion that IRSA logging from #263 works (since the underlying log4j module is using v1 client). On the other hand, since most of the S3 client Airbyte uses to fetch data is using the v2 SDK, should we also include software.amazon.awssdk:sts as a dependency? Looking into AWS's implementation for WebIdentityTokenCredentialsProvider, client are expected to retrieve from a hardcoded class path, hence this might be the reason for the error above.

I can open a PR similar to #263 with these changes if the above makes sense.

ps. I hadn't had the time to setup a cluster to test this idea yet - feel free to validate if it's quicker on your end.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


larrywax seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

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

Successfully merging this pull request may close these issues.

6 participants