Skip to content

[WIP] Add environment variables to kpack image CR#1600

Merged
aashah merged 3 commits intocloudfoundry:masterfrom
akhinos:kpack-env
May 8, 2020
Merged

[WIP] Add environment variables to kpack image CR#1600
aashah merged 3 commits intocloudfoundry:masterfrom
akhinos:kpack-env

Conversation

@kramerul
Copy link
Copy Markdown
Contributor

@kramerul kramerul commented Apr 28, 2020

Thanks for contributing to cloud_controller_ng. To speed up the process of reviewing your pull request please provide us with:

  • A short explanation of the proposed change:

See

  • An explanation of the use cases your change solves

Env variables are not passed to kpack builds

  • Links to any other associated PRs

  • I have reviewed the contributing guide

  • I have viewed, signed, and submitted the Contributor License Agreement

  • I have made this pull request to the master branch

  • I have run all the unit tests using bundle exec rake

  • I have run CF Acceptance Tests

@cf-gitbot
Copy link
Copy Markdown

cf-gitbot commented Apr 28, 2020

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/172550328

The labels on this github issue will be updated when the story is started.

aashah
aashah reviewed Apr 28, 2020
View reviewed changes
Comment thread lib/cloud_controller/kpack/stager.rb Outdated
},
build: {
env: staging_details.environment_variables.to_a.
select{ | key, value | key =~ /^BP[A-Z]{0,1}_/ }.
Copy link
Copy Markdown
Contributor

@aashah aashah Apr 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why there is a whitelist on environment variables prefixed with BP*?

Copy link
Copy Markdown
Contributor Author

@kramerul kramerul Apr 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This white list is not really required. It was only a gut feeling that feeding in variables like VCAP_SERVICES could cause problems. We also had these kind of problems when the to_s was missing in the line below.

Copy link
Copy Markdown
Contributor

@aashah aashah Apr 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, VCAP_SERVICES is not going to be the future method that bound services supply credentials to the app. There is upcoming work for this here - https://www.pivotaltracker.com/story/show/172562308

I think I'd still suggest relaxing the constraint, and maybe only filter out VCAP_SERVICES. There are many other environment variables that app developers set, and BP* is overly restrictive (and perhaps too specific to a single buildpack like bellsoft-liberica buildpack).

@aashah
Copy link
Copy Markdown
Contributor

aashah commented Apr 28, 2020

Hey @kramerul,

Thanks for submitting this PR. There's actually a prioritized story (link) near the top of our backlog to do this! I'm sure whoever on the team picks it up will be happy to know there's a PR :)

I see you've marked it as WIP as well, what other work is on your TODO list?

  • Aakash

@kramerul
Copy link
Copy Markdown
Contributor Author

kramerul commented Apr 29, 2020

Hi @aashah ,

I marked this PR as WIP, because I never filed a PR for cloud controller and I wasn't sure if I needed to run the acceptance tests for this rather small PR.
For the acceptance tests I would have needed a running CloudFoundry, which is quite a lot of work for me, since we are only dealing with cf-for-k8s at the moment. I have no experience with BOSH at all.

@aashah
Copy link
Copy Markdown
Contributor

aashah commented Apr 29, 2020

Ah yeah, that's totally fine! Definitely agree, CATs is not required for this PR. Once we resolve some of the discussion on that "constraint" in the code, I think I can check out the PR and try it out!

@aashah
Copy link
Copy Markdown
Contributor

aashah commented Apr 29, 2020

It also looks like there are several rubocop errors detailed here - https://travis-ci.org/github/cloudfoundry/cloud_controller_ng/jobs/680412878

@kramerul
Copy link
Copy Markdown
Contributor Author

kramerul commented Apr 30, 2020

Hi @aashah,

I removed the select for the variables starting with BP*.

@aashah
Copy link
Copy Markdown
Contributor

aashah commented Apr 30, 2020

Hey @kramerul,

I think your original gut feeling around wanting to prevent VCAP_SERVICES from being passed in resonates with me. Echo'ing some of the feedback from above, could you keep & update the filter to only prevent VCAP_SERVICES?

Additionally, there are many silly rubocop errors that need to be taken care of before I can merge the PR - https://travis-ci.org/github/cloudfoundry/cloud_controller_ng/jobs/681337312

  • Aakash

@kramerul
Copy link
Copy Markdown
Contributor Author

kramerul commented May 4, 2020

Hi @aashah,

I'm now filtering out VCAP_SERVICES and also fixed the rubocop issues.

@aashah
Copy link
Copy Markdown
Contributor

aashah commented May 8, 2020

Looks good to me!

Dev acceptance:

$ cf push --no-start node
Pushing app node to org o / space s as admin...
Getting app info...
Creating app with these attributes...
...
memory usage:   1024M
     state   since                  cpu    memory   disk     details
#0   down    2020-05-08T23:08:33Z   0.0%   0 of 0   0 of 0

$ cf set-env node FOO BAR
Setting env variable 'FOO' for app node in org o / space s as admin...
OK
TIP: Use 'cf restage node' to ensure your env variable changes take effect

$ cf start node
Starting app node in org o / space s as admin...

Staging app and tracing logs...

...
   Build successful
...

type:           web
instances:      1/1
memory usage:   1024M
     state     since                  cpu    memory    disk      details
#0   running   2020-05-08T23:09:48Z   0.0%   0 of 1G   0 of 1G

$ k get build eeb7efc2-9c97-48f4-99e2-dd008ca4917f-build-1-2dr9c -n cf-workloads-staging -o yaml
apiVersion: build.pivotal.io/v1alpha1
kind: Build
metadata:
...
spec:
  env:
  - name: FOO
    value: BAR
...

@aashah aashah merged commit 078281a into cloudfoundry:master May 8, 2020
aashah pushed a commit that referenced this pull request May 12, 2020
Fix app env vars when updating an existing Image
cb41b2e 
* In the original PR (#1600), we only applied environment variables when
creating the first "Image" resource

[#172301179]

Authored-by: Aakash Shah <ashah@pivotal.io>
@tcdowney tcdowney mentioned this pull request Mar 7, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@aashah aashah aashah left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

accepted

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kramerul @cf-gitbot @aashah