Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Deep check on security context #397

Merged
merged 1 commit into from
Apr 11, 2022

Conversation

pmahindrakar-oss
Copy link
Contributor

Signed-off-by: Prafulla Mahindrakar [email protected]

TL;DR

During testing of the project-domain setting for workflow execution config discovered that UI passes empty security context
This causes the workflow execution config to be picked from this empty context which causes the executions to be launched using default service account which is incorrect since it should have checked further in launchplan spec , matchable attribute , application config in that order.

This PR fixes the issue by doing a deep check on the security context.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

flyteorg/flyte#2070

Follow-up issue

NA

Signed-off-by: Prafulla Mahindrakar <[email protected]>
@codecov
Copy link

codecov bot commented Apr 11, 2022

Codecov Report

Merging #397 (f9fd6fe) into master (d9b1c03) will increase coverage by 0.32%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #397      +/-   ##
==========================================
+ Coverage   60.80%   61.12%   +0.32%     
==========================================
  Files         151      154       +3     
  Lines       10799    11110     +311     
==========================================
+ Hits         6566     6791     +225     
- Misses       3529     3611      +82     
- Partials      704      708       +4     
Flag Coverage Δ
unittests 60.07% <100.00%> (+0.29%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/manager/impl/execution_manager.go 74.03% <100.00%> (-0.11%) ⬇️
dataproxy/service.go 38.88% <0.00%> (-24.38%) ⬇️
pkg/repositories/gormimpl/node_execution_repo.go 67.66% <0.00%> (-5.92%) ⬇️
pkg/repositories/database.go 44.06% <0.00%> (-2.67%) ⬇️
pkg/repositories/config/migrations.go 2.65% <0.00%> (-0.15%) ⬇️
pkg/rpc/adminservice/base.go 4.12% <0.00%> (-0.05%) ⬇️
pkg/manager/impl/task_execution_manager.go 71.75% <0.00%> (ø)
...c/notifications/implementations/event_publisher.go 94.28% <0.00%> (ø)
pkg/async/cloudevent/factory.go 84.00% <0.00%> (ø)
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d9b1c03...f9fd6fe. Read the comment docs.

@pmahindrakar-oss pmahindrakar-oss merged commit c89898e into master Apr 11, 2022
@pmahindrakar-oss pmahindrakar-oss deleted the pmahindrakar/fix-security-context branch April 11, 2022 19:21
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants